| Message ID | a2cd5692-7a9e-7fef-6c09-6c694df1b23e@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: i801: next set of improvements | expand |
On 15.06.2023 00:24, Andi Shyti wrote: > Hi Heiner, > > 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? > I think yes. Otherwise we may interrupt an active transfer, or a user may start a transfer whilst we are in cleanup. Note: i2c_mark_adapter_suspended() takes the I2C bus lock, therefore it will sleep until an active transfer is finished. > I'm OK with it, though. > >> /* Restore config registers to avoid hard hang on some systems */ >> outb_p(priv->original_hstcnt, SMBHSTCNT(priv)); >> i801_disable_host_notify(priv); >> @@ -1807,6 +1811,7 @@ static int i801_suspend(struct device *dev) >> { >> struct i801_priv *priv = dev_get_drvdata(dev); >> >> + i2c_mark_adapter_suspended(&priv->adapter); >> outb_p(priv->original_hstcnt, SMBHSTCNT(priv)); >> pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg); >> return 0; >> @@ -1818,6 +1823,7 @@ static int i801_resume(struct device *dev) >> >> i801_setup_hstcfg(priv); >> i801_enable_host_notify(&priv->adapter); >> + i2c_mark_adapter_resumed(&priv->adapter); > > BTW, I see that very few drivers are using suspended and resumed > and I wonder why. Should these perhaps be added in the basic pm > functions? > For my understanding, which functions are you referring to? > I'm OK to r-b this, but i want first Jean to give it an ack. > > Andi
Hi Heiner, On Thu, Jun 15, 2023 at 11:17:12PM +0200, Heiner Kallweit wrote: > On 15.06.2023 00:24, Andi Shyti wrote: > > Hi Heiner, > > > > 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? > > > I think yes. Otherwise we may interrupt an active transfer, or a user > may start a transfer whilst we are in cleanup. > Note: i2c_mark_adapter_suspended() takes the I2C bus lock, therefore it > will sleep until an active transfer is finished. yes, I think you are right. > > I'm OK with it, though. > > > >> /* Restore config registers to avoid hard hang on some systems */ > >> outb_p(priv->original_hstcnt, SMBHSTCNT(priv)); > >> i801_disable_host_notify(priv); > >> @@ -1807,6 +1811,7 @@ static int i801_suspend(struct device *dev) > >> { > >> struct i801_priv *priv = dev_get_drvdata(dev); > >> > >> + i2c_mark_adapter_suspended(&priv->adapter); > >> outb_p(priv->original_hstcnt, SMBHSTCNT(priv)); > >> pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg); > >> return 0; > >> @@ -1818,6 +1823,7 @@ static int i801_resume(struct device *dev) > >> > >> i801_setup_hstcfg(priv); > >> i801_enable_host_notify(&priv->adapter); > >> + i2c_mark_adapter_resumed(&priv->adapter); > > > > BTW, I see that very few drivers are using suspended and resumed > > and I wonder why. Should these perhaps be added in the basic pm > > functions? > > > For my understanding, which functions are you referring to? I am referring about having a more generalised pm function which can mark the i2c adapter supsended or resumed even before or after the driver specific functions are called. This way all drivers can benefit from it. In any case this out of the scope of this patch. I'm going to give my approval, if then Jean has something to say, I guess there is still time to chime in. Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thank you, Andi > > I'm OK to r-b this, but i want first Jean to give it an ack. > > > > Andi >
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,
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); + /* Restore config registers to avoid hard hang on some systems */ outb_p(priv->original_hstcnt, SMBHSTCNT(priv)); i801_disable_host_notify(priv); @@ -1807,6 +1811,7 @@ static int i801_suspend(struct device *dev) { struct i801_priv *priv = dev_get_drvdata(dev); + i2c_mark_adapter_suspended(&priv->adapter); outb_p(priv->original_hstcnt, SMBHSTCNT(priv)); pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg); return 0; @@ -1818,6 +1823,7 @@ static int i801_resume(struct device *dev) i801_setup_hstcfg(priv); i801_enable_host_notify(&priv->adapter); + i2c_mark_adapter_resumed(&priv->adapter); return 0; }
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(+)