[PATCH v3 0/4] occ: fsi and hwmon: Extract and provide the SBEFIFO FFDC

[PATCH v3 0/4] occ: fsi and hwmon: Extract and provide the SBEFIFO FFDC

[Eddie James]

Currently, users have no way to obtain the FFDC (First Failure Data
Capture) provided by the SBEFIFO when an operation fails. To remedy this,
add code in the FSI OCC driver to store this FFDC in the user's response
buffer and set the response length accordingly.
On the hwmon side, there is a need at the application level to perform
side-band operations in response to SBE errors. Therefore, add a new
binary sysfs file that provides the FFDC (or lack thereof) when there is
an SBEFIFO error. Now applications can take action when an SBE error is
detected.

Changes since v2:
 - Add documentation

Changes since v1:
 - Remove the magic value that indicated an SBE/SBEFIFO error with no
   FFDC.
 - Remove binary sysfs state management and intead just clear the error
   flag when the whole FFDC has been read.

Eddie James (4):
  fsi: occ: Use a large buffer for responses
  fsi: occ: Store the SBEFIFO FFDC in the user response buffer
  docs: ABI: testing: Document the OCC hwmon FFDC binary interface
  hwmon: (occ) Provide the SBEFIFO FFDC in binary sysfs

 .../sysfs-bus-platform-devices-occ-hwmon      |  13 ++
 drivers/fsi/fsi-occ.c                         | 163 +++++++++---------
 drivers/hwmon/occ/p9_sbe.c                    |  86 ++++++++-
 include/linux/fsi-occ.h                       |   2 +
 4 files changed, 184 insertions(+), 80 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-devices-occ-hwmon

-- 
2.27.0

[PATCH v3 1/4] fsi: occ: Use a large buffer for responses

[Eddie James]

Allocate a large buffer for each OCC to handle response data. This
removes memory allocation during an operation, and also allows for
the maximum amount of SBE FFDC.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/fsi/fsi-occ.c   | 109 ++++++++++++++++------------------------
 include/linux/fsi-occ.h |   2 +
 2 files changed, 45 insertions(+), 66 deletions(-)

diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
index b0c9322078a1..ace3ec7767e5 100644
--- a/drivers/fsi/fsi-occ.c
+++ b/drivers/fsi/fsi-occ.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/miscdevice.h>
+#include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/fsi-occ.h>
@@ -42,13 +43,6 @@
 
 #define OCC_P10_SRAM_MODE	0x58	/* Normal mode, OCB channel 2 */
 
-/*
- * Assume we don't have much FFDC, if we do we'll overflow and
- * fail the command. This needs to be big enough for simple
- * commands as well.
- */
-#define OCC_SBE_STATUS_WORDS	32
-
 #define OCC_TIMEOUT_MS		1000
 #define OCC_CMD_IN_PRG_WAIT_MS	50
 
@@ -60,6 +54,7 @@ struct occ {
 	char name[32];
 	int idx;
 	u8 sequence_number;
+	void *buffer;
 	enum versions version;
 	struct miscdevice mdev;
 	struct mutex occ_lock;
@@ -250,8 +245,10 @@ 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_len, resp_data_len;
-	__be32 *resp, cmd[6];
+	size_t cmd_len, resp_data_len;
+	size_t resp_len = OCC_MAX_RESP_WORDS;
+	__be32 *resp = occ->buffer;
+	__be32 cmd[6];
 	int idx = 0, rc;
 
 	/*
@@ -278,19 +275,19 @@ static int occ_getsram(struct occ *occ, u32 offset, void *data, ssize_t len)
 	cmd[1] = cpu_to_be32(SBEFIFO_CMD_GET_OCC_SRAM);
 	cmd[4 + idx] = cpu_to_be32(data_len);
 
-	resp_len = (data_len >> 2) + OCC_SBE_STATUS_WORDS;
-	resp = kzalloc(resp_len << 2, GFP_KERNEL);
-	if (!resp)
-		return -ENOMEM;
-
 	rc = sbefifo_submit(occ->sbefifo, cmd, cmd_len, resp, &resp_len);
 	if (rc)
-		goto free;
+		return rc;
 
 	rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_GET_OCC_SRAM,
 				  resp, resp_len, &resp_len);
-	if (rc)
-		goto free;
+	if (rc > 0) {
+		dev_err(occ->dev, "SRAM read returned failure status: %08x\n",
+			rc);
+		return -EBADMSG;
+	} else if (rc) {
+		return rc;
+	}
 
 	resp_data_len = be32_to_cpu(resp[resp_len - 1]);
 	if (resp_data_len != data_len) {
@@ -301,39 +298,21 @@ static int occ_getsram(struct occ *occ, u32 offset, void *data, ssize_t len)
 		memcpy(data, resp, len);
 	}
 
-free:
-	/* Convert positive SBEI status */
-	if (rc > 0) {
-		dev_err(occ->dev, "SRAM read returned failure status: %08x\n",
-			rc);
-		rc = -EBADMSG;
-	}
-
-	kfree(resp);
 	return rc;
 }
 
 static int occ_putsram(struct occ *occ, const void *data, ssize_t len,
 		       u8 seq_no, u16 checksum)
 {
-	size_t cmd_len, buf_len, resp_len, resp_data_len;
 	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
-	__be32 *buf;
+	size_t cmd_len, resp_data_len;
+	size_t resp_len = OCC_MAX_RESP_WORDS;
+	__be32 *buf = occ->buffer;
 	u8 *byte_buf;
 	int idx = 0, rc;
 
 	cmd_len = (occ->version == occ_p10) ? 6 : 5;
-
-	/*
-	 * We use the same buffer for command and response, make
-	 * sure it's big enough
-	 */
-	resp_len = OCC_SBE_STATUS_WORDS;
 	cmd_len += data_len >> 2;
-	buf_len = max(cmd_len, resp_len);
-	buf = kzalloc(buf_len << 2, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
 
 	/*
 	 * Magic sequence to do SBE putsram command. SBE will transfer
@@ -384,12 +363,17 @@ static int occ_putsram(struct occ *occ, const void *data, ssize_t len,
 
 	rc = sbefifo_submit(occ->sbefifo, buf, cmd_len, buf, &resp_len);
 	if (rc)
-		goto free;
+		return rc;
 
 	rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_PUT_OCC_SRAM,
 				  buf, resp_len, &resp_len);
-	if (rc)
-		goto free;
+	if (rc > 0) {
+		dev_err(occ->dev, "SRAM write returned failure status: %08x\n",
+			rc);
+		return -EBADMSG;
+	} else if (rc) {
+		return rc;
+	}
 
 	if (resp_len != 1) {
 		dev_err(occ->dev, "SRAM write response length invalid: %zd\n",
@@ -405,27 +389,16 @@ static int occ_putsram(struct occ *occ, const void *data, ssize_t len,
 		}
 	}
 
-free:
-	/* Convert positive SBEI status */
-	if (rc > 0) {
-		dev_err(occ->dev, "SRAM write returned failure status: %08x\n",
-			rc);
-		rc = -EBADMSG;
-	}
-
-	kfree(buf);
 	return rc;
 }
 
 static int occ_trigger_attn(struct occ *occ)
 {
-	__be32 buf[OCC_SBE_STATUS_WORDS];
-	size_t cmd_len, resp_len, resp_data_len;
+	__be32 *buf = occ->buffer;
+	size_t cmd_len, resp_data_len;
+	size_t resp_len = OCC_MAX_RESP_WORDS;
 	int idx = 0, rc;
 
-	BUILD_BUG_ON(OCC_SBE_STATUS_WORDS < 8);
-	resp_len = OCC_SBE_STATUS_WORDS;
-
 	switch (occ->version) {
 	default:
 	case occ_p9:
@@ -450,12 +423,17 @@ static int occ_trigger_attn(struct occ *occ)
 
 	rc = sbefifo_submit(occ->sbefifo, buf, cmd_len, buf, &resp_len);
 	if (rc)
-		goto error;
+		return rc;
 
 	rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_PUT_OCC_SRAM,
 				  buf, resp_len, &resp_len);
-	if (rc)
-		goto error;
+	if (rc > 0) {
+		dev_err(occ->dev, "SRAM attn returned failure status: %08x\n",
+			rc);
+		return -EBADMSG;
+	} else if (rc) {
+		return rc;
+	}
 
 	if (resp_len != 1) {
 		dev_err(occ->dev, "SRAM attn response length invalid: %zd\n",
@@ -471,14 +449,6 @@ static int occ_trigger_attn(struct occ *occ)
 		}
 	}
 
- error:
-	/* Convert positive SBEI status */
-	if (rc > 0) {
-		dev_err(occ->dev, "SRAM attn returned failure status: %08x\n",
-			rc);
-		rc = -EBADMSG;
-	}
-
 	return rc;
 }
 
@@ -635,6 +605,10 @@ static int occ_probe(struct platform_device *pdev)
 	if (!occ)
 		return -ENOMEM;
 
+	occ->buffer = kvmalloc(OCC_MAX_RESP_WORDS * 4, GFP_KERNEL);
+	if (!occ->buffer)
+		return -ENOMEM;
+
 	occ->version = (enum versions)md;
 	occ->dev = dev;
 	occ->sbefifo = dev->parent;
@@ -670,6 +644,7 @@ static int occ_probe(struct platform_device *pdev)
 	if (rc) {
 		dev_err(dev, "failed to register miscdevice: %d\n", rc);
 		ida_simple_remove(&occ_ida, occ->idx);
+		kvfree(occ->buffer);
 		return rc;
 	}
 
@@ -685,6 +660,8 @@ static int occ_remove(struct platform_device *pdev)
 {
 	struct occ *occ = platform_get_drvdata(pdev);
 
+	kvfree(occ->buffer);
+
 	misc_deregister(&occ->mdev);
 
 	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
diff --git a/include/linux/fsi-occ.h b/include/linux/fsi-occ.h
index d4cdc2aa6e33..7ee3dbd7f4b3 100644
--- a/include/linux/fsi-occ.h
+++ b/include/linux/fsi-occ.h
@@ -19,6 +19,8 @@ struct device;
 #define OCC_RESP_CRIT_OCB		0xE3
 #define OCC_RESP_CRIT_HW		0xE4
 
+#define OCC_MAX_RESP_WORDS		2048
+
 int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
 		   void *response, size_t *resp_len);
 
-- 
2.27.0

Re: [PATCH v3 1/4] fsi: occ: Use a large buffer for responses

[Joel Stanley]

On Mon, 27 Sept 2021 at 15:59, Eddie James <eajames@linux.ibm.com> wrote:
>
> Allocate a large buffer for each OCC to handle response data. This
> removes memory allocation during an operation, and also allows for
> the maximum amount of SBE FFDC.

Why do we need this change? (is it fixing a bug, did the host change,
is it an unimplemented feature, etc)

>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/fsi/fsi-occ.c   | 109 ++++++++++++++++------------------------
>  include/linux/fsi-occ.h |   2 +
>  2 files changed, 45 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
> index b0c9322078a1..ace3ec7767e5 100644
> --- a/drivers/fsi/fsi-occ.c
> +++ b/drivers/fsi/fsi-occ.c
> @@ -10,6 +10,7 @@
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/miscdevice.h>
> +#include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/fsi-occ.h>
> @@ -42,13 +43,6 @@
>
>  #define OCC_P10_SRAM_MODE      0x58    /* Normal mode, OCB channel 2 */
>
> -/*
> - * Assume we don't have much FFDC, if we do we'll overflow and
> - * fail the command. This needs to be big enough for simple
> - * commands as well.
> - */
> -#define OCC_SBE_STATUS_WORDS   32
> -
>  #define OCC_TIMEOUT_MS         1000
>  #define OCC_CMD_IN_PRG_WAIT_MS 50
>
> @@ -60,6 +54,7 @@ struct occ {
>         char name[32];
>         int idx;
>         u8 sequence_number;
> +       void *buffer;
>         enum versions version;
>         struct miscdevice mdev;
>         struct mutex occ_lock;
> @@ -250,8 +245,10 @@ 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_len, resp_data_len;
> -       __be32 *resp, cmd[6];
> +       size_t cmd_len, resp_data_len;
> +       size_t resp_len = OCC_MAX_RESP_WORDS;
> +       __be32 *resp = occ->buffer;
> +       __be32 cmd[6];
>         int idx = 0, rc;
>
>         /*
> @@ -278,19 +275,19 @@ static int occ_getsram(struct occ *occ, u32 offset, void *data, ssize_t len)
>         cmd[1] = cpu_to_be32(SBEFIFO_CMD_GET_OCC_SRAM);
>         cmd[4 + idx] = cpu_to_be32(data_len);
>
> -       resp_len = (data_len >> 2) + OCC_SBE_STATUS_WORDS;
> -       resp = kzalloc(resp_len << 2, GFP_KERNEL);

Previously the driver would zero the buffer before using it. Should
you add a memset here?

> @@ -635,6 +605,10 @@ static int occ_probe(struct platform_device *pdev)
>         if (!occ)
>                 return -ENOMEM;
>
> +       occ->buffer = kvmalloc(OCC_MAX_RESP_WORDS * 4, GFP_KERNEL);

Why do you allocate WORDS * 4?

> diff --git a/include/linux/fsi-occ.h b/include/linux/fsi-occ.h
> index d4cdc2aa6e33..7ee3dbd7f4b3 100644
> --- a/include/linux/fsi-occ.h
> +++ b/include/linux/fsi-occ.h
> @@ -19,6 +19,8 @@ struct device;
>  #define OCC_RESP_CRIT_OCB              0xE3
>  #define OCC_RESP_CRIT_HW               0xE4
>
> +#define OCC_MAX_RESP_WORDS             2048

Does this need to go in the header?

Re: [PATCH v3 1/4] fsi: occ: Use a large buffer for responses

[Eddie James]

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:
>> Allocate a large buffer for each OCC to handle response data. This
>> removes memory allocation during an operation, and also allows for
>> the maximum amount of SBE FFDC.
> Why do we need this change? (is it fixing a bug, did the host change,
> is it an unimplemented feature, etc)


It allows for the maximum amount of SBE FFDC, so an unimplemented 
feature. Previously for the putsram and attn commands, only 32 words 
would have been available, and for getsram, only up to the size of the 
transfer. SBE FFDC might be up to 8Kb.


>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   drivers/fsi/fsi-occ.c   | 109 ++++++++++++++++------------------------
>>   include/linux/fsi-occ.h |   2 +
>>   2 files changed, 45 insertions(+), 66 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
>> index b0c9322078a1..ace3ec7767e5 100644
>> --- a/drivers/fsi/fsi-occ.c
>> +++ b/drivers/fsi/fsi-occ.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/list.h>
>>   #include <linux/miscdevice.h>
>> +#include <linux/mm.h>
>>   #include <linux/module.h>
>>   #include <linux/mutex.h>
>>   #include <linux/fsi-occ.h>
>> @@ -42,13 +43,6 @@
>>
>>   #define OCC_P10_SRAM_MODE      0x58    /* Normal mode, OCB channel 2 */
>>
>> -/*
>> - * Assume we don't have much FFDC, if we do we'll overflow and
>> - * fail the command. This needs to be big enough for simple
>> - * commands as well.
>> - */
>> -#define OCC_SBE_STATUS_WORDS   32
>> -
>>   #define OCC_TIMEOUT_MS         1000
>>   #define OCC_CMD_IN_PRG_WAIT_MS 50
>>
>> @@ -60,6 +54,7 @@ struct occ {
>>          char name[32];
>>          int idx;
>>          u8 sequence_number;
>> +       void *buffer;
>>          enum versions version;
>>          struct miscdevice mdev;
>>          struct mutex occ_lock;
>> @@ -250,8 +245,10 @@ 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_len, resp_data_len;
>> -       __be32 *resp, cmd[6];
>> +       size_t cmd_len, resp_data_len;
>> +       size_t resp_len = OCC_MAX_RESP_WORDS;
>> +       __be32 *resp = occ->buffer;
>> +       __be32 cmd[6];
>>          int idx = 0, rc;
>>
>>          /*
>> @@ -278,19 +275,19 @@ static int occ_getsram(struct occ *occ, u32 offset, void *data, ssize_t len)
>>          cmd[1] = cpu_to_be32(SBEFIFO_CMD_GET_OCC_SRAM);
>>          cmd[4 + idx] = cpu_to_be32(data_len);
>>
>> -       resp_len = (data_len >> 2) + OCC_SBE_STATUS_WORDS;
>> -       resp = kzalloc(resp_len << 2, GFP_KERNEL);
> Previously the driver would zero the buffer before using it. Should
> you add a memset here?


Based on the rest of the code, I don't see that it's necessary for it be 
initialized to 0.


>
>> @@ -635,6 +605,10 @@ static int occ_probe(struct platform_device *pdev)
>>          if (!occ)
>>                  return -ENOMEM;
>>
>> +       occ->buffer = kvmalloc(OCC_MAX_RESP_WORDS * 4, GFP_KERNEL);
> Why do you allocate WORDS * 4?


I suppose it's an assumption that words are 4 bytes, but the SBE 
operates that way. I will add a #define for it at least. I didn't want 
to define 8192 because the SBE expects the length in words, so I'd 
rather multiply in one place than divide in several places.


>
>> diff --git a/include/linux/fsi-occ.h b/include/linux/fsi-occ.h
>> index d4cdc2aa6e33..7ee3dbd7f4b3 100644
>> --- a/include/linux/fsi-occ.h
>> +++ b/include/linux/fsi-occ.h
>> @@ -19,6 +19,8 @@ struct device;
>>   #define OCC_RESP_CRIT_OCB              0xE3
>>   #define OCC_RESP_CRIT_HW               0xE4
>>
>> +#define OCC_MAX_RESP_WORDS             2048
> Does this need to go in the header?


Yes, the hwmon driver needs it.

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

[Eddie James]

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

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;
-- 
2.27.0

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

[Joel Stanley]

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;
> +}

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

[Eddie James]

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;
>> +}

[PATCH v3 3/4] docs: ABI: testing: Document the OCC hwmon FFDC binary interface

[Eddie James]

Add documentation for the new binary sysfs that will dump the SBEFIFO
FFDC.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 .../testing/sysfs-bus-platform-devices-occ-hwmon    | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-devices-occ-hwmon

diff --git a/Documentation/ABI/testing/sysfs-bus-platform-devices-occ-hwmon b/Documentation/ABI/testing/sysfs-bus-platform-devices-occ-hwmon
new file mode 100644
index 000000000000..b24d7ab0278f
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-platform-devices-occ-hwmon
@@ -0,0 +1,13 @@
+What:		/sys/bus/platform/devices/occ-hwmon.X/ffdc
+KernelVersion:	5.15
+Contact:	eajames@linux.ibm.com
+Description:
+		Contains the First Failure Data Capture from the SBEFIFO
+		hardware, if there is any from a previous transfer. Otherwise,
+		the file is empty. The data is cleared when it's been
+		completely read by a user. As the name suggests, only the data
+		from the first error is saved, until it's cleared upon read. The OCC hwmon driver, running on
+		a Baseboard Management Controller (BMC), communicates with
+		POWER9 and up processors over the Self-Boot Engine (SBE) FIFO.
+		In many error conditions, the SBEFIFO will return error data
+		indicating the type of error and system state, etc.
-- 
2.27.0

[PATCH v3 4/4] hwmon: (occ) Provide the SBEFIFO FFDC in binary sysfs

[Eddie James]

Save any FFDC provided by the OCC driver, and provide it to userspace
through a binary sysfs entry. Notify userspace pollers when there is an
error too.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
Changes since v1:
 - Remove "collected" error state in favor of a boolean
 - Clear the error flag once the FFDC has been completely read once
 - Only store FFDC if there is no FFDC waiting to be retrieved

 drivers/hwmon/occ/p9_sbe.c | 86 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index 9709f2b9c052..e50243580269 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -4,18 +4,79 @@
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/fsi-occ.h>
+#include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/platform_device.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
 
 #include "common.h"
 
 struct p9_sbe_occ {
 	struct occ occ;
+	bool sbe_error;
+	void *ffdc;
+	size_t ffdc_len;
+	size_t ffdc_size;
+	struct mutex sbe_error_lock;	/* lock access to ffdc data */
 	struct device *sbe;
 };
 
 #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
 
+static ssize_t ffdc_read(struct file *filp, struct kobject *kobj,
+			 struct bin_attribute *battr, char *buf, loff_t pos,
+			 size_t count)
+{
+	ssize_t rc = 0;
+	struct occ *occ = dev_get_drvdata(kobj_to_dev(kobj));
+	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
+
+	mutex_lock(&ctx->sbe_error_lock);
+	if (ctx->sbe_error) {
+		rc = memory_read_from_buffer(buf, count, &pos, ctx->ffdc,
+					     ctx->ffdc_len);
+		if (pos >= ctx->ffdc_len)
+			ctx->sbe_error = false;
+	}
+	mutex_unlock(&ctx->sbe_error_lock);
+
+	return rc;
+}
+static BIN_ATTR_RO(ffdc, OCC_MAX_RESP_WORDS * 4);
+
+static bool p9_sbe_occ_save_ffdc(struct p9_sbe_occ *ctx, const void *resp,
+				 size_t resp_len)
+{
+	bool notify = false;
+
+	mutex_lock(&ctx->sbe_error_lock);
+	if (!ctx->sbe_error) {
+		if (resp_len > ctx->ffdc_size) {
+			if (ctx->ffdc)
+				kvfree(ctx->ffdc);
+			ctx->ffdc = kvmalloc(resp_len, GFP_KERNEL);
+			if (!ctx->ffdc) {
+				ctx->ffdc_len = 0;
+				ctx->ffdc_size = 0;
+				goto done;
+			}
+
+			ctx->ffdc_size = resp_len;
+		}
+
+		notify = true;
+		ctx->sbe_error = true;
+		ctx->ffdc_len = resp_len;
+		memcpy(ctx->ffdc, resp, resp_len);
+	}
+
+done:
+	mutex_unlock(&ctx->sbe_error_lock);
+	return notify;
+}
+
 static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
 {
 	struct occ_response *resp = &occ->resp;
@@ -24,8 +85,15 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
 	int rc;
 
 	rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
-	if (rc < 0)
+	if (rc < 0) {
+		if (resp_len) {
+			if (p9_sbe_occ_save_ffdc(ctx, resp, resp_len))
+				sysfs_notify(&occ->bus_dev->kobj, NULL,
+					     bin_attr_ffdc.attr.name);
+		}
+
 		return rc;
+	}
 
 	switch (resp->return_status) {
 	case OCC_RESP_CMD_IN_PRG:
@@ -65,6 +133,8 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
 	if (!ctx)
 		return -ENOMEM;
 
+	mutex_init(&ctx->sbe_error_lock);
+
 	ctx->sbe = pdev->dev.parent;
 	occ = &ctx->occ;
 	occ->bus_dev = &pdev->dev;
@@ -78,6 +148,15 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
 	if (rc == -ESHUTDOWN)
 		rc = -ENODEV;	/* Host is shutdown, don't spew errors */
 
+	if (!rc) {
+		rc = device_create_bin_file(occ->bus_dev, &bin_attr_ffdc);
+		if (rc) {
+			dev_warn(occ->bus_dev,
+				 "failed to create SBE error ffdc file\n");
+			rc = 0;
+		}
+	}
+
 	return rc;
 }
 
@@ -86,9 +165,14 @@ static int p9_sbe_occ_remove(struct platform_device *pdev)
 	struct occ *occ = platform_get_drvdata(pdev);
 	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
 
+	device_remove_bin_file(occ->bus_dev, &bin_attr_ffdc);
+
 	ctx->sbe = NULL;
 	occ_shutdown(occ);
 
+	if (ctx->ffdc)
+		kvfree(ctx->ffdc);
+
 	return 0;
 }
 
-- 
2.27.0

Re: [PATCH v3 4/4] hwmon: (occ) Provide the SBEFIFO FFDC in binary sysfs

[Guenter Roeck]

On Mon, Sep 27, 2021 at 10:59:25AM -0500, Eddie James wrote:
> Save any FFDC provided by the OCC driver, and provide it to userspace
> through a binary sysfs entry. Notify userspace pollers when there is an
> error too.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

For my reference (waiting for infra patches to be accepted/acked):

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
> Changes since v1:
>  - Remove "collected" error state in favor of a boolean
>  - Clear the error flag once the FFDC has been completely read once
>  - Only store FFDC if there is no FFDC waiting to be retrieved
> 
>  drivers/hwmon/occ/p9_sbe.c | 86 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index 9709f2b9c052..e50243580269 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -4,18 +4,79 @@
>  #include <linux/device.h>
>  #include <linux/errno.h>
>  #include <linux/fsi-occ.h>
> +#include <linux/mm.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/platform_device.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
>  
>  #include "common.h"
>  
>  struct p9_sbe_occ {
>  	struct occ occ;
> +	bool sbe_error;
> +	void *ffdc;
> +	size_t ffdc_len;
> +	size_t ffdc_size;
> +	struct mutex sbe_error_lock;	/* lock access to ffdc data */
>  	struct device *sbe;
>  };
>  
>  #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
>  
> +static ssize_t ffdc_read(struct file *filp, struct kobject *kobj,
> +			 struct bin_attribute *battr, char *buf, loff_t pos,
> +			 size_t count)
> +{
> +	ssize_t rc = 0;
> +	struct occ *occ = dev_get_drvdata(kobj_to_dev(kobj));
> +	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
> +
> +	mutex_lock(&ctx->sbe_error_lock);
> +	if (ctx->sbe_error) {
> +		rc = memory_read_from_buffer(buf, count, &pos, ctx->ffdc,
> +					     ctx->ffdc_len);
> +		if (pos >= ctx->ffdc_len)
> +			ctx->sbe_error = false;
> +	}
> +	mutex_unlock(&ctx->sbe_error_lock);
> +
> +	return rc;
> +}
> +static BIN_ATTR_RO(ffdc, OCC_MAX_RESP_WORDS * 4);
> +
> +static bool p9_sbe_occ_save_ffdc(struct p9_sbe_occ *ctx, const void *resp,
> +				 size_t resp_len)
> +{
> +	bool notify = false;
> +
> +	mutex_lock(&ctx->sbe_error_lock);
> +	if (!ctx->sbe_error) {
> +		if (resp_len > ctx->ffdc_size) {
> +			if (ctx->ffdc)
> +				kvfree(ctx->ffdc);
> +			ctx->ffdc = kvmalloc(resp_len, GFP_KERNEL);
> +			if (!ctx->ffdc) {
> +				ctx->ffdc_len = 0;
> +				ctx->ffdc_size = 0;
> +				goto done;
> +			}
> +
> +			ctx->ffdc_size = resp_len;
> +		}
> +
> +		notify = true;
> +		ctx->sbe_error = true;
> +		ctx->ffdc_len = resp_len;
> +		memcpy(ctx->ffdc, resp, resp_len);
> +	}
> +
> +done:
> +	mutex_unlock(&ctx->sbe_error_lock);
> +	return notify;
> +}
> +
>  static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
>  {
>  	struct occ_response *resp = &occ->resp;
> @@ -24,8 +85,15 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
>  	int rc;
>  
>  	rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
> -	if (rc < 0)
> +	if (rc < 0) {
> +		if (resp_len) {
> +			if (p9_sbe_occ_save_ffdc(ctx, resp, resp_len))
> +				sysfs_notify(&occ->bus_dev->kobj, NULL,
> +					     bin_attr_ffdc.attr.name);
> +		}
> +
>  		return rc;
> +	}
>  
>  	switch (resp->return_status) {
>  	case OCC_RESP_CMD_IN_PRG:
> @@ -65,6 +133,8 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
>  	if (!ctx)
>  		return -ENOMEM;
>  
> +	mutex_init(&ctx->sbe_error_lock);
> +
>  	ctx->sbe = pdev->dev.parent;
>  	occ = &ctx->occ;
>  	occ->bus_dev = &pdev->dev;
> @@ -78,6 +148,15 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
>  	if (rc == -ESHUTDOWN)
>  		rc = -ENODEV;	/* Host is shutdown, don't spew errors */
>  
> +	if (!rc) {
> +		rc = device_create_bin_file(occ->bus_dev, &bin_attr_ffdc);
> +		if (rc) {
> +			dev_warn(occ->bus_dev,
> +				 "failed to create SBE error ffdc file\n");
> +			rc = 0;
> +		}
> +	}
> +
>  	return rc;
>  }
>  
> @@ -86,9 +165,14 @@ static int p9_sbe_occ_remove(struct platform_device *pdev)
>  	struct occ *occ = platform_get_drvdata(pdev);
>  	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
>  
> +	device_remove_bin_file(occ->bus_dev, &bin_attr_ffdc);
> +
>  	ctx->sbe = NULL;
>  	occ_shutdown(occ);
>  
> +	if (ctx->ffdc)
> +		kvfree(ctx->ffdc);
> +
>  	return 0;
>  }
>