[RFC PATCH 0/2] Throttle I2C transfers to UCD9000 devices

[RFC PATCH 0/2] Throttle I2C transfers to UCD9000 devices

[Andrew Jeffery]

Hello,

While working with system designs making use of TI's UCD90320 Power
Sequencer we've found that communication with the device isn't terribly
reliable.

It appears that back-to-back transfers where commands addressed to the
device are put onto the bus with intervals between STOP and START in the
neighbourhood of 250us or less can cause bad behaviour. This primarily
happens during driver probe while scanning the device to determine its
capabilities.

We have observed the device causing excessive clock stretches and bus
lockups, and also corruption of the device's volatile state (requiring it
to be reset).  The latter is particularly disruptive in that the controlled
rails are brought down either by:

1. The corruption causing a fault condition, or
2. Asserting the device's reset line to recover

A further observation is that pacing transfers to the device appears to
mitigate the bad behaviour. We're in discussion with TI to better
understand the limitations and at least get the behaviour documented.

This short series implements the mitigation in terms of a throttle in the
i2c_client associated with the device's driver. Before the first
communication with the device in the probe() of ucd9000 we configure the
i2c_client to throttle transfers with a minimum of a 1ms delay (with the
delay exposed as a module parameter).

The series is RFC for several reasons:

The first is to sus out feelings on the general direction. The problem is
pretty unfortunate - are there better ways to implement the mitigation?

If there aren't, then:

I'd like thoughts on whether we want to account for i2c-dev clients.
Implementing throttling in i2c_client feels like a solution-by-proxy as the
throttling is really a property of the targeted device, but we don't have a
coherent representation between platform devices and devices associated
with i2c-dev clients. At the moment we'd have to resort to address-based
lookups for platform data stashed in the transfer functions.

Next is that I've only implemented throttling for SMBus devices. I don't
yet have a use-case for throttling non-SMBus devices so I'm not sure it's
worth poking at it, but would appreciate thoughts there.

Further, I've had a bit of a stab at dealing with atomic transfers that's
not been tested. Hopefully it makes sense.

Finally I'm also interested in feedback on exposing the control in a little
more general manner than having to implement a module parameter in all
drivers that want to take advantage of throttling. This isn't a big problem
at the moment, but if anyone has thoughts there then I'm happy to poke at
those too.

Please review!

Andrew

Andrew Jeffery (2):
  i2c: smbus: Allow throttling of transfers to client devices
  hwmon: (pmbus/ucd9000) Throttle SMBus transfers to avoid poor
    behaviour

 drivers/hwmon/pmbus/ucd9000.c |   6 ++
 drivers/i2c/i2c-core-base.c   |   8 +-
 drivers/i2c/i2c-core-smbus.c  | 149 +++++++++++++++++++++++++++-------
 drivers/i2c/i2c-core.h        |  22 +++++
 include/linux/i2c.h           |   3 +
 5 files changed, 157 insertions(+), 31 deletions(-)

-- 
2.25.1

[RFC PATCH 1/2] i2c: smbus: Allow throttling of transfers to client devices

[Andrew Jeffery]

Some devices fail to cope in disastrous ways with the small command
turn-around times enabled by in-kernel device drivers.

Introduce per-client throttling of transfers to avoid these issues.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/i2c/i2c-core-base.c  |   8 +-
 drivers/i2c/i2c-core-smbus.c | 149 ++++++++++++++++++++++++++++-------
 drivers/i2c/i2c-core.h       |  22 ++++++
 include/linux/i2c.h          |   3 +
 4 files changed, 151 insertions(+), 31 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 5ec082e2039d..d5a519bcadf9 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -867,13 +867,17 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
 struct i2c_client *
 i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 {
+	struct i2c_client_priv	*priv;
 	struct i2c_client	*client;
 	int			status;
 
-	client = kzalloc(sizeof *client, GFP_KERNEL);
-	if (!client)
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
 		return ERR_PTR(-ENOMEM);
 
+	mutex_init(&priv->throttle_lock);
+	client = &priv->client;
+
 	client->adapter = adap;
 
 	client->dev.platform_data = info->platform_data;
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index f5c9787992e9..4a9692a37e1e 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -10,6 +10,7 @@
  * SMBus 2.0 support by Mark Studebaker <mdsxyz123@yahoo.com> and
  * Jean Delvare <jdelvare@suse.de>
  */
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
@@ -21,6 +22,9 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/smbus.h>
 
+static s32 i2c_smbus_throttle_xfer(const struct i2c_client *client,
+				   char read_write, u8 command, int protocol,
+				   union i2c_smbus_data *data);
 
 /* The SMBus parts */
 
@@ -95,9 +99,9 @@ s32 i2c_smbus_read_byte(const struct i2c_client *client)
 	union i2c_smbus_data data;
 	int status;
 
-	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-				I2C_SMBUS_READ, 0,
-				I2C_SMBUS_BYTE, &data);
+	status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, 0,
+					 I2C_SMBUS_BYTE, &data);
+
 	return (status < 0) ? status : data.byte;
 }
 EXPORT_SYMBOL(i2c_smbus_read_byte);
@@ -112,8 +116,8 @@ EXPORT_SYMBOL(i2c_smbus_read_byte);
  */
 s32 i2c_smbus_write_byte(const struct i2c_client *client, u8 value)
 {
-	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-	                      I2C_SMBUS_WRITE, value, I2C_SMBUS_BYTE, NULL);
+	return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, value,
+				       I2C_SMBUS_BYTE, NULL);
 }
 EXPORT_SYMBOL(i2c_smbus_write_byte);
 
@@ -130,9 +134,9 @@ s32 i2c_smbus_read_byte_data(const struct i2c_client *client, u8 command)
 	union i2c_smbus_data data;
 	int status;
 
-	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-				I2C_SMBUS_READ, command,
-				I2C_SMBUS_BYTE_DATA, &data);
+	status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, command,
+					 I2C_SMBUS_BYTE_DATA, &data);
+
 	return (status < 0) ? status : data.byte;
 }
 EXPORT_SYMBOL(i2c_smbus_read_byte_data);
@@ -150,10 +154,10 @@ s32 i2c_smbus_write_byte_data(const struct i2c_client *client, u8 command,
 			      u8 value)
 {
 	union i2c_smbus_data data;
+
 	data.byte = value;
-	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-			      I2C_SMBUS_WRITE, command,
-			      I2C_SMBUS_BYTE_DATA, &data);
+	return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, command,
+				       I2C_SMBUS_BYTE_DATA, &data);
 }
 EXPORT_SYMBOL(i2c_smbus_write_byte_data);
 
@@ -170,9 +174,9 @@ s32 i2c_smbus_read_word_data(const struct i2c_client *client, u8 command)
 	union i2c_smbus_data data;
 	int status;
 
-	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-				I2C_SMBUS_READ, command,
-				I2C_SMBUS_WORD_DATA, &data);
+	status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, command,
+					 I2C_SMBUS_WORD_DATA, &data);
+
 	return (status < 0) ? status : data.word;
 }
 EXPORT_SYMBOL(i2c_smbus_read_word_data);
@@ -190,10 +194,10 @@ s32 i2c_smbus_write_word_data(const struct i2c_client *client, u8 command,
 			      u16 value)
 {
 	union i2c_smbus_data data;
+
 	data.word = value;
-	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-			      I2C_SMBUS_WRITE, command,
-			      I2C_SMBUS_WORD_DATA, &data);
+	return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, command,
+				       I2C_SMBUS_WORD_DATA, &data);
 }
 EXPORT_SYMBOL(i2c_smbus_write_word_data);
 
@@ -218,9 +222,9 @@ s32 i2c_smbus_read_block_data(const struct i2c_client *client, u8 command,
 	union i2c_smbus_data data;
 	int status;
 
-	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-				I2C_SMBUS_READ, command,
-				I2C_SMBUS_BLOCK_DATA, &data);
+	status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, command,
+					 I2C_SMBUS_BLOCK_DATA, &data);
+
 	if (status)
 		return status;
 
@@ -248,9 +252,8 @@ s32 i2c_smbus_write_block_data(const struct i2c_client *client, u8 command,
 		length = I2C_SMBUS_BLOCK_MAX;
 	data.block[0] = length;
 	memcpy(&data.block[1], values, length);
-	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-			      I2C_SMBUS_WRITE, command,
-			      I2C_SMBUS_BLOCK_DATA, &data);
+	return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, command,
+				       I2C_SMBUS_BLOCK_DATA, &data);
 }
 EXPORT_SYMBOL(i2c_smbus_write_block_data);
 
@@ -264,9 +267,9 @@ s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, u8 command,
 	if (length > I2C_SMBUS_BLOCK_MAX)
 		length = I2C_SMBUS_BLOCK_MAX;
 	data.block[0] = length;
-	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-				I2C_SMBUS_READ, command,
-				I2C_SMBUS_I2C_BLOCK_DATA, &data);
+	status = i2c_smbus_throttle_xfer(client, I2C_SMBUS_READ, command,
+					 I2C_SMBUS_I2C_BLOCK_DATA, &data);
+
 	if (status < 0)
 		return status;
 
@@ -284,9 +287,8 @@ s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client, u8 command,
 		length = I2C_SMBUS_BLOCK_MAX;
 	data.block[0] = length;
 	memcpy(data.block + 1, values, length);
-	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
-			      I2C_SMBUS_WRITE, command,
-			      I2C_SMBUS_I2C_BLOCK_DATA, &data);
+	return i2c_smbus_throttle_xfer(client, I2C_SMBUS_WRITE, command,
+				       I2C_SMBUS_I2C_BLOCK_DATA, &data);
 }
 EXPORT_SYMBOL(i2c_smbus_write_i2c_block_data);
 
@@ -547,6 +549,71 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 }
 EXPORT_SYMBOL(i2c_smbus_xfer);
 
+static int i2c_smbus_throttle_enter(const struct i2c_client *client)
+		__acquires(&priv->throttle_lock)
+{
+	struct i2c_client_priv *priv;
+	ktime_t earliest;
+	int rc;
+
+	priv = to_i2c_client_priv(client);
+
+	if (i2c_in_atomic_xfer_mode()) {
+		if (!mutex_trylock(&priv->throttle_lock))
+			return -EAGAIN;
+	} else {
+		rc = mutex_lock_interruptible(&priv->throttle_lock);
+		if (rc)
+			return rc;
+	}
+	earliest = ktime_add_us(priv->last, priv->delay_us);
+
+	if (priv->delay_us && ktime_before(ktime_get(), earliest)) {
+		if (i2c_in_atomic_xfer_mode()) {
+			mutex_unlock(&priv->throttle_lock);
+			return -EAGAIN;
+		}
+
+		usleep_range(priv->delay_us, 2 * priv->delay_us);
+	}
+
+	return 0;
+}
+
+static void i2c_smbus_throttle_exit(const struct i2c_client *client)
+		__releases(&priv->throttle_lock)
+{
+	struct i2c_client_priv *priv;
+
+	priv = to_i2c_client_priv(client);
+
+	if (priv->delay_us)
+		priv->last = ktime_get();
+	mutex_unlock(&priv->throttle_lock);
+}
+
+static s32 i2c_smbus_throttle_xfer(const struct i2c_client *client,
+				   char read_write, u8 command, int protocol,
+				   union i2c_smbus_data *data)
+{
+	s32 res;
+
+	res = i2c_smbus_throttle_enter(client);
+	if (res)
+		return res;
+
+	res = __i2c_lock_bus_helper(client->adapter);
+	if (!res)
+		res = __i2c_smbus_xfer(client->adapter, client->addr,
+				       client->flags, read_write, command,
+				       protocol, data);
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+	i2c_smbus_throttle_exit(client);
+
+	return res;
+}
+
 s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 		     unsigned short flags, char read_write,
 		     u8 command, int protocol, union i2c_smbus_data *data)
@@ -715,3 +782,27 @@ int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
 }
 EXPORT_SYMBOL_GPL(of_i2c_setup_smbus_alert);
 #endif
+
+int i2c_smbus_throttle_client(struct i2c_client *client,
+			       unsigned long delay_us)
+{
+	struct i2c_client_priv *priv;
+	int rc;
+
+	priv = to_i2c_client_priv(client);
+
+	if (i2c_in_atomic_xfer_mode()) {
+		if (!mutex_trylock(&priv->throttle_lock))
+			return -EAGAIN;
+	} else {
+		rc = mutex_lock_interruptible(&priv->throttle_lock);
+		if (rc)
+			return rc;
+	}
+	priv->delay_us = delay_us;
+	priv->last = ktime_get();
+	mutex_unlock(&priv->throttle_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_throttle_client);
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 8ce261167a2d..c763c6c431f9 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -4,6 +4,28 @@
  */
 
 #include <linux/rwsem.h>
+#include <linux/i2c.h>
+#include <linux/timekeeping.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+
+struct i2c_client_priv {
+	struct i2c_client client;
+
+	/*
+	 * Per-client access throttling, described in terms of microsecond
+	 * delay between the end of the nth transfer and the start of the
+	 * (n+1)th transfer. Used to manage devices which respond poorly to
+	 * rapid back-to-back commands.
+	 *
+	 * Do it in a wrapper struct to preserve const-ness of the i2c_smbus_*
+	 * interfaces.
+	 */
+	struct mutex throttle_lock;
+	unsigned long delay_us;
+	ktime_t last;
+};
+#define to_i2c_client_priv(c) container_of(c, struct i2c_client_priv, client)
 
 struct i2c_devinfo {
 	struct list_head	list;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fc55ea41d323..adc91587d7fa 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -181,8 +181,11 @@ s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client,
 s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
 					      u8 command, u8 length,
 					      u8 *values);
+int i2c_smbus_throttle_client(struct i2c_client *client,
+			       unsigned long delay_us);
 int i2c_get_device_id(const struct i2c_client *client,
 		      struct i2c_device_identity *id);
+
 #endif /* I2C */
 
 /**
-- 
2.25.1

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

[Andrew Jeffery]

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.

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;
+module_param(smbus_delay_us, ulong, 0664);
+
 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) {
-- 
2.25.1

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

[Guenter Roeck]

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) {
> 

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

[Andrew Jeffery]

On Mon, 14 Sep 2020, at 23:44, Guenter Roeck wrote:
> 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.

Ah, this kind of info is what I was hoping for. Thanks.

> 
> > 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.

Yeah, 250us was what we came to after 5 minutes of playing with values and a 
logic analyzer, we didn't really take the time to determine a minimum with 
confidence. TI mentioned the minimum time between transfers in their test 
environment is 2.5ms, so 1ms is aggressive by comparison.

> 
> > +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. 

Certainly agree with the latter, and regarding the former, it was mostly a
convenient mechanism for me to experiment with values. I agree that it's
not something we would want to be changed arbitrarily by a system admin.

> I would suggest an implementation similar to other
> affected devices; again, see zl6100.c or ltc2978.c for examples.

Yes, thanks for these pointers, I will take a look.

Andrew

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

[Andrew Jeffery]

On Mon, 14 Sep 2020, at 23:44, Guenter Roeck wrote:
> 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. 

Can you clarify what you mean here? Initially I interpreted your statement as 
meaning "Don't impose delays on the UCD90160 when the issues have only been 
demonstrated with the UCD90320". But I've since looked at zl6100.c and its 
delay is also exposed as a module parameter, which makes me wonder whether it 
was unclear that smbus_delay_us here is specific to the driver's i2c_client and 
is not a delay imposed on all SMBus accesses from the associated master. That 
is, with the implementation I've posted here, other (non-UCD9000) devices on 
the same bus are _not_ impacted by this value.

> I would suggest an implementation similar to other
> affected devices; again, see zl6100.c or ltc2978.c for examples.

I've had a look at these two examples. As you suggest the delays in zl6100.c 
look pretty similar to what this series implements in the i2c core. I'm finding 
it hard to dislodge the feeling that open-coding the waits is error prone, but 
to avoid that and not implement the waits in the i2c core means having almost 
duplicate implementations of handlers for i2c_smbus_{read,write}*() and 
pmbus_{read,write}*() calls in the driver.

Andrew

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

[Guenter Roeck]

On Wed, Sep 16, 2020 at 02:51:08PM +0930, Andrew Jeffery wrote:
> 
> 
> On Mon, 14 Sep 2020, at 23:44, Guenter Roeck wrote:
> > 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. 
> 
> Can you clarify what you mean here? Initially I interpreted your statement as 
> meaning "Don't impose delays on the UCD90160 when the issues have only been 
> demonstrated with the UCD90320". But I've since looked at zl6100.c and its 

Yes, that is what I meant.

> delay is also exposed as a module parameter, which makes me wonder whether it 

My bad. Not how I would implement it today. I'd also not use udelay() if I were
to re-implement this today.

> was unclear that smbus_delay_us here is specific to the driver's i2c_client and 
> is not a delay imposed on all SMBus accesses from the associated master. That 
> is, with the implementation I've posted here, other (non-UCD9000) devices on 
> the same bus are _not_ impacted by this value.
> 
The delay is specific to the driver's i2c client.

> > I would suggest an implementation similar to other
> > affected devices; again, see zl6100.c or ltc2978.c for examples.
> 
> I've had a look at these two examples. As you suggest the delays in zl6100.c 
> look pretty similar to what this series implements in the i2c core. I'm finding 
> it hard to dislodge the feeling that open-coding the waits is error prone, but 
> to avoid that and not implement the waits in the i2c core means having almost 
> duplicate implementations of handlers for i2c_smbus_{read,write}*() and 
> pmbus_{read,write}*() calls in the driver.
> 

Not sure I can follow you here. Anyway, it seems to me that you are set on
an implementation in the i2c core. I personally don't like that approach,
but I'll accept a change in the ucd9000 driver to make use of it. Please
leave the zl6100 code alone, though - it took me long enough to get that
working, and I won't have time to test any changes.

Thanks,
Guenter

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

[Andrew Jeffery]

On Thu, 17 Sep 2020, at 01:26, Guenter Roeck wrote:
> > I've had a look at these two examples. As you suggest the delays in zl6100.c 
> > look pretty similar to what this series implements in the i2c core. I'm finding 
> > it hard to dislodge the feeling that open-coding the waits is error prone, but 
> > to avoid that and not implement the waits in the i2c core means having almost 
> > duplicate implementations of handlers for i2c_smbus_{read,write}*() and 
> > pmbus_{read,write}*() calls in the driver.
> > 
> 
> Not sure I can follow you here. Anyway, it seems to me that you are set on
> an implementation in the i2c core. I personally don't like that approach,

Not really set on it, but it does seem convenient. I'm looking at whether 
delays resolve the issues we have with the max31785 as well (I have a bunch of 
patches that introduce retries under the various circumstances we've hit poor 
behaviour).

> but I'll accept a change in the ucd9000 driver to make use of it. Please
> leave the zl6100 code alone, though - it took me long enough to get that
> working, and I won't have time to test any changes.

No worries. If you don't have time to test changes it reduces the motivation to 
find a general approach, and so maybe isolating the work-arounds to the ucd9000 
is the way to go.

Thanks for the feedback.

Andrew

Re: [RFC PATCH 0/2] Throttle I2C transfers to UCD9000 devices

[Guenter Roeck]

On 9/14/20 5:28 AM, Andrew Jeffery wrote:
> Hello,
> 
> While working with system designs making use of TI's UCD90320 Power
> Sequencer we've found that communication with the device isn't terribly
> reliable.
> 
> It appears that back-to-back transfers where commands addressed to the
> device are put onto the bus with intervals between STOP and START in the
> neighbourhood of 250us or less can cause bad behaviour. This primarily
> happens during driver probe while scanning the device to determine its
> capabilities.
> 
> We have observed the device causing excessive clock stretches and bus
> lockups, and also corruption of the device's volatile state (requiring it
> to be reset).  The latter is particularly disruptive in that the controlled
> rails are brought down either by:
> 
> 1. The corruption causing a fault condition, or
> 2. Asserting the device's reset line to recover
> 
> A further observation is that pacing transfers to the device appears to
> mitigate the bad behaviour. We're in discussion with TI to better
> understand the limitations and at least get the behaviour documented.
> 
> This short series implements the mitigation in terms of a throttle in the
> i2c_client associated with the device's driver. Before the first
> communication with the device in the probe() of ucd9000 we configure the
> i2c_client to throttle transfers with a minimum of a 1ms delay (with the
> delay exposed as a module parameter).
> 
> The series is RFC for several reasons:
> 
> The first is to sus out feelings on the general direction. The problem is
> pretty unfortunate - are there better ways to implement the mitigation?
> 
> If there aren't, then:
> 
> I'd like thoughts on whether we want to account for i2c-dev clients.
> Implementing throttling in i2c_client feels like a solution-by-proxy as the
> throttling is really a property of the targeted device, but we don't have a
> coherent representation between platform devices and devices associated
> with i2c-dev clients. At the moment we'd have to resort to address-based
> lookups for platform data stashed in the transfer functions.
> 
> Next is that I've only implemented throttling for SMBus devices. I don't
> yet have a use-case for throttling non-SMBus devices so I'm not sure it's
> worth poking at it, but would appreciate thoughts there.
> 
> Further, I've had a bit of a stab at dealing with atomic transfers that's
> not been tested. Hopefully it makes sense.
> 
> Finally I'm also interested in feedback on exposing the control in a little
> more general manner than having to implement a module parameter in all
> drivers that want to take advantage of throttling. This isn't a big problem
> at the moment, but if anyone has thoughts there then I'm happy to poke at
> those too.
> 

As mentioned in patch 2/2, I don't think a module parameter is a good idea.
I think this should be implemented on driver level, similar to zl6100.c,
it should be limited to affected devices and not be user controllable.

In respect to implementation in the i2c core vs in drivers: So far we
encountered this problem for some Zilker labs devices and for some LTC
devices. While the solution needed here looks similar to the solution
implemented for Zilker labs devices, the solution for LTC devices is
different. I am not sure if an implementation in the i2c core is
desirable. It looks quite invasive to me, and it won't solve the problem
for all devices since it isn't always a simple "wait <n> microseconds
between accesses". For example, some devices may require a wait after
a write but not after a read, or a wait only after certain commands (such
as commands writing to an EEPROM). Other devices may require a mechanism
different to "wait a certain period of time". It seems all but impossible
to implement a generic mechanism on i2c level.

Thanks,
Guenter

> Please review!
> 
> Andrew
> 
> Andrew Jeffery (2):
>   i2c: smbus: Allow throttling of transfers to client devices
>   hwmon: (pmbus/ucd9000) Throttle SMBus transfers to avoid poor
>     behaviour
> 
>  drivers/hwmon/pmbus/ucd9000.c |   6 ++
>  drivers/i2c/i2c-core-base.c   |   8 +-
>  drivers/i2c/i2c-core-smbus.c  | 149 +++++++++++++++++++++++++++-------
>  drivers/i2c/i2c-core.h        |  22 +++++
>  include/linux/i2c.h           |   3 +
>  5 files changed, 157 insertions(+), 31 deletions(-)
> 

Re: [RFC PATCH 0/2] Throttle I2C transfers to UCD9000 devices

[Andrew Jeffery]

On Tue, 15 Sep 2020, at 02:13, Guenter Roeck wrote:
> On 9/14/20 5:28 AM, Andrew Jeffery wrote:
> > Hello,
> > 
> > While working with system designs making use of TI's UCD90320 Power
> > Sequencer we've found that communication with the device isn't terribly
> > reliable.
> > 
> > It appears that back-to-back transfers where commands addressed to the
> > device are put onto the bus with intervals between STOP and START in the
> > neighbourhood of 250us or less can cause bad behaviour. This primarily
> > happens during driver probe while scanning the device to determine its
> > capabilities.
> > 
> > We have observed the device causing excessive clock stretches and bus
> > lockups, and also corruption of the device's volatile state (requiring it
> > to be reset).  The latter is particularly disruptive in that the controlled
> > rails are brought down either by:
> > 
> > 1. The corruption causing a fault condition, or
> > 2. Asserting the device's reset line to recover
> > 
> > A further observation is that pacing transfers to the device appears to
> > mitigate the bad behaviour. We're in discussion with TI to better
> > understand the limitations and at least get the behaviour documented.
> > 
> > This short series implements the mitigation in terms of a throttle in the
> > i2c_client associated with the device's driver. Before the first
> > communication with the device in the probe() of ucd9000 we configure the
> > i2c_client to throttle transfers with a minimum of a 1ms delay (with the
> > delay exposed as a module parameter).
> > 
> > The series is RFC for several reasons:
> > 
> > The first is to sus out feelings on the general direction. The problem is
> > pretty unfortunate - are there better ways to implement the mitigation?
> > 
> > If there aren't, then:
> > 
> > I'd like thoughts on whether we want to account for i2c-dev clients.
> > Implementing throttling in i2c_client feels like a solution-by-proxy as the
> > throttling is really a property of the targeted device, but we don't have a
> > coherent representation between platform devices and devices associated
> > with i2c-dev clients. At the moment we'd have to resort to address-based
> > lookups for platform data stashed in the transfer functions.
> > 
> > Next is that I've only implemented throttling for SMBus devices. I don't
> > yet have a use-case for throttling non-SMBus devices so I'm not sure it's
> > worth poking at it, but would appreciate thoughts there.
> > 
> > Further, I've had a bit of a stab at dealing with atomic transfers that's
> > not been tested. Hopefully it makes sense.
> > 
> > Finally I'm also interested in feedback on exposing the control in a little
> > more general manner than having to implement a module parameter in all
> > drivers that want to take advantage of throttling. This isn't a big problem
> > at the moment, but if anyone has thoughts there then I'm happy to poke at
> > those too.
> > 
> 
> As mentioned in patch 2/2, I don't think a module parameter is a good idea.
> I think this should be implemented on driver level, similar to zl6100.c,
> it should be limited to affected devices and not be user controllable.

Yep. I will look at zl6100.c.

> 
> In respect to implementation in the i2c core vs in drivers: So far we
> encountered this problem for some Zilker labs devices and for some LTC
> devices. While the solution needed here looks similar to the solution
> implemented for Zilker labs devices, the solution for LTC devices is
> different. I am not sure if an implementation in the i2c core is
> desirable. It looks quite invasive to me, and it won't solve the problem
> for all devices since it isn't always a simple "wait <n> microseconds
> between accesses". For example, some devices may require a wait after
> a write but not after a read, or a wait only after certain commands (such
> as commands writing to an EEPROM). Other devices may require a mechanism
> different to "wait a certain period of time". It seems all but impossible
> to implement a generic mechanism on i2c level.

Yep, that's fair. I went this route to avoid implementing two sets of handlers 
providing the pacing in the driver (for before and after we register with the 
pmbus core), but it is invasive as you point out. Let me look at your suggested 
alternatives and get back to you.

Andrew

Re: [RFC PATCH 0/2] Throttle I2C transfers to UCD9000 devices

[Andrew Jeffery]

On Tue, 15 Sep 2020, at 02:13, Guenter Roeck wrote:
> On 9/14/20 5:28 AM, Andrew Jeffery wrote:
> > Hello,
> > 
> > While working with system designs making use of TI's UCD90320 Power
> > Sequencer we've found that communication with the device isn't terribly
> > reliable.
> > 
> > It appears that back-to-back transfers where commands addressed to the
> > device are put onto the bus with intervals between STOP and START in the
> > neighbourhood of 250us or less can cause bad behaviour. This primarily
> > happens during driver probe while scanning the device to determine its
> > capabilities.
> > 
> > We have observed the device causing excessive clock stretches and bus
> > lockups, and also corruption of the device's volatile state (requiring it
> > to be reset).  The latter is particularly disruptive in that the controlled
> > rails are brought down either by:
> > 
> > 1. The corruption causing a fault condition, or
> > 2. Asserting the device's reset line to recover
> > 
> > A further observation is that pacing transfers to the device appears to
> > mitigate the bad behaviour. We're in discussion with TI to better
> > understand the limitations and at least get the behaviour documented.
> > 
> > This short series implements the mitigation in terms of a throttle in the
> > i2c_client associated with the device's driver. Before the first
> > communication with the device in the probe() of ucd9000 we configure the
> > i2c_client to throttle transfers with a minimum of a 1ms delay (with the
> > delay exposed as a module parameter).
> > 
> > The series is RFC for several reasons:
> > 
> > The first is to sus out feelings on the general direction. The problem is
> > pretty unfortunate - are there better ways to implement the mitigation?
> > 
> > If there aren't, then:
> > 
> > I'd like thoughts on whether we want to account for i2c-dev clients.
> > Implementing throttling in i2c_client feels like a solution-by-proxy as the
> > throttling is really a property of the targeted device, but we don't have a
> > coherent representation between platform devices and devices associated
> > with i2c-dev clients. At the moment we'd have to resort to address-based
> > lookups for platform data stashed in the transfer functions.
> > 
> > Next is that I've only implemented throttling for SMBus devices. I don't
> > yet have a use-case for throttling non-SMBus devices so I'm not sure it's
> > worth poking at it, but would appreciate thoughts there.
> > 
> > Further, I've had a bit of a stab at dealing with atomic transfers that's
> > not been tested. Hopefully it makes sense.
> > 
> > Finally I'm also interested in feedback on exposing the control in a little
> > more general manner than having to implement a module parameter in all
> > drivers that want to take advantage of throttling. This isn't a big problem
> > at the moment, but if anyone has thoughts there then I'm happy to poke at
> > those too.
> > 
> 
> As mentioned in patch 2/2, I don't think a module parameter is a good idea.
> I think this should be implemented on driver level, similar to zl6100.c,
> it should be limited to affected devices and not be user controllable.
> 
> In respect to implementation in the i2c core vs in drivers: So far we
> encountered this problem for some Zilker labs devices and for some LTC
> devices. While the solution needed here looks similar to the solution
> implemented for Zilker labs devices, the solution for LTC devices is
> different. I am not sure if an implementation in the i2c core is
> desirable. It looks quite invasive to me, and it won't solve the problem
> for all devices since it isn't always a simple "wait <n> microseconds
> between accesses". For example, some devices may require a wait after
> a write but not after a read, or a wait only after certain commands (such
> as commands writing to an EEPROM). Other devices may require a mechanism
> different to "wait a certain period of time". It seems all but impossible
> to implement a generic mechanism on i2c level.

So I think it could be handled with an optional i2c client callback: e.g.

struct i2c_client {
...
bool (*prepare_device)(const struct i2c_client *client);
}

This way the logic to delay is kept inside the driver, catering to both the 
Zilker and the LTC devices. If the problem exists only after specific 
operations then we can stash some state in the client in the same way I've done 
in patch 1, test that state in the callback and only do the "preparation" if 
it's necessary.

I can knock that up and post another RFC, just so we can get a feel for how 
that solution looks.

Andrew