Re: [PATCH 1/4] i2c: i801: Use i2c_mark_adapter_suspended/resumed

[Jean Delvare <jdelvare@suse.de>]

Hi Andi, Heiner,

Adding Wolfram Sang who introduced the i2c_mark_adapter_suspended() API
originally.

On Thu, 15 Jun 2023 00:24:39 +0200, Andi Shyti wrote:
> On Sat, Mar 04, 2023 at 10:31:23PM +0100, Heiner Kallweit wrote:
> > When entering the shutdown/remove/suspend callbacks, at first we should
> > ensure that transfers are finished and I2C core can't start further
> > transfers. Use i2c_mark_adapter_suspended() for this purpose.
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > ---
> >  drivers/i2c/busses/i2c-i801.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index ac5326747..d6a0c3b53 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1773,6 +1773,8 @@ static void i801_remove(struct pci_dev *dev)
> >  {
> >  	struct i801_priv *priv = pci_get_drvdata(dev);
> >  
> > +	i2c_mark_adapter_suspended(&priv->adapter);
> > +
> >  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
> >  	i801_disable_host_notify(priv);
> >  	i801_del_mux(priv);
> > @@ -1796,6 +1798,8 @@ static void i801_shutdown(struct pci_dev *dev)
> >  {
> >  	struct i801_priv *priv = pci_get_drvdata(dev);
> >  
> > +	i2c_mark_adapter_suspended(&priv->adapter);
> > +  
> 
> is this really needed in the shutdown and remove function?

The very same question came to my mind. I would really expect the
driver core to do all the reference counting needed so that a device
can't possibly be removed when any of its children is still active. If
that's not the case then something is very wrong in the device driver
model itself, and I certainly hope that the proper fix wouldn't be
subsystem-specific and implemented in every device driver separately.

FWIW, I see 13 I2C bus drivers calling i2c_mark_adapter_suspended() at
the moment, and only one of them is calling it in shutdown
(i2c-qcom-geni). None of them is calling it in remove. If that's not
needed for other drivers then I can't see why that would be needed for
i2c-i801.

As far as the remove() path is concerned, my expectation is that if
everything is undone in the opposite way of the probe() path then
everything should be fine. It turns out this is not the case of the
current i2c-i801 driver. The original HSTCNT register value is being
restored too early in i801_remove(). I'm to blame for this, the bug was
introduced by commit 9b5bf5878138 ("i2c: i801: Restore INTREN on
unload") which is mine. This should be fixed separately before any
other change.

Once this is fixed, unless you are able to actually trigger a bug in
the remove() path, then I see no good reason to add
i2c_mark_adapter_suspended() to that code path.

For shutdown, I'm unsure. Wolfram, what's your take?

Thanks,
-- 
Jean Delvare
SUSE L3 Support