linux-kernel - Re: [RFC PATCH 2/2] hwmon: (pmbus/ucd9000) Throttle SMBus transfers to avoid poor behaviour

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 for Android: free password hash cracker in your pocket

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

Message-ID: <71067b18-c4bc-533a-0069-f21069c5fd0d@roeck-us.net>
Date:   Mon, 14 Sep 2020 07:14:02 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Andrew Jeffery <andrew@...id.au>, linux-hwmon@...r.kernel.org,
        linux-i2c@...r.kernel.org
Cc:     jdelvare@...e.com, wsa@...nel.org, joel@....id.au,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] hwmon: (pmbus/ucd9000) Throttle SMBus transfers
 to avoid poor behaviour

On 9/14/20 5:28 AM, Andrew Jeffery wrote:
> Short turn-around times between transfers to e.g. the UCD90320 can lead
> to problematic behaviour, including excessive clock stretching, bus
> lockups and potential corruption of the device's volatile state.
> 
> Introduce transfer throttling for the device with a minimum access
> delay of 1ms.
> 

Some Zilker labs devices have the same problem, though not as bad
to need a 1ms delay. See zl6100.c. Various LTS devices have a similar
problem, but there it is possible to poll the device until it is ready.
See ltc2978.c.

> Signed-off-by: Andrew Jeffery <andrew@...id.au>
> ---
>  drivers/hwmon/pmbus/ucd9000.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> index 81f4c4f166cd..a0b97d035326 100644
> --- a/drivers/hwmon/pmbus/ucd9000.c
> +++ b/drivers/hwmon/pmbus/ucd9000.c
> @@ -9,6 +9,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/moduleparam.h>
>  #include <linux/of_device.h>
>  #include <linux/init.h>
>  #include <linux/err.h>
> @@ -18,6 +19,9 @@
>  #include <linux/gpio/driver.h>
>  #include "pmbus.h"
>  
> +static unsigned long smbus_delay_us = 1000;

Is that to be on the super-safe side ? Patch 0 talks about needing 250 uS.

> +module_param(smbus_delay_us, ulong, 0664);
> +

I would not want to have this in user control, and it should not affect devices
not known to be affected. I would suggest an implementation similar to other
affected devices; again, see zl6100.c or ltc2978.c for examples.

Thanks,
Guenter

>  enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd90320, ucd9090,
>  	     ucd90910 };
>  
> @@ -502,6 +506,8 @@ static int ucd9000_probe(struct i2c_client *client,
>  				     I2C_FUNC_SMBUS_BLOCK_DATA))
>  		return -ENODEV;
>  
> +	i2c_smbus_throttle_client(client, smbus_delay_us);
> +
>  	ret = i2c_smbus_read_block_data(client, UCD9000_DEVICE_ID,
>  					block_buffer);
>  	if (ret < 0) {
>