Re: [PATCH 3/4] hwmon: (kb9002) Add driver for Kandou KB9002 retimer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The driver introduces its own mutex for register accesses instead of using the standard hwmon lock, violating subsystem guidelines.
- [Medium] Probe loop retries repeatedly on fatal I2C read failures, causing extreme bus delays on missing devices.
- [Medium] Unconditional requirement for I2C_FUNC_I2C prevents driver from loading on SMBus-only adapters.
- [Medium] Missing suspend/resume handlers will cause devices strapped to raw I2C to fail after system resume.
--

--- Patch [3]: [PATCH 3/4] hwmon: (kb9002) Add driver for Kandou KB9002 retimer ---
commit 929a2de2e63d6d8333f36707bf6da054f63aeab6
Author: Andy Chung <Andy.Chung@amd.com>
Subject: 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. This patch adds 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

[ ... ]

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

[Severity: Medium]
According to the hwmon subsystem guidelines, drivers must use hwmon_lock()
and hwmon_unlock() to serialize attributes registered by other means, such
as the debugfs entries.

Is it appropriate to introduce a custom mutex here instead of saving the
hwmon_dev pointer during kb9002_probe() and utilizing the standard hwmon lock
in functions like kb9002_fw_read()?

[ ... ]

> +static int kb9002_read_revid(struct i2c_client *client, u32 *revid)
> +{
> +	u8 dummy;
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < KB9002_REVID_READ_RETRIES; i++) {
> +		ret = kb9002_i2c_read(client, KB9002_HW_REG_REVID, revid);
> +		if (!ret && (*revid >> 24) != 0xff)
> +			return 0;
> +		/* Drain one byte from the chip to re-align the I2C FIFO. */
> +		i2c_master_recv(client, &dummy, 1);
> +	}
> +
> +	return ret ? ret : -EIO;
> +}

[Severity: Medium]
If the initial I2C read fails with a hard error, such as when the device is
missing and kb9002_i2c_read() returns a negative error code, this loop does
not short-circuit.

Could this needlessly loop and execute a failing i2c_master_recv() on every
iteration, causing unnecessary bus traffic and probe delays?

[ ... ]

> +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]
This check unconditionally requires I2C_FUNC_I2C.

Could this prevent the driver from loading on SMBus-only adapters even if
the retimer is properly strapped to SMBus mode and does not require the raw
I2C fallback?

Would it be better to defer checking for raw I2C functionality to
kb9002_enable_smbus_target() where it is actually needed?

[ ... ]

> +static struct i2c_driver kb9002_driver = {
> +	.driver = {
> +		.name = KB9002_DEV_NAME,
> +		.of_match_table = kb9002_of_match,
> +	},
> +	.probe = kb9002_probe,
> +	.id_table = kb9002_id,
> +};
> +module_i2c_driver(kb9002_driver);

[Severity: Medium]
If the retimer is strapped to raw I2C mode, the driver switches it to SMBus
mode during probe.

If the device loses power during system suspend and defaults back to raw
I2C mode on wake, will all subsequent SMBus accesses by the driver fail?

Does this driver need pm_ops with a resume handler to restore the device
state after suspend?

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