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

[Andi Shyti <andi.shyti@kernel.org>]

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
>