Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 16 Sep 2020 08:56:51 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Andrew Jeffery <andrew@aj.id.au>
Cc:     linux-hwmon@vger.kernel.org, linux-i2c@vger.kernel.org,
        Jean Delvare <jdelvare@suse.com>, wsa@kernel.org,
        Joel Stanley <joel@jms.id.au>, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] hwmon: (pmbus/ucd9000) Throttle SMBus transfers
 to avoid poor behaviour
Message-ID: <20200916155651.GA90122@roeck-us.net>
References: <20200914122811.3295678-1-andrew@aj.id.au>
 <20200914122811.3295678-3-andrew@aj.id.au>
 <71067b18-c4bc-533a-0069-f21069c5fd0d@roeck-us.net>
 <48962472-b025-4b0d-90e9-60469bebf206@www.fastmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <48962472-b025-4b0d-90e9-60469bebf206@www.fastmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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