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

[PATCH v4 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 v3:
 - Rebase
 - Add a check for valid FFDC length
 - Add comments about SBE words being four bytes

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                         | 164 +++++++++---------
 drivers/hwmon/occ/p9_sbe.c                    |  86 ++++++++-
 include/linux/fsi-occ.h                       |   2 +
 4 files changed, 186 insertions(+), 79 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-devices-occ-hwmon

-- 
2.27.0

[PATCH v4 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. Previously the driver would have been
limited to 32 words of FFDC in the responses to PutSRAM and ATTN
commands.

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

diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
index ecf738411fe2..b4302776026d 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>
@@ -33,13 +34,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
 
@@ -51,6 +45,7 @@ struct occ {
 	char name[32];
 	int idx;
 	u8 sequence_number;
+	void *buffer;
 	enum versions version;
 	struct miscdevice mdev;
 	struct mutex occ_lock;
@@ -241,8 +236,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;
 
 	/*
@@ -269,19 +266,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) {
@@ -292,39 +289,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
@@ -361,12 +340,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",
@@ -382,27 +366,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:
@@ -427,12 +400,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",
@@ -448,14 +426,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;
 }
 
@@ -590,6 +560,11 @@ static int occ_probe(struct platform_device *pdev)
 	if (!occ)
 		return -ENOMEM;
 
+	/* SBE words are always four bytes */
+	occ->buffer = kvmalloc(OCC_MAX_RESP_WORDS * 4, GFP_KERNEL);
+	if (!occ->buffer)
+		return -ENOMEM;
+
 	occ->version = (uintptr_t)of_device_get_match_data(dev);
 	occ->dev = dev;
 	occ->sbefifo = dev->parent;
@@ -625,6 +600,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;
 	}
 
@@ -640,6 +616,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

[PATCH v4 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>
---
 drivers/fsi/fsi-occ.c | 66 +++++++++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
index b4302776026d..7eaab1be0aa4 100644
--- a/drivers/fsi/fsi-occ.c
+++ b/drivers/fsi/fsi-occ.c
@@ -46,6 +46,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;
@@ -208,6 +211,22 @@ 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)
+{
+	if (resp_len > parsed_len) {
+		size_t dh = resp_len - parsed_len;
+		size_t ffdc_len = (dh - 1) * 4; /* SBE words are four bytes */
+		__be32 *ffdc = &resp[parsed_len];
+
+		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)
 {
@@ -236,7 +255,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];
@@ -271,16 +290,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);
@@ -296,7 +316,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;
@@ -343,18 +363,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]);
@@ -372,7 +393,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;
 
@@ -403,18 +424,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]);
@@ -437,6 +459,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;
@@ -445,11 +468,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;
 	}
 
@@ -459,6 +484,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
@@ -509,7 +538,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;
 	}
@@ -525,10 +554,11 @@ 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;
 	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

[PATCH v4 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 v4 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>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 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 v4 4/4] hwmon: (occ) Provide the SBEFIFO FFDC in binary sysfs

[Joel Stanley]

On Tue, 19 Oct 2021 at 20:53, Eddie James <eajames@linux.ibm.com> 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>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Guenter, I will merge this through the fsi tree unless you have objections.

Cheers,

Joel

> ---
>  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 v4 4/4] hwmon: (occ) Provide the SBEFIFO FFDC in binary sysfs

[Guenter Roeck]

On 10/21/21 4:09 PM, Joel Stanley wrote:
> On Tue, 19 Oct 2021 at 20:53, Eddie James <eajames@linux.ibm.com> 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>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> Guenter, I will merge this through the fsi tree unless you have objections.
> 

Go ahead.

Guenter

> Cheers,
> 
> Joel
> 
>> ---
>>   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 v4 0/4] occ: fsi and hwmon: Extract and provide the SBEFIFO FFDC

[Joel Stanley]

On Tue, 19 Oct 2021 at 20:53, Eddie James <eajames@linux.ibm.com> wrote:
>
> 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.

Thanks, I've merged these. I took the chance to add some of your
responses to the commit messages as they were useful.

>
> Changes since v3:
>  - Rebase
>  - Add a check for valid FFDC length
>  - Add comments about SBE words being four bytes
>
> 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                         | 164 +++++++++---------
>  drivers/hwmon/occ/p9_sbe.c                    |  86 ++++++++-
>  include/linux/fsi-occ.h                       |   2 +
>  4 files changed, 186 insertions(+), 79 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-devices-occ-hwmon
>
> --
> 2.27.0
>