| Message ID | 20190312145554.25488-2-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/2] i2c: i2c-designware-platdrv: Cleanup setting of the adapter number | expand |
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 >
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>
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!
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 |
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(-)