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

[Guenter Roeck <linux@roeck-us.net>]

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@aj.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) {
>