Re: [PATCH v3 1/3] hwmon: (pt5161l) Fix bugs in pt5161l_read_block_data()

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] The driver incorrectly defines and uses a custom mutex instead of the required hwmon locking API.
--

commit 9e1f24f685e2634190aef8a25e65494baf85ed6c
Author: Sanman Pradhan <psanman@juniper.net>

    hwmon: (pt5161l) Fix bugs in pt5161l_read_block_data()
    
    This patch fixes a buffer overrun by resizing the local read buffer to
    I2C_SMBUS_BLOCK_MAX. It also ensures the function returns -EIO when
    all retries are exhausted with an unexpected length instead of returning
    a positive byte count.

> diff --git a/drivers/hwmon/pt5161l.c b/drivers/hwmon/pt5161l.c
> index 20e3cfa625f17..89d4da8aa4c09 100644
> --- a/drivers/hwmon/pt5161l.c
> +++ b/drivers/hwmon/pt5161l.c
> @@ -121,7 +121,7 @@ static int pt5161l_read_block_data(struct pt5161l_data *data, u32 address,
>  	int ret, tries;
>  	u8 remain_len = len;
>  	u8 curr_len;
> -	u8 wbuf[16], rbuf[24];
> +	u8 wbuf[16], rbuf[I2C_SMBUS_BLOCK_MAX];

[ ... ]

This isn't a bug introduced by this patch, but looking at the pt5161l driver
as a whole, it appears to define and use a custom mutex instead of the required
hwmon locking API.

The subsystem guidelines mandate that drivers must use hwmon_lock() and
hwmon_unlock() to implement locking for attributes registered by means other
than devm_hwmon_device_register_with_info() (such as debugfs attributes).

Instead, this driver implements its own custom struct mutex lock inside
struct pt5161l_data and uses mutex_lock(&data->lock) to protect I2C operations
for debugfs attributes like fw_ver.

To comply with the architectural rules of the subsystem, should the driver save
the hwmon_dev pointer and utilize the standard hwmon_lock() and hwmon_unlock()
functions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260408163029.353777-1-sanman.pradhan@hpe.com?part=1