[PATCH 0/3] OCC: fsi and hwmon: Set sequence number in submit interface

[PATCH 0/3] OCC: fsi and hwmon: Set sequence number in submit interface

[Eddie James]

Conflicting sequence numbers have resulted in users of the OCC interface
getting the wrong response. For example, both the hwmon driver and an
application might send a transfer near the same time with the same sequence
number, and then one or both will get an incorrect respnse, but cannot tell
because the sequence number looks correct.
Perform the sequence numbering in the submit interface to make sure each
transfer has a unique sequence number. This also requires that the submit
interface perform the checksum calculation for the command. Adjust the hwmon
driver accordingly too.

Eddie James (3):
  fsi: occ: Force sequence numbering per OCC
  hwmon: (occ) Remove sequence numbering and checksum calculation
  fsi: occ: Add dynamic debug to dump command and response

 drivers/fsi/fsi-occ.c      | 98 +++++++++++++++++++++++++++++++-------
 drivers/hwmon/occ/common.c | 30 +++++-------
 drivers/hwmon/occ/common.h |  3 +-
 drivers/hwmon/occ/p8_i2c.c | 15 +++---
 drivers/hwmon/occ/p9_sbe.c |  4 +-
 5 files changed, 105 insertions(+), 45 deletions(-)

-- 
2.27.0

[PATCH 1/3] fsi: occ: Force sequence numbering per OCC

[Eddie James]

Set and increment the sequence number during the submit operation.
This prevents sequence number conflicts between different users of
the interface. A sequence number conflict may result in a user
getting an OCC response meant for a different command. Since the
sequence number is now modified, the checksum must be calculated and
set before submitting the command.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/fsi/fsi-occ.c | 54 +++++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 17 deletions(-)

diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
index b223f0ef337b..ecf738411fe2 100644
--- a/drivers/fsi/fsi-occ.c
+++ b/drivers/fsi/fsi-occ.c
@@ -50,6 +50,7 @@ struct occ {
 	struct device *sbefifo;
 	char name[32];
 	int idx;
+	u8 sequence_number;
 	enum versions version;
 	struct miscdevice mdev;
 	struct mutex occ_lock;
@@ -141,8 +142,7 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
 {
 	struct occ_client *client = file->private_data;
 	size_t rlen, data_length;
-	u16 checksum = 0;
-	ssize_t rc, i;
+	ssize_t rc;
 	u8 *cmd;
 
 	if (!client)
@@ -156,9 +156,6 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
 	/* Construct the command */
 	cmd = client->buffer;
 
-	/* Sequence number (we could increment and compare with response) */
-	cmd[0] = 1;
-
 	/*
 	 * Copy the user command (assume user data follows the occ command
 	 * format)
@@ -178,14 +175,7 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
 		goto done;
 	}
 
-	/* Calculate checksum */
-	for (i = 0; i < data_length + 4; ++i)
-		checksum += cmd[i];
-
-	cmd[data_length + 4] = checksum >> 8;
-	cmd[data_length + 5] = checksum & 0xFF;
-
-	/* Submit command */
+	/* Submit command; 4 bytes before the data and 2 bytes after */
 	rlen = PAGE_SIZE;
 	rc = fsi_occ_submit(client->occ->dev, cmd, data_length + 6, cmd,
 			    &rlen);
@@ -314,11 +304,13 @@ static int occ_getsram(struct occ *occ, u32 offset, void *data, ssize_t len)
 	return rc;
 }
 
-static int occ_putsram(struct occ *occ, const void *data, ssize_t len)
+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;
+	u8 *byte_buf;
 	int idx = 0, rc;
 
 	cmd_len = (occ->version == occ_p10) ? 6 : 5;
@@ -358,6 +350,15 @@ static int occ_putsram(struct occ *occ, const void *data, ssize_t len)
 	buf[4 + idx] = cpu_to_be32(data_len);
 	memcpy(&buf[5 + idx], data, len);
 
+	byte_buf = (u8 *)&buf[5 + idx];
+	/*
+	 * Overwrite the first byte with our sequence number and the last two
+	 * bytes with the checksum.
+	 */
+	byte_buf[0] = seq_no;
+	byte_buf[len - 2] = checksum >> 8;
+	byte_buf[len - 1] = checksum & 0xff;
+
 	rc = sbefifo_submit(occ->sbefifo, buf, cmd_len, buf, &resp_len);
 	if (rc)
 		goto free;
@@ -467,9 +468,12 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
 	struct occ *occ = dev_get_drvdata(dev);
 	struct occ_response *resp = response;
 	u8 seq_no;
+	u16 checksum = 0;
 	u16 resp_data_length;
+	const u8 *byte_request = (const u8 *)request;
 	unsigned long start;
 	int rc;
+	size_t i;
 
 	if (!occ)
 		return -ENODEV;
@@ -479,11 +483,26 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
 		return -EINVAL;
 	}
 
+	/* Checksum the request, ignoring first byte (sequence number). */
+	for (i = 1; i < req_len - 2; ++i)
+		checksum += byte_request[i];
+
 	mutex_lock(&occ->occ_lock);
 
-	/* Extract the seq_no from the command (first byte) */
-	seq_no = *(const u8 *)request;
-	rc = occ_putsram(occ, request, req_len);
+	/*
+	 * Get a sequence number and update the counter. Avoid a sequence
+	 * number of 0 which would pass the response check below even if the
+	 * OCC response is uninitialized. Any sequence number the user is
+	 * trying to send is overwritten since this function is the only common
+	 * interface to the OCC and therefore the only place we can guarantee
+	 * unique sequence numbers.
+	 */
+	seq_no = occ->sequence_number++;
+	if (!occ->sequence_number)
+		occ->sequence_number = 1;
+	checksum += seq_no;
+
+	rc = occ_putsram(occ, request, req_len, seq_no, checksum);
 	if (rc)
 		goto done;
 
@@ -574,6 +593,7 @@ static int occ_probe(struct platform_device *pdev)
 	occ->version = (uintptr_t)of_device_get_match_data(dev);
 	occ->dev = dev;
 	occ->sbefifo = dev->parent;
+	occ->sequence_number = 1;
 	mutex_init(&occ->occ_lock);
 
 	if (dev->of_node) {
-- 
2.27.0

[PATCH 2/3] hwmon: (occ) Remove sequence numbering and checksum calculation

[Eddie James]

Checksumming of the request and sequence numbering is now done in the
OCC interface driver in order to keep unique sequence numbers. So
remove those in the hwmon driver.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/hwmon/occ/common.c | 30 ++++++++++++------------------
 drivers/hwmon/occ/common.h |  3 +--
 drivers/hwmon/occ/p8_i2c.c | 15 +++++++++------
 drivers/hwmon/occ/p9_sbe.c |  4 ++--
 4 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 0d68a78be980..fc298268c89e 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -132,22 +132,20 @@ struct extended_sensor {
 static int occ_poll(struct occ *occ)
 {
 	int rc;
-	u16 checksum = occ->poll_cmd_data + occ->seq_no + 1;
-	u8 cmd[8];
+	u8 cmd[7];
 	struct occ_poll_response_header *header;
 
 	/* big endian */
-	cmd[0] = occ->seq_no++;		/* sequence number */
+	cmd[0] = 0;			/* sequence number */
 	cmd[1] = 0;			/* cmd type */
 	cmd[2] = 0;			/* data length msb */
 	cmd[3] = 1;			/* data length lsb */
 	cmd[4] = occ->poll_cmd_data;	/* data */
-	cmd[5] = checksum >> 8;		/* checksum msb */
-	cmd[6] = checksum & 0xFF;	/* checksum lsb */
-	cmd[7] = 0;
+	cmd[5] = 0;			/* checksum msb */
+	cmd[6] = 0;			/* checksum lsb */
 
 	/* mutex should already be locked if necessary */
-	rc = occ->send_cmd(occ, cmd);
+	rc = occ->send_cmd(occ, cmd, sizeof(cmd));
 	if (rc) {
 		occ->last_error = rc;
 		if (occ->error_count++ > OCC_ERROR_COUNT_THRESHOLD)
@@ -184,25 +182,23 @@ static int occ_set_user_power_cap(struct occ *occ, u16 user_power_cap)
 {
 	int rc;
 	u8 cmd[8];
-	u16 checksum = 0x24;
 	__be16 user_power_cap_be = cpu_to_be16(user_power_cap);
 
-	cmd[0] = 0;
-	cmd[1] = 0x22;
-	cmd[2] = 0;
-	cmd[3] = 2;
+	cmd[0] = 0;	/* sequence number */
+	cmd[1] = 0x22;	/* cmd type */
+	cmd[2] = 0;	/* data length msb */
+	cmd[3] = 2;	/* data length lsb */
 
 	memcpy(&cmd[4], &user_power_cap_be, 2);
 
-	checksum += cmd[4] + cmd[5];
-	cmd[6] = checksum >> 8;
-	cmd[7] = checksum & 0xFF;
+	cmd[6] = 0;	/* checksum msb */
+	cmd[7] = 0;	/* checksum lsb */
 
 	rc = mutex_lock_interruptible(&occ->lock);
 	if (rc)
 		return rc;
 
-	rc = occ->send_cmd(occ, cmd);
+	rc = occ->send_cmd(occ, cmd, sizeof(cmd));
 
 	mutex_unlock(&occ->lock);
 
@@ -1151,8 +1147,6 @@ int occ_setup(struct occ *occ, const char *name)
 {
 	int rc;
 
-	/* start with 1 to avoid false match with zero-initialized SRAM buffer */
-	occ->seq_no = 1;
 	mutex_init(&occ->lock);
 	occ->groups[0] = &occ->group;
 
diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
index e6df719770e8..5020117be740 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -95,9 +95,8 @@ struct occ {
 	struct occ_sensors sensors;
 
 	int powr_sample_time_us;	/* average power sample time */
-	u8 seq_no;
 	u8 poll_cmd_data;		/* to perform OCC poll command */
-	int (*send_cmd)(struct occ *occ, u8 *cmd);
+	int (*send_cmd)(struct occ *occ, u8 *cmd, size_t len);
 
 	unsigned long next_update;
 	struct mutex lock;		/* lock OCC access */
diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c
index 0cf8588be35a..22af189eafa6 100644
--- a/drivers/hwmon/occ/p8_i2c.c
+++ b/drivers/hwmon/occ/p8_i2c.c
@@ -97,18 +97,21 @@ static int p8_i2c_occ_putscom_u32(struct i2c_client *client, u32 address,
 }
 
 static int p8_i2c_occ_putscom_be(struct i2c_client *client, u32 address,
-				 u8 *data)
+				 u8 *data, size_t len)
 {
-	__be32 data0, data1;
+	__be32 data0 = 0, data1 = 0;
 
-	memcpy(&data0, data, 4);
-	memcpy(&data1, data + 4, 4);
+	memcpy(&data0, data, min(len, 4UL));
+	if (len > 4UL) {
+		len -= 4;
+		memcpy(&data1, data + 4, min(len, 4UL));
+	}
 
 	return p8_i2c_occ_putscom_u32(client, address, be32_to_cpu(data0),
 				      be32_to_cpu(data1));
 }
 
-static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd)
+static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
 {
 	int i, rc;
 	unsigned long start;
@@ -127,7 +130,7 @@ static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd)
 		return rc;
 
 	/* write command (expected to already be BE), we need bus-endian... */
-	rc = p8_i2c_occ_putscom_be(client, OCB_DATA3, cmd);
+	rc = p8_i2c_occ_putscom_be(client, OCB_DATA3, cmd, len);
 	if (rc)
 		return rc;
 
diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index f6387cc0b754..9709f2b9c052 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -16,14 +16,14 @@ struct p9_sbe_occ {
 
 #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
 
-static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
+static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
 {
 	struct occ_response *resp = &occ->resp;
 	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
 	size_t resp_len = sizeof(*resp);
 	int rc;
 
-	rc = fsi_occ_submit(ctx->sbe, cmd, 8, resp, &resp_len);
+	rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
 	if (rc < 0)
 		return rc;
 
-- 
2.27.0

[PATCH 3/3] fsi: occ: Add dynamic debug to dump command and response

[Eddie James]

Use the dynamic branching capability of the dynamic debug subsystem
to dump the command and response with the correct OCC device name.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/fsi/fsi-occ.c | 44 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
index ecf738411fe2..641a6869b9df 100644
--- a/drivers/fsi/fsi-occ.c
+++ b/drivers/fsi/fsi-occ.c
@@ -21,6 +21,15 @@
 #include <linux/uaccess.h>
 #include <asm/unaligned.h>
 
+#if !defined(CONFIG_DYNAMIC_DEBUG_CORE)
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)
+#if defined(DEBUG)
+#define DYNAMIC_DEBUG_BRANCH(descriptor) true
+#else /* DEBUG */
+#define DYNAMIC_DEBUG_BRANCH(descriptor) false
+#endif /* DEBUG */
+#endif /* CONFIG_DYNAMIC_DEBUG_CORE */
+
 #define OCC_SRAM_BYTES		4096
 #define OCC_CMD_DATA_BYTES	4090
 #define OCC_RESP_DATA_BYTES	4089
@@ -359,6 +368,20 @@ static int occ_putsram(struct occ *occ, const void *data, ssize_t len,
 	byte_buf[len - 2] = checksum >> 8;
 	byte_buf[len - 1] = checksum & 0xff;
 
+	{
+		DEFINE_DYNAMIC_DEBUG_METADATA(ddm_occ_cmd, "OCC command");
+
+		if (DYNAMIC_DEBUG_BRANCH(ddm_occ_cmd)) {
+			char prefix[64];
+
+			snprintf(prefix, sizeof(prefix), "%s %s: cmd ",
+				 dev_driver_string(occ->dev),
+				 dev_name(occ->dev));
+			print_hex_dump(KERN_DEBUG, prefix, DUMP_PREFIX_OFFSET,
+				       16, 4, byte_buf, len, false);
+		}
+	}
+
 	rc = sbefifo_submit(occ->sbefifo, buf, cmd_len, buf, &resp_len);
 	if (rc)
 		goto free;
@@ -556,6 +579,27 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
 	}
 
 	*resp_len = resp_data_length + 7;
+
+	{
+		DEFINE_DYNAMIC_DEBUG_METADATA(ddm_occ_rsp,
+					      "OCC response");
+		DEFINE_DYNAMIC_DEBUG_METADATA(ddm_occ_full_rsp,
+					      "OCC full response");
+
+		if (DYNAMIC_DEBUG_BRANCH(ddm_occ_full_rsp) ||
+		    DYNAMIC_DEBUG_BRANCH(ddm_occ_rsp)) {
+			char prefix[64];
+			size_t l = DYNAMIC_DEBUG_BRANCH(ddm_occ_full_rsp) ?
+				*resp_len : 16;
+
+			snprintf(prefix, sizeof(prefix), "%s %s: rsp ",
+				 dev_driver_string(occ->dev),
+				 dev_name(occ->dev));
+			print_hex_dump(KERN_DEBUG, prefix, DUMP_PREFIX_OFFSET,
+				       16, 4, resp, l, false);
+		}
+	}
+
 	rc = occ_verify_checksum(occ, resp, resp_data_length);
 
  done:
-- 
2.27.0

Re: [PATCH 2/3] hwmon: (occ) Remove sequence numbering and checksum calculation

[Guenter Roeck]

On Fri, Jul 16, 2021 at 10:18:49AM -0500, Eddie James wrote:
> Checksumming of the request and sequence numbering is now done in the
> OCC interface driver in order to keep unique sequence numbers. So
> remove those in the hwmon driver.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

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

> ---
>  drivers/hwmon/occ/common.c | 30 ++++++++++++------------------
>  drivers/hwmon/occ/common.h |  3 +--
>  drivers/hwmon/occ/p8_i2c.c | 15 +++++++++------
>  drivers/hwmon/occ/p9_sbe.c |  4 ++--
>  4 files changed, 24 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> index 0d68a78be980..fc298268c89e 100644
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -132,22 +132,20 @@ struct extended_sensor {
>  static int occ_poll(struct occ *occ)
>  {
>  	int rc;
> -	u16 checksum = occ->poll_cmd_data + occ->seq_no + 1;
> -	u8 cmd[8];
> +	u8 cmd[7];
>  	struct occ_poll_response_header *header;
>  
>  	/* big endian */
> -	cmd[0] = occ->seq_no++;		/* sequence number */
> +	cmd[0] = 0;			/* sequence number */
>  	cmd[1] = 0;			/* cmd type */
>  	cmd[2] = 0;			/* data length msb */
>  	cmd[3] = 1;			/* data length lsb */
>  	cmd[4] = occ->poll_cmd_data;	/* data */
> -	cmd[5] = checksum >> 8;		/* checksum msb */
> -	cmd[6] = checksum & 0xFF;	/* checksum lsb */
> -	cmd[7] = 0;
> +	cmd[5] = 0;			/* checksum msb */
> +	cmd[6] = 0;			/* checksum lsb */
>  
>  	/* mutex should already be locked if necessary */
> -	rc = occ->send_cmd(occ, cmd);
> +	rc = occ->send_cmd(occ, cmd, sizeof(cmd));
>  	if (rc) {
>  		occ->last_error = rc;
>  		if (occ->error_count++ > OCC_ERROR_COUNT_THRESHOLD)
> @@ -184,25 +182,23 @@ static int occ_set_user_power_cap(struct occ *occ, u16 user_power_cap)
>  {
>  	int rc;
>  	u8 cmd[8];
> -	u16 checksum = 0x24;
>  	__be16 user_power_cap_be = cpu_to_be16(user_power_cap);
>  
> -	cmd[0] = 0;
> -	cmd[1] = 0x22;
> -	cmd[2] = 0;
> -	cmd[3] = 2;
> +	cmd[0] = 0;	/* sequence number */
> +	cmd[1] = 0x22;	/* cmd type */
> +	cmd[2] = 0;	/* data length msb */
> +	cmd[3] = 2;	/* data length lsb */
>  
>  	memcpy(&cmd[4], &user_power_cap_be, 2);
>  
> -	checksum += cmd[4] + cmd[5];
> -	cmd[6] = checksum >> 8;
> -	cmd[7] = checksum & 0xFF;
> +	cmd[6] = 0;	/* checksum msb */
> +	cmd[7] = 0;	/* checksum lsb */
>  
>  	rc = mutex_lock_interruptible(&occ->lock);
>  	if (rc)
>  		return rc;
>  
> -	rc = occ->send_cmd(occ, cmd);
> +	rc = occ->send_cmd(occ, cmd, sizeof(cmd));
>  
>  	mutex_unlock(&occ->lock);
>  
> @@ -1151,8 +1147,6 @@ int occ_setup(struct occ *occ, const char *name)
>  {
>  	int rc;
>  
> -	/* start with 1 to avoid false match with zero-initialized SRAM buffer */
> -	occ->seq_no = 1;
>  	mutex_init(&occ->lock);
>  	occ->groups[0] = &occ->group;
>  
> diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
> index e6df719770e8..5020117be740 100644
> --- a/drivers/hwmon/occ/common.h
> +++ b/drivers/hwmon/occ/common.h
> @@ -95,9 +95,8 @@ struct occ {
>  	struct occ_sensors sensors;
>  
>  	int powr_sample_time_us;	/* average power sample time */
> -	u8 seq_no;
>  	u8 poll_cmd_data;		/* to perform OCC poll command */
> -	int (*send_cmd)(struct occ *occ, u8 *cmd);
> +	int (*send_cmd)(struct occ *occ, u8 *cmd, size_t len);
>  
>  	unsigned long next_update;
>  	struct mutex lock;		/* lock OCC access */
> diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c
> index 0cf8588be35a..22af189eafa6 100644
> --- a/drivers/hwmon/occ/p8_i2c.c
> +++ b/drivers/hwmon/occ/p8_i2c.c
> @@ -97,18 +97,21 @@ static int p8_i2c_occ_putscom_u32(struct i2c_client *client, u32 address,
>  }
>  
>  static int p8_i2c_occ_putscom_be(struct i2c_client *client, u32 address,
> -				 u8 *data)
> +				 u8 *data, size_t len)
>  {
> -	__be32 data0, data1;
> +	__be32 data0 = 0, data1 = 0;
>  
> -	memcpy(&data0, data, 4);
> -	memcpy(&data1, data + 4, 4);
> +	memcpy(&data0, data, min(len, 4UL));
> +	if (len > 4UL) {
> +		len -= 4;
> +		memcpy(&data1, data + 4, min(len, 4UL));
> +	}
>  
>  	return p8_i2c_occ_putscom_u32(client, address, be32_to_cpu(data0),
>  				      be32_to_cpu(data1));
>  }
>  
> -static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd)
> +static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
>  {
>  	int i, rc;
>  	unsigned long start;
> @@ -127,7 +130,7 @@ static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd)
>  		return rc;
>  
>  	/* write command (expected to already be BE), we need bus-endian... */
> -	rc = p8_i2c_occ_putscom_be(client, OCB_DATA3, cmd);
> +	rc = p8_i2c_occ_putscom_be(client, OCB_DATA3, cmd, len);
>  	if (rc)
>  		return rc;
>  
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index f6387cc0b754..9709f2b9c052 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -16,14 +16,14 @@ struct p9_sbe_occ {
>  
>  #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
>  
> -static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
> +static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
>  {
>  	struct occ_response *resp = &occ->resp;
>  	struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
>  	size_t resp_len = sizeof(*resp);
>  	int rc;
>  
> -	rc = fsi_occ_submit(ctx->sbe, cmd, 8, resp, &resp_len);
> +	rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
>  	if (rc < 0)
>  		return rc;
>  

Re: [PATCH 2/3] hwmon: (occ) Remove sequence numbering and checksum calculation

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4362 bytes --]

Hi Eddie,

I love your patch! Perhaps something to improve:

[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on linux/master linus/master v5.14-rc1 next-20210716]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Eddie-James/OCC-fsi-and-hwmon-Set-sequence-number-in-submit-interface/20210718-103535
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: arm-randconfig-r033-20210718 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 5d5b08761f944d5b9822d582378333cc4b36a0a7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/5e8ecc23325ef0310d83a4520071aae18418f88a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eddie-James/OCC-fsi-and-hwmon-Set-sequence-number-in-submit-interface/20210718-103535
        git checkout 5e8ecc23325ef0310d83a4520071aae18418f88a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/hwmon/occ/p8_i2c.c:104:23: warning: comparison of distinct pointer types ('typeof (len) *' (aka 'unsigned int *') and 'typeof (4UL) *' (aka 'unsigned long *')) [-Wcompare-distinct-pointer-types]
           memcpy(&data0, data, min(len, 4UL));
                                ^~~~~~~~~~~~~
   include/linux/minmax.h:45:19: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                           ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
           __builtin_choose_expr(__safe_cmp(x, y), \
                                 ^~~~~~~~~~~~~~~~
   include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
                   (__typecheck(x, y) && __no_side_effects(x, y))
                    ^~~~~~~~~~~~~~~~~
   include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
           (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
   drivers/hwmon/occ/p8_i2c.c:107:28: warning: comparison of distinct pointer types ('typeof (len) *' (aka 'unsigned int *') and 'typeof (4UL) *' (aka 'unsigned long *')) [-Wcompare-distinct-pointer-types]
                   memcpy(&data1, data + 4, min(len, 4UL));
                                            ^~~~~~~~~~~~~
   include/linux/minmax.h:45:19: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                           ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
           __builtin_choose_expr(__safe_cmp(x, y), \
                                 ^~~~~~~~~~~~~~~~
   include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
                   (__typecheck(x, y) && __no_side_effects(x, y))
                    ^~~~~~~~~~~~~~~~~
   include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
           (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
   2 warnings generated.


vim +104 drivers/hwmon/occ/p8_i2c.c

    98	
    99	static int p8_i2c_occ_putscom_be(struct i2c_client *client, u32 address,
   100					 u8 *data, size_t len)
   101	{
   102		__be32 data0 = 0, data1 = 0;
   103	
 > 104		memcpy(&data0, data, min(len, 4UL));
   105		if (len > 4UL) {
   106			len -= 4;
   107			memcpy(&data1, data + 4, min(len, 4UL));
   108		}
   109	
   110		return p8_i2c_occ_putscom_u32(client, address, be32_to_cpu(data0),
   111					      be32_to_cpu(data1));
   112	}
   113	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34688 bytes --]

Re: [PATCH 2/3] hwmon: (occ) Remove sequence numbering and checksum calculation

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2611 bytes --]

Hi Eddie,

I love your patch! Perhaps something to improve:

[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on linux/master linus/master v5.14-rc1 next-20210716]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Eddie-James/OCC-fsi-and-hwmon-Set-sequence-number-in-submit-interface/20210718-103535
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: i386-randconfig-s001-20210718 (attached as .config)
compiler: gcc-10 (Ubuntu 10.3.0-1ubuntu1~20.04) 10.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/5e8ecc23325ef0310d83a4520071aae18418f88a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eddie-James/OCC-fsi-and-hwmon-Set-sequence-number-in-submit-interface/20210718-103535
        git checkout 5e8ecc23325ef0310d83a4520071aae18418f88a
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hwmon/occ/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/hwmon/occ/p8_i2c.c:104:30: sparse: sparse: incompatible types in comparison expression (different type sizes):
>> drivers/hwmon/occ/p8_i2c.c:104:30: sparse:    unsigned int *
>> drivers/hwmon/occ/p8_i2c.c:104:30: sparse:    unsigned long *
   drivers/hwmon/occ/p8_i2c.c:107:42: sparse: sparse: incompatible types in comparison expression (different type sizes):
   drivers/hwmon/occ/p8_i2c.c:107:42: sparse:    unsigned int *
   drivers/hwmon/occ/p8_i2c.c:107:42: sparse:    unsigned long *

vim +104 drivers/hwmon/occ/p8_i2c.c

    98	
    99	static int p8_i2c_occ_putscom_be(struct i2c_client *client, u32 address,
   100					 u8 *data, size_t len)
   101	{
   102		__be32 data0 = 0, data1 = 0;
   103	
 > 104		memcpy(&data0, data, min(len, 4UL));
   105		if (len > 4UL) {
   106			len -= 4;
   107			memcpy(&data1, data + 4, min(len, 4UL));
   108		}
   109	
   110		return p8_i2c_occ_putscom_u32(client, address, be32_to_cpu(data0),
   111					      be32_to_cpu(data1));
   112	}
   113	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37124 bytes --]

Re: [PATCH 3/3] fsi: occ: Add dynamic debug to dump command and response

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6285 bytes --]

Hi Eddie,

I love your patch! Yet something to improve:

[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on linus/master v5.14-rc2 next-20210716]
[cannot apply to linux/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Eddie-James/OCC-fsi-and-hwmon-Set-sequence-number-in-submit-interface/20210718-103535
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: csky-randconfig-r014-20210718 (attached as .config)
compiler: csky-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/2501575bac95640481d86c6d27cd675055987aa8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eddie-James/OCC-fsi-and-hwmon-Set-sequence-number-in-submit-interface/20210718-103535
        git checkout 2501575bac95640481d86c6d27cd675055987aa8
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=csky 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/fsi/fsi-occ.c: In function 'occ_putsram':
>> drivers/fsi/fsi-occ.c:372:3: error: implicit declaration of function 'DEFINE_DYNAMIC_DEBUG_METADATA' [-Werror=implicit-function-declaration]
     372 |   DEFINE_DYNAMIC_DEBUG_METADATA(ddm_occ_cmd, "OCC command");
         |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/fsi/fsi-occ.c:372:33: error: 'ddm_occ_cmd' undeclared (first use in this function)
     372 |   DEFINE_DYNAMIC_DEBUG_METADATA(ddm_occ_cmd, "OCC command");
         |                                 ^~~~~~~~~~~
   drivers/fsi/fsi-occ.c:372:33: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/fsi/fsi-occ.c:374:7: error: implicit declaration of function 'DYNAMIC_DEBUG_BRANCH' [-Werror=implicit-function-declaration]
     374 |   if (DYNAMIC_DEBUG_BRANCH(ddm_occ_cmd)) {
         |       ^~~~~~~~~~~~~~~~~~~~
   drivers/fsi/fsi-occ.c: In function 'fsi_occ_submit':
>> drivers/fsi/fsi-occ.c:584:33: error: 'ddm_occ_rsp' undeclared (first use in this function)
     584 |   DEFINE_DYNAMIC_DEBUG_METADATA(ddm_occ_rsp,
         |                                 ^~~~~~~~~~~
>> drivers/fsi/fsi-occ.c:586:33: error: 'ddm_occ_full_rsp' undeclared (first use in this function)
     586 |   DEFINE_DYNAMIC_DEBUG_METADATA(ddm_occ_full_rsp,
         |                                 ^~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/DEFINE_DYNAMIC_DEBUG_METADATA +372 drivers/fsi/fsi-occ.c

   315	
   316	static int occ_putsram(struct occ *occ, const void *data, ssize_t len,
   317			       u8 seq_no, u16 checksum)
   318	{
   319		size_t cmd_len, buf_len, resp_len, resp_data_len;
   320		u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
   321		__be32 *buf;
   322		u8 *byte_buf;
   323		int idx = 0, rc;
   324	
   325		cmd_len = (occ->version == occ_p10) ? 6 : 5;
   326	
   327		/*
   328		 * We use the same buffer for command and response, make
   329		 * sure it's big enough
   330		 */
   331		resp_len = OCC_SBE_STATUS_WORDS;
   332		cmd_len += data_len >> 2;
   333		buf_len = max(cmd_len, resp_len);
   334		buf = kzalloc(buf_len << 2, GFP_KERNEL);
   335		if (!buf)
   336			return -ENOMEM;
   337	
   338		/*
   339		 * Magic sequence to do SBE putsram command. SBE will transfer
   340		 * data to specified SRAM address.
   341		 */
   342		buf[0] = cpu_to_be32(cmd_len);
   343		buf[1] = cpu_to_be32(SBEFIFO_CMD_PUT_OCC_SRAM);
   344	
   345		switch (occ->version) {
   346		default:
   347		case occ_p9:
   348			buf[2] = cpu_to_be32(1);	/* Normal mode */
   349			buf[3] = cpu_to_be32(OCC_P9_SRAM_CMD_ADDR);
   350			break;
   351		case occ_p10:
   352			idx = 1;
   353			buf[2] = cpu_to_be32(OCC_P10_SRAM_MODE);
   354			buf[3] = 0;
   355			buf[4] = cpu_to_be32(OCC_P10_SRAM_CMD_ADDR);
   356			break;
   357		}
   358	
   359		buf[4 + idx] = cpu_to_be32(data_len);
   360		memcpy(&buf[5 + idx], data, len);
   361	
   362		byte_buf = (u8 *)&buf[5 + idx];
   363		/*
   364		 * Overwrite the first byte with our sequence number and the last two
   365		 * bytes with the checksum.
   366		 */
   367		byte_buf[0] = seq_no;
   368		byte_buf[len - 2] = checksum >> 8;
   369		byte_buf[len - 1] = checksum & 0xff;
   370	
   371		{
 > 372			DEFINE_DYNAMIC_DEBUG_METADATA(ddm_occ_cmd, "OCC command");
   373	
 > 374			if (DYNAMIC_DEBUG_BRANCH(ddm_occ_cmd)) {
   375				char prefix[64];
   376	
   377				snprintf(prefix, sizeof(prefix), "%s %s: cmd ",
   378					 dev_driver_string(occ->dev),
   379					 dev_name(occ->dev));
   380				print_hex_dump(KERN_DEBUG, prefix, DUMP_PREFIX_OFFSET,
   381					       16, 4, byte_buf, len, false);
   382			}
   383		}
   384	
   385		rc = sbefifo_submit(occ->sbefifo, buf, cmd_len, buf, &resp_len);
   386		if (rc)
   387			goto free;
   388	
   389		rc = sbefifo_parse_status(occ->sbefifo, SBEFIFO_CMD_PUT_OCC_SRAM,
   390					  buf, resp_len, &resp_len);
   391		if (rc)
   392			goto free;
   393	
   394		if (resp_len != 1) {
   395			dev_err(occ->dev, "SRAM write response length invalid: %zd\n",
   396				resp_len);
   397			rc = -EBADMSG;
   398		} else {
   399			resp_data_len = be32_to_cpu(buf[0]);
   400			if (resp_data_len != data_len) {
   401				dev_err(occ->dev,
   402					"SRAM write expected %d bytes got %zd\n",
   403					data_len, resp_data_len);
   404				rc = -EBADMSG;
   405			}
   406		}
   407	
   408	free:
   409		/* Convert positive SBEI status */
   410		if (rc > 0) {
   411			dev_err(occ->dev, "SRAM write returned failure status: %08x\n",
   412				rc);
   413			rc = -EBADMSG;
   414		}
   415	
   416		kfree(buf);
   417		return rc;
   418	}
   419	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34611 bytes --]

Re: [PATCH 1/3] fsi: occ: Force sequence numbering per OCC

[Joel Stanley]

On Fri, 16 Jul 2021 at 15:19, Eddie James <eajames@linux.ibm.com> wrote:
>
> Set and increment the sequence number during the submit operation.
> This prevents sequence number conflicts between different users of
> the interface. A sequence number conflict may result in a user
> getting an OCC response meant for a different command. Since the
> sequence number is now modified, the checksum must be calculated and
> set before submitting the command.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> @@ -479,11 +483,26 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
>                 return -EINVAL;
>         }
>
> +       /* Checksum the request, ignoring first byte (sequence number). */
> +       for (i = 1; i < req_len - 2; ++i)
> +               checksum += byte_request[i];
> +

This could go below, after you've got the sequence number, so the
checksumming all happens in the same spot?

The driver has become a bit of a maze, I can't tell how you're
deciding what goes in fsi_occ_submit vs occ_write vs occ_putsram. If
oyu have some ideas on how to simplify it then I would welcome those
changes.



>         mutex_lock(&occ->occ_lock);
>
> -       /* Extract the seq_no from the command (first byte) */
> -       seq_no = *(const u8 *)request;
> -       rc = occ_putsram(occ, request, req_len);
> +       /*
> +        * Get a sequence number and update the counter. Avoid a sequence
> +        * number of 0 which would pass the response check below even if the
> +        * OCC response is uninitialized. Any sequence number the user is
> +        * trying to send is overwritten since this function is the only common
> +        * interface to the OCC and therefore the only place we can guarantee
> +        * unique sequence numbers.
> +        */
> +       seq_no = occ->sequence_number++;
> +       if (!occ->sequence_number)
> +               occ->sequence_number = 1;
> +       checksum += seq_no;
> +
> +       rc = occ_putsram(occ, request, req_len, seq_no, checksum);
>         if (rc)
>                 goto done;

Re: [PATCH 2/3] hwmon: (occ) Remove sequence numbering and checksum calculation

[Joel Stanley]

On Fri, 16 Jul 2021 at 15:19, Eddie James <eajames@linux.ibm.com> wrote:
>
> Checksumming of the request and sequence numbering is now done in the
> OCC interface driver in order to keep unique sequence numbers. So
> remove those in the hwmon driver.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/hwmon/occ/common.c | 30 ++++++++++++------------------
>  drivers/hwmon/occ/common.h |  3 +--
>  drivers/hwmon/occ/p8_i2c.c | 15 +++++++++------
>  drivers/hwmon/occ/p9_sbe.c |  4 ++--
>  4 files changed, 24 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> index 0d68a78be980..fc298268c89e 100644
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -132,22 +132,20 @@ struct extended_sensor {
>  static int occ_poll(struct occ *occ)
>  {
>         int rc;
> -       u16 checksum = occ->poll_cmd_data + occ->seq_no + 1;
> -       u8 cmd[8];
> +       u8 cmd[7];

The shortening of the command seems unrelated?

If you leave it at 8 then you avoid the special casing below. Is there
any downside to sending the extra 0 byte at the end?

>         struct occ_poll_response_header *header;
>
>         /* big endian */
> -       cmd[0] = occ->seq_no++;         /* sequence number */
> +       cmd[0] = 0;                     /* sequence number */
>         cmd[1] = 0;                     /* cmd type */
>         cmd[2] = 0;                     /* data length msb */
>         cmd[3] = 1;                     /* data length lsb */
>         cmd[4] = occ->poll_cmd_data;    /* data */
> -       cmd[5] = checksum >> 8;         /* checksum msb */
> -       cmd[6] = checksum & 0xFF;       /* checksum lsb */
> -       cmd[7] = 0;
> +       cmd[5] = 0;                     /* checksum msb */
> +       cmd[6] = 0;                     /* checksum lsb */

> --- a/drivers/hwmon/occ/p8_i2c.c> +++ b/drivers/hwmon/occ/p8_i2c.c
> @@ -97,18 +97,21 @@ static int p8_i2c_occ_putscom_u32(struct i2c_client *client, u32 address,
>  }
>
>  static int p8_i2c_occ_putscom_be(struct i2c_client *client, u32 address,
> -                                u8 *data)
> +                                u8 *data, size_t len)
>  {
> -       __be32 data0, data1;
> +       __be32 data0 = 0, data1 = 0;
>
> -       memcpy(&data0, data, 4);
> -       memcpy(&data1, data + 4, 4);
> +       memcpy(&data0, data, min(len, 4UL));

The UL here seems unnecessary (and dropping it should fix your 0day
bot warnings).

But I think it would be simpler to go back to a fixed length of 8.

> +       if (len > 4UL) {
> +               len -= 4;
> +               memcpy(&data1, data + 4, min(len, 4UL));
> +       }
>
>         return p8_i2c_occ_putscom_u32(client, address, be32_to_cpu(data0),
>                                       be32_to_cpu(data1));
>  }

Re: [PATCH 1/3] fsi: occ: Force sequence numbering per OCC

[Eddie James]

On Wed, 2021-07-21 at 02:37 +0000, Joel Stanley wrote:
> On Fri, 16 Jul 2021 at 15:19, Eddie James <eajames@linux.ibm.com>
> wrote:
> > Set and increment the sequence number during the submit operation.
> > This prevents sequence number conflicts between different users of
> > the interface. A sequence number conflict may result in a user
> > getting an OCC response meant for a different command. Since the
> > sequence number is now modified, the checksum must be calculated
> > and
> > set before submitting the command.
> > 
> > Signed-off-by: Eddie James <eajames@linux.ibm.com>
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
> > @@ -479,11 +483,26 @@ int fsi_occ_submit(struct device *dev, const
> > void *request, size_t req_len,
> >                 return -EINVAL;
> >         }
> > 
> > +       /* Checksum the request, ignoring first byte (sequence
> > number). */
> > +       for (i = 1; i < req_len - 2; ++i)
> > +               checksum += byte_request[i];
> > +
> 
> This could go below, after you've got the sequence number, so the
> checksumming all happens in the same spot?

It definitely could, I had the idea to do the checksumming outside the
mutex in case it took a long time? Probably not worth it though.

> 
> The driver has become a bit of a maze, I can't tell how you're
> deciding what goes in fsi_occ_submit vs occ_write vs occ_putsram. If
> oyu have some ideas on how to simplify it then I would welcome those
> changes.

Well, it doesn't really matter in fsi_occ_submit vs occ_putsram, as the
latter is only called in the former. occ_write wouldn't be used by the
hwmon interface, which is why we're moving some of that to
fsi_occ_submit, to have more in common. Agree it could probably be
organized better but I don't immediately have a good idea how to do
that.

Thanks for the review!
Eddie

> 
> 
> 
> >         mutex_lock(&occ->occ_lock);
> > 
> > -       /* Extract the seq_no from the command (first byte) */
> > -       seq_no = *(const u8 *)request;
> > -       rc = occ_putsram(occ, request, req_len);
> > +       /*
> > +        * Get a sequence number and update the counter. Avoid a
> > sequence
> > +        * number of 0 which would pass the response check below
> > even if the
> > +        * OCC response is uninitialized. Any sequence number the
> > user is
> > +        * trying to send is overwritten since this function is the
> > only common
> > +        * interface to the OCC and therefore the only place we can
> > guarantee
> > +        * unique sequence numbers.
> > +        */
> > +       seq_no = occ->sequence_number++;
> > +       if (!occ->sequence_number)
> > +               occ->sequence_number = 1;
> > +       checksum += seq_no;
> > +
> > +       rc = occ_putsram(occ, request, req_len, seq_no, checksum);
> >         if (rc)
> >                 goto done;

Re: [PATCH 2/3] hwmon: (occ) Remove sequence numbering and checksum calculation

[Eddie James]

On Wed, 2021-07-21 at 02:43 +0000, Joel Stanley wrote:
> On Fri, 16 Jul 2021 at 15:19, Eddie James <eajames@linux.ibm.com>
> wrote:
> > Checksumming of the request and sequence numbering is now done in
> > the
> > OCC interface driver in order to keep unique sequence numbers. So
> > remove those in the hwmon driver.
> > 
> > Signed-off-by: Eddie James <eajames@linux.ibm.com>
> > ---
> >  drivers/hwmon/occ/common.c | 30 ++++++++++++------------------
> >  drivers/hwmon/occ/common.h |  3 +--
> >  drivers/hwmon/occ/p8_i2c.c | 15 +++++++++------
> >  drivers/hwmon/occ/p9_sbe.c |  4 ++--
> >  4 files changed, 24 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/hwmon/occ/common.c
> > b/drivers/hwmon/occ/common.c
> > index 0d68a78be980..fc298268c89e 100644
> > --- a/drivers/hwmon/occ/common.c
> > +++ b/drivers/hwmon/occ/common.c
> > @@ -132,22 +132,20 @@ struct extended_sensor {
> >  static int occ_poll(struct occ *occ)
> >  {
> >         int rc;
> > -       u16 checksum = occ->poll_cmd_data + occ->seq_no + 1;
> > -       u8 cmd[8];
> > +       u8 cmd[7];
> 
> The shortening of the command seems unrelated?
> 
> If you leave it at 8 then you avoid the special casing below. Is
> there
> any downside to sending the extra 0 byte at the end?

Yes, it would break the checksumming unfortunately. The checksum is
calculated and added at the last two bytes, so if you send more than
your command actually is, the checksum will be in the wrong spot.
> 
> >         struct occ_poll_response_header *header;
> > 
> >         /* big endian */
> > -       cmd[0] = occ->seq_no++;         /* sequence number */
> > +       cmd[0] = 0;                     /* sequence number */
> >         cmd[1] = 0;                     /* cmd type */
> >         cmd[2] = 0;                     /* data length msb */
> >         cmd[3] = 1;                     /* data length lsb */
> >         cmd[4] = occ->poll_cmd_data;    /* data */
> > -       cmd[5] = checksum >> 8;         /* checksum msb */
> > -       cmd[6] = checksum & 0xFF;       /* checksum lsb */
> > -       cmd[7] = 0;
> > +       cmd[5] = 0;                     /* checksum msb */
> > +       cmd[6] = 0;                     /* checksum lsb */
> > --- a/drivers/hwmon/occ/p8_i2c.c> +++ b/drivers/hwmon/occ/p8_i2c.c
> > @@ -97,18 +97,21 @@ static int p8_i2c_occ_putscom_u32(struct
> > i2c_client *client, u32 address,
> >  }
> > 
> >  static int p8_i2c_occ_putscom_be(struct i2c_client *client, u32
> > address,
> > -                                u8 *data)
> > +                                u8 *data, size_t len)
> >  {
> > -       __be32 data0, data1;
> > +       __be32 data0 = 0, data1 = 0;
> > 
> > -       memcpy(&data0, data, 4);
> > -       memcpy(&data1, data + 4, 4);
> > +       memcpy(&data0, data, min(len, 4UL));
> 
> The UL here seems unnecessary (and dropping it should fix your 0day
> bot warnings).

Yea, I think I just need min_t

Thanks for the review!
Eddie

> 
> But I think it would be simpler to go back to a fixed length of 8.
> 
> > +       if (len > 4UL) {
> > +               len -= 4;
> > +               memcpy(&data1, data + 4, min(len, 4UL));
> > +       }
> > 
> >         return p8_i2c_occ_putscom_u32(client, address,
> > be32_to_cpu(data0),
> >                                       be32_to_cpu(data1));
> >  }

Re: [PATCH 3/3] fsi: occ: Add dynamic debug to dump command and response

[Jeremy Kerr]

Hi Eddie,

> Use the dynamic branching capability of the dynamic debug subsystem
> to dump the command and response with the correct OCC device name.

Would this be better done with a tracepoint? Given it's a fairly
well-defined pair of events, and there's data to dump in both cases.

We have a couple of existing tracepoionts in the core code if that
helps...

Cheers,


Jeremy