[PATCH 0/4] i2c: i801: next set of improvements

[PATCH 0/4] i2c: i801: next set of improvements

[Heiner Kallweit]

Next set of improvements.

Heiner Kallweit (4):
  i2c: i801: Use i2c_mark_adapter_suspended/resumed
  i2c: i801: Replace acpi_lock with I2C bus lock
  i2c: i801: Improve i801_block_transaction_byte_by_byte
  i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS

 drivers/i2c/busses/i2c-i801.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

-- 
2.39.2

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

[Heiner Kallweit]

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);
+
 	/* 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;
 }
-- 
2.39.2

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

[Andi Shyti]

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'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?

I'm OK to r-b this, but i want first Jean to give it an ack.

Andi

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

[Heiner Kallweit]

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

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

[Andi Shyti]

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
> 

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

[Heiner Kallweit]

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@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.
> 
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@kernel.org> 
> 
> Thank you,
> Andi
> 
>>> I'm OK to r-b this, but i want first Jean to give it an ack.
>>>
>>> Andi
>>

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

[Jean Delvare]

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,
-- 
Jean Delvare
SUSE L3 Support

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

[Heiner Kallweit]

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,

[PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Heiner Kallweit]

I2C core ensures in i2c_smbus_xfer() that the I2C bus lock is held when
calling the smbus_xfer callback. That's i801_access() in our case.
I think it's safe in general to assume that the I2C bus lock is held
when the smbus_xfer callback is called.
Therefore I see no need to define an own mutex.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index d6a0c3b53..7641bd0ac 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -289,10 +289,9 @@ struct i801_priv {
 
 	/*
 	 * If set to true the host controller registers are reserved for
-	 * ACPI AML use. Protected by acpi_lock.
+	 * ACPI AML use.
 	 */
 	bool acpi_reserved;
-	struct mutex acpi_lock;
 };
 
 #define FEATURE_SMBUS_PEC	BIT(0)
@@ -871,11 +870,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	int hwpec, ret;
 	struct i801_priv *priv = i2c_get_adapdata(adap);
 
-	mutex_lock(&priv->acpi_lock);
-	if (priv->acpi_reserved) {
-		mutex_unlock(&priv->acpi_lock);
+	if (priv->acpi_reserved)
 		return -EBUSY;
-	}
 
 	pm_runtime_get_sync(&priv->pci_dev->dev);
 
@@ -916,7 +912,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 
 	pm_runtime_mark_last_busy(&priv->pci_dev->dev);
 	pm_runtime_put_autosuspend(&priv->pci_dev->dev);
-	mutex_unlock(&priv->acpi_lock);
 	return ret;
 }
 
@@ -1566,7 +1561,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
 	 * further access from the driver itself. This device is now owned
 	 * by the system firmware.
 	 */
-	mutex_lock(&priv->acpi_lock);
+	i2c_lock_bus(&priv->adapter, I2C_LOCK_SEGMENT);
 
 	if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) {
 		priv->acpi_reserved = true;
@@ -1586,7 +1581,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
 	else
 		status = acpi_os_write_port(address, (u32)*value, bits);
 
-	mutex_unlock(&priv->acpi_lock);
+	i2c_unlock_bus(&priv->adapter, I2C_LOCK_SEGMENT);
 
 	return status;
 }
@@ -1640,7 +1635,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	priv->adapter.dev.parent = &dev->dev;
 	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
 	priv->adapter.retries = 3;
-	mutex_init(&priv->acpi_lock);
 
 	priv->pci_dev = dev;
 	priv->features = id->driver_data;
-- 
2.39.2

Re: [PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Andi Shyti]

Hi Heiner,

On Sat, Mar 04, 2023 at 10:33:05PM +0100, Heiner Kallweit wrote:
> I2C core ensures in i2c_smbus_xfer() that the I2C bus lock is held when
> calling the smbus_xfer callback. That's i801_access() in our case.
> I think it's safe in general to assume that the I2C bus lock is held
> when the smbus_xfer callback is called.
> Therefore I see no need to define an own mutex.

I think it makes sense... unless I missed something I don't see
anything else being racy in i801_access().

Have you checked i801_acpi_io_handler()?

Andi

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index d6a0c3b53..7641bd0ac 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -289,10 +289,9 @@ struct i801_priv {
>  
>  	/*
>  	 * If set to true the host controller registers are reserved for
> -	 * ACPI AML use. Protected by acpi_lock.
> +	 * ACPI AML use.
>  	 */
>  	bool acpi_reserved;
> -	struct mutex acpi_lock;
>  };
>  
>  #define FEATURE_SMBUS_PEC	BIT(0)
> @@ -871,11 +870,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	int hwpec, ret;
>  	struct i801_priv *priv = i2c_get_adapdata(adap);
>  
> -	mutex_lock(&priv->acpi_lock);
> -	if (priv->acpi_reserved) {
> -		mutex_unlock(&priv->acpi_lock);
> +	if (priv->acpi_reserved)
>  		return -EBUSY;
> -	}
>  
>  	pm_runtime_get_sync(&priv->pci_dev->dev);
>  
> @@ -916,7 +912,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  
>  	pm_runtime_mark_last_busy(&priv->pci_dev->dev);
>  	pm_runtime_put_autosuspend(&priv->pci_dev->dev);
> -	mutex_unlock(&priv->acpi_lock);
>  	return ret;
>  }
>  
> @@ -1566,7 +1561,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
>  	 * further access from the driver itself. This device is now owned
>  	 * by the system firmware.
>  	 */
> -	mutex_lock(&priv->acpi_lock);
> +	i2c_lock_bus(&priv->adapter, I2C_LOCK_SEGMENT);
>  
>  	if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) {
>  		priv->acpi_reserved = true;
> @@ -1586,7 +1581,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
>  	else
>  		status = acpi_os_write_port(address, (u32)*value, bits);
>  
> -	mutex_unlock(&priv->acpi_lock);
> +	i2c_unlock_bus(&priv->adapter, I2C_LOCK_SEGMENT);
>  
>  	return status;
>  }
> @@ -1640,7 +1635,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	priv->adapter.dev.parent = &dev->dev;
>  	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
>  	priv->adapter.retries = 3;
> -	mutex_init(&priv->acpi_lock);
>  
>  	priv->pci_dev = dev;
>  	priv->features = id->driver_data;
> -- 
> 2.39.2
> 
> 

Re: [PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Heiner Kallweit]

On 15.06.2023 00:31, Andi Shyti wrote:
> Hi Heiner,
> 
> On Sat, Mar 04, 2023 at 10:33:05PM +0100, Heiner Kallweit wrote:
>> I2C core ensures in i2c_smbus_xfer() that the I2C bus lock is held when
>> calling the smbus_xfer callback. That's i801_access() in our case.
>> I think it's safe in general to assume that the I2C bus lock is held
>> when the smbus_xfer callback is called.
>> Therefore I see no need to define an own mutex.
> 
> I think it makes sense... unless I missed something I don't see
> anything else being racy in i801_access().
> 
> Have you checked i801_acpi_io_handler()?
> 
acpi_os_read_port() resolves to a simple inb() et al.
Therefore I don't see anything in i801_acpi_io_handler()
that would speak against using the I2C bus lock.

> Andi
> 
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 14 ++++----------
>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index d6a0c3b53..7641bd0ac 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -289,10 +289,9 @@ struct i801_priv {
>>  
>>  	/*
>>  	 * If set to true the host controller registers are reserved for
>> -	 * ACPI AML use. Protected by acpi_lock.
>> +	 * ACPI AML use.
>>  	 */
>>  	bool acpi_reserved;
>> -	struct mutex acpi_lock;
>>  };
>>  
>>  #define FEATURE_SMBUS_PEC	BIT(0)
>> @@ -871,11 +870,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>  	int hwpec, ret;
>>  	struct i801_priv *priv = i2c_get_adapdata(adap);
>>  
>> -	mutex_lock(&priv->acpi_lock);
>> -	if (priv->acpi_reserved) {
>> -		mutex_unlock(&priv->acpi_lock);
>> +	if (priv->acpi_reserved)
>>  		return -EBUSY;
>> -	}
>>  
>>  	pm_runtime_get_sync(&priv->pci_dev->dev);
>>  
>> @@ -916,7 +912,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>  
>>  	pm_runtime_mark_last_busy(&priv->pci_dev->dev);
>>  	pm_runtime_put_autosuspend(&priv->pci_dev->dev);
>> -	mutex_unlock(&priv->acpi_lock);
>>  	return ret;
>>  }
>>  
>> @@ -1566,7 +1561,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
>>  	 * further access from the driver itself. This device is now owned
>>  	 * by the system firmware.
>>  	 */
>> -	mutex_lock(&priv->acpi_lock);
>> +	i2c_lock_bus(&priv->adapter, I2C_LOCK_SEGMENT);
>>  
>>  	if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) {
>>  		priv->acpi_reserved = true;
>> @@ -1586,7 +1581,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
>>  	else
>>  		status = acpi_os_write_port(address, (u32)*value, bits);
>>  
>> -	mutex_unlock(&priv->acpi_lock);
>> +	i2c_unlock_bus(&priv->adapter, I2C_LOCK_SEGMENT);
>>  
>>  	return status;
>>  }
>> @@ -1640,7 +1635,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>  	priv->adapter.dev.parent = &dev->dev;
>>  	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
>>  	priv->adapter.retries = 3;
>> -	mutex_init(&priv->acpi_lock);
>>  
>>  	priv->pci_dev = dev;
>>  	priv->features = id->driver_data;
>> -- 
>> 2.39.2
>>
>>

Re: [PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Andi Shyti]

Hi Heiner,

On Sat, Mar 04, 2023 at 10:33:05PM +0100, Heiner Kallweit wrote:
> I2C core ensures in i2c_smbus_xfer() that the I2C bus lock is held when
> calling the smbus_xfer callback. That's i801_access() in our case.
> I think it's safe in general to assume that the I2C bus lock is held
> when the smbus_xfer callback is called.
> Therefore I see no need to define an own mutex.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Thanks,
Andi

Re: [PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Jean Delvare]

Hi Heiner,

On Sat, 04 Mar 2023 22:33:05 +0100, Heiner Kallweit wrote:
> I2C core ensures in i2c_smbus_xfer() that the I2C bus lock is held when
> calling the smbus_xfer callback. That's i801_access() in our case.
> I think it's safe in general to assume that the I2C bus lock is held
> when the smbus_xfer callback is called.
> Therefore I see no need to define an own mutex.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index d6a0c3b53..7641bd0ac 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -289,10 +289,9 @@ struct i801_priv {
>  
>  	/*
>  	 * If set to true the host controller registers are reserved for
> -	 * ACPI AML use. Protected by acpi_lock.
> +	 * ACPI AML use.
>  	 */
>  	bool acpi_reserved;
> -	struct mutex acpi_lock;
>  };
>  
>  #define FEATURE_SMBUS_PEC	BIT(0)
> @@ -871,11 +870,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	int hwpec, ret;
>  	struct i801_priv *priv = i2c_get_adapdata(adap);
>  
> -	mutex_lock(&priv->acpi_lock);
> -	if (priv->acpi_reserved) {
> -		mutex_unlock(&priv->acpi_lock);
> +	if (priv->acpi_reserved)
>  		return -EBUSY;
> -	}
>  
>  	pm_runtime_get_sync(&priv->pci_dev->dev);
>  
> @@ -916,7 +912,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  
>  	pm_runtime_mark_last_busy(&priv->pci_dev->dev);
>  	pm_runtime_put_autosuspend(&priv->pci_dev->dev);
> -	mutex_unlock(&priv->acpi_lock);
>  	return ret;
>  }
>  
> @@ -1566,7 +1561,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
>  	 * further access from the driver itself. This device is now owned
>  	 * by the system firmware.
>  	 */
> -	mutex_lock(&priv->acpi_lock);
> +	i2c_lock_bus(&priv->adapter, I2C_LOCK_SEGMENT);
>  
>  	if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) {
>  		priv->acpi_reserved = true;
> @@ -1586,7 +1581,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
>  	else
>  		status = acpi_os_write_port(address, (u32)*value, bits);
>  
> -	mutex_unlock(&priv->acpi_lock);
> +	i2c_unlock_bus(&priv->adapter, I2C_LOCK_SEGMENT);
>  
>  	return status;
>  }
> @@ -1640,7 +1635,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	priv->adapter.dev.parent = &dev->dev;
>  	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
>  	priv->adapter.retries = 3;
> -	mutex_init(&priv->acpi_lock);
>  
>  	priv->pci_dev = dev;
>  	priv->features = id->driver_data;

Looks reasonable, I also can't see any reason why that wouldn't work.
But locking and power management can be tricky of course. I'll test
this for some time, but I don't think my system actually suffers from
this ACPI resource conflict, so this most probably won't be testing
much in practice.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Heiner Kallweit]

On 26.06.2023 19:59, Jean Delvare wrote:
> Hi Heiner,
> 
> On Sat, 04 Mar 2023 22:33:05 +0100, Heiner Kallweit wrote:
>> I2C core ensures in i2c_smbus_xfer() that the I2C bus lock is held when
>> calling the smbus_xfer callback. That's i801_access() in our case.
>> I think it's safe in general to assume that the I2C bus lock is held
>> when the smbus_xfer callback is called.
>> Therefore I see no need to define an own mutex.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 14 ++++----------
>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index d6a0c3b53..7641bd0ac 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -289,10 +289,9 @@ struct i801_priv {
>>  
>>  	/*
>>  	 * If set to true the host controller registers are reserved for
>> -	 * ACPI AML use. Protected by acpi_lock.
>> +	 * ACPI AML use.
>>  	 */
>>  	bool acpi_reserved;
>> -	struct mutex acpi_lock;
>>  };
>>  
>>  #define FEATURE_SMBUS_PEC	BIT(0)
>> @@ -871,11 +870,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>  	int hwpec, ret;
>>  	struct i801_priv *priv = i2c_get_adapdata(adap);
>>  
>> -	mutex_lock(&priv->acpi_lock);
>> -	if (priv->acpi_reserved) {
>> -		mutex_unlock(&priv->acpi_lock);
>> +	if (priv->acpi_reserved)
>>  		return -EBUSY;
>> -	}
>>  
>>  	pm_runtime_get_sync(&priv->pci_dev->dev);
>>  
>> @@ -916,7 +912,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>  
>>  	pm_runtime_mark_last_busy(&priv->pci_dev->dev);
>>  	pm_runtime_put_autosuspend(&priv->pci_dev->dev);
>> -	mutex_unlock(&priv->acpi_lock);
>>  	return ret;
>>  }
>>  
>> @@ -1566,7 +1561,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
>>  	 * further access from the driver itself. This device is now owned
>>  	 * by the system firmware.
>>  	 */
>> -	mutex_lock(&priv->acpi_lock);
>> +	i2c_lock_bus(&priv->adapter, I2C_LOCK_SEGMENT);
>>  
>>  	if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) {
>>  		priv->acpi_reserved = true;
>> @@ -1586,7 +1581,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
>>  	else
>>  		status = acpi_os_write_port(address, (u32)*value, bits);
>>  
>> -	mutex_unlock(&priv->acpi_lock);
>> +	i2c_unlock_bus(&priv->adapter, I2C_LOCK_SEGMENT);
>>  
>>  	return status;
>>  }
>> @@ -1640,7 +1635,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>  	priv->adapter.dev.parent = &dev->dev;
>>  	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
>>  	priv->adapter.retries = 3;
>> -	mutex_init(&priv->acpi_lock);
>>  
>>  	priv->pci_dev = dev;
>>  	priv->features = id->driver_data;
> 
> Looks reasonable, I also can't see any reason why that wouldn't work.
> But locking and power management can be tricky of course. I'll test
> this for some time, but I don't think my system actually suffers from
> this ACPI resource conflict, so this most probably won't be testing
> much in practice.
> 
What's your opinion after more testing?

> Thanks,

Re: [PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Jean Delvare]

Hi Heiner,

On Sun, 27 Aug 2023 18:21:43 +0200, Heiner Kallweit wrote:
> On 26.06.2023 19:59, Jean Delvare wrote:
> > Looks reasonable, I also can't see any reason why that wouldn't work.
> > But locking and power management can be tricky of course. I'll test
> > this for some time, but I don't think my system actually suffers from
> > this ACPI resource conflict, so this most probably won't be testing
> > much in practice.
>
> What's your opinion after more testing?

Positive, as I did not hit any problem. As said before, my testing is
limited by design and thus is no guarantee that the change is OK in all
cases, but at least it's good enough to merge it and see what happens.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Heiner Kallweit]

On 28.08.2023 09:01, Jean Delvare wrote:
> Hi Heiner,
> 
> On Sun, 27 Aug 2023 18:21:43 +0200, Heiner Kallweit wrote:
>> On 26.06.2023 19:59, Jean Delvare wrote:
>>> Looks reasonable, I also can't see any reason why that wouldn't work.
>>> But locking and power management can be tricky of course. I'll test
>>> this for some time, but I don't think my system actually suffers from
>>> this ACPI resource conflict, so this most probably won't be testing
>>> much in practice.
>>
>> What's your opinion after more testing?
> 
> Positive, as I did not hit any problem. As said before, my testing is
> limited by design and thus is no guarantee that the change is OK in all
> cases, but at least it's good enough to merge it and see what happens.
> 
Then I'll resubmit the patch because the series has been broken up.

[PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Heiner Kallweit]

Here we don't have to write SMBHSTCNT in each iteration of the loop.
Bit SMBHSTCNT_START is internally cleared immediately, therefore
we don't have to touch the value of SMBHSTCNT until the last byte.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 7641bd0ac..e1350a8cc 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -677,11 +677,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	for (i = 1; i <= len; i++) {
 		if (i == len && read_write == I2C_SMBUS_READ)
 			smbcmd |= SMBHSTCNT_LAST_BYTE;
-		outb_p(smbcmd, SMBHSTCNT(priv));
 
 		if (i == 1)
-			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
-			       SMBHSTCNT(priv));
+			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
+		else if (smbcmd & SMBHSTCNT_LAST_BYTE)
+			outb_p(smbcmd, SMBHSTCNT(priv));
 
 		status = i801_wait_byte_done(priv);
 		if (status)
-- 
2.39.2

Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Andi Shyti]

Hi Heiner,

On Sat, Mar 04, 2023 at 10:36:34PM +0100, Heiner Kallweit wrote:
> Here we don't have to write SMBHSTCNT in each iteration of the loop.
> Bit SMBHSTCNT_START is internally cleared immediately, therefore
> we don't have to touch the value of SMBHSTCNT until the last byte.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 7641bd0ac..e1350a8cc 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -677,11 +677,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  	for (i = 1; i <= len; i++) {
>  		if (i == len && read_write == I2C_SMBUS_READ)
>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
> -		outb_p(smbcmd, SMBHSTCNT(priv));
>  
>  		if (i == 1)
> -			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
> -			       SMBHSTCNT(priv));
> +			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
> +		else if (smbcmd & SMBHSTCNT_LAST_BYTE)
> +			outb_p(smbcmd, SMBHSTCNT(priv));

Looks reasonable to me... Jean, any opinion here?

Andi

Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Andi Shyti]

Hi Heiner,

On Sat, Mar 04, 2023 at 10:36:34PM +0100, Heiner Kallweit wrote:
> Here we don't have to write SMBHSTCNT in each iteration of the loop.
> Bit SMBHSTCNT_START is internally cleared immediately, therefore
> we don't have to touch the value of SMBHSTCNT until the last byte.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Acked-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Jean Delvare]

Hi Heiner, Andi,

On Sat, 04 Mar 2023 22:36:34 +0100, Heiner Kallweit wrote:
> Here we don't have to write SMBHSTCNT in each iteration of the loop.
> Bit SMBHSTCNT_START is internally cleared immediately, therefore
> we don't have to touch the value of SMBHSTCNT until the last byte.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 7641bd0ac..e1350a8cc 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -677,11 +677,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  	for (i = 1; i <= len; i++) {
>  		if (i == len && read_write == I2C_SMBUS_READ)
>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
> -		outb_p(smbcmd, SMBHSTCNT(priv));
>  
>  		if (i == 1)
> -			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
> -			       SMBHSTCNT(priv));
> +			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
> +		else if (smbcmd & SMBHSTCNT_LAST_BYTE)
> +			outb_p(smbcmd, SMBHSTCNT(priv));
>  
>  		status = i801_wait_byte_done(priv);
>  		if (status)

I tested this and it works, but I don't understand how.

I thought that writing to SMBHSTCNT was what was telling the host
controller to proceed with the next byte. If writing to SMBHSTCNT for
each byte isn't needed, then what causes the next byte to be processed?
Does this happen as soon as SMBHSTSTS_BYTE_DONE is written? If so, then
what guarantees that we set SMBHSTCNT_LAST_BYTE *before* the last byte
is actually processed?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Heiner Kallweit]

On 27.06.2023 15:46, Jean Delvare wrote:
> Hi Heiner, Andi,
> 
> On Sat, 04 Mar 2023 22:36:34 +0100, Heiner Kallweit wrote:
>> Here we don't have to write SMBHSTCNT in each iteration of the loop.
>> Bit SMBHSTCNT_START is internally cleared immediately, therefore
>> we don't have to touch the value of SMBHSTCNT until the last byte.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 7641bd0ac..e1350a8cc 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -677,11 +677,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>  	for (i = 1; i <= len; i++) {
>>  		if (i == len && read_write == I2C_SMBUS_READ)
>>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
>> -		outb_p(smbcmd, SMBHSTCNT(priv));
>>  
>>  		if (i == 1)
>> -			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
>> -			       SMBHSTCNT(priv));
>> +			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
>> +		else if (smbcmd & SMBHSTCNT_LAST_BYTE)
>> +			outb_p(smbcmd, SMBHSTCNT(priv));
>>  
>>  		status = i801_wait_byte_done(priv);
>>  		if (status)
> 
> I tested this and it works, but I don't understand how.
> 
> I thought that writing to SMBHSTCNT was what was telling the host
> controller to proceed with the next byte. If writing to SMBHSTCNT for
> each byte isn't needed, then what causes the next byte to be processed?
> Does this happen as soon as SMBHSTSTS_BYTE_DONE is written? If so, then
> what guarantees that we set SMBHSTCNT_LAST_BYTE *before* the last byte
> is actually processed?
> 
It's my understanding that writing SMBHSTSTS_BYTE_DONE tells the host to
continue with the next byte.
We set SMBHSTCNT_LAST_BYTE whilst the host is receiving the last byte.
Apparently the host checks for SMBHSTCNT_LAST_BYTE once it received
a byte, in order to determine whether to ack the byte or not.
So SMBHSTCNT_LAST_BYTE doesn't have to be set before the host starts
receiving the last byte.
For writes SMBHSTCNT_LAST_BYTE isn't used.

Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Jean Delvare]

Hi Heiner,

On Sun, 27 Aug 2023 19:14:38 +0200, Heiner Kallweit wrote:
> On 27.06.2023 15:46, Jean Delvare wrote:
> > Hi Heiner, Andi,
> > 
> > On Sat, 04 Mar 2023 22:36:34 +0100, Heiner Kallweit wrote:  
> >> Here we don't have to write SMBHSTCNT in each iteration of the loop.
> >> Bit SMBHSTCNT_START is internally cleared immediately, therefore
> >> we don't have to touch the value of SMBHSTCNT until the last byte.
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >>  drivers/i2c/busses/i2c-i801.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >> index 7641bd0ac..e1350a8cc 100644
> >> --- a/drivers/i2c/busses/i2c-i801.c
> >> +++ b/drivers/i2c/busses/i2c-i801.c
> >> @@ -677,11 +677,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
> >>  	for (i = 1; i <= len; i++) {
> >>  		if (i == len && read_write == I2C_SMBUS_READ)
> >>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
> >> -		outb_p(smbcmd, SMBHSTCNT(priv));
> >>  
> >>  		if (i == 1)
> >> -			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
> >> -			       SMBHSTCNT(priv));
> >> +			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
> >> +		else if (smbcmd & SMBHSTCNT_LAST_BYTE)
> >> +			outb_p(smbcmd, SMBHSTCNT(priv));
> >>  
> >>  		status = i801_wait_byte_done(priv);
> >>  		if (status)  
> > 
> > I tested this and it works, but I don't understand how.
> > 
> > I thought that writing to SMBHSTCNT was what was telling the host
> > controller to proceed with the next byte. If writing to SMBHSTCNT for
> > each byte isn't needed, then what causes the next byte to be processed?
> > Does this happen as soon as SMBHSTSTS_BYTE_DONE is written? If so, then
> > what guarantees that we set SMBHSTCNT_LAST_BYTE *before* the last byte
> > is actually processed?
>
> It's my understanding that writing SMBHSTSTS_BYTE_DONE tells the host to
> continue with the next byte.

That's indeed possible, and quite likely, considering that your patch
works.

> We set SMBHSTCNT_LAST_BYTE whilst the host is receiving the last byte.
> Apparently the host checks for SMBHSTCNT_LAST_BYTE once it received
> a byte, in order to determine whether to ack the byte or not.
> So SMBHSTCNT_LAST_BYTE doesn't have to be set before the host starts
> receiving the last byte.

How is this not racy?

In the interrupt-driven case, at the end of a block read transaction,
we set SMBHSTCNT_LAST_BYTE at the end of i801_isr_byte_done(), then
return to i801_isr() where we write 1 to SMBHSTSTS_BYTE_DONE to clear
it. This lets the controller handle the last byte with the knowledge
that this is the last byte.

However, in the poll-driven case, SMBHSTSTS_BYTE_DONE is being cleared
at the end of the loop in i801_block_transaction_byte_by_byte(), then
at the beginning of the next iteration, we write SMBHSTCNT_LAST_BYTE,
then wait for completion. If the controller is super fast (or, to be
more realistic, the i2c-i801 driver gets preempted between writing
SMBHSTSTS_BYTE_DONE and writing SMBHSTCNT_LAST_BYTE) then the byte may
have been already read and acked, before we have the time to let the
controller know that no ACK should be sent. This looks racy. Am I
missing something?

If nothing else, the fact that the order is different between the
interrupt-driven and poll-driven cases is fishy.

I must add that the problem is not related to your patch, I just
happened to notice it while reviewing your patch.

> For writes SMBHSTCNT_LAST_BYTE isn't used.

Agreed.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Jean Delvare]

On Mon, 28 Aug 2023 15:27:47 +0200, Jean Delvare wrote:
> On Sun, 27 Aug 2023 19:14:38 +0200, Heiner Kallweit wrote:
> > We set SMBHSTCNT_LAST_BYTE whilst the host is receiving the last byte.
> > Apparently the host checks for SMBHSTCNT_LAST_BYTE once it received
> > a byte, in order to determine whether to ack the byte or not.
> > So SMBHSTCNT_LAST_BYTE doesn't have to be set before the host starts
> > receiving the last byte.  
> 
> How is this not racy?
> 
> In the interrupt-driven case, at the end of a block read transaction,
> we set SMBHSTCNT_LAST_BYTE at the end of i801_isr_byte_done(), then
> return to i801_isr() where we write 1 to SMBHSTSTS_BYTE_DONE to clear
> it. This lets the controller handle the last byte with the knowledge
> that this is the last byte.
> 
> However, in the poll-driven case, SMBHSTSTS_BYTE_DONE is being cleared
> at the end of the loop in i801_block_transaction_byte_by_byte(), then
> at the beginning of the next iteration, we write SMBHSTCNT_LAST_BYTE,
> then wait for completion. If the controller is super fast (or, to be
> more realistic, the i2c-i801 driver gets preempted between writing
> SMBHSTSTS_BYTE_DONE and writing SMBHSTCNT_LAST_BYTE) then the byte may
> have been already read and acked, before we have the time to let the
> controller know that no ACK should be sent. This looks racy. Am I
> missing something?

I made a little experiment which, I think, proves my point.

Firstly, I loaded the i2c-i801 driver with disable_features=0x12, to
make sure the poll-based byte-by-byte code path is used. The EEPROM
data starts with:

     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00: 92 13 0b 02 04 21 02 01 03 52 01 08 0a 00 fc 00

The following commands read the first 4 bytes with an I2C Block Read
command, then fetch the next byte from the EEPROM (without specifying
the offset):

# /usr/local/sbin/i2cget -y 4 0x53 0 i 4 ; /usr/local/sbin/i2cget -y 4 0x53
0x92 0x13 0x0b 0x02
0x04

As you can see, I get the 5 first bytes of the EEPROM, as expected.

Then I added an arbitrary delay in the driver where I think the race
condition exists:

--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -684,8 +684,10 @@ static int i801_block_transaction_byte_b
 
 		if (i == 1)
 			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
-		else if (smbcmd & SMBHSTCNT_LAST_BYTE)
+		else if (smbcmd & SMBHSTCNT_LAST_BYTE) {
+			usleep_range(10000, 20000);
 			outb_p(smbcmd, SMBHSTCNT(priv));
+		}
 
 		status = i801_wait_byte_done(priv);
 		if (status)

I loaded the modified i2c-i801 driver, still with
disable_features=0x12. Running the same commands again, I now get:

# /usr/local/sbin/i2cget -y 4 0x53 0 i 4 ; /usr/local/sbin/i2cget -y 4 0x53
0x92 0x13 0x0b 0x02
0x21

So I get EEPROM bytes 0-3 then it jumps to offset 5 directly. This
means that the EEPROM started sending the byte at offset 4 at the end
of the I2C Block Read transfer, due to the controller sending an ACK
after the byte at offset 3.

For the code to be safe, we need to set SMBHSTCNT_LAST_BYTE *before*
clearing SMBHSTSTS_BYTE_DONE.

Note: the transfers do not fail, only the internal register pointer of
the EEPROM gets screwed, so this is probably not an issue for devices
which don't rely on an internal register pointer, only EEPROM-like
devices are affected.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Heiner Kallweit]

On 28.08.2023 17:10, Jean Delvare wrote:
> On Mon, 28 Aug 2023 15:27:47 +0200, Jean Delvare wrote:
>> On Sun, 27 Aug 2023 19:14:38 +0200, Heiner Kallweit wrote:
>>> We set SMBHSTCNT_LAST_BYTE whilst the host is receiving the last byte.
>>> Apparently the host checks for SMBHSTCNT_LAST_BYTE once it received
>>> a byte, in order to determine whether to ack the byte or not.
>>> So SMBHSTCNT_LAST_BYTE doesn't have to be set before the host starts
>>> receiving the last byte.  
>>
>> How is this not racy?
>>
>> In the interrupt-driven case, at the end of a block read transaction,
>> we set SMBHSTCNT_LAST_BYTE at the end of i801_isr_byte_done(), then
>> return to i801_isr() where we write 1 to SMBHSTSTS_BYTE_DONE to clear
>> it. This lets the controller handle the last byte with the knowledge
>> that this is the last byte.
>>
>> However, in the poll-driven case, SMBHSTSTS_BYTE_DONE is being cleared
>> at the end of the loop in i801_block_transaction_byte_by_byte(), then
>> at the beginning of the next iteration, we write SMBHSTCNT_LAST_BYTE,
>> then wait for completion. If the controller is super fast (or, to be
>> more realistic, the i2c-i801 driver gets preempted between writing
>> SMBHSTSTS_BYTE_DONE and writing SMBHSTCNT_LAST_BYTE) then the byte may
>> have been already read and acked, before we have the time to let the
>> controller know that no ACK should be sent. This looks racy. Am I
>> missing something?
> 
> I made a little experiment which, I think, proves my point.
> 
> Firstly, I loaded the i2c-i801 driver with disable_features=0x12, to
> make sure the poll-based byte-by-byte code path is used. The EEPROM
> data starts with:
> 
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00: 92 13 0b 02 04 21 02 01 03 52 01 08 0a 00 fc 00
> 
> The following commands read the first 4 bytes with an I2C Block Read
> command, then fetch the next byte from the EEPROM (without specifying
> the offset):
> 
> # /usr/local/sbin/i2cget -y 4 0x53 0 i 4 ; /usr/local/sbin/i2cget -y 4 0x53
> 0x92 0x13 0x0b 0x02
> 0x04
> 
> As you can see, I get the 5 first bytes of the EEPROM, as expected.
> 
> Then I added an arbitrary delay in the driver where I think the race
> condition exists:
> 
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -684,8 +684,10 @@ static int i801_block_transaction_byte_b
>  
>  		if (i == 1)
>  			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
> -		else if (smbcmd & SMBHSTCNT_LAST_BYTE)
> +		else if (smbcmd & SMBHSTCNT_LAST_BYTE) {
> +			usleep_range(10000, 20000);
>  			outb_p(smbcmd, SMBHSTCNT(priv));
> +		}
>  
>  		status = i801_wait_byte_done(priv);
>  		if (status)
> 
> I loaded the modified i2c-i801 driver, still with
> disable_features=0x12. Running the same commands again, I now get:
> 
> # /usr/local/sbin/i2cget -y 4 0x53 0 i 4 ; /usr/local/sbin/i2cget -y 4 0x53
> 0x92 0x13 0x0b 0x02
> 0x21
> 
> So I get EEPROM bytes 0-3 then it jumps to offset 5 directly. This
> means that the EEPROM started sending the byte at offset 4 at the end
> of the I2C Block Read transfer, due to the controller sending an ACK
> after the byte at offset 3.
> 
> For the code to be safe, we need to set SMBHSTCNT_LAST_BYTE *before*
> clearing SMBHSTSTS_BYTE_DONE.
> 

I think you're right with the preemption scenario. I'll submit a fix
for it.

> Note: the transfers do not fail, only the internal register pointer of
> the EEPROM gets screwed, so this is probably not an issue for devices
> which don't rely on an internal register pointer, only EEPROM-like
> devices are affected.
> 

Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Heiner Kallweit]

On 28.08.2023 15:27, Jean Delvare wrote:
> Hi Heiner,
> 
> On Sun, 27 Aug 2023 19:14:38 +0200, Heiner Kallweit wrote:
>> On 27.06.2023 15:46, Jean Delvare wrote:
>>> Hi Heiner, Andi,
>>>
>>> On Sat, 04 Mar 2023 22:36:34 +0100, Heiner Kallweit wrote:  
>>>> Here we don't have to write SMBHSTCNT in each iteration of the loop.
>>>> Bit SMBHSTCNT_START is internally cleared immediately, therefore
>>>> we don't have to touch the value of SMBHSTCNT until the last byte.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/i2c/busses/i2c-i801.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>>> index 7641bd0ac..e1350a8cc 100644
>>>> --- a/drivers/i2c/busses/i2c-i801.c
>>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>>> @@ -677,11 +677,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>>>  	for (i = 1; i <= len; i++) {
>>>>  		if (i == len && read_write == I2C_SMBUS_READ)
>>>>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
>>>> -		outb_p(smbcmd, SMBHSTCNT(priv));
>>>>  
>>>>  		if (i == 1)
>>>> -			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
>>>> -			       SMBHSTCNT(priv));
>>>> +			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
>>>> +		else if (smbcmd & SMBHSTCNT_LAST_BYTE)
>>>> +			outb_p(smbcmd, SMBHSTCNT(priv));
>>>>  
>>>>  		status = i801_wait_byte_done(priv);
>>>>  		if (status)  
>>>
>>> I tested this and it works, but I don't understand how.
>>>
>>> I thought that writing to SMBHSTCNT was what was telling the host
>>> controller to proceed with the next byte. If writing to SMBHSTCNT for
>>> each byte isn't needed, then what causes the next byte to be processed?
>>> Does this happen as soon as SMBHSTSTS_BYTE_DONE is written? If so, then
>>> what guarantees that we set SMBHSTCNT_LAST_BYTE *before* the last byte
>>> is actually processed?
>>
>> It's my understanding that writing SMBHSTSTS_BYTE_DONE tells the host to
>> continue with the next byte.
> 
> That's indeed possible, and quite likely, considering that your patch
> works.
> 

This understanding is backed by the following from Byte Done Status
description in (at least) ICH9 specification:
When not using the 32 Byte Buffer, hardware will drive the SMBCLK signal
low when the DS bit is set until SW clears the bit.

>> We set SMBHSTCNT_LAST_BYTE whilst the host is receiving the last byte.
>> Apparently the host checks for SMBHSTCNT_LAST_BYTE once it received
>> a byte, in order to determine whether to ack the byte or not.
>> So SMBHSTCNT_LAST_BYTE doesn't have to be set before the host starts
>> receiving the last byte.
> 
> How is this not racy?
> 
> In the interrupt-driven case, at the end of a block read transaction,
> we set SMBHSTCNT_LAST_BYTE at the end of i801_isr_byte_done(), then
> return to i801_isr() where we write 1 to SMBHSTSTS_BYTE_DONE to clear
> it. This lets the controller handle the last byte with the knowledge
> that this is the last byte.
> 
> However, in the poll-driven case, SMBHSTSTS_BYTE_DONE is being cleared
> at the end of the loop in i801_block_transaction_byte_by_byte(), then
> at the beginning of the next iteration, we write SMBHSTCNT_LAST_BYTE,
> then wait for completion. If the controller is super fast (or, to be
> more realistic, the i2c-i801 driver gets preempted between writing
> SMBHSTSTS_BYTE_DONE and writing SMBHSTCNT_LAST_BYTE) then the byte may
> have been already read and acked, before we have the time to let the
> controller know that no ACK should be sent. This looks racy. Am I
> missing something?
> 
> If nothing else, the fact that the order is different between the
> interrupt-driven and poll-driven cases is fishy.
> 
> I must add that the problem is not related to your patch, I just
> happened to notice it while reviewing your patch.
> 
>> For writes SMBHSTCNT_LAST_BYTE isn't used.
> 
> Agreed.
> 

[PATCH 4/4] i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS

[Heiner Kallweit]

By using the newer macro DEFINE_SIMPLE_DEV_PM_OPS we can get rid
of the conditional compiling.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index e1350a8cc..bd2349768 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1800,7 +1800,6 @@ static void i801_shutdown(struct pci_dev *dev)
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int i801_suspend(struct device *dev)
 {
 	struct i801_priv *priv = dev_get_drvdata(dev);
@@ -1821,9 +1820,8 @@ static int i801_resume(struct device *dev)
 
 	return 0;
 }
-#endif
 
-static SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume);
 
 static struct pci_driver i801_driver = {
 	.name		= DRV_NAME,
@@ -1832,7 +1830,7 @@ static struct pci_driver i801_driver = {
 	.remove		= i801_remove,
 	.shutdown	= i801_shutdown,
 	.driver		= {
-		.pm	= &i801_pm_ops,
+		.pm	= pm_sleep_ptr(&i801_pm_ops),
 		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
 	},
 };
-- 
2.39.2

Re: [PATCH 4/4] i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS

[Andi Shyti]

Hi Heiner,

On Sat, Mar 04, 2023 at 10:37:34PM +0100, Heiner Kallweit wrote:
> By using the newer macro DEFINE_SIMPLE_DEV_PM_OPS we can get rid
> of the conditional compiling.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index e1350a8cc..bd2349768 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1800,7 +1800,6 @@ static void i801_shutdown(struct pci_dev *dev)
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int i801_suspend(struct device *dev)
>  {
>  	struct i801_priv *priv = dev_get_drvdata(dev);
> @@ -1821,9 +1820,8 @@ static int i801_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
> -static SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume);
> +static DEFINE_SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume);
>  
>  static struct pci_driver i801_driver = {
>  	.name		= DRV_NAME,
> @@ -1832,7 +1830,7 @@ static struct pci_driver i801_driver = {
>  	.remove		= i801_remove,
>  	.shutdown	= i801_shutdown,
>  	.driver		= {
> -		.pm	= &i801_pm_ops,
> +		.pm	= pm_sleep_ptr(&i801_pm_ops),

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

Re: [PATCH 4/4] i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS

[Jean Delvare]

On Sat, 04 Mar 2023 22:37:34 +0100, Heiner Kallweit wrote:
> By using the newer macro DEFINE_SIMPLE_DEV_PM_OPS we can get rid
> of the conditional compiling.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index e1350a8cc..bd2349768 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1800,7 +1800,6 @@ static void i801_shutdown(struct pci_dev *dev)
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int i801_suspend(struct device *dev)
>  {
>  	struct i801_priv *priv = dev_get_drvdata(dev);
> @@ -1821,9 +1820,8 @@ static int i801_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
> -static SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume);
> +static DEFINE_SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume);
>  
>  static struct pci_driver i801_driver = {
>  	.name		= DRV_NAME,
> @@ -1832,7 +1830,7 @@ static struct pci_driver i801_driver = {
>  	.remove		= i801_remove,
>  	.shutdown	= i801_shutdown,
>  	.driver		= {
> -		.pm	= &i801_pm_ops,
> +		.pm	= pm_sleep_ptr(&i801_pm_ops),
>  		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>  	},
>  };


Nice clean up.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 4/4] i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 561 bytes --]

On Sat, Mar 04, 2023 at 10:37:34PM +0100, Heiner Kallweit wrote:
> By using the newer macro DEFINE_SIMPLE_DEV_PM_OPS we can get rid
> of the conditional compiling.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

So, only this patch is left which only needs rebasing, right? Great
work, guys, for keeping at it! Could you also kindly have a look for the
remaining i801 patches from this year while we are at it?

http://patchwork.ozlabs.org/project/linux-i2c/list/?series=&submitter=&state=&q=i801&archive=&delegate=

Thanks everyone!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS

[Heiner Kallweit]

On 22.09.2023 11:54, Wolfram Sang wrote:
> On Sat, Mar 04, 2023 at 10:37:34PM +0100, Heiner Kallweit wrote:
>> By using the newer macro DEFINE_SIMPLE_DEV_PM_OPS we can get rid
>> of the conditional compiling.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> So, only this patch is left which only needs rebasing, right? Great
> work, guys, for keeping at it! Could you also kindly have a look for the
> remaining i801 patches from this year while we are at it?
> The same patch from another author has been applied in the meantime:
a6273e413a9a ("i2c: i801: Remove #ifdef guards for PM related functions")
So you can set my patch to superseeded.

> http://patchwork.ozlabs.org/project/linux-i2c/list/?series=&submitter=&state=&q=i801&archive=&delegate=
> 
> Thanks everyone!
> 

Re: [PATCH 0/4] i2c: i801: next set of improvements

[Heiner Kallweit]

On 04.03.2023 22:30, Heiner Kallweit wrote:
> Next set of improvements.
> 
> Heiner Kallweit (4):
>   i2c: i801: Use i2c_mark_adapter_suspended/resumed
>   i2c: i801: Replace acpi_lock with I2C bus lock
>   i2c: i801: Improve i801_block_transaction_byte_by_byte
>   i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS
> 
>  drivers/i2c/busses/i2c-i801.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
Jean, any chance to have a look at it?