Re: [PATCH 4/4] hwmon: (kb9002) Add documentation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The driver uses a private mutex instead of `hwmon_lock()`, violating hwmon core guidelines.
- [Medium] The driver unconditionally requires `I2C_FUNC_I2C`, permanently breaking support for SMBus-only controllers.
--

--- Patch [4]: [PATCH 4/4] hwmon: (kb9002) Add documentation ---
commit 929a2de2e63d6d8333f36707bf6da054f63aeab6
Author: Andy Chung <Andy.Chung@amd.com>

hwmon: (kb9002) Add driver for Kandou KB9002 retimer

The Kandou KB9002 is an 8-lane PCIe 5.0 retimer that exposes an SMBus
target with mandatory PEC. Add a hwmon driver reporting the firmware
aggregated maximum die temperature as temp1_input, with the firmware
version and boot status under debugfs.

> diff --git a/drivers/hwmon/kb9002.c b/drivers/hwmon/kb9002.c
> new file mode 100644
> index 0000000000000..2a3dffe52a62f
> --- /dev/null
> +++ b/drivers/hwmon/kb9002.c
> @@ -0,0 +1,473 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

[ ... ]

> +struct kb9002_data {
> +	struct i2c_client *client;
> +	struct mutex lock;	/* serialises register accesses */
> +};

[Severity: Medium]
Does using a private mutex instead of hwmon_lock() violate the hwmon
subsystem guidelines? The guidelines mandate using hwmon_lock() and
hwmon_unlock() to serialize attributes registered outside the core,
such as debugfs, with sysfs and thermal operations. By discarding the
hwmon_dev pointer returned during registration, the driver cannot
correctly call hwmon_lock().

[ ... ]

> +static int kb9002_fw_read(struct kb9002_data *data, u16 reg, u32 *val)
> +{
> +	struct i2c_client *client = data->client;
> +	u8 addr[2];
> +	u8 rbuf[I2C_SMBUS_BLOCK_MAX];
> +	int ret;
> +
> +	put_unaligned_le16(reg, addr);
> +
> +	mutex_lock(&data->lock);

[Severity: Medium]
This isn't a bug in the locking logic itself, but shouldn't this code use
hwmon_lock() instead of the private lock to serialize I2C/SMBus accesses?

[ ... ]

> +static int kb9002_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct kb9002_data *data;
> +	struct device *hwmon_dev;
> +	u32 vid;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_BLOCK_DATA |
> +				     I2C_FUNC_SMBUS_PEC | I2C_FUNC_I2C))
> +		return -ENODEV;

[Severity: Medium]
Does unconditionally requiring I2C_FUNC_I2C prevent the driver from binding
on SMBus-only controllers?

Raw I2C functionality appears to be used only in a fallback path
(kb9002_enable_smbus_target()) to switch the device out of raw-I2C mode.
By mandating it upfront, the driver unnecessarily denies service on
legitimate SMBus-only systems where the hardware is already configured
for SMBus mode.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-kb9002-upstream-v1-0-8fd2f0b135d8@amd.com?part=4