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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Acked-by: Guenter Roeck <[email protected]>
> ---
> 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;
>
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 <[email protected]>
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/[email protected]
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 <[email protected]>
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/[email protected]
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 <[email protected]>
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/[email protected]
On Fri, 16 Jul 2021 at 15:19, Eddie James <[email protected]> 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 <[email protected]>
> ---
> 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));
> }
On Wed, 2021-07-21 at 02:43 +0000, Joel Stanley wrote:
> On Fri, 16 Jul 2021 at 15:19, Eddie James <[email protected]>
> 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 <[email protected]>
> > ---
> > 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));
> > }
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