Re: [PATCH v3 2/4] fsi: occ: Store the SBEFIFO FFDC in the user response buffer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 10/15/21 12:05 AM, Joel Stanley wrote:
On Mon, 27 Sept 2021 at 15:59, Eddie James <eajames@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
---
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;
+}



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux