2020-11-11 08:15:15

by Kaixu Xia

Subject: [PATCH] hwmon: Fix unsigned 'reg' compared with zero in amc6821_update_device

From: Kaixu Xia <[email protected]>

The unsigned variable reg is assigned a return value from the call
to i2c_smbus_read_byte_data(), which may return negative error code.

Fixes coccicheck warning:

./drivers/hwmon/amc6821.c:215:6-9: WARNING: Unsigned expression compared with zero: reg > 0
./drivers/hwmon/amc6821.c:228:6-9: WARNING: Unsigned expression compared with zero: reg > 0

Reported-by: Tosk Robot <[email protected]>
Signed-off-by: Kaixu Xia <[email protected]>
---
drivers/hwmon/amc6821.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 6b1ce2242c61..ce7c9f412538 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -166,7 +166,7 @@ static struct amc6821_data *amc6821_update_device(struct device *dev)
struct amc6821_data *data = dev_get_drvdata(dev);
struct i2c_client *client = data->client;
int timeout = HZ;
- u8 reg;
+ s8 reg;
int i;

mutex_lock(&data->update_lock);
--
2.20.0

2020-11-11 14:32:13

by Guenter Roeck

Subject: Re: [PATCH] hwmon: Fix unsigned 'reg' compared with zero in amc6821_update_device

On 11/11/20 12:13 AM, [email protected] wrote:
> From: Kaixu Xia <[email protected]>
>
> The unsigned variable reg is assigned a return value from the call
> to i2c_smbus_read_byte_data(), which may return negative error code.
>
> Fixes coccicheck warning:
>
> ./drivers/hwmon/amc6821.c:215:6-9: WARNING: Unsigned expression compared with zero: reg > 0
> ./drivers/hwmon/amc6821.c:228:6-9: WARNING: Unsigned expression compared with zero: reg > 0
>
> Reported-by: Tosk Robot <[email protected]>
> Signed-off-by: Kaixu Xia <[email protected]>
> ---
> drivers/hwmon/amc6821.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 6b1ce2242c61..ce7c9f412538 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -166,7 +166,7 @@ static struct amc6821_data *amc6821_update_device(struct device *dev)
> struct amc6821_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> int timeout = HZ;
> - u8 reg;
> + s8 reg;

That is pointless since the return value is not checked. On top of that,
there are many similar assignments to u8 variables, return values
from calls to i2c_smbus_read_byte_data() are never checked in
this function, and the u8 part is actually needed. If you want to
fix it, please go ahead, but please fix the entire driver and please
do it correctly (here: the variable should be 'int' and the return
value should be checked).

Thanks,
Guenter