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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 26.06.2023 19:20, Jean Delvare wrote:
> 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.
> 
I think there's a little bit more to be done for proper cleanup
in reverse order in remove() and in the error path of probe().
I'll come up with a patch.

> 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,