On 15.06.2023 23:45, Andi Shyti wrote:
> 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@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?
>>>
>> 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.
>
In case of i801 this bus driver is a pci_driver and the remove callback
is handled by the PCI core (pci_device_remove()), I2C core isn't involved.
At a first glance I fail to see how we could inject I2C-specific code.
> 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@xxxxxxxxxx>
>
> Thank you,
> Andi
>
>>> I'm OK to r-b this, but i want first Jean to give it an ack.
>>>
>>> Andi
>>
