[PATCH 00/10] i2c: i801: Series with improvements

[PATCH 00/10] i2c: i801: Series with improvements

[Heiner Kallweit]

This series includes a number of improvements to the i801 driver.

Heiner Kallweit (10):
  i2c: i801: Don't call pm_runtime_allow
  i2c: i801: Improve disabling runtime pm
  i2c: i801: Make p2sb_spinlock a mutex
  i2c: i801: Remove not needed debug message
  i2c: i801: Improve is_dell_system_with_lis3lv02d
  i2c: i801: Remove not needed check for PCI_COMMAND_INTX_DISABLE
  i2c: i801: Improve i801_acpi_probe/remove functions
  i2c: i801: Improve i801_add_mux
  i2c: i801: Improve register_dell_lis3lv02d_i2c_device
  i2c: i801: Improve handling platform data for tco device

 drivers/i2c/busses/i2c-i801.c | 138 +++++++++++-----------------------
 1 file changed, 44 insertions(+), 94 deletions(-)

-- 
2.32.0

[PATCH 01/10] i2c: i801: Don't call pm_runtime_allow

[Heiner Kallweit]

Drivers should not call pm_runtime_allow(), see
Documentation/power/pci.rst. Therefore remove the call and leave this
to user space. Also remove the not needed call to pm_runtime_forbid().

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 92ec291c0..362e74761 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1891,7 +1891,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	pm_runtime_set_autosuspend_delay(&dev->dev, 1000);
 	pm_runtime_use_autosuspend(&dev->dev);
 	pm_runtime_put_autosuspend(&dev->dev);
-	pm_runtime_allow(&dev->dev);
 
 	return 0;
 }
@@ -1900,7 +1899,6 @@ static void i801_remove(struct pci_dev *dev)
 {
 	struct i801_priv *priv = pci_get_drvdata(dev);
 
-	pm_runtime_forbid(&dev->dev);
 	pm_runtime_get_noresume(&dev->dev);
 
 	i801_disable_host_notify(priv);
-- 
2.32.0

Re: [PATCH 01/10] i2c: i801: Don't call pm_runtime_allow

[Jean Delvare]

Hi Heiner,

On Sun, 01 Aug 2021 16:16:56 +0200, Heiner Kallweit wrote:
> Drivers should not call pm_runtime_allow(), see
> Documentation/power/pci.rst. Therefore remove the call and leave this
> to user space. Also remove the not needed call to pm_runtime_forbid().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 92ec291c0..362e74761 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1891,7 +1891,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	pm_runtime_set_autosuspend_delay(&dev->dev, 1000);
>  	pm_runtime_use_autosuspend(&dev->dev);
>  	pm_runtime_put_autosuspend(&dev->dev);
> -	pm_runtime_allow(&dev->dev);
>  
>  	return 0;
>  }
> @@ -1900,7 +1899,6 @@ static void i801_remove(struct pci_dev *dev)
>  {
>  	struct i801_priv *priv = pci_get_drvdata(dev);
>  
> -	pm_runtime_forbid(&dev->dev);
>  	pm_runtime_get_noresume(&dev->dev);
>  
>  	i801_disable_host_notify(priv);

These calls were added by Jarkko (Cc'd) and I'm not familiar with power
management so I'll need an explicit ack from him before I can accept
this patch.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 01/10] i2c: i801: Don't call pm_runtime_allow

[Heiner Kallweit]

On 02.08.2021 14:53, Jean Delvare wrote:
> Hi Heiner,
> 
> On Sun, 01 Aug 2021 16:16:56 +0200, Heiner Kallweit wrote:
>> Drivers should not call pm_runtime_allow(), see
>> Documentation/power/pci.rst. Therefore remove the call and leave this
>> to user space. Also remove the not needed call to pm_runtime_forbid().
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 92ec291c0..362e74761 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1891,7 +1891,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>  	pm_runtime_set_autosuspend_delay(&dev->dev, 1000);
>>  	pm_runtime_use_autosuspend(&dev->dev);
>>  	pm_runtime_put_autosuspend(&dev->dev);
>> -	pm_runtime_allow(&dev->dev);
>>  
>>  	return 0;
>>  }
>> @@ -1900,7 +1899,6 @@ static void i801_remove(struct pci_dev *dev)
>>  {
>>  	struct i801_priv *priv = pci_get_drvdata(dev);
>>  
>> -	pm_runtime_forbid(&dev->dev);
>>  	pm_runtime_get_noresume(&dev->dev);
>>  
>>  	i801_disable_host_notify(priv);
> 
> These calls were added by Jarkko (Cc'd) and I'm not familiar with power
> management so I'll need an explicit ack from him before I can accept
> this patch.
> 
The calls were part of the initial submission for rpm support and supposedly
just copied from another driver. But fine with me to wait for his feedback.

Heiner

Re: [PATCH 01/10] i2c: i801: Don't call pm_runtime_allow

[Jarkko Nikula]

Hi

On 8/2/21 7:31 PM, Heiner Kallweit wrote:
> On 02.08.2021 14:53, Jean Delvare wrote:
>> Hi Heiner,
>>
>> On Sun, 01 Aug 2021 16:16:56 +0200, Heiner Kallweit wrote:
>>> Drivers should not call pm_runtime_allow(), see
>>> Documentation/power/pci.rst. Therefore remove the call and leave this
>>> to user space. Also remove the not needed call to pm_runtime_forbid().
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>   drivers/i2c/busses/i2c-i801.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>> index 92ec291c0..362e74761 100644
>>> --- a/drivers/i2c/busses/i2c-i801.c
>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>> @@ -1891,7 +1891,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>>   	pm_runtime_set_autosuspend_delay(&dev->dev, 1000);
>>>   	pm_runtime_use_autosuspend(&dev->dev);
>>>   	pm_runtime_put_autosuspend(&dev->dev);
>>> -	pm_runtime_allow(&dev->dev);
>>>   
>>>   	return 0;
>>>   }
>>> @@ -1900,7 +1899,6 @@ static void i801_remove(struct pci_dev *dev)
>>>   {
>>>   	struct i801_priv *priv = pci_get_drvdata(dev);
>>>   
>>> -	pm_runtime_forbid(&dev->dev);
>>>   	pm_runtime_get_noresume(&dev->dev);
>>>   
>>>   	i801_disable_host_notify(priv);
>>
>> These calls were added by Jarkko (Cc'd) and I'm not familiar with power
>> management so I'll need an explicit ack from him before I can accept
>> this patch.
>>
> The calls were part of the initial submission for rpm support and supposedly
> just copied from another driver. But fine with me to wait for his feedback.
> 
Yes, I'm quite sure I've copied it from another driver :-)

This patch will cause the device here won't go automatically to D3 
before some user space script allows it. E.g

echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control

I think this is kind of PM regression with this patch. It's not clear to 
me from the Documentation/power/pci.rst why driver should not call the 
pm_runtime_allow() and what would be allowed kernel alternative for it.

Rafael: what would be the correct way here to allow runtime PM from the 
driver or does it really require some user space script for it?

Jarkko

Re: [PATCH 01/10] i2c: i801: Don't call pm_runtime_allow

[Rafael J. Wysocki]

On Wed, Aug 4, 2021 at 3:36 PM Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
>
> Hi
>
> On 8/2/21 7:31 PM, Heiner Kallweit wrote:
> > On 02.08.2021 14:53, Jean Delvare wrote:
> >> Hi Heiner,
> >>
> >> On Sun, 01 Aug 2021 16:16:56 +0200, Heiner Kallweit wrote:
> >>> Drivers should not call pm_runtime_allow(), see
> >>> Documentation/power/pci.rst. Therefore remove the call and leave this
> >>> to user space. Also remove the not needed call to pm_runtime_forbid().
> >>>
> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >>> ---
> >>>   drivers/i2c/busses/i2c-i801.c | 2 --
> >>>   1 file changed, 2 deletions(-)
> >>>
> >>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >>> index 92ec291c0..362e74761 100644
> >>> --- a/drivers/i2c/busses/i2c-i801.c
> >>> +++ b/drivers/i2c/busses/i2c-i801.c
> >>> @@ -1891,7 +1891,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >>>     pm_runtime_set_autosuspend_delay(&dev->dev, 1000);
> >>>     pm_runtime_use_autosuspend(&dev->dev);
> >>>     pm_runtime_put_autosuspend(&dev->dev);
> >>> -   pm_runtime_allow(&dev->dev);
> >>>
> >>>     return 0;
> >>>   }
> >>> @@ -1900,7 +1899,6 @@ static void i801_remove(struct pci_dev *dev)
> >>>   {
> >>>     struct i801_priv *priv = pci_get_drvdata(dev);
> >>>
> >>> -   pm_runtime_forbid(&dev->dev);
> >>>     pm_runtime_get_noresume(&dev->dev);
> >>>
> >>>     i801_disable_host_notify(priv);
> >>
> >> These calls were added by Jarkko (Cc'd) and I'm not familiar with power
> >> management so I'll need an explicit ack from him before I can accept
> >> this patch.
> >>
> > The calls were part of the initial submission for rpm support and supposedly
> > just copied from another driver. But fine with me to wait for his feedback.
> >
> Yes, I'm quite sure I've copied it from another driver :-)
>
> This patch will cause the device here won't go automatically to D3
> before some user space script allows it. E.g
>
> echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control
>
> I think this is kind of PM regression with this patch. It's not clear to
> me from the Documentation/power/pci.rst why driver should not call the
> pm_runtime_allow() and what would be allowed kernel alternative for it.

Please see the comment in local_pci_probe().

Because the PCI bus type is involved in power management, the driver
needs to cooperate.

> Rafael: what would be the correct way here to allow runtime PM from the
> driver or does it really require some user space script for it?

No, it doesn't.

Re: [PATCH 01/10] i2c: i801: Don't call pm_runtime_allow

[Heiner Kallweit]

On 04.08.2021 16:06, Rafael J. Wysocki wrote:
> On Wed, Aug 4, 2021 at 3:36 PM Jarkko Nikula
> <jarkko.nikula@linux.intel.com> wrote:
>>
>> Hi
>>
>> On 8/2/21 7:31 PM, Heiner Kallweit wrote:
>>> On 02.08.2021 14:53, Jean Delvare wrote:
>>>> Hi Heiner,
>>>>
>>>> On Sun, 01 Aug 2021 16:16:56 +0200, Heiner Kallweit wrote:
>>>>> Drivers should not call pm_runtime_allow(), see
>>>>> Documentation/power/pci.rst. Therefore remove the call and leave this
>>>>> to user space. Also remove the not needed call to pm_runtime_forbid().
>>>>>
>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>> ---
>>>>>   drivers/i2c/busses/i2c-i801.c | 2 --
>>>>>   1 file changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>>>> index 92ec291c0..362e74761 100644
>>>>> --- a/drivers/i2c/busses/i2c-i801.c
>>>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>>>> @@ -1891,7 +1891,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>>>>     pm_runtime_set_autosuspend_delay(&dev->dev, 1000);
>>>>>     pm_runtime_use_autosuspend(&dev->dev);
>>>>>     pm_runtime_put_autosuspend(&dev->dev);
>>>>> -   pm_runtime_allow(&dev->dev);
>>>>>
>>>>>     return 0;
>>>>>   }
>>>>> @@ -1900,7 +1899,6 @@ static void i801_remove(struct pci_dev *dev)
>>>>>   {
>>>>>     struct i801_priv *priv = pci_get_drvdata(dev);
>>>>>
>>>>> -   pm_runtime_forbid(&dev->dev);
>>>>>     pm_runtime_get_noresume(&dev->dev);
>>>>>
>>>>>     i801_disable_host_notify(priv);
>>>>
>>>> These calls were added by Jarkko (Cc'd) and I'm not familiar with power
>>>> management so I'll need an explicit ack from him before I can accept
>>>> this patch.
>>>>
>>> The calls were part of the initial submission for rpm support and supposedly
>>> just copied from another driver. But fine with me to wait for his feedback.
>>>
>> Yes, I'm quite sure I've copied it from another driver :-)
>>
>> This patch will cause the device here won't go automatically to D3
>> before some user space script allows it. E.g
>>
>> echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control
>>
>> I think this is kind of PM regression with this patch. It's not clear to
>> me from the Documentation/power/pci.rst why driver should not call the
>> pm_runtime_allow() and what would be allowed kernel alternative for it.
> 
> Please see the comment in local_pci_probe().
> 
> Because the PCI bus type is involved in power management, the driver
> needs to cooperate.
> 
>> Rafael: what would be the correct way here to allow runtime PM from the
>> driver or does it really require some user space script for it?
> 
> No, it doesn't.
> 

PCI core code includes the following because of historic issues
with broken ACPI support on some platforms:

void pci_pm_init(struct pci_dev *dev)
{
	int pm;
	u16 status;
	u16 pmc;

	pm_runtime_forbid(&dev->dev);
	pm_runtime_set_active(&dev->dev);
	pm_runtime_enable(&dev->dev);

That's why RPM has to be enabled by userspace for PCI devices:
echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control

Or drivers (that know that they can't be used on one of the broken
platforms) call pm_runtime_allow(), what however is explicitly
discouraged.

Not sure whether any of the old broken platforms is still relevant,
therefore I started a discussion about it, which however ended
w/o tangible result. See here:
https://www.spinics.net/lists/linux-pci/msg103281.html

I work around this restriction with the following in an init script,
not sure how common distro's deal with this.

# enable Runtime PM for all PCI devices
for i in /sys/bus/pci/devices/*/power/control; do
        echo auto > $i
done

Re: [PATCH 01/10] i2c: i801: Don't call pm_runtime_allow

[Jean Delvare]

Hi Heiner,

On Wed, 4 Aug 2021 21:02:39 +0200, Heiner Kallweit wrote:
> On 04.08.2021 16:06, Rafael J. Wysocki wrote:
> > On Wed, Aug 4, 2021 at 3:36 PM Jarkko Nikula
> >> Yes, I'm quite sure I've copied it from another driver :-)
> >>
> >> This patch will cause the device here won't go automatically to D3
> >> before some user space script allows it. E.g
> >>
> >> echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control
> >>
> >> I think this is kind of PM regression with this patch. It's not clear to
> >> me from the Documentation/power/pci.rst why driver should not call the
> >> pm_runtime_allow() and what would be allowed kernel alternative for it.  
> > 
> > Please see the comment in local_pci_probe().
> > 
> > Because the PCI bus type is involved in power management, the driver
> > needs to cooperate.
> >   
> >> Rafael: what would be the correct way here to allow runtime PM from the
> >> driver or does it really require some user space script for it?  
> > 
> > No, it doesn't.
> 
> PCI core code includes the following because of historic issues
> with broken ACPI support on some platforms:
> 
> void pci_pm_init(struct pci_dev *dev)
> {
> 	int pm;
> 	u16 status;
> 	u16 pmc;
> 
> 	pm_runtime_forbid(&dev->dev);
> 	pm_runtime_set_active(&dev->dev);
> 	pm_runtime_enable(&dev->dev);
> 
> That's why RPM has to be enabled by userspace for PCI devices:
> echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control
> 
> Or drivers (that know that they can't be used on one of the broken
> platforms) call pm_runtime_allow(), what however is explicitly
> discouraged.
> 
> Not sure whether any of the old broken platforms is still relevant,
> therefore I started a discussion about it, which however ended
> w/o tangible result. See here:
> https://www.spinics.net/lists/linux-pci/msg103281.html
> 
> I work around this restriction with the following in an init script,
> not sure how common distro's deal with this.
> 
> # enable Runtime PM for all PCI devices
> for i in /sys/bus/pci/devices/*/power/control; do
>         echo auto > $i
> done

FWIW, my distribution (openSUSE Leap 15.2) doesn't do anything with
these attributes, basically leaving the decision to the drivers. As a
result, your proposed patch leads to the following change for me:

-/sys/bus/pci/devices/0000:00:1f.3/power/control:auto
+/sys/bus/pci/devices/0000:00:1f.3/power/control:on

I don't see that as an improvement.

I also see that several other drivers I'm using (pcieport,
snd_hda_intel, amdgpu) do enable runtime power management, so the
i2c-i801 driver isn't an exception in this respect. Therefore I am not
willing to accept this patch, sorry.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 01/10] i2c: i801: Don't call pm_runtime_allow

[Rafael J. Wysocki]

On Thu, Aug 5, 2021 at 10:31 AM Jean Delvare <jdelvare@suse.de> wrote:
>
> Hi Heiner,
>
> On Wed, 4 Aug 2021 21:02:39 +0200, Heiner Kallweit wrote:
> > On 04.08.2021 16:06, Rafael J. Wysocki wrote:
> > > On Wed, Aug 4, 2021 at 3:36 PM Jarkko Nikula
> > >> Yes, I'm quite sure I've copied it from another driver :-)
> > >>
> > >> This patch will cause the device here won't go automatically to D3
> > >> before some user space script allows it. E.g
> > >>
> > >> echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control
> > >>
> > >> I think this is kind of PM regression with this patch. It's not clear to
> > >> me from the Documentation/power/pci.rst why driver should not call the
> > >> pm_runtime_allow() and what would be allowed kernel alternative for it.
> > >
> > > Please see the comment in local_pci_probe().
> > >
> > > Because the PCI bus type is involved in power management, the driver
> > > needs to cooperate.
> > >
> > >> Rafael: what would be the correct way here to allow runtime PM from the
> > >> driver or does it really require some user space script for it?
> > >
> > > No, it doesn't.
> >
> > PCI core code includes the following because of historic issues
> > with broken ACPI support on some platforms:
> >
> > void pci_pm_init(struct pci_dev *dev)
> > {
> >       int pm;
> >       u16 status;
> >       u16 pmc;
> >
> >       pm_runtime_forbid(&dev->dev);
> >       pm_runtime_set_active(&dev->dev);
> >       pm_runtime_enable(&dev->dev);
> >
> > That's why RPM has to be enabled by userspace for PCI devices:
> > echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control
> >
> > Or drivers (that know that they can't be used on one of the broken
> > platforms) call pm_runtime_allow(), what however is explicitly
> > discouraged.
> >
> > Not sure whether any of the old broken platforms is still relevant,
> > therefore I started a discussion about it, which however ended
> > w/o tangible result. See here:
> > https://www.spinics.net/lists/linux-pci/msg103281.html
> >
> > I work around this restriction with the following in an init script,
> > not sure how common distro's deal with this.
> >
> > # enable Runtime PM for all PCI devices
> > for i in /sys/bus/pci/devices/*/power/control; do
> >         echo auto > $i
> > done
>
> FWIW, my distribution (openSUSE Leap 15.2) doesn't do anything with
> these attributes, basically leaving the decision to the drivers. As a
> result, your proposed patch leads to the following change for me:
>
> -/sys/bus/pci/devices/0000:00:1f.3/power/control:auto
> +/sys/bus/pci/devices/0000:00:1f.3/power/control:on
>
> I don't see that as an improvement.
>
> I also see that several other drivers I'm using (pcieport,
> snd_hda_intel, amdgpu) do enable runtime power management, so the
> i2c-i801 driver isn't an exception in this respect. Therefore I am not
> willing to accept this patch, sorry.

I tend to agree with this judgement.  Drivers that already call
pm_runtime_allow() should be allowed to continue doing that or users
will be confused.

Calling pm_runtime_allow() from a PCI driver isn't nice with respect
to user space, because if the latter doesn't want the device to
runtime-suspend (whatever the reason), it needs to change the
PM-runtime control setting whenever the driver is bound to the device,
but that arguably is not a major problem.

It would still be useful to have a way to adjust the default behavior
for all PCI devices on a per-platform basis IMO.

Re: [PATCH 01/10] i2c: i801: Don't call pm_runtime_allow

[Rafael J. Wysocki]

On Wed, Aug 4, 2021 at 9:02 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 04.08.2021 16:06, Rafael J. Wysocki wrote:
> > On Wed, Aug 4, 2021 at 3:36 PM Jarkko Nikula
> > <jarkko.nikula@linux.intel.com> wrote:
> >>
> >> Hi
> >>
> >> On 8/2/21 7:31 PM, Heiner Kallweit wrote:
> >>> On 02.08.2021 14:53, Jean Delvare wrote:
> >>>> Hi Heiner,
> >>>>
> >>>> On Sun, 01 Aug 2021 16:16:56 +0200, Heiner Kallweit wrote:
> >>>>> Drivers should not call pm_runtime_allow(), see
> >>>>> Documentation/power/pci.rst. Therefore remove the call and leave this
> >>>>> to user space. Also remove the not needed call to pm_runtime_forbid().
> >>>>>
> >>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >>>>> ---
> >>>>>   drivers/i2c/busses/i2c-i801.c | 2 --
> >>>>>   1 file changed, 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >>>>> index 92ec291c0..362e74761 100644
> >>>>> --- a/drivers/i2c/busses/i2c-i801.c
> >>>>> +++ b/drivers/i2c/busses/i2c-i801.c
> >>>>> @@ -1891,7 +1891,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >>>>>     pm_runtime_set_autosuspend_delay(&dev->dev, 1000);
> >>>>>     pm_runtime_use_autosuspend(&dev->dev);
> >>>>>     pm_runtime_put_autosuspend(&dev->dev);
> >>>>> -   pm_runtime_allow(&dev->dev);
> >>>>>
> >>>>>     return 0;
> >>>>>   }
> >>>>> @@ -1900,7 +1899,6 @@ static void i801_remove(struct pci_dev *dev)
> >>>>>   {
> >>>>>     struct i801_priv *priv = pci_get_drvdata(dev);
> >>>>>
> >>>>> -   pm_runtime_forbid(&dev->dev);
> >>>>>     pm_runtime_get_noresume(&dev->dev);
> >>>>>
> >>>>>     i801_disable_host_notify(priv);
> >>>>
> >>>> These calls were added by Jarkko (Cc'd) and I'm not familiar with power
> >>>> management so I'll need an explicit ack from him before I can accept
> >>>> this patch.
> >>>>
> >>> The calls were part of the initial submission for rpm support and supposedly
> >>> just copied from another driver. But fine with me to wait for his feedback.
> >>>
> >> Yes, I'm quite sure I've copied it from another driver :-)
> >>
> >> This patch will cause the device here won't go automatically to D3
> >> before some user space script allows it. E.g
> >>
> >> echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control
> >>
> >> I think this is kind of PM regression with this patch. It's not clear to
> >> me from the Documentation/power/pci.rst why driver should not call the
> >> pm_runtime_allow() and what would be allowed kernel alternative for it.
> >
> > Please see the comment in local_pci_probe().
> >
> > Because the PCI bus type is involved in power management, the driver
> > needs to cooperate.
> >
> >> Rafael: what would be the correct way here to allow runtime PM from the
> >> driver or does it really require some user space script for it?
> >
> > No, it doesn't.
> >
>
> PCI core code includes the following because of historic issues
> with broken ACPI support on some platforms:
>
> void pci_pm_init(struct pci_dev *dev)
> {
>         int pm;
>         u16 status;
>         u16 pmc;
>
>         pm_runtime_forbid(&dev->dev);
>         pm_runtime_set_active(&dev->dev);
>         pm_runtime_enable(&dev->dev);

Well, thanks for reminding me about that!

> That's why RPM has to be enabled by userspace for PCI devices:
> echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control

Not really enabled, but rather "unlocked".

> Or drivers (that know that they can't be used on one of the broken
> platforms) call pm_runtime_allow(), what however is explicitly
> discouraged.

The problem here is that whether or not PM-runtime works in the given
configuration is not a property of a driver or an individual device,
but it depends on the platform.

Also if the driver is unbound from the device, the modified setting is
left behind it which isn't particularly nice.

> Not sure whether any of the old broken platforms is still relevant,

That's a good question, but it boils down to whether or not any of
them are still in use, which is hard to measure.

> therefore I started a discussion about it, which however ended
> w/o tangible result. See here:
> https://www.spinics.net/lists/linux-pci/msg103281.html

So I'm thinking that there could be a global flag accessible via a
kernel command line option, say pci_pm_runtime=allow/deny that would
allow the default behavior to be adjusted.  Now, the default value of
that flag could depend on some heuristics, like the BIOS date or
whether or not the system has ACPI etc.

> I work around this restriction with the following in an init script,
> not sure how common distro's deal with this.

Some of them use powertop to do an equivalent of the loop below IIRC.

> # enable Runtime PM for all PCI devices
> for i in /sys/bus/pci/devices/*/power/control; do
>         echo auto > $i
> done

Re: [PATCH 01/10] i2c: i801: Don't call pm_runtime_allow

[Heiner Kallweit]

On 06.08.2021 15:52, Rafael J. Wysocki wrote:
> On Wed, Aug 4, 2021 at 9:02 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 04.08.2021 16:06, Rafael J. Wysocki wrote:
>>> On Wed, Aug 4, 2021 at 3:36 PM Jarkko Nikula
>>> <jarkko.nikula@linux.intel.com> wrote:
>>>>
>>>> Hi
>>>>
>>>> On 8/2/21 7:31 PM, Heiner Kallweit wrote:
>>>>> On 02.08.2021 14:53, Jean Delvare wrote:
>>>>>> Hi Heiner,
>>>>>>
>>>>>> On Sun, 01 Aug 2021 16:16:56 +0200, Heiner Kallweit wrote:
>>>>>>> Drivers should not call pm_runtime_allow(), see
>>>>>>> Documentation/power/pci.rst. Therefore remove the call and leave this
>>>>>>> to user space. Also remove the not needed call to pm_runtime_forbid().
>>>>>>>
>>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>>>> ---
>>>>>>>   drivers/i2c/busses/i2c-i801.c | 2 --
>>>>>>>   1 file changed, 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>>>>>> index 92ec291c0..362e74761 100644
>>>>>>> --- a/drivers/i2c/busses/i2c-i801.c
>>>>>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>>>>>> @@ -1891,7 +1891,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>>>>>>     pm_runtime_set_autosuspend_delay(&dev->dev, 1000);
>>>>>>>     pm_runtime_use_autosuspend(&dev->dev);
>>>>>>>     pm_runtime_put_autosuspend(&dev->dev);
>>>>>>> -   pm_runtime_allow(&dev->dev);
>>>>>>>
>>>>>>>     return 0;
>>>>>>>   }
>>>>>>> @@ -1900,7 +1899,6 @@ static void i801_remove(struct pci_dev *dev)
>>>>>>>   {
>>>>>>>     struct i801_priv *priv = pci_get_drvdata(dev);
>>>>>>>
>>>>>>> -   pm_runtime_forbid(&dev->dev);
>>>>>>>     pm_runtime_get_noresume(&dev->dev);
>>>>>>>
>>>>>>>     i801_disable_host_notify(priv);
>>>>>>
>>>>>> These calls were added by Jarkko (Cc'd) and I'm not familiar with power
>>>>>> management so I'll need an explicit ack from him before I can accept
>>>>>> this patch.
>>>>>>
>>>>> The calls were part of the initial submission for rpm support and supposedly
>>>>> just copied from another driver. But fine with me to wait for his feedback.
>>>>>
>>>> Yes, I'm quite sure I've copied it from another driver :-)
>>>>
>>>> This patch will cause the device here won't go automatically to D3
>>>> before some user space script allows it. E.g
>>>>
>>>> echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control
>>>>
>>>> I think this is kind of PM regression with this patch. It's not clear to
>>>> me from the Documentation/power/pci.rst why driver should not call the
>>>> pm_runtime_allow() and what would be allowed kernel alternative for it.
>>>
>>> Please see the comment in local_pci_probe().
>>>
>>> Because the PCI bus type is involved in power management, the driver
>>> needs to cooperate.
>>>
>>>> Rafael: what would be the correct way here to allow runtime PM from the
>>>> driver or does it really require some user space script for it?
>>>
>>> No, it doesn't.
>>>
>>
>> PCI core code includes the following because of historic issues
>> with broken ACPI support on some platforms:
>>
>> void pci_pm_init(struct pci_dev *dev)
>> {
>>         int pm;
>>         u16 status;
>>         u16 pmc;
>>
>>         pm_runtime_forbid(&dev->dev);
>>         pm_runtime_set_active(&dev->dev);
>>         pm_runtime_enable(&dev->dev);
> 
> Well, thanks for reminding me about that!
> 
>> That's why RPM has to be enabled by userspace for PCI devices:
>> echo auto > /sys/bus/pci/devices/0000\:00\:1f.3/power/control
> 
> Not really enabled, but rather "unlocked".
> 
>> Or drivers (that know that they can't be used on one of the broken
>> platforms) call pm_runtime_allow(), what however is explicitly
>> discouraged.
> 
> The problem here is that whether or not PM-runtime works in the given
> configuration is not a property of a driver or an individual device,
> but it depends on the platform.
> 
> Also if the driver is unbound from the device, the modified setting is
> left behind it which isn't particularly nice.
> 
>> Not sure whether any of the old broken platforms is still relevant,
> 
> That's a good question, but it boils down to whether or not any of
> them are still in use, which is hard to measure.
> 
>> therefore I started a discussion about it, which however ended
>> w/o tangible result. See here:
>> https://www.spinics.net/lists/linux-pci/msg103281.html
> 
> So I'm thinking that there could be a global flag accessible via a
> kernel command line option, say pci_pm_runtime=allow/deny that would
> allow the default behavior to be adjusted.  Now, the default value of
> that flag could depend on some heuristics, like the BIOS date or
> whether or not the system has ACPI etc.
> 
Right, such proposals have been made. See e.g. here:
https://www.spinics.net/lists/linux-pci/msg103313.html

1. use dmi_get_bios_year() as indicator
2. Use ACPI version (major.minor) as indicator

Now it just takes a brave person who says:
There's no perfect indicator, let's go with the following and see
whether anybody complains.

>> I work around this restriction with the following in an init script,
>> not sure how common distro's deal with this.
> 
> Some of them use powertop to do an equivalent of the loop below IIRC.
> 
>> # enable Runtime PM for all PCI devices
>> for i in /sys/bus/pci/devices/*/power/control; do
>>         echo auto > $i
>> done

[PATCH 02/10] i2c: i801: Improve disabling runtime pm

[Heiner Kallweit]

Setting the autosuspend delay to a negative value disables runtime pm in
a little bit smarter way, because we need no cleanup when removing the
driver. Note that this is safe when reloading the driver, because the
call to pm_runtime_set_autosuspend_delay() in probe() will reverse the
effect. See update_autosuspend() for details.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 362e74761..bdb619bc0 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1628,7 +1628,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
 		 * BIOS is accessing the host controller so prevent it from
 		 * suspending automatically from now on.
 		 */
-		pm_runtime_get_sync(&pdev->dev);
+		pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
 	}
 
 	if ((function & ACPI_IO_MASK) == ACPI_READ)
@@ -1668,11 +1668,6 @@ static void i801_acpi_remove(struct i801_priv *priv)
 
 	acpi_remove_address_space_handler(adev->handle,
 		ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
-
-	mutex_lock(&priv->acpi_lock);
-	if (priv->acpi_reserved)
-		pm_runtime_put(&priv->pci_dev->dev);
-	mutex_unlock(&priv->acpi_lock);
 }
 #else
 static inline int i801_acpi_probe(struct i801_priv *priv) { return 0; }
-- 
2.32.0

Re: [PATCH 02/10] i2c: i801: Improve disabling runtime pm

[Jean Delvare]

On Sun, 01 Aug 2021 16:17:48 +0200, Heiner Kallweit wrote:
> Setting the autosuspend delay to a negative value disables runtime pm in
> a little bit smarter way, because we need no cleanup when removing the
> driver. Note that this is safe when reloading the driver, because the
> call to pm_runtime_set_autosuspend_delay() in probe() will reverse the
> effect. See update_autosuspend() for details.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 362e74761..bdb619bc0 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1628,7 +1628,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
>  		 * BIOS is accessing the host controller so prevent it from
>  		 * suspending automatically from now on.
>  		 */
> -		pm_runtime_get_sync(&pdev->dev);
> +		pm_runtime_set_autosuspend_delay(&pdev->dev, -1);

Surprisingly, very few drivers use this. But this indeed looks correct.

>  	}
>  
>  	if ((function & ACPI_IO_MASK) == ACPI_READ)
> @@ -1668,11 +1668,6 @@ static void i801_acpi_remove(struct i801_priv *priv)
>  
>  	acpi_remove_address_space_handler(adev->handle,
>  		ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
> -
> -	mutex_lock(&priv->acpi_lock);
> -	if (priv->acpi_reserved)
> -		pm_runtime_put(&priv->pci_dev->dev);
> -	mutex_unlock(&priv->acpi_lock);
>  }
>  #else
>  static inline int i801_acpi_probe(struct i801_priv *priv) { return 0; }

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

-- 
Jean Delvare
SUSE L3 Support

[PATCH 03/10] i2c: i801: Make p2sb_spinlock a mutex

[Heiner Kallweit]

p2sb_spinlock is used in i801_add_tco_spt() only and in process context
only. Therefore a mutex is sufficient, and we can make the definition
local to i801_add_tco_spt().

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index bdb619bc0..504f02e1e 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1492,12 +1492,11 @@ static const struct itco_wdt_platform_data spt_tco_platform_data = {
 	.version = 4,
 };
 
-static DEFINE_SPINLOCK(p2sb_spinlock);
-
 static struct platform_device *
 i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 		 struct resource *tco_res)
 {
+	static DEFINE_MUTEX(p2sb_mutex);
 	struct resource *res;
 	unsigned int devfn;
 	u64 base64_addr;
@@ -1510,7 +1509,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 	 * enumerated by the PCI subsystem, so we need to unhide/hide it
 	 * to lookup the P2SB BAR.
 	 */
-	spin_lock(&p2sb_spinlock);
+	mutex_lock(&p2sb_mutex);
 
 	devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
 
@@ -1528,7 +1527,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 	/* Hide the P2SB device, if it was hidden before */
 	if (hidden)
 		pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, hidden);
-	spin_unlock(&p2sb_spinlock);
+	mutex_unlock(&p2sb_mutex);
 
 	res = &tco_res[1];
 	if (pci_dev->device == PCI_DEVICE_ID_INTEL_DNV_SMBUS)
-- 
2.32.0

Re: [PATCH 03/10] i2c: i801: Make p2sb_spinlock a mutex

[Jean Delvare]

On Sun, 01 Aug 2021 16:18:38 +0200, Heiner Kallweit wrote:
> p2sb_spinlock is used in i801_add_tco_spt() only and in process context
> only. Therefore a mutex is sufficient, and we can make the definition
> local to i801_add_tco_spt().

Mika, no objection?

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index bdb619bc0..504f02e1e 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1492,12 +1492,11 @@ static const struct itco_wdt_platform_data spt_tco_platform_data = {
>  	.version = 4,
>  };
>  
> -static DEFINE_SPINLOCK(p2sb_spinlock);
> -
>  static struct platform_device *
>  i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
>  		 struct resource *tco_res)
>  {
> +	static DEFINE_MUTEX(p2sb_mutex);

To be on the safe side, we should explicitly #include <linux/mutex.h>.

>  	struct resource *res;
>  	unsigned int devfn;
>  	u64 base64_addr;
> @@ -1510,7 +1509,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
>  	 * enumerated by the PCI subsystem, so we need to unhide/hide it
>  	 * to lookup the P2SB BAR.
>  	 */
> -	spin_lock(&p2sb_spinlock);
> +	mutex_lock(&p2sb_mutex);
>  
>  	devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
>  
> @@ -1528,7 +1527,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
>  	/* Hide the P2SB device, if it was hidden before */
>  	if (hidden)
>  		pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, hidden);
> -	spin_unlock(&p2sb_spinlock);
> +	mutex_unlock(&p2sb_mutex);
>  
>  	res = &tco_res[1];
>  	if (pci_dev->device == PCI_DEVICE_ID_INTEL_DNV_SMBUS)

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 03/10] i2c: i801: Make p2sb_spinlock a mutex

[Mika Westerberg]

On Thu, Aug 05, 2021 at 10:49:39AM +0200, Jean Delvare wrote:
> On Sun, 01 Aug 2021 16:18:38 +0200, Heiner Kallweit wrote:
> > p2sb_spinlock is used in i801_add_tco_spt() only and in process context
> > only. Therefore a mutex is sufficient, and we can make the definition
> > local to i801_add_tco_spt().
> 
> Mika, no objection?

No objections from my side :)

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > ---
> >  drivers/i2c/busses/i2c-i801.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index bdb619bc0..504f02e1e 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1492,12 +1492,11 @@ static const struct itco_wdt_platform_data spt_tco_platform_data = {
> >  	.version = 4,
> >  };
> >  
> > -static DEFINE_SPINLOCK(p2sb_spinlock);
> > -
> >  static struct platform_device *
> >  i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
> >  		 struct resource *tco_res)
> >  {
> > +	static DEFINE_MUTEX(p2sb_mutex);
> 
> To be on the safe side, we should explicitly #include <linux/mutex.h>.
> 
> >  	struct resource *res;
> >  	unsigned int devfn;
> >  	u64 base64_addr;
> > @@ -1510,7 +1509,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
> >  	 * enumerated by the PCI subsystem, so we need to unhide/hide it
> >  	 * to lookup the P2SB BAR.
> >  	 */
> > -	spin_lock(&p2sb_spinlock);
> > +	mutex_lock(&p2sb_mutex);
> >  
> >  	devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
> >  
> > @@ -1528,7 +1527,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
> >  	/* Hide the P2SB device, if it was hidden before */
> >  	if (hidden)
> >  		pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, hidden);
> > -	spin_unlock(&p2sb_spinlock);
> > +	mutex_unlock(&p2sb_mutex);
> >  
> >  	res = &tco_res[1];
> >  	if (pci_dev->device == PCI_DEVICE_ID_INTEL_DNV_SMBUS)
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> 
> -- 
> Jean Delvare
> SUSE L3 Support

[PATCH 04/10] i2c: i801: Remove not needed debug message

[Heiner Kallweit]

If a user is interested in such details he can enable smbus tracing.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 504f02e1e..d971ee20c 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -558,10 +558,6 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 					priv->len);
 				/* FIXME: Recover */
 				priv->len = I2C_SMBUS_BLOCK_MAX;
-			} else {
-				dev_dbg(&priv->pci_dev->dev,
-					"SMBus block read size is %d\n",
-					priv->len);
 			}
 			priv->data[-1] = priv->len;
 		}
-- 
2.32.0

Re: [PATCH 04/10] i2c: i801: Remove not needed debug message

[Jean Delvare]

On Sun, 01 Aug 2021 16:19:24 +0200, Heiner Kallweit wrote:
> If a user is interested in such details he can enable smbus tracing.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 504f02e1e..d971ee20c 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -558,10 +558,6 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>  					priv->len);
>  				/* FIXME: Recover */
>  				priv->len = I2C_SMBUS_BLOCK_MAX;
> -			} else {
> -				dev_dbg(&priv->pci_dev->dev,
> -					"SMBus block read size is %d\n",
> -					priv->len);
>  			}
>  			priv->data[-1] = priv->len;
>  		}

I think this is a leftover from the development phase, when we were just
adding support for this feature and needed to verify that the interrupt
callback was being called when and only when needed. I agree it no
longer has any value and can be removed now.

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

-- 
Jean Delvare
SUSE L3 Support

[PATCH 05/10] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Heiner Kallweit]

Replace the ugly cast of the return_value pointer with proper usage.
In addition use dmi_match() instead of open-coding it.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index d971ee20c..a6287c520 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1191,7 +1191,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
 
 	kfree(info);
 
-	*((bool *)return_value) = true;
+	*return_value = obj_handle;
 	return AE_CTRL_TERMINATE;
 
 smo88xx_not_found:
@@ -1201,13 +1201,10 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
 
 static bool is_dell_system_with_lis3lv02d(void)
 {
-	bool found;
-	const char *vendor;
+	acpi_handle found = NULL;
 
-	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
-	if (!vendor || strcmp(vendor, "Dell Inc."))
+	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
 		return false;
-
 	/*
 	 * Check that ACPI device SMO88xx is present and is functioning.
 	 * Function acpi_get_devices() already filters all ACPI devices
@@ -1216,9 +1213,7 @@ static bool is_dell_system_with_lis3lv02d(void)
 	 * accelerometer but unfortunately ACPI does not provide any other
 	 * information (like I2C address).
 	 */
-	found = false;
-	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
-			 (void **)&found);
+	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &found);
 
 	return found;
 }
-- 
2.32.0

Re: [PATCH 05/10] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Jean Delvare]

Hi Heiner,

On Sun, 01 Aug 2021 16:20:19 +0200, Heiner Kallweit wrote:
> Replace the ugly cast of the return_value pointer with proper usage.
> In addition use dmi_match() instead of open-coding it.

Pali, would you be able to test this patch?

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index d971ee20c..a6287c520 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1191,7 +1191,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
>  
>  	kfree(info);
>  
> -	*((bool *)return_value) = true;
> +	*return_value = obj_handle;
>  	return AE_CTRL_TERMINATE;
>  
>  smo88xx_not_found:
> @@ -1201,13 +1201,10 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
>  
>  static bool is_dell_system_with_lis3lv02d(void)
>  {
> -	bool found;
> -	const char *vendor;
> +	acpi_handle found = NULL;
>  
> -	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> -	if (!vendor || strcmp(vendor, "Dell Inc."))
> +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
>  		return false;

Looks good to me.

> -

I see no reason to remove that blank line.

>  	/*
>  	 * Check that ACPI device SMO88xx is present and is functioning.
>  	 * Function acpi_get_devices() already filters all ACPI devices
> @@ -1216,9 +1213,7 @@ static bool is_dell_system_with_lis3lv02d(void)
>  	 * accelerometer but unfortunately ACPI does not provide any other
>  	 * information (like I2C address).
>  	 */
> -	found = false;
> -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> -			 (void **)&found);
> +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &found);

The choice of return value by the acpi_get_devices() designer is very
unfortunate. It would have been much more convenient if the return
value was different whether a match has been found or not. Oh well.

I agree that the proposed change is a nicer way to work around this
limitation. Unfortunately I can't test this as I do not own a Dell
laptop. Were you able to test it? If not, I hope Pali will.

>  
>  	return found;
>  }

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 05/10] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Pali Rohár]

Hello!

On Thursday 05 August 2021 11:51:56 Jean Delvare wrote:
> Hi Heiner,
> 
> On Sun, 01 Aug 2021 16:20:19 +0200, Heiner Kallweit wrote:
> > Replace the ugly cast of the return_value pointer with proper usage.
> > In addition use dmi_match() instead of open-coding it.
> 
> Pali, would you be able to test this patch?

Tested now on Latitude E6440 and patch is working fine (no difference).

> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > ---
> >  drivers/i2c/busses/i2c-i801.c | 13 ++++---------
> >  1 file changed, 4 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index d971ee20c..a6287c520 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1191,7 +1191,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
> >  
> >  	kfree(info);
> >  
> > -	*((bool *)return_value) = true;
> > +	*return_value = obj_handle;

You are missing a cast here. "obj_handle" is of unknown typedef type
acpi_handle and *return_value is of type void*. So this can generate a
compile warning (either now or in future).

So you need to write it something like this:

  *((acpi_handle *)return_value) = obj_handle;

But what is benefit of this change? Is not usage of explicit true and
false values better than some acpi_handle type of undefined value stored
in obj_handle?

> >  	return AE_CTRL_TERMINATE;
> >  
> >  smo88xx_not_found:
> > @@ -1201,13 +1201,10 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
> >  
> >  static bool is_dell_system_with_lis3lv02d(void)
> >  {
> > -	bool found;
> > -	const char *vendor;
> > +	acpi_handle found = NULL;

Anyway, it looks strange to use name "found" for object handle
variable. I would expect that something named "found" is storing
something which refers to 2-state logic and not some handle value.

> >  
> > -	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> > -	if (!vendor || strcmp(vendor, "Dell Inc."))
> > +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
> >  		return false;
> 
> Looks good to me.
> 
> > -
> 
> I see no reason to remove that blank line.
> 
> >  	/*
> >  	 * Check that ACPI device SMO88xx is present and is functioning.
> >  	 * Function acpi_get_devices() already filters all ACPI devices
> > @@ -1216,9 +1213,7 @@ static bool is_dell_system_with_lis3lv02d(void)
> >  	 * accelerometer but unfortunately ACPI does not provide any other
> >  	 * information (like I2C address).
> >  	 */
> > -	found = false;
> > -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> > -			 (void **)&found);

Just to explain my motivation: I originally assigned found to false
value immediately before acpi_get_devices() function call to show that
this is the place where variable is used due to to API of that function.

> > +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &found);
> 
> The choice of return value by the acpi_get_devices() designer is very
> unfortunate. It would have been much more convenient if the return
> value was different whether a match has been found or not. Oh well.

I agree, it is very _original_ way...

> I agree that the proposed change is a nicer way to work around this
> limitation. Unfortunately I can't test this as I do not own a Dell
> laptop. Were you able to test it? If not, I hope Pali will.
> 
> >  
> >  	return found;
> >  }
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> 
> -- 
> Jean Delvare
> SUSE L3 Support

Re: [PATCH 05/10] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Heiner Kallweit]

On 05.08.2021 21:11, Pali Rohár wrote:
> Hello!
> 
> On Thursday 05 August 2021 11:51:56 Jean Delvare wrote:
>> Hi Heiner,
>>
>> On Sun, 01 Aug 2021 16:20:19 +0200, Heiner Kallweit wrote:
>>> Replace the ugly cast of the return_value pointer with proper usage.
>>> In addition use dmi_match() instead of open-coding it.
>>
>> Pali, would you be able to test this patch?
> 
> Tested now on Latitude E6440 and patch is working fine (no difference).
> 
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/i2c/busses/i2c-i801.c | 13 ++++---------
>>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>> index d971ee20c..a6287c520 100644
>>> --- a/drivers/i2c/busses/i2c-i801.c
>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>> @@ -1191,7 +1191,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
>>>  
>>>  	kfree(info);
>>>  
>>> -	*((bool *)return_value) = true;
>>> +	*return_value = obj_handle;
> 
> You are missing a cast here. "obj_handle" is of unknown typedef type
> acpi_handle and *return_value is of type void*. So this can generate a
> compile warning (either now or in future).
> 

acpi_handle is defined as:  typedef void *acpi_handle;
Therefore compiler is happy (as long as acpi_handle is any pointer type).

> So you need to write it something like this:
> 
>   *((acpi_handle *)return_value) = obj_handle;
> 
> But what is benefit of this change? Is not usage of explicit true and
> false values better than some acpi_handle type of undefined value stored
> in obj_handle?
> 
>From a logical perspective I agree. My motivation is that I see explicit
casts as a last resort and try to avoid them as far as possible.
The current code abuses a void* variable to store a bool. This makes the
implicit assumption that a pointer variable is always big enough to
store a bool.

With regard to "acpi_handle of undefined value": I'm just interested
in the information whether handle is NULL or not. That's the normal
implicit cast to bool like in every if(pointer) clause. 

>>>  	return AE_CTRL_TERMINATE;
>>>  
>>>  smo88xx_not_found:
>>> @@ -1201,13 +1201,10 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
>>>  
>>>  static bool is_dell_system_with_lis3lv02d(void)
>>>  {
>>> -	bool found;
>>> -	const char *vendor;
>>> +	acpi_handle found = NULL;
> 
> Anyway, it looks strange to use name "found" for object handle
> variable. I would expect that something named "found" is storing
> something which refers to 2-state logic and not some handle value.
> 
>>>  
>>> -	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
>>> -	if (!vendor || strcmp(vendor, "Dell Inc."))
>>> +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
>>>  		return false;
>>
>> Looks good to me.
>>
>>> -
>>
>> I see no reason to remove that blank line.
>>
>>>  	/*
>>>  	 * Check that ACPI device SMO88xx is present and is functioning.
>>>  	 * Function acpi_get_devices() already filters all ACPI devices
>>> @@ -1216,9 +1213,7 @@ static bool is_dell_system_with_lis3lv02d(void)
>>>  	 * accelerometer but unfortunately ACPI does not provide any other
>>>  	 * information (like I2C address).
>>>  	 */
>>> -	found = false;
>>> -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
>>> -			 (void **)&found);
> 
> Just to explain my motivation: I originally assigned found to false
> value immediately before acpi_get_devices() function call to show that
> this is the place where variable is used due to to API of that function.
> 
>>> +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &found);
>>
>> The choice of return value by the acpi_get_devices() designer is very
>> unfortunate. It would have been much more convenient if the return
>> value was different whether a match has been found or not. Oh well.
> 
> I agree, it is very _original_ way...
> 
>> I agree that the proposed change is a nicer way to work around this
>> limitation. Unfortunately I can't test this as I do not own a Dell
>> laptop. Were you able to test it? If not, I hope Pali will.
>>
>>>  
>>>  	return found;
>>>  }
>>
>> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>>
>> -- 
>> Jean Delvare
>> SUSE L3 Support

Re: [PATCH 05/10] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Pali Rohár]

On Thursday 05 August 2021 21:42:23 Heiner Kallweit wrote:
> On 05.08.2021 21:11, Pali Rohár wrote:
> > Hello!
> > 
> > On Thursday 05 August 2021 11:51:56 Jean Delvare wrote:
> >> Hi Heiner,
> >>
> >> On Sun, 01 Aug 2021 16:20:19 +0200, Heiner Kallweit wrote:
> >>> Replace the ugly cast of the return_value pointer with proper usage.
> >>> In addition use dmi_match() instead of open-coding it.
> >>
> >> Pali, would you be able to test this patch?
> > 
> > Tested now on Latitude E6440 and patch is working fine (no difference).
> > 
> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >>> ---
> >>>  drivers/i2c/busses/i2c-i801.c | 13 ++++---------
> >>>  1 file changed, 4 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >>> index d971ee20c..a6287c520 100644
> >>> --- a/drivers/i2c/busses/i2c-i801.c
> >>> +++ b/drivers/i2c/busses/i2c-i801.c
> >>> @@ -1191,7 +1191,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
> >>>  
> >>>  	kfree(info);
> >>>  
> >>> -	*((bool *)return_value) = true;
> >>> +	*return_value = obj_handle;
> > 
> > You are missing a cast here. "obj_handle" is of unknown typedef type
> > acpi_handle and *return_value is of type void*. So this can generate a
> > compile warning (either now or in future).
> > 
> 
> acpi_handle is defined as:  typedef void *acpi_handle;
> Therefore compiler is happy (as long as acpi_handle is any pointer type).

But point of this typedefing is to hide real type and let user to use
this "unknown" type without excepting any specific type.

"Therefore compiler is happy" here is there just a "hack" which
currently mute casting warning. But I think it is not something which
should be used as it is against how API / code of specific function was
designed.

For me this situation looks like: Somebody created API and specified how
to use it. It was realized that specified usage is not ideal for some
operations. And then people started "hacking" this API to look better
in these special cases.

But solution for this issue is to fix API (or create a new API which
better for this purpose), not hacking or workarounding it to looks
better by hiding / workarounding other important details.

> > So you need to write it something like this:
> > 
> >   *((acpi_handle *)return_value) = obj_handle;
> > 
> > But what is benefit of this change? Is not usage of explicit true and
> > false values better than some acpi_handle type of undefined value stored
> > in obj_handle?
> > 
> From a logical perspective I agree. My motivation is that I see explicit
> casts as a last resort and try to avoid them as far as possible.

But in this case you really should not avoid casting. It is different
pointer type of unknown (or rather hidden) type. Currently it does not
throw warning (maybe because compiler is not smart enough). But it does
not mean that code is really semantically correct or that in future
compiler (or its new version) does not throw warning.

Syntactically code looks better, but only until reader start studding
what code is really doing.

> The current code abuses a void* variable to store a bool. This makes the
> implicit assumption that a pointer variable is always big enough to
> store a bool.

I understand your concerns and also your motivation. API is not ideal
for usage. But both current and your proposed solution is just a hack to
workaround this API usage.

I think that according to C standard it is possible to cast between
pointer and non-pointer (integer-like) types only via uintptr_t (or
intptr_t) type...

So compliant C code could look like this?

    void func(void **ret) {
        *ret = (void *)(uintptr_t)1;
    }

    bool test(void) {
        void *found = (uintptr_t)0;
        func(&found);
        return (uintptr_t)found;
    }

or test() function may be simplified:

    bool test(void) {
        void *found = NULL;
        func(&found);
        return found;
    }

(but for me it looks strange if I'm reading _word_ NULL when used as a
false value in 2-state logic variable)

> With regard to "acpi_handle of undefined value": I'm just interested
> in the information whether handle is NULL or not. That's the normal
> implicit cast to bool like in every if(pointer) clause. 

Yes, of course, this is fully valid.

> >>>  	return AE_CTRL_TERMINATE;
> >>>  
> >>>  smo88xx_not_found:
> >>> @@ -1201,13 +1201,10 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
> >>>  
> >>>  static bool is_dell_system_with_lis3lv02d(void)
> >>>  {
> >>> -	bool found;
> >>> -	const char *vendor;
> >>> +	acpi_handle found = NULL;
> > 
> > Anyway, it looks strange to use name "found" for object handle
> > variable. I would expect that something named "found" is storing
> > something which refers to 2-state logic and not some handle value.
> > 
> >>>  
> >>> -	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> >>> -	if (!vendor || strcmp(vendor, "Dell Inc."))
> >>> +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
> >>>  		return false;
> >>
> >> Looks good to me.
> >>
> >>> -
> >>
> >> I see no reason to remove that blank line.
> >>
> >>>  	/*
> >>>  	 * Check that ACPI device SMO88xx is present and is functioning.
> >>>  	 * Function acpi_get_devices() already filters all ACPI devices
> >>> @@ -1216,9 +1213,7 @@ static bool is_dell_system_with_lis3lv02d(void)
> >>>  	 * accelerometer but unfortunately ACPI does not provide any other
> >>>  	 * information (like I2C address).
> >>>  	 */
> >>> -	found = false;
> >>> -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> >>> -			 (void **)&found);
> > 
> > Just to explain my motivation: I originally assigned found to false
> > value immediately before acpi_get_devices() function call to show that
> > this is the place where variable is used due to to API of that function.
> > 
> >>> +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &found);
> >>
> >> The choice of return value by the acpi_get_devices() designer is very
> >> unfortunate. It would have been much more convenient if the return
> >> value was different whether a match has been found or not. Oh well.
> > 
> > I agree, it is very _original_ way...
> > 
> >> I agree that the proposed change is a nicer way to work around this
> >> limitation. Unfortunately I can't test this as I do not own a Dell
> >> laptop. Were you able to test it? If not, I hope Pali will.
> >>
> >>>  
> >>>  	return found;
> >>>  }
> >>
> >> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> >>
> >> -- 
> >> Jean Delvare
> >> SUSE L3 Support
> 

Re: [PATCH 05/10] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Jean Delvare]

Hi Pali, Heiner,

On Fri, 6 Aug 2021 01:08:18 +0200, Pali Rohár wrote:
> On Thursday 05 August 2021 21:42:23 Heiner Kallweit wrote:
> > On 05.08.2021 21:11, Pali Rohár wrote:  
> > > On Thursday 05 August 2021 11:51:56 Jean Delvare wrote:  
> > >> On Sun, 01 Aug 2021 16:20:19 +0200, Heiner Kallweit wrote:  
> > >>> Replace the ugly cast of the return_value pointer with proper usage.
> > >>> In addition use dmi_match() instead of open-coding it.  
> > >>
> > >> Pali, would you be able to test this patch?  
> > > 
> > > Tested now on Latitude E6440 and patch is working fine (no difference).

Thank you for joining the discussion and testing.

> > >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > >>> ---
> > >>>  drivers/i2c/busses/i2c-i801.c | 13 ++++---------
> > >>>  1 file changed, 4 insertions(+), 9 deletions(-)
> > >>>
> > >>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > >>> index d971ee20c..a6287c520 100644
> > >>> --- a/drivers/i2c/busses/i2c-i801.c
> > >>> +++ b/drivers/i2c/busses/i2c-i801.c
> > >>> @@ -1191,7 +1191,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
> > >>>  
> > >>>  	kfree(info);
> > >>>  
> > >>> -	*((bool *)return_value) = true;
> > >>> +	*return_value = obj_handle;  
> > > 
> > > You are missing a cast here. "obj_handle" is of unknown typedef type
> > > acpi_handle and *return_value is of type void*. So this can generate a
> > > compile warning (either now or in future).
> > 
> > acpi_handle is defined as:  typedef void *acpi_handle;
> > Therefore compiler is happy (as long as acpi_handle is any pointer type).  
> 
> But point of this typedefing is to hide real type and let user to use
> this "unknown" type without excepting any specific type.
> 
> "Therefore compiler is happy" here is there just a "hack" which
> currently mute casting warning. But I think it is not something which
> should be used as it is against how API / code of specific function was
> designed.

But you can't respect that "unknown type" and still code cleanly. The
definition of acpi_handle hides the fact that this is a pointer type.
And you can't code cleanly if you need to manipulate an "object" but
have no idea whether it's a pointer, a scalar value or a structure.

(Well, you *could* with an API which does all the manipulation for you.
But that's not what we have here.)

In my opinion, the only value of "acpi_handle" as it is currently
defined is to let people know what type of data is found behind that
pointer. But I would much prefer real structure and an explicit pointer
to it.

> For me this situation looks like: Somebody created API and specified how
> to use it. It was realized that specified usage is not ideal for some
> operations. And then people started "hacking" this API to look better
> in these special cases.
> 
> But solution for this issue is to fix API (or create a new API which
> better for this purpose), not hacking or workarounding it to looks
> better by hiding / workarounding other important details.

The practical issue here is that we are talking about ACPICA, which is
partly developed outside / independently of the rest of the kernel. If
you can convince the ACPICA developers to implement a better
alternative to acpi_get_devices(), and/or make acpi_handle a better
type, I'll be more than happy to use that in the i2c-i801 driver. But I
don't see that happening any time soon, if ever, and for the time
being, we have to live with the poor API we are given.

> > > So you need to write it something like this:
> > > 
> > >   *((acpi_handle *)return_value) = obj_handle;
> > > 
> > > But what is benefit of this change? Is not usage of explicit true and
> > > false values better than some acpi_handle type of undefined value stored
> > > in obj_handle?
> >
> > From a logical perspective I agree. My motivation is that I see explicit
> > casts as a last resort and try to avoid them as far as possible.  

I tend to agree with that, FWIW.

> But in this case you really should not avoid casting. It is different
> pointer type of unknown (or rather hidden) type. Currently it does not
> throw warning (maybe because compiler is not smart enough). But it does
> not mean that code is really semantically correct or that in future
> compiler (or its new version) does not throw warning.

It's not about the smartness of the compiler. acpi_handle is equal to
void *, and Heiner's code is perfectly valid for a void *. No cast
needed with any compiler or on any platform.

Of course, things would break if the definition of acpi_handle ever
changes. Which would be great new actually, as I wrote above. And we
can revisit the code then.

> Syntactically code looks better, but only until reader start studding
> what code is really doing.

I have to admit I got pretty confused when reading it at first. On the
other hand, I was equally confused by the code that it attempts to
replace ^^

> > The current code abuses a void* variable to store a bool. This makes the
> > implicit assumption that a pointer variable is always big enough to
> > store a bool.  
> 
> I understand your concerns and also your motivation. API is not ideal
> for usage. But both current and your proposed solution is just a hack to
> workaround this API usage.
> 
> I think that according to C standard it is possible to cast between
> pointer and non-pointer (integer-like) types only via uintptr_t (or
> intptr_t) type...
> 
> So compliant C code could look like this?
> 
>     void func(void **ret) {
>         *ret = (void *)(uintptr_t)1;
>     }
> 
>     bool test(void) {
>         void *found = (uintptr_t)0;
>         func(&found);
>         return (uintptr_t)found;
>     }
> 
> or test() function may be simplified:
> 
>     bool test(void) {
>         void *found = NULL;
>         func(&found);
>         return found;
>     }
> 
> (but for me it looks strange if I'm reading _word_ NULL when used as a
> false value in 2-state logic variable)

I don't like this and I'm not even sure if that is allowed in the kernel.

> > With regard to "acpi_handle of undefined value": I'm just interested
> > in the information whether handle is NULL or not. That's the normal
> > implicit cast to bool like in every if(pointer) clause.   
> 
> Yes, of course, this is fully valid.
> (...)
> > > Anyway, it looks strange to use name "found" for object handle
> > > variable. I would expect that something named "found" is storing
> > > something which refers to 2-state logic and not some handle value.

It's actually a rather common pattern for lookup functions, returning
NULL when the expected item wasn't found, or a pointer to the item if
found. What makes things a bit weird here is that we don't actually
care about acpi_handle. All we need is a pointer to pretty much
anything, to differentiate between the found and not found cases.

Therefore I would propose the following alternative:

--- linux-5.13.orig/drivers/i2c/busses/i2c-i801.c	2021-08-06 11:11:44.275200299 +0200
+++ linux-5.13/drivers/i2c/busses/i2c-i801.c	2021-08-06 11:18:19.847469822 +0200
@@ -1194,7 +1194,7 @@ static acpi_status check_acpi_smo88xx_de
 
 	kfree(info);
 
-	*((bool *)return_value) = true;
+	*return_value = hid;	/* Could be any address, used as true value */
 	return AE_CTRL_TERMINATE;
 
 smo88xx_not_found:
@@ -1204,13 +1204,10 @@ static acpi_status check_acpi_smo88xx_de
 
 static bool is_dell_system_with_lis3lv02d(void)
 {
-	bool found;
-	const char *vendor;
+	acpi_handle found = NULL;
 
-	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
-	if (!vendor || strcmp(vendor, "Dell Inc."))
+	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
 		return false;
-
 	/*
 	 * Check that ACPI device SMO88xx is present and is functioning.
 	 * Function acpi_get_devices() already filters all ACPI devices
@@ -1219,9 +1216,7 @@ static bool is_dell_system_with_lis3lv02
 	 * accelerometer but unfortunately ACPI does not provide any other
 	 * information (like I2C address).
 	 */
-	found = false;
-	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
-			 (void **)&found);
+	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &found);
 
 	return found;
 }

Basically, it's Heiner's patch except for one line, the idea is to
return the HID string pointer (which has type char *) instead of the
acpi_handle. That way we don't depend on an opaque ACPI type. (I first
tried with the matching acpi_smo8800_ids entry but compiler complained
about incompatible pointer types due to the const).

Actually, I think we could use pretty much ANY pointer. Heck, that
would work too:

	*return_value = &disable_features;	/* Could be any address, used as true value */

Would be kinda confusing, but the comment is supposed to address that.
In fact we could even do:

	*return_value = &i;	/* Could be any address, used as true value */

That's the address of a local variable on the stack, which will no
longer exist by the time we check it, but that's still a non-NULL
pointer so it would work :-D Seriously, let's not do that, simply
because static code analyzers would possibly complain.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 05/10] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Pali Rohár]

On Friday 06 August 2021 11:55:19 Jean Delvare wrote:
> Hi Pali, Heiner,
> 
> On Fri, 6 Aug 2021 01:08:18 +0200, Pali Rohár wrote:
> > On Thursday 05 August 2021 21:42:23 Heiner Kallweit wrote:
> > > On 05.08.2021 21:11, Pali Rohár wrote:  
> > > > On Thursday 05 August 2021 11:51:56 Jean Delvare wrote:  
> > > >> On Sun, 01 Aug 2021 16:20:19 +0200, Heiner Kallweit wrote:  
> > > >>> Replace the ugly cast of the return_value pointer with proper usage.
> > > >>> In addition use dmi_match() instead of open-coding it.  
> > > >>
> > > >> Pali, would you be able to test this patch?  
> > > > 
> > > > Tested now on Latitude E6440 and patch is working fine (no difference).
> 
> Thank you for joining the discussion and testing.
> 
> > > >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > > >>> ---
> > > >>>  drivers/i2c/busses/i2c-i801.c | 13 ++++---------
> > > >>>  1 file changed, 4 insertions(+), 9 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > > >>> index d971ee20c..a6287c520 100644
> > > >>> --- a/drivers/i2c/busses/i2c-i801.c
> > > >>> +++ b/drivers/i2c/busses/i2c-i801.c
> > > >>> @@ -1191,7 +1191,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
> > > >>>  
> > > >>>  	kfree(info);
> > > >>>  
> > > >>> -	*((bool *)return_value) = true;
> > > >>> +	*return_value = obj_handle;  
> > > > 
> > > > You are missing a cast here. "obj_handle" is of unknown typedef type
> > > > acpi_handle and *return_value is of type void*. So this can generate a
> > > > compile warning (either now or in future).
> > > 
> > > acpi_handle is defined as:  typedef void *acpi_handle;
> > > Therefore compiler is happy (as long as acpi_handle is any pointer type).  
> > 
> > But point of this typedefing is to hide real type and let user to use
> > this "unknown" type without excepting any specific type.
> > 
> > "Therefore compiler is happy" here is there just a "hack" which
> > currently mute casting warning. But I think it is not something which
> > should be used as it is against how API / code of specific function was
> > designed.
> 
> But you can't respect that "unknown type" and still code cleanly. The
> definition of acpi_handle hides the fact that this is a pointer type.
> And you can't code cleanly if you need to manipulate an "object" but
> have no idea whether it's a pointer, a scalar value or a structure.
> 
> (Well, you *could* with an API which does all the manipulation for you.
> But that's not what we have here.)
> 
> In my opinion, the only value of "acpi_handle" as it is currently
> defined is to let people know what type of data is found behind that
> pointer. But I would much prefer real structure and an explicit pointer
> to it.

+1

> > For me this situation looks like: Somebody created API and specified how
> > to use it. It was realized that specified usage is not ideal for some
> > operations. And then people started "hacking" this API to look better
> > in these special cases.
> > 
> > But solution for this issue is to fix API (or create a new API which
> > better for this purpose), not hacking or workarounding it to looks
> > better by hiding / workarounding other important details.
> 
> The practical issue here is that we are talking about ACPICA, which is
> partly developed outside / independently of the rest of the kernel. If
> you can convince the ACPICA developers to implement a better
> alternative to acpi_get_devices(), and/or make acpi_handle a better
> type, I'll be more than happy to use that in the i2c-i801 driver. But I
> don't see that happening any time soon, if ever, and for the time
> being, we have to live with the poor API we are given.

Now I'm looking how is acpi_get_devices() used on other places and
basically it is used like before this patch...

    my_type my_value;
    ...
    *(my_type *)return_value = my_value;

(and in most cases my_type is acpi_handle)

So I'm not sure if it is a good idea to change this "common" usage just
in one place in one driver in kernel.

I do not remember details, but I think I chose this bool casting because
it was (and still is) common pattern how this function is used in
kernel.

> > > > So you need to write it something like this:
> > > > 
> > > >   *((acpi_handle *)return_value) = obj_handle;
> > > > 
> > > > But what is benefit of this change? Is not usage of explicit true and
> > > > false values better than some acpi_handle type of undefined value stored
> > > > in obj_handle?
> > >
> > > From a logical perspective I agree. My motivation is that I see explicit
> > > casts as a last resort and try to avoid them as far as possible.  
> 
> I tend to agree with that, FWIW.
> 
> > But in this case you really should not avoid casting. It is different
> > pointer type of unknown (or rather hidden) type. Currently it does not
> > throw warning (maybe because compiler is not smart enough). But it does
> > not mean that code is really semantically correct or that in future
> > compiler (or its new version) does not throw warning.
> 
> It's not about the smartness of the compiler. acpi_handle is equal to
> void *, and Heiner's code is perfectly valid for a void *. No cast
> needed with any compiler or on any platform.
> 
> Of course, things would break if the definition of acpi_handle ever
> changes. Which would be great new actually, as I wrote above. And we
> can revisit the code then.
> 
> > Syntactically code looks better, but only until reader start studding
> > what code is really doing.
> 
> I have to admit I got pretty confused when reading it at first. On the
> other hand, I was equally confused by the code that it attempts to
> replace ^^
> 
> > > The current code abuses a void* variable to store a bool. This makes the
> > > implicit assumption that a pointer variable is always big enough to
> > > store a bool.  
> > 
> > I understand your concerns and also your motivation. API is not ideal
> > for usage. But both current and your proposed solution is just a hack to
> > workaround this API usage.
> > 
> > I think that according to C standard it is possible to cast between
> > pointer and non-pointer (integer-like) types only via uintptr_t (or
> > intptr_t) type...
> > 
> > So compliant C code could look like this?
> > 
> >     void func(void **ret) {
> >         *ret = (void *)(uintptr_t)1;
> >     }
> > 
> >     bool test(void) {
> >         void *found = (uintptr_t)0;
> >         func(&found);
> >         return (uintptr_t)found;
> >     }
> > 
> > or test() function may be simplified:
> > 
> >     bool test(void) {
> >         void *found = NULL;
> >         func(&found);
> >         return found;
> >     }
> > 
> > (but for me it looks strange if I'm reading _word_ NULL when used as a
> > false value in 2-state logic variable)
> 
> I don't like this and I'm not even sure if that is allowed in the kernel.

I do not like it too...

Now I'm looking at kernel code and uintptr_t and pointer casting is used.

> > > With regard to "acpi_handle of undefined value": I'm just interested
> > > in the information whether handle is NULL or not. That's the normal
> > > implicit cast to bool like in every if(pointer) clause.   
> > 
> > Yes, of course, this is fully valid.
> > (...)
> > > > Anyway, it looks strange to use name "found" for object handle
> > > > variable. I would expect that something named "found" is storing
> > > > something which refers to 2-state logic and not some handle value.
> 
> It's actually a rather common pattern for lookup functions, returning
> NULL when the expected item wasn't found, or a pointer to the item if
> found. What makes things a bit weird here is that we don't actually
> care about acpi_handle. All we need is a pointer to pretty much
> anything, to differentiate between the found and not found cases.
> 
> Therefore I would propose the following alternative:
> 
> --- linux-5.13.orig/drivers/i2c/busses/i2c-i801.c	2021-08-06 11:11:44.275200299 +0200
> +++ linux-5.13/drivers/i2c/busses/i2c-i801.c	2021-08-06 11:18:19.847469822 +0200
> @@ -1194,7 +1194,7 @@ static acpi_status check_acpi_smo88xx_de
>  
>  	kfree(info);
>  
> -	*((bool *)return_value) = true;
> +	*return_value = hid;	/* Could be any address, used as true value */
>  	return AE_CTRL_TERMINATE;
>  
>  smo88xx_not_found:
> @@ -1204,13 +1204,10 @@ static acpi_status check_acpi_smo88xx_de
>  
>  static bool is_dell_system_with_lis3lv02d(void)
>  {
> -	bool found;
> -	const char *vendor;
> +	acpi_handle found = NULL;

Then you should define it as: "void *found = NULL;" and not as
acpi_handle anymore.

>  
> -	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> -	if (!vendor || strcmp(vendor, "Dell Inc."))
> +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
>  		return false;
> -
>  	/*
>  	 * Check that ACPI device SMO88xx is present and is functioning.
>  	 * Function acpi_get_devices() already filters all ACPI devices
> @@ -1219,9 +1216,7 @@ static bool is_dell_system_with_lis3lv02
>  	 * accelerometer but unfortunately ACPI does not provide any other
>  	 * information (like I2C address).
>  	 */
> -	found = false;
> -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> -			 (void **)&found);
> +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &found);
>  
>  	return found;
>  }
> 
> Basically, it's Heiner's patch except for one line, the idea is to
> return the HID string pointer (which has type char *) instead of the
> acpi_handle. That way we don't depend on an opaque ACPI type. (I first
> tried with the matching acpi_smo8800_ids entry but compiler complained
> about incompatible pointer types due to the const).
> 
> Actually, I think we could use pretty much ANY pointer. Heck, that
> would work too:
> 
> 	*return_value = &disable_features;	/* Could be any address, used as true value */
> 
> Would be kinda confusing, but the comment is supposed to address that.
> In fact we could even do:
> 
> 	*return_value = &i;	/* Could be any address, used as true value */
> 
> That's the address of a local variable on the stack, which will no
> longer exist by the time we check it, but that's still a non-NULL
> pointer so it would work :-D Seriously, let's not do that, simply
> because static code analyzers would possibly complain.

Ehm.. :-) Rather do not talk about other _interesting_ options. Somebody
could listen and come up with this idea as another workaround for
misusing ACPI functions API...

Basically using random pointer values or context-valid pointers just as
true value can cause issues of leaking pointer to context where it is
not valid anymore. And very smart code analyzers are correct if they
throw error that in variable is stored dangling pointer.

> -- 
> Jean Delvare
> SUSE L3 Support

Re: [PATCH 05/10] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Jean Delvare]

On Fri, 6 Aug 2021 12:47:00 +0200, Pali Rohár wrote:
> On Friday 06 August 2021 11:55:19 Jean Delvare wrote:
> > Therefore I would propose the following alternative:
> > 
> > --- linux-5.13.orig/drivers/i2c/busses/i2c-i801.c	2021-08-06 11:11:44.275200299 +0200
> > +++ linux-5.13/drivers/i2c/busses/i2c-i801.c	2021-08-06 11:18:19.847469822 +0200
> > @@ -1194,7 +1194,7 @@ static acpi_status check_acpi_smo88xx_de
> >  
> >  	kfree(info);
> >  
> > -	*((bool *)return_value) = true;
> > +	*return_value = hid;	/* Could be any address, used as true value */
> >  	return AE_CTRL_TERMINATE;
> >  
> >  smo88xx_not_found:
> > @@ -1204,13 +1204,10 @@ static acpi_status check_acpi_smo88xx_de
> >  
> >  static bool is_dell_system_with_lis3lv02d(void)
> >  {
> > -	bool found;
> > -	const char *vendor;
> > +	acpi_handle found = NULL;  
> 
> Then you should define it as: "void *found = NULL;" and not as
> acpi_handle anymore.

Oops, you are right of course. That being said, that could (should?)
already be the case with Heiner's patch.

> >  
> > -	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> > -	if (!vendor || strcmp(vendor, "Dell Inc."))
> > +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
> >  		return false;
> > -
> >  	/*
> >  	 * Check that ACPI device SMO88xx is present and is functioning.
> >  	 * Function acpi_get_devices() already filters all ACPI devices
> > @@ -1219,9 +1216,7 @@ static bool is_dell_system_with_lis3lv02
> >  	 * accelerometer but unfortunately ACPI does not provide any other
> >  	 * information (like I2C address).
> >  	 */
> > -	found = false;
> > -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> > -			 (void **)&found);
> > +	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &found);
> >  
> >  	return found;
> >  }
> > 
> > Basically, it's Heiner's patch except for one line, the idea is to
> > return the HID string pointer (which has type char *) instead of the
> > acpi_handle. That way we don't depend on an opaque ACPI type. (I first
> > tried with the matching acpi_smo8800_ids entry but compiler complained
> > about incompatible pointer types due to the const).
> > 
> > Actually, I think we could use pretty much ANY pointer. Heck, that
> > would work too:
> > 
> > 	*return_value = &disable_features;	/* Could be any address, used as true value */
> > 
> > Would be kinda confusing, but the comment is supposed to address that.
> > In fact we could even do:
> > 
> > 	*return_value = &i;	/* Could be any address, used as true value */
> > 
> > That's the address of a local variable on the stack, which will no
> > longer exist by the time we check it, but that's still a non-NULL
> > pointer so it would work :-D Seriously, let's not do that, simply
> > because static code analyzers would possibly complain.  
> 
> Ehm.. :-) Rather do not talk about other _interesting_ options. Somebody
> could listen and come up with this idea as another workaround for
> misusing ACPI functions API...
> 
> Basically using random pointer values or context-valid pointers just as
> true value can cause issues of leaking pointer to context where it is
> not valid anymore. And very smart code analyzers are correct if they
> throw error that in variable is stored dangling pointer.

Fully agreed, I was only making fun of the situation, this last
proposal was absolutely not meant to be taken seriously.

-- 
Jean Delvare
SUSE L3 Support

[PATCH 06/10] i2c: i801: Remove not needed check for PCI_COMMAND_INTX_DISABLE

[Heiner Kallweit]

do_pci_enable_device() takes care that PCI_COMMAND_INTX_DISABLE
is cleared if a legacy interrupt is used.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index a6287c520..5b9eebc1c 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1825,19 +1825,12 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		priv->features &= ~FEATURE_IRQ;
 
 	if (priv->features & FEATURE_IRQ) {
-		u16 pcictl, pcists;
+		u16 pcists;
 
 		/* Complain if an interrupt is already pending */
 		pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
 		if (pcists & PCI_STATUS_INTERRUPT)
 			dev_warn(&dev->dev, "An interrupt is pending!\n");
-
-		/* Check if interrupts have been disabled */
-		pci_read_config_word(priv->pci_dev, PCI_COMMAND, &pcictl);
-		if (pcictl & PCI_COMMAND_INTX_DISABLE) {
-			dev_info(&dev->dev, "Interrupts are disabled\n");
-			priv->features &= ~FEATURE_IRQ;
-		}
 	}
 
 	if (priv->features & FEATURE_IRQ) {
-- 
2.32.0

Re: [PATCH 06/10] i2c: i801: Remove not needed check for PCI_COMMAND_INTX_DISABLE

[Jean Delvare]

Hi Heiner,

On Sun, 01 Aug 2021 16:21:08 +0200, Heiner Kallweit wrote:
> do_pci_enable_device() takes care that PCI_COMMAND_INTX_DISABLE
> is cleared if a legacy interrupt is used.

Only if pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin) returned a
non-zero pin, if I read the code correctly. While I can't remember the
context in which I wrote this piece of code, I suppose that pin == 0
was the situation where this test was needed. I mean, the board
designer can legitimately not wire the interrupt pin, and require that
polling is being used, right?

In your favor, I can't find any online kernel log with this message.
However that doesn't mean I'm comfortable removing the safety check.

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index a6287c520..5b9eebc1c 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1825,19 +1825,12 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  		priv->features &= ~FEATURE_IRQ;
>  
>  	if (priv->features & FEATURE_IRQ) {
> -		u16 pcictl, pcists;
> +		u16 pcists;
>  
>  		/* Complain if an interrupt is already pending */
>  		pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
>  		if (pcists & PCI_STATUS_INTERRUPT)
>  			dev_warn(&dev->dev, "An interrupt is pending!\n");
> -
> -		/* Check if interrupts have been disabled */
> -		pci_read_config_word(priv->pci_dev, PCI_COMMAND, &pcictl);
> -		if (pcictl & PCI_COMMAND_INTX_DISABLE) {
> -			dev_info(&dev->dev, "Interrupts are disabled\n");
> -			priv->features &= ~FEATURE_IRQ;
> -		}
>  	}
>  
>  	if (priv->features & FEATURE_IRQ) {


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 06/10] i2c: i801: Remove not needed check for PCI_COMMAND_INTX_DISABLE

[Heiner Kallweit]

On 05.08.2021 12:41, Jean Delvare wrote:
> Hi Heiner,
> 
> On Sun, 01 Aug 2021 16:21:08 +0200, Heiner Kallweit wrote:
>> do_pci_enable_device() takes care that PCI_COMMAND_INTX_DISABLE
>> is cleared if a legacy interrupt is used.
> 
> Only if pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin) returned a
> non-zero pin, if I read the code correctly. While I can't remember the
> context in which I wrote this piece of code, I suppose that pin == 0
> was the situation where this test was needed. I mean, the board
> designer can legitimately not wire the interrupt pin, and require that
> polling is being used, right?
> 
I think we have such a use case, but it's handled in ACPI and results
in dev->irq == IRQ_NOTCONNECTED.

In case of pin == 0 pci_dev->irq is 0, and I'd expect that irq_to_desc(0)
returns NULL and request_threaded_irq() returns -EINVAL. This would
result in switching to polling.

Having said that I see no scenario where the check would be needed.

> In your favor, I can't find any online kernel log with this message.
> However that doesn't mean I'm comfortable removing the safety check.
> 
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 9 +--------
>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index a6287c520..5b9eebc1c 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1825,19 +1825,12 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>  		priv->features &= ~FEATURE_IRQ;
>>  
>>  	if (priv->features & FEATURE_IRQ) {
>> -		u16 pcictl, pcists;
>> +		u16 pcists;
>>  
>>  		/* Complain if an interrupt is already pending */
>>  		pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
>>  		if (pcists & PCI_STATUS_INTERRUPT)
>>  			dev_warn(&dev->dev, "An interrupt is pending!\n");
>> -
>> -		/* Check if interrupts have been disabled */
>> -		pci_read_config_word(priv->pci_dev, PCI_COMMAND, &pcictl);
>> -		if (pcictl & PCI_COMMAND_INTX_DISABLE) {
>> -			dev_info(&dev->dev, "Interrupts are disabled\n");
>> -			priv->features &= ~FEATURE_IRQ;
>> -		}
>>  	}
>>  
>>  	if (priv->features & FEATURE_IRQ) {
> 
> 

Re: [PATCH 06/10] i2c: i801: Remove not needed check for PCI_COMMAND_INTX_DISABLE

[Jean Delvare]

On Thu, 5 Aug 2021 22:04:18 +0200, Heiner Kallweit wrote:
> On 05.08.2021 12:41, Jean Delvare wrote:
> > On Sun, 01 Aug 2021 16:21:08 +0200, Heiner Kallweit wrote:  
> >> do_pci_enable_device() takes care that PCI_COMMAND_INTX_DISABLE
> >> is cleared if a legacy interrupt is used.  
> > 
> > Only if pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin) returned a
> > non-zero pin, if I read the code correctly. While I can't remember the
> > context in which I wrote this piece of code, I suppose that pin == 0
> > was the situation where this test was needed. I mean, the board
> > designer can legitimately not wire the interrupt pin, and require that
> > polling is being used, right?
>
> I think we have such a use case, but it's handled in ACPI and results
> in dev->irq == IRQ_NOTCONNECTED.

But not all systems use ACPI. The i2c-i801 driver could be used on
non-ACPI systems. I don't know if this is actually the case though. But
we definitely allow building kernels with ACPI disabled and I2C_I801
enabled.

> In case of pin == 0 pci_dev->irq is 0, and I'd expect that irq_to_desc(0)
> returns NULL and request_threaded_irq() returns -EINVAL. This would
> result in switching to polling.

Reading the !CONFIG_SPARSE_IRQ version of that function, it doesn't
seem so. irq_to_desc(0) would return &irq_desc[0]. IRQ 0 is not
invalid, it was the system clock on legacy PC systems, and probably
still is for compatibility reasons. I suppose the CONFIG_SPARSE_IRQ
version of irq_to_desc() is compatible with that too.

That being said, I suppose IRQ 0 is requested early at boot, so the
i2c-i801 driver would get -EBUSY or similar when trying to request it,
which in turn would result in falling back to polling mode, which is
what we want.

> Having said that I see no scenario where the check would be needed.
> 
> > In your favor, I can't find any online kernel log with this message.
> > However that doesn't mean I'm comfortable removing the safety check.

I'm still uncertain about what to do here. On the one hand, the check
can't hurt, and if we hit a corner case, could provide useful debugging
information. On the other hand, it may be dead code if you are correct,
and I don't like dead code.

I suppose we could remove the code for now, and see if anyone reports a
regression.

-- 
Jean Delvare
SUSE L3 Support

[PATCH 07/10] i2c: i801: Improve i801_acpi_probe/remove functions

[Heiner Kallweit]

By using ACPI_HANDLE() the handler argument can be retrieved directly.
Both address space handler functions check the handler argument and
return an error if it's NULL. This allows to further simplify the code.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5b9eebc1c..5fa8dc1cb 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1633,31 +1633,22 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
 
 static int i801_acpi_probe(struct i801_priv *priv)
 {
-	struct acpi_device *adev;
+	acpi_handle ah = ACPI_HANDLE(&priv->pci_dev->dev);
 	acpi_status status;
 
-	adev = ACPI_COMPANION(&priv->pci_dev->dev);
-	if (adev) {
-		status = acpi_install_address_space_handler(adev->handle,
-				ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler,
-				NULL, priv);
-		if (ACPI_SUCCESS(status))
-			return 0;
-	}
+	status = acpi_install_address_space_handler(ah, ACPI_ADR_SPACE_SYSTEM_IO,
+						    i801_acpi_io_handler, NULL, priv);
+	if (ACPI_SUCCESS(status))
+		return 0;
 
 	return acpi_check_resource_conflict(&priv->pci_dev->resource[SMBBAR]);
 }
 
 static void i801_acpi_remove(struct i801_priv *priv)
 {
-	struct acpi_device *adev;
-
-	adev = ACPI_COMPANION(&priv->pci_dev->dev);
-	if (!adev)
-		return;
+	acpi_handle ah = ACPI_HANDLE(&priv->pci_dev->dev);
 
-	acpi_remove_address_space_handler(adev->handle,
-		ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
+	acpi_remove_address_space_handler(ah, ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
 }
 #else
 static inline int i801_acpi_probe(struct i801_priv *priv) { return 0; }
-- 
2.32.0

Re: [PATCH 07/10] i2c: i801: Improve i801_acpi_probe/remove functions

[Jean Delvare]

On Sun, 01 Aug 2021 16:21:54 +0200, Heiner Kallweit wrote:
> By using ACPI_HANDLE() the handler argument can be retrieved directly.
> Both address space handler functions check the handler argument and
> return an error if it's NULL. This allows to further simplify the code.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 23 +++++++----------------
>  1 file changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 5b9eebc1c..5fa8dc1cb 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1633,31 +1633,22 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
>  
>  static int i801_acpi_probe(struct i801_priv *priv)
>  {
> -	struct acpi_device *adev;
> +	acpi_handle ah = ACPI_HANDLE(&priv->pci_dev->dev);
>  	acpi_status status;
>  
> -	adev = ACPI_COMPANION(&priv->pci_dev->dev);
> -	if (adev) {
> -		status = acpi_install_address_space_handler(adev->handle,
> -				ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler,
> -				NULL, priv);
> -		if (ACPI_SUCCESS(status))
> -			return 0;
> -	}
> +	status = acpi_install_address_space_handler(ah, ACPI_ADR_SPACE_SYSTEM_IO,
> +						    i801_acpi_io_handler, NULL, priv);
> +	if (ACPI_SUCCESS(status))
> +		return 0;
>  
>  	return acpi_check_resource_conflict(&priv->pci_dev->resource[SMBBAR]);
>  }
>  
>  static void i801_acpi_remove(struct i801_priv *priv)
>  {
> -	struct acpi_device *adev;
> -
> -	adev = ACPI_COMPANION(&priv->pci_dev->dev);
> -	if (!adev)
> -		return;
> +	acpi_handle ah = ACPI_HANDLE(&priv->pci_dev->dev);
>  
> -	acpi_remove_address_space_handler(adev->handle,
> -		ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
> +	acpi_remove_address_space_handler(ah, ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
>  }
>  #else
>  static inline int i801_acpi_probe(struct i801_priv *priv) { return 0; }

Looks completely reasonable.

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

Mika, no objection?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 07/10] i2c: i801: Improve i801_acpi_probe/remove functions

[Mika Westerberg]

On Thu, Aug 05, 2021 at 03:38:57PM +0200, Jean Delvare wrote:
> On Sun, 01 Aug 2021 16:21:54 +0200, Heiner Kallweit wrote:
> > By using ACPI_HANDLE() the handler argument can be retrieved directly.
> > Both address space handler functions check the handler argument and
> > return an error if it's NULL. This allows to further simplify the code.
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > ---
> >  drivers/i2c/busses/i2c-i801.c | 23 +++++++----------------
> >  1 file changed, 7 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index 5b9eebc1c..5fa8dc1cb 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1633,31 +1633,22 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
> >  
> >  static int i801_acpi_probe(struct i801_priv *priv)
> >  {
> > -	struct acpi_device *adev;
> > +	acpi_handle ah = ACPI_HANDLE(&priv->pci_dev->dev);
> >  	acpi_status status;
> >  
> > -	adev = ACPI_COMPANION(&priv->pci_dev->dev);
> > -	if (adev) {
> > -		status = acpi_install_address_space_handler(adev->handle,
> > -				ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler,
> > -				NULL, priv);
> > -		if (ACPI_SUCCESS(status))
> > -			return 0;
> > -	}
> > +	status = acpi_install_address_space_handler(ah, ACPI_ADR_SPACE_SYSTEM_IO,
> > +						    i801_acpi_io_handler, NULL, priv);
> > +	if (ACPI_SUCCESS(status))
> > +		return 0;
> >  
> >  	return acpi_check_resource_conflict(&priv->pci_dev->resource[SMBBAR]);
> >  }
> >  
> >  static void i801_acpi_remove(struct i801_priv *priv)
> >  {
> > -	struct acpi_device *adev;
> > -
> > -	adev = ACPI_COMPANION(&priv->pci_dev->dev);
> > -	if (!adev)
> > -		return;
> > +	acpi_handle ah = ACPI_HANDLE(&priv->pci_dev->dev);
> >  
> > -	acpi_remove_address_space_handler(adev->handle,
> > -		ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
> > +	acpi_remove_address_space_handler(ah, ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler);
> >  }
> >  #else
> >  static inline int i801_acpi_probe(struct i801_priv *priv) { return 0; }
> 
> Looks completely reasonable.
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> Tested-by: Jean Delvare <jdelvare@suse.de>
> 
> Mika, no objection?

No.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH 08/10] i2c: i801: Improve i801_add_mux

[Heiner Kallweit]

The return value of i801_add_mux() isn't used, so let's change it to void.
In addition remove the not needed cast to struct gpiod_lookup.
GPIO_LOOKUP() uses GPIO_LOOKUP_IDX() that includes this cast.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5fa8dc1cb..958b2e14b 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1389,7 +1389,7 @@ static const struct dmi_system_id mux_dmi_table[] = {
 };
 
 /* Setup multiplexing if needed */
-static int i801_add_mux(struct i801_priv *priv)
+static void i801_add_mux(struct i801_priv *priv)
 {
 	struct device *dev = &priv->adapter.dev;
 	const struct i801_mux_config *mux_config;
@@ -1398,7 +1398,7 @@ static int i801_add_mux(struct i801_priv *priv)
 	int i;
 
 	if (!priv->mux_drvdata)
-		return 0;
+		return;
 	mux_config = priv->mux_drvdata;
 
 	/* Prepare the platform data */
@@ -1414,13 +1414,11 @@ static int i801_add_mux(struct i801_priv *priv)
 			      struct_size(lookup, table, mux_config->n_gpios + 1),
 			      GFP_KERNEL);
 	if (!lookup)
-		return -ENOMEM;
+		return;
 	lookup->dev_id = "i2c-mux-gpio";
-	for (i = 0; i < mux_config->n_gpios; i++) {
-		lookup->table[i] = (struct gpiod_lookup)
-			GPIO_LOOKUP(mux_config->gpio_chip,
-				    mux_config->gpios[i], "mux", 0);
-	}
+	for (i = 0; i < mux_config->n_gpios; i++)
+		lookup->table[i] = GPIO_LOOKUP(mux_config->gpio_chip,
+					       mux_config->gpios[i], "mux", 0);
 	gpiod_add_lookup_table(lookup);
 	priv->lookup = lookup;
 
@@ -1438,8 +1436,6 @@ static int i801_add_mux(struct i801_priv *priv)
 		gpiod_remove_lookup_table(lookup);
 		dev_err(dev, "Failed to register i2c-mux-gpio device\n");
 	}
-
-	return PTR_ERR_OR_ZERO(priv->mux_pdev);
 }
 
 static void i801_del_mux(struct i801_priv *priv)
@@ -1469,7 +1465,7 @@ static unsigned int i801_get_adapter_class(struct i801_priv *priv)
 	return class;
 }
 #else
-static inline int i801_add_mux(struct i801_priv *priv) { return 0; }
+static inline void i801_add_mux(struct i801_priv *priv) { }
 static inline void i801_del_mux(struct i801_priv *priv) { }
 
 static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
-- 
2.32.0

Re: [PATCH 08/10] i2c: i801: Improve i801_add_mux

[Jean Delvare]

On Sun, 01 Aug 2021 16:22:31 +0200, Heiner Kallweit wrote:
> The return value of i801_add_mux() isn't used, so let's change it to void.
> In addition remove the not needed cast to struct gpiod_lookup.
> GPIO_LOOKUP() uses GPIO_LOOKUP_IDX() that includes this cast.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> (...)

Fine with me.

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

-- 
Jean Delvare
SUSE L3 Support

[PATCH 09/10] i2c: i801: Improve register_dell_lis3lv02d_i2c_device

[Heiner Kallweit]

- Use an initializer for struct i2c_board_info info
- Use dmi_match()
- Simplify loop logic

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 958b2e14b..1ca92a1e0 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1245,28 +1245,18 @@ static const struct {
 
 static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
 {
-	struct i2c_board_info info;
-	const char *dmi_product_name;
+	struct i2c_board_info info = { .type = "lis3lv02d" };
 	int i;
 
-	dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
-	for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {
-		if (strcmp(dmi_product_name,
-			   dell_lis3lv02d_devices[i].dmi_product_name) == 0)
-			break;
-	}
-
-	if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) {
-		dev_warn(&priv->pci_dev->dev,
-			 "Accelerometer lis3lv02d is present on SMBus but its"
-			 " address is unknown, skipping registration\n");
-		return;
-	}
+	for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i)
+		if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) {
+			info.addr = dell_lis3lv02d_devices[i].i2c_addr;
+			i2c_new_client_device(&priv->adapter, &info);
+			return;
+		}
 
-	memset(&info, 0, sizeof(struct i2c_board_info));
-	info.addr = dell_lis3lv02d_devices[i].i2c_addr;
-	strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
-	i2c_new_client_device(&priv->adapter, &info);
+	pci_warn(priv->pci_dev,
+		 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n");
 }
 
 /* Register optional slaves */
-- 
2.32.0

Re: [PATCH 09/10] i2c: i801: Improve register_dell_lis3lv02d_i2c_device

[Jean Delvare]

On Sun, 01 Aug 2021 16:23:34 +0200, Heiner Kallweit wrote:
> - Use an initializer for struct i2c_board_info info
> - Use dmi_match()
> - Simplify loop logic
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 958b2e14b..1ca92a1e0 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1245,28 +1245,18 @@ static const struct {
>  
>  static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
>  {
> -	struct i2c_board_info info;
> -	const char *dmi_product_name;
> +	struct i2c_board_info info = { .type = "lis3lv02d" };

Can it be moved to the inner loop where it is used, so that
initialization only takes place when needed? I don't know how the
compiler handles that, to be honest.

>  	int i;
>  
> -	dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> -	for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {
> -		if (strcmp(dmi_product_name,
> -			   dell_lis3lv02d_devices[i].dmi_product_name) == 0)
> -			break;
> -	}
> -
> -	if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) {
> -		dev_warn(&priv->pci_dev->dev,
> -			 "Accelerometer lis3lv02d is present on SMBus but its"
> -			 " address is unknown, skipping registration\n");
> -		return;
> -	}
> +	for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i)

Outer block without curly braces is discouraged for readability and
maintenance reasons (see Documentation/process/coding-style.rst section
3).

> +		if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) {

This causes dmi_get_system_info(DMI_PRODUCT_NAME) to be called for
every iteration of the loop, slowing down the lookup. It's an exported
function so it can't be inlined by the compiler. I know this happens
only once, but we try to keep boot times as short as possible.

> +			info.addr = dell_lis3lv02d_devices[i].i2c_addr;
> +			i2c_new_client_device(&priv->adapter, &info);
> +			return;
> +		}
>  
> -	memset(&info, 0, sizeof(struct i2c_board_info));
> -	info.addr = dell_lis3lv02d_devices[i].i2c_addr;
> -	strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> -	i2c_new_client_device(&priv->adapter, &info);
> +	pci_warn(priv->pci_dev,
> +		 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n");
>  }
>  
>  /* Register optional slaves */


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 09/10] i2c: i801: Improve register_dell_lis3lv02d_i2c_device

[Heiner Kallweit]

On 05.08.2021 16:23, Jean Delvare wrote:
> On Sun, 01 Aug 2021 16:23:34 +0200, Heiner Kallweit wrote:
>> - Use an initializer for struct i2c_board_info info
>> - Use dmi_match()
>> - Simplify loop logic
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 28 +++++++++-------------------
>>  1 file changed, 9 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 958b2e14b..1ca92a1e0 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1245,28 +1245,18 @@ static const struct {
>>  
>>  static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
>>  {
>> -	struct i2c_board_info info;
>> -	const char *dmi_product_name;
>> +	struct i2c_board_info info = { .type = "lis3lv02d" };
> 
> Can it be moved to the inner loop where it is used, so that
> initialization only takes place when needed? I don't know how the
> compiler handles that, to be honest.
> 
>>  	int i;
>>  
>> -	dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
>> -	for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {
>> -		if (strcmp(dmi_product_name,
>> -			   dell_lis3lv02d_devices[i].dmi_product_name) == 0)
>> -			break;
>> -	}
>> -
>> -	if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) {
>> -		dev_warn(&priv->pci_dev->dev,
>> -			 "Accelerometer lis3lv02d is present on SMBus but its"
>> -			 " address is unknown, skipping registration\n");
>> -		return;
>> -	}
>> +	for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i)
> 
> Outer block without curly braces is discouraged for readability and
> maintenance reasons (see Documentation/process/coding-style.rst section
> 3).
> 
>> +		if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) {
> 
> This causes dmi_get_system_info(DMI_PRODUCT_NAME) to be called for
> every iteration of the loop, slowing down the lookup. It's an exported
> function so it can't be inlined by the compiler. I know this happens
> only once, but we try to keep boot times as short as possible.
> 
I'm aware of this. However we just talk about a small in-memory operation and
the performance impact should be neglectable. dmi_get_system_info() is just
the following:

const char *dmi_get_system_info(int field)
{
	return dmi_ident[field];
}
EXPORT_SYMBOL(dmi_get_system_info);

I'd rate the simpler and better maintainable code higher.
But that's just a personal opinion and mileage may vary.


>> +			info.addr = dell_lis3lv02d_devices[i].i2c_addr;
>> +			i2c_new_client_device(&priv->adapter, &info);
>> +			return;
>> +		}
>>  
>> -	memset(&info, 0, sizeof(struct i2c_board_info));
>> -	info.addr = dell_lis3lv02d_devices[i].i2c_addr;
>> -	strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
>> -	i2c_new_client_device(&priv->adapter, &info);
>> +	pci_warn(priv->pci_dev,
>> +		 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n");
>>  }
>>  
>>  /* Register optional slaves */
> 
> 

Re: [PATCH 09/10] i2c: i801: Improve register_dell_lis3lv02d_i2c_device

[Jean Delvare]

Hi Heiner,

On Fri, 6 Aug 2021 22:49:00 +0200, Heiner Kallweit wrote:
> On 05.08.2021 16:23, Jean Delvare wrote:
> > On Sun, 01 Aug 2021 16:23:34 +0200, Heiner Kallweit wrote:  
> >> +		if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) {  
> > 
> > This causes dmi_get_system_info(DMI_PRODUCT_NAME) to be called for
> > every iteration of the loop, slowing down the lookup. It's an exported
> > function so it can't be inlined by the compiler. I know this happens
> > only once, but we try to keep boot times as short as possible.
> >   
> I'm aware of this. However we just talk about a small in-memory operation and
> the performance impact should be neglectable. dmi_get_system_info() is just
> the following:
> 
> const char *dmi_get_system_info(int field)
> {
> 	return dmi_ident[field];
> }
> EXPORT_SYMBOL(dmi_get_system_info);
> 
> I'd rate the simpler and better maintainable code higher.
> But that's just a personal opinion and mileage may vary.

I'm not worried about multiple calls to dmi_get_system_info(), which is
indeed simple and inlined anyway, but about multiple calls to dmi_match
(which can't be inlined). Function calls have a high cost (which is the
very reason why the compiler will try to inline functions whenever
possible).

I wouldn't mind if you were replacing several lines of code,
but here you are only removing one local variable and one simple line
of code, or 15 bytes of binary code total. But you add up to 8 function
calls, and that number could grow in the future as we add support for
more devices. That's why I say the benefit of the change is
questionable.

If it was new code, I probably wouldn't mind. But when changing
existing code, I need to be convinced that the new code is
unquestionably better than what we had before. That's not the case here.

(And don't get me wrong, I would love to live in a world where you don't
have do choose between best performance and and systematic use of
existing APIs. Alas, we often have to make choices in either direction.)

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 09/10] i2c: i801: Improve register_dell_lis3lv02d_i2c_device

[Heiner Kallweit]

On 09.08.2021 15:33, Jean Delvare wrote:
> Hi Heiner,
> 
> On Fri, 6 Aug 2021 22:49:00 +0200, Heiner Kallweit wrote:
>> On 05.08.2021 16:23, Jean Delvare wrote:
>>> On Sun, 01 Aug 2021 16:23:34 +0200, Heiner Kallweit wrote:  
>>>> +		if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) {  
>>>
>>> This causes dmi_get_system_info(DMI_PRODUCT_NAME) to be called for
>>> every iteration of the loop, slowing down the lookup. It's an exported
>>> function so it can't be inlined by the compiler. I know this happens
>>> only once, but we try to keep boot times as short as possible.
>>>   
>> I'm aware of this. However we just talk about a small in-memory operation and
>> the performance impact should be neglectable. dmi_get_system_info() is just
>> the following:
>>
>> const char *dmi_get_system_info(int field)
>> {
>> 	return dmi_ident[field];
>> }
>> EXPORT_SYMBOL(dmi_get_system_info);
>>
>> I'd rate the simpler and better maintainable code higher.
>> But that's just a personal opinion and mileage may vary.
> 
> I'm not worried about multiple calls to dmi_get_system_info(), which is
> indeed simple and inlined anyway, but about multiple calls to dmi_match
> (which can't be inlined). Function calls have a high cost (which is the
> very reason why the compiler will try to inline functions whenever
> possible).
> 
> I wouldn't mind if you were replacing several lines of code,
> but here you are only removing one local variable and one simple line
> of code, or 15 bytes of binary code total. But you add up to 8 function
> calls, and that number could grow in the future as we add support for
> more devices. That's why I say the benefit of the change is
> questionable.
> 

This code is called only if is_dell_system_with_lis3lv02d() returns true,
what further reduces the impact of what you mention. But sure, the preference
is a question of personal taste. Let me know whether you have any other
review comments regarding v2 of the series, then I'll drop this change in v3.

> If it was new code, I probably wouldn't mind. But when changing
> existing code, I need to be convinced that the new code is
> unquestionably better than what we had before. That's not the case here.
> 
> (And don't get me wrong, I would love to live in a world where you don't
> have do choose between best performance and and systematic use of
> existing APIs. Alas, we often have to make choices in either direction.)
> 

[PATCH 10/10] i2c: i801: Improve handling platform data for tco device

[Heiner Kallweit]

The platform data structures are used in the respective i801_add_tco
functions only. Therefore we can make the definitions local to these
functions.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 1ca92a1e0..64217479a 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1464,15 +1464,14 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
 }
 #endif
 
-static const struct itco_wdt_platform_data spt_tco_platform_data = {
-	.name = "Intel PCH",
-	.version = 4,
-};
-
 static struct platform_device *
 i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 		 struct resource *tco_res)
 {
+	static const struct itco_wdt_platform_data pldata = {
+		.name = "Intel PCH",
+		.version = 4,
+	};
 	static DEFINE_MUTEX(p2sb_mutex);
 	struct resource *res;
 	unsigned int devfn;
@@ -1516,22 +1515,20 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 	res->flags = IORESOURCE_MEM;
 
 	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
-					tco_res, 2, &spt_tco_platform_data,
-					sizeof(spt_tco_platform_data));
+					tco_res, 2, &pldata, sizeof(pldata));
 }
 
-static const struct itco_wdt_platform_data cnl_tco_platform_data = {
-	.name = "Intel PCH",
-	.version = 6,
-};
-
 static struct platform_device *
 i801_add_tco_cnl(struct i801_priv *priv, struct pci_dev *pci_dev,
 		 struct resource *tco_res)
 {
-	return platform_device_register_resndata(&pci_dev->dev,
-			"iTCO_wdt", -1, tco_res, 1, &cnl_tco_platform_data,
-			sizeof(cnl_tco_platform_data));
+	static const struct itco_wdt_platform_data pldata = {
+		.name = "Intel PCH",
+		.version = 6,
+	};
+
+	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
+						 tco_res, 1, &pldata, sizeof(pldata));
 }
 
 static void i801_add_tco(struct i801_priv *priv)
-- 
2.32.0

Re: [PATCH 10/10] i2c: i801: Improve handling platform data for tco device

[Jean Delvare]

On Sun, 01 Aug 2021 16:24:30 +0200, Heiner Kallweit wrote:
> The platform data structures are used in the respective i801_add_tco
> functions only. Therefore we can make the definitions local to these
> functions.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 1ca92a1e0..64217479a 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1464,15 +1464,14 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
>  }
>  #endif
>  
> -static const struct itco_wdt_platform_data spt_tco_platform_data = {
> -	.name = "Intel PCH",
> -	.version = 4,
> -};
> -
>  static struct platform_device *
>  i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
>  		 struct resource *tco_res)
>  {
> +	static const struct itco_wdt_platform_data pldata = {
> +		.name = "Intel PCH",
> +		.version = 4,
> +	};
>  	static DEFINE_MUTEX(p2sb_mutex);
>  	struct resource *res;
>  	unsigned int devfn;
> @@ -1516,22 +1515,20 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
>  	res->flags = IORESOURCE_MEM;
>  
>  	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> -					tco_res, 2, &spt_tco_platform_data,
> -					sizeof(spt_tco_platform_data));
> +					tco_res, 2, &pldata, sizeof(pldata));
>  }
>  
> -static const struct itco_wdt_platform_data cnl_tco_platform_data = {
> -	.name = "Intel PCH",
> -	.version = 6,
> -};
> -
>  static struct platform_device *
>  i801_add_tco_cnl(struct i801_priv *priv, struct pci_dev *pci_dev,
>  		 struct resource *tco_res)
>  {
> -	return platform_device_register_resndata(&pci_dev->dev,
> -			"iTCO_wdt", -1, tco_res, 1, &cnl_tco_platform_data,
> -			sizeof(cnl_tco_platform_data));
> +	static const struct itco_wdt_platform_data pldata = {
> +		.name = "Intel PCH",
> +		.version = 6,
> +	};
> +
> +	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> +						 tco_res, 1, &pldata, sizeof(pldata));
>  }
>  
>  static void i801_add_tco(struct i801_priv *priv)

I don't really care either way, to be honest. But fine with me.

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 10/10] i2c: i801: Improve handling platform data for tco device

[Heiner Kallweit]

On 05.08.2021 20:32, Jean Delvare wrote:
> On Sun, 01 Aug 2021 16:24:30 +0200, Heiner Kallweit wrote:
>> The platform data structures are used in the respective i801_add_tco
>> functions only. Therefore we can make the definitions local to these
>> functions.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 27 ++++++++++++---------------
>>  1 file changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 1ca92a1e0..64217479a 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1464,15 +1464,14 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
>>  }
>>  #endif
>>  
>> -static const struct itco_wdt_platform_data spt_tco_platform_data = {
>> -	.name = "Intel PCH",
>> -	.version = 4,
>> -};
>> -
>>  static struct platform_device *
>>  i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
>>  		 struct resource *tco_res)
>>  {
>> +	static const struct itco_wdt_platform_data pldata = {
>> +		.name = "Intel PCH",
>> +		.version = 4,
>> +	};
>>  	static DEFINE_MUTEX(p2sb_mutex);
>>  	struct resource *res;
>>  	unsigned int devfn;
>> @@ -1516,22 +1515,20 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
>>  	res->flags = IORESOURCE_MEM;
>>  
>>  	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
>> -					tco_res, 2, &spt_tco_platform_data,
>> -					sizeof(spt_tco_platform_data));
>> +					tco_res, 2, &pldata, sizeof(pldata));
>>  }
>>  
>> -static const struct itco_wdt_platform_data cnl_tco_platform_data = {
>> -	.name = "Intel PCH",
>> -	.version = 6,
>> -};
>> -
>>  static struct platform_device *
>>  i801_add_tco_cnl(struct i801_priv *priv, struct pci_dev *pci_dev,
>>  		 struct resource *tco_res)
>>  {
>> -	return platform_device_register_resndata(&pci_dev->dev,
>> -			"iTCO_wdt", -1, tco_res, 1, &cnl_tco_platform_data,
>> -			sizeof(cnl_tco_platform_data));
>> +	static const struct itco_wdt_platform_data pldata = {
>> +		.name = "Intel PCH",
>> +		.version = 6,
>> +	};
>> +
>> +	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
>> +						 tco_res, 1, &pldata, sizeof(pldata));
>>  }
>>  
>>  static void i801_add_tco(struct i801_priv *priv)
> 
> I don't really care either way, to be honest. But fine with me.
> 
To explain the motivation: I try to restrict visibility of a variable as much
as possible. This reduces risk of misuse / mistakes / name collisions.

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