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@xxxxxxxxx> > > --- > > 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