diff mbox series

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

Message ID 20210927155925.15485-3-eajames@linux.ibm.com
State New
Headers show
Series occ: fsi and hwmon: Extract and provide the SBEFIFO FFDC | expand

Commit Message

Eddie James Sept. 27, 2021, 3:59 p.m. UTC 
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.

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(-)

Comments

Joel Stanley Oct. 15, 2021, 5:05 a.m. UTC | #1 
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?

Will existing userspace handle this?

>
> 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?

> +       size_t ffdc_len = (dh - 1) * 4;
> +       __be32 *ffdc = &resp[resp_len - dh];

Should you be checking that this number is sensible?

> +
> +       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;
> +}
Eddie James Oct. 19, 2021, 8:16 p.m. UTC | #2 
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;
>> +}
diff mbox series

Patch

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;
+	size_t ffdc_len = (dh - 1) * 4;
+	__be32 *ffdc = &resp[resp_len - dh];
+
+	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;
+}
+
 static int occ_verify_checksum(struct occ *occ, struct occ_response *resp,
 			       u16 data_length)
 {
@@ -245,7 +262,7 @@  static int occ_verify_checksum(struct occ *occ, struct occ_response *resp,
 static int occ_getsram(struct occ *occ, u32 offset, void *data, ssize_t len)
 {
 	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
-	size_t cmd_len, resp_data_len;
+	size_t cmd_len, parsed_len, resp_data_len;
 	size_t resp_len = OCC_MAX_RESP_WORDS;
 	__be32 *resp = occ->buffer;
 	__be32 cmd[6];
@@ -280,16 +297,17 @@  static int occ_getsram(struct occ *occ, u32 offset, void *data, ssize_t len)
 		return rc;
 
 	rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_GET_OCC_SRAM,
-				  resp, resp_len, &resp_len);
+				  resp, resp_len, &parsed_len);
 	if (rc > 0) {
 		dev_err(occ->dev, "SRAM read returned failure status: %08x\n",
 			rc);
-		return -EBADMSG;
+		occ_save_ffdc(occ, resp, parsed_len, resp_len);
+		return -ECOMM;
 	} else if (rc) {
 		return rc;
 	}
 
-	resp_data_len = be32_to_cpu(resp[resp_len - 1]);
+	resp_data_len = be32_to_cpu(resp[parsed_len - 1]);
 	if (resp_data_len != data_len) {
 		dev_err(occ->dev, "SRAM read expected %d bytes got %zd\n",
 			data_len, resp_data_len);
@@ -305,7 +323,7 @@  static int occ_putsram(struct occ *occ, const void *data, ssize_t len,
 		       u8 seq_no, u16 checksum)
 {
 	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
-	size_t cmd_len, resp_data_len;
+	size_t cmd_len, parsed_len, resp_data_len;
 	size_t resp_len = OCC_MAX_RESP_WORDS;
 	__be32 *buf = occ->buffer;
 	u8 *byte_buf;
@@ -366,18 +384,19 @@  static int occ_putsram(struct occ *occ, const void *data, ssize_t len,
 		return rc;
 
 	rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_PUT_OCC_SRAM,
-				  buf, resp_len, &resp_len);
+				  buf, resp_len, &parsed_len);
 	if (rc > 0) {
 		dev_err(occ->dev, "SRAM write returned failure status: %08x\n",
 			rc);
-		return -EBADMSG;
+		occ_save_ffdc(occ, buf, parsed_len, resp_len);
+		return -ECOMM;
 	} else if (rc) {
 		return rc;
 	}
 
-	if (resp_len != 1) {
+	if (parsed_len != 1) {
 		dev_err(occ->dev, "SRAM write response length invalid: %zd\n",
-			resp_len);
+			parsed_len);
 		rc = -EBADMSG;
 	} else {
 		resp_data_len = be32_to_cpu(buf[0]);
@@ -395,7 +414,7 @@  static int occ_putsram(struct occ *occ, const void *data, ssize_t len,
 static int occ_trigger_attn(struct occ *occ)
 {
 	__be32 *buf = occ->buffer;
-	size_t cmd_len, resp_data_len;
+	size_t cmd_len, parsed_len, resp_data_len;
 	size_t resp_len = OCC_MAX_RESP_WORDS;
 	int idx = 0, rc;
 
@@ -426,18 +445,19 @@  static int occ_trigger_attn(struct occ *occ)
 		return rc;
 
 	rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_PUT_OCC_SRAM,
-				  buf, resp_len, &resp_len);
+				  buf, resp_len, &parsed_len);
 	if (rc > 0) {
 		dev_err(occ->dev, "SRAM attn returned failure status: %08x\n",
 			rc);
-		return -EBADMSG;
+		occ_save_ffdc(occ, buf, parsed_len, resp_len);
+		return -ECOMM;
 	} else if (rc) {
 		return rc;
 	}
 
-	if (resp_len != 1) {
+	if (parsed_len != 1) {
 		dev_err(occ->dev, "SRAM attn response length invalid: %zd\n",
-			resp_len);
+			parsed_len);
 		rc = -EBADMSG;
 	} else {
 		resp_data_len = be32_to_cpu(buf[0]);
@@ -460,6 +480,7 @@  int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
 		msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
 	struct occ *occ = dev_get_drvdata(dev);
 	struct occ_response *resp = response;
+	size_t user_resp_len = *resp_len;
 	u8 seq_no;
 	u16 checksum = 0;
 	u16 resp_data_length;
@@ -468,11 +489,13 @@  int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
 	int rc;
 	size_t i;
 
+	*resp_len = 0;
+
 	if (!occ)
 		return -ENODEV;
 
-	if (*resp_len < 7) {
-		dev_dbg(dev, "Bad resplen %zd\n", *resp_len);
+	if (user_resp_len < 7) {
+		dev_dbg(dev, "Bad resplen %zd\n", user_resp_len);
 		return -EINVAL;
 	}
 
@@ -482,6 +505,10 @@  int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
 
 	mutex_lock(&occ->occ_lock);
 
+	occ->client_buffer = response;
+	occ->client_buffer_size = user_resp_len;
+	occ->client_response_size = 0;
+
 	/*
 	 * Get a sequence number and update the counter. Avoid a sequence
 	 * number of 0 which would pass the response check below even if the
@@ -532,7 +559,7 @@  int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
 	resp_data_length = get_unaligned_be16(&resp->data_length);
 
 	/* Message size is data length + 5 bytes header + 2 bytes checksum */
-	if ((resp_data_length + 7) > *resp_len) {
+	if ((resp_data_length + 7) > user_resp_len) {
 		rc = -EMSGSIZE;
 		goto done;
 	}
@@ -548,7 +575,7 @@  int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
 			goto done;
 	}
 
-	*resp_len = resp_data_length + 7;
+	occ->client_response_size = resp_data_length + 7;
 
 	{
 		DEFINE_DYNAMIC_DEBUG_METADATA(ddm_occ_rsp,
@@ -560,7 +587,7 @@  int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
 		    DYNAMIC_DEBUG_BRANCH(ddm_occ_rsp)) {
 			char prefix[64];
 			size_t l = DYNAMIC_DEBUG_BRANCH(ddm_occ_full_rsp) ?
-				*resp_len : 16;
+				occ->client_response_size : 16;
 
 			snprintf(prefix, sizeof(prefix), "%s %s: rsp ",
 				 dev_driver_string(occ->dev),
@@ -573,6 +600,7 @@  int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
 	rc = occ_verify_checksum(occ, resp, resp_data_length);
 
  done:
+	*resp_len = occ->client_response_size;
 	mutex_unlock(&occ->occ_lock);
 
 	return rc;