Messages in this thread | | | Subject | Re: [PATCH v3 2/4] fsi: occ: Store the SBEFIFO FFDC in the user response buffer | From | Eddie James <> | Date | Tue, 19 Oct 2021 15:16:22 -0500 |
| |
On 10/15/21 12:05 AM, Joel Stanley wrote: > On Mon, 27 Sept 2021 at 15:59, Eddie James <eajames@linux.ibm.com> wrote: >> If the SBEFIFO response indicates an error, store the response in the >> user buffer and return an error. Previously, the user had no way of >> obtaining the SBEFIFO FFDC. > How does this look for userspace?
The user's buffer now contains data in the event of a failure. No change in the event of a successful transfer.
> > Will existing userspace handle this?
Yes, unless a poorly-designed application is relying on the data being the same after a failed transfer... In that case I would argue that the application should be fixed.
> >> Signed-off-by: Eddie James <eajames@linux.ibm.com> >> --- >> Changes since v1: >> - Don't store any magic value; only return non-zero resp_len in the error >> case if there is FFDC >> >> drivers/fsi/fsi-occ.c | 66 ++++++++++++++++++++++++++++++------------- >> 1 file changed, 47 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c >> index ace3ec7767e5..1d5f6fdc2a34 100644 >> --- a/drivers/fsi/fsi-occ.c >> +++ b/drivers/fsi/fsi-occ.c >> @@ -55,6 +55,9 @@ struct occ { >> int idx; >> u8 sequence_number; >> void *buffer; >> + void *client_buffer; >> + size_t client_buffer_size; >> + size_t client_response_size; >> enum versions version; >> struct miscdevice mdev; >> struct mutex occ_lock; >> @@ -217,6 +220,20 @@ static const struct file_operations occ_fops = { >> .release = occ_release, >> }; >> >> +static void occ_save_ffdc(struct occ *occ, __be32 *resp, size_t parsed_len, >> + size_t resp_len) >> +{ >> + size_t dh = resp_len - parsed_len; > Is there any chance that parsed_len is larger than resp_len?
No, based on the sbefifo_parse_status function.
> >> + size_t ffdc_len = (dh - 1) * 4; >> + __be32 *ffdc = &resp[resp_len - dh]; > Should you be checking that this number is sensible?
Do you mean ffdc_len or (resp_len - dh)? I was basing all this on the sbefifo_parse_status code, but I see that obviously:
resp_len - dh = resp_len - (resp_len - parsed_len) = parsed_len
So I will simplify.
As for ffdc_len, it is conceivable that dh is 0, so I will add a check for that.
Thanks Joel!
Eddie
> >> + >> + if (ffdc_len > occ->client_buffer_size) >> + ffdc_len = occ->client_buffer_size; >> + >> + memcpy(occ->client_buffer, ffdc, ffdc_len); >> + occ->client_response_size = ffdc_len; >> +}
| |