Patchwork [v2,2/2] i2c: i2c-designware-platdrv: Always use a dynamic adapter number

login
register
mail settings
Submitter Hans de Goede
Date March 12, 2019, 2:55 p.m.
Message ID <20190312145554.25488-2-hdegoede@redhat.com>
Download mbox | patch
Permalink /patch/747237/
State New
Headers show

Comments

Hans de Goede - March 12, 2019, 2:55 p.m.
Before this commit the i2c-designware-platdrv assumes that if the pdev
has an apci-companion it should use a dynamic adapter-nr and it sets
adapter->nr to -1, otherwise it will use pdev->id as adapter->nr.

There are 3 ways how platform_device-s to which i2c-designware-platdrv
will bind can be instantiated:

1) Through of / devicetree
2) Through ACPI enumeration
3) Explicitly instantiated through platform_device_create + add

1) In case of devicetree-instantiation the drivers/of code always sets
pdev->id to PLATFORM_DEVID_NONE, which is -1 so in this case both paths
to set adapter->nr end up doing the same thing.

2) In case of ACPI instantiation the device will always have an
ACPI-companion, so we are already using dynamic adapter-nrs.

3) There are 2 places manually instantiating a designware_i2c platform_dev:
drivers/mfd/intel_quark_i2c_gpio.c
drivers/mfd/intel-lpss.c

In the intel_quark_i2c_gpio.c case pdev->id is always 0, so switching to
dynamic adapter-nrs here could lead to the bus-number no longer being
stable, but the quark X1000 only has 1 i2c-controller, which will also
be assigned bus-number 0 when using dynamic adapter-nrs.

In the intel-lpss.c case intel_lpss_probe() is called from either
intel-lpss-acpi.c in which case there always is an ACPI-companion, or
from intel-lpss-pci.c. In most cases devices handled by intel-lpss-pci.c
also have an ACPI-companion, so we use a dynamic adapter-nr. But in some
cases the ACPI-companion is missing and we would use pdev->id (allocated
from intel_lpss_devid_ida). Devices which use the intel-lpss-pci.c code
typically have many i2c busses, so using pdev->id in this case may lead
to a bus-number conflict, triggering a WARN(id < 0, "couldn't get idr")
in i2c-core-base.c causing an oops an the adapter registration to fail.
So in this case using non dynamic adapter-nrs is actually undesirable.

One machine on which this oops was triggering is the Apollo Lake based
Acer TravelMate Spin B118.

TL;DR: Switching to always using dynamic adapter-numbers does not make
any difference in most cases and in the one case where it does make a
difference the behavior change is desirable because the old behavior
caused an oops.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1687065
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Simply always use dynamic adapter numbers
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
Andy Shevchenko - March 12, 2019, 3:38 p.m.
On Tue, Mar 12, 2019 at 03:55:54PM +0100, Hans de Goede wrote:
> Before this commit the i2c-designware-platdrv assumes that if the pdev
> has an apci-companion it should use a dynamic adapter-nr and it sets
> adapter->nr to -1, otherwise it will use pdev->id as adapter->nr.
> 
> There are 3 ways how platform_device-s to which i2c-designware-platdrv
> will bind can be instantiated:
> 
> 1) Through of / devicetree
> 2) Through ACPI enumeration
> 3) Explicitly instantiated through platform_device_create + add
> 
> 1) In case of devicetree-instantiation the drivers/of code always sets
> pdev->id to PLATFORM_DEVID_NONE, which is -1 so in this case both paths
> to set adapter->nr end up doing the same thing.
> 
> 2) In case of ACPI instantiation the device will always have an
> ACPI-companion, so we are already using dynamic adapter-nrs.
> 
> 3) There are 2 places manually instantiating a designware_i2c platform_dev:
> drivers/mfd/intel_quark_i2c_gpio.c
> drivers/mfd/intel-lpss.c
> 
> In the intel_quark_i2c_gpio.c case pdev->id is always 0, so switching to
> dynamic adapter-nrs here could lead to the bus-number no longer being
> stable, but the quark X1000 only has 1 i2c-controller, which will also
> be assigned bus-number 0 when using dynamic adapter-nrs.
> 
> In the intel-lpss.c case intel_lpss_probe() is called from either
> intel-lpss-acpi.c in which case there always is an ACPI-companion, or
> from intel-lpss-pci.c. In most cases devices handled by intel-lpss-pci.c
> also have an ACPI-companion, so we use a dynamic adapter-nr. But in some
> cases the ACPI-companion is missing and we would use pdev->id (allocated
> from intel_lpss_devid_ida). Devices which use the intel-lpss-pci.c code
> typically have many i2c busses, so using pdev->id in this case may lead
> to a bus-number conflict, triggering a WARN(id < 0, "couldn't get idr")
> in i2c-core-base.c causing an oops an the adapter registration to fail.
> So in this case using non dynamic adapter-nrs is actually undesirable.
> 
> One machine on which this oops was triggering is the Apollo Lake based
> Acer TravelMate Spin B118.
> 
> TL;DR: Switching to always using dynamic adapter-numbers does not make
> any difference in most cases and in the one case where it does make a
> difference the behavior change is desirable because the old behavior
> caused an oops.
> 

Thanks, this looks better, indeed.
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1687065
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Simply always use dynamic adapter numbers
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 30529839cbd2..416f89b8f881 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -363,10 +363,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>  	adap->class = I2C_CLASS_DEPRECATED;
>  	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>  	adap->dev.of_node = pdev->dev.of_node;
> -	if (has_acpi_companion(&pdev->dev))
> -		adap->nr = -1;
> -	else
> -		adap->nr = pdev->id;
> +	adap->nr = -1;
>  
>  	dev_pm_set_driver_flags(&pdev->dev,
>  				DPM_FLAG_SMART_PREPARE |
> -- 
> 2.20.1
>
Jarkko Nikula - March 13, 2019, 8:15 a.m.
On 3/12/19 5:38 PM, Andy Shevchenko wrote:
> On Tue, Mar 12, 2019 at 03:55:54PM +0100, Hans de Goede wrote:
>> Before this commit the i2c-designware-platdrv assumes that if the pdev
>> has an apci-companion it should use a dynamic adapter-nr and it sets
>> adapter->nr to -1, otherwise it will use pdev->id as adapter->nr.
>>
>> There are 3 ways how platform_device-s to which i2c-designware-platdrv
>> will bind can be instantiated:
>>
>> 1) Through of / devicetree
>> 2) Through ACPI enumeration
>> 3) Explicitly instantiated through platform_device_create + add
>>
>> 1) In case of devicetree-instantiation the drivers/of code always sets
>> pdev->id to PLATFORM_DEVID_NONE, which is -1 so in this case both paths
>> to set adapter->nr end up doing the same thing.
>>
>> 2) In case of ACPI instantiation the device will always have an
>> ACPI-companion, so we are already using dynamic adapter-nrs.
>>
>> 3) There are 2 places manually instantiating a designware_i2c platform_dev:
>> drivers/mfd/intel_quark_i2c_gpio.c
>> drivers/mfd/intel-lpss.c
>>
>> In the intel_quark_i2c_gpio.c case pdev->id is always 0, so switching to
>> dynamic adapter-nrs here could lead to the bus-number no longer being
>> stable, but the quark X1000 only has 1 i2c-controller, which will also
>> be assigned bus-number 0 when using dynamic adapter-nrs.
>>
>> In the intel-lpss.c case intel_lpss_probe() is called from either
>> intel-lpss-acpi.c in which case there always is an ACPI-companion, or
>> from intel-lpss-pci.c. In most cases devices handled by intel-lpss-pci.c
>> also have an ACPI-companion, so we use a dynamic adapter-nr. But in some
>> cases the ACPI-companion is missing and we would use pdev->id (allocated
>> from intel_lpss_devid_ida). Devices which use the intel-lpss-pci.c code
>> typically have many i2c busses, so using pdev->id in this case may lead
>> to a bus-number conflict, triggering a WARN(id < 0, "couldn't get idr")
>> in i2c-core-base.c causing an oops an the adapter registration to fail.
>> So in this case using non dynamic adapter-nrs is actually undesirable.
>>
>> One machine on which this oops was triggering is the Apollo Lake based
>> Acer TravelMate Spin B118.
>>
>> TL;DR: Switching to always using dynamic adapter-numbers does not make
>> any difference in most cases and in the one case where it does make a
>> difference the behavior change is desirable because the old behavior
>> caused an oops.
>>
> 
> Thanks, this looks better, indeed.
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Wolfram Sang - March 13, 2019, 5:10 p.m.
On Tue, Mar 12, 2019 at 03:55:54PM +0100, Hans de Goede wrote:
> Before this commit the i2c-designware-platdrv assumes that if the pdev
> has an apci-companion it should use a dynamic adapter-nr and it sets
> adapter->nr to -1, otherwise it will use pdev->id as adapter->nr.
> 
> There are 3 ways how platform_device-s to which i2c-designware-platdrv
> will bind can be instantiated:
> 
> 1) Through of / devicetree
> 2) Through ACPI enumeration
> 3) Explicitly instantiated through platform_device_create + add
> 
> 1) In case of devicetree-instantiation the drivers/of code always sets
> pdev->id to PLATFORM_DEVID_NONE, which is -1 so in this case both paths
> to set adapter->nr end up doing the same thing.
> 
> 2) In case of ACPI instantiation the device will always have an
> ACPI-companion, so we are already using dynamic adapter-nrs.
> 
> 3) There are 2 places manually instantiating a designware_i2c platform_dev:
> drivers/mfd/intel_quark_i2c_gpio.c
> drivers/mfd/intel-lpss.c
> 
> In the intel_quark_i2c_gpio.c case pdev->id is always 0, so switching to
> dynamic adapter-nrs here could lead to the bus-number no longer being
> stable, but the quark X1000 only has 1 i2c-controller, which will also
> be assigned bus-number 0 when using dynamic adapter-nrs.
> 
> In the intel-lpss.c case intel_lpss_probe() is called from either
> intel-lpss-acpi.c in which case there always is an ACPI-companion, or
> from intel-lpss-pci.c. In most cases devices handled by intel-lpss-pci.c
> also have an ACPI-companion, so we use a dynamic adapter-nr. But in some
> cases the ACPI-companion is missing and we would use pdev->id (allocated
> from intel_lpss_devid_ida). Devices which use the intel-lpss-pci.c code
> typically have many i2c busses, so using pdev->id in this case may lead
> to a bus-number conflict, triggering a WARN(id < 0, "couldn't get idr")
> in i2c-core-base.c causing an oops an the adapter registration to fail.
> So in this case using non dynamic adapter-nrs is actually undesirable.
> 
> One machine on which this oops was triggering is the Apollo Lake based
> Acer TravelMate Spin B118.
> 
> TL;DR: Switching to always using dynamic adapter-numbers does not make
> any difference in most cases and in the one case where it does make a
> difference the behavior change is desirable because the old behavior
> caused an oops.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1687065
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied to for-current, thanks!

Patch

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 30529839cbd2..416f89b8f881 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -363,10 +363,7 @@  static int dw_i2c_plat_probe(struct platform_device *pdev)
 	adap->class = I2C_CLASS_DEPRECATED;
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->dev.of_node = pdev->dev.of_node;
-	if (has_acpi_companion(&pdev->dev))
-		adap->nr = -1;
-	else
-		adap->nr = pdev->id;
+	adap->nr = -1;
 
 	dev_pm_set_driver_flags(&pdev->dev,
 				DPM_FLAG_SMART_PREPARE |