linux-kernel - Re: [PATCH v1 1/1] hwmon: (adc128d818): filter out 0x1ff reading

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <92a7ac6d-3600-4bc4-8dad-b679a8e18251@roeck-us.net>
Date: Thu, 6 Feb 2025 12:56:54 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Jerry C Chen <Jerry_C_Chen@...ynn.com>, patrick@...cx.xyz,
 Jean Delvare <jdelvare@...e.com>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] hwmon: (adc128d818): filter out 0x1ff reading

On 2/5/25 22:35, Jerry C Chen wrote:
> ASPEED i2c driver doesn't support SMBUS timeout thus we will get dirty
> data while SCL hold time over 35ms, filter out default register value
> 0x1ff to avoid abnormal data reading.

If the Aspeed i2c driver has a problem, it should be fixed there,
not in drivers utilizing it.

> 
> Signed-off-by: Jerry C Chen <Jerry_C_Chen@...ynn.com>
> ---
>   drivers/hwmon/adc128d818.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/adc128d818.c b/drivers/hwmon/adc128d818.c
> index 5e805d4ee76a..8197d3f14ea7 100644
> --- a/drivers/hwmon/adc128d818.c
> +++ b/drivers/hwmon/adc128d818.c
> @@ -88,7 +88,7 @@ static struct adc128_data *adc128_update_device(struct device *dev)
>                  for (i = 0; i < num_inputs[data->mode]; i++) {
>                          rv = i2c_smbus_read_word_swapped(client,
>                                                           ADC128_REG_IN(i));
> -                       if (rv < 0)
> +                       if (rv < 0 || rv == 511)

That would return a pointer to 511 to the caller, which is most definitely
not useful and would very likely result in a crash. That makes me wonder
you have actually tested this patch.

On top of that, 511 or 0x1ff is a perfectly valid return value. I find
zero evidence in the datasheet suggesting that a returned value of 0x1ff
would suggest an invalid value. More specifically, the datasheet does
not say if bit 0..3 are returned as 0 or as 1.

>                                  goto abort;
>                          data->in[0][i] = rv >> 4;
> 
> --
> 2.25.1
> 
> WIWYNN PROPRIETARY
> This email (and any attachments) contains proprietary or confidential information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited. If you are not the intended recipient, please notify the sender and delete this email immediately.

That means that I would not be able to apply the patch anyway, even if it was perfect.

Guenter