diff mbox series

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

Message ID 20210927155925.15485-2-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 
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(-)

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:
>
> 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?
Eddie James Oct. 19, 2021, 8:22 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:
>> 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.
diff mbox series

Patch

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