[PATCH 0/2] intel-lpss-pci-i2c: Fix oops when there is no ACPI fwnode

[PATCH 0/2] intel-lpss-pci-i2c: Fix oops when there is no ACPI fwnode

[Hans de Goede]

Hi All,

Here is a patch series fixing an oops when there is no ACPI fwnode
associated with a PCI enumerated LPSS I2C controller:
https://bugzilla.redhat.com/show_bug.cgi?id=1687065

This is fixed by the first patch in this series, which modifies
i2c-designware-platdrv.c to fix this. The fix is enabled conditionally
because otherwise it might break things on platforms where i2c-designware
controllers are enumerated through e.g. devicetree.

The second patch modifies intel-lpss-pci.c to pass the new
"linux,use-dynamic-adapter-nr" devicetree property to activate the fix.

The first patch should be merged first, so that the devicetree prop name
is "stable", then the second patch can be merged independently through the
mfd tree.

Regards,

Hans

[PATCH 1/2] i2c: i2c-designware-platdrv: Allow a dynamic adap. nr without an ACPI fwnode

[Hans de Goede]

Before this commit the i2c-designware-platdrv assumes that if the pdev
has an apci-companion it should use a dynamic adapter-nr and otherwise
it will use pdev->id as adapter-nr.

On some devices e.g. the Apollo Lake using Acer TravelMate Spin B118,
some of the LPSS i2c-adapters are enumerated through PCI and do not have
an ACPI fwnode. These devices are handled as mfd devices so they end up
using the i2c-designware-platdrv driver.

This results in the i2c-adapter being registered with the mfd generated
pdev->id as adapter-nr, which conflicts with existing adapters, triggering
a WARN(id < 0, "couldn't get idr") in i2c-core-base.c and causing the
adapter registration to fail.

This commit adds support for setting a "linux,use-dynamic-adapter-nr"
device property on the device to make i2c-designware-platdrv use dynamic
adapter-nrs on devices without an ACPI fwnode, together with changes to
drivers/mfd/intel-lpss-pci.c to set this, this fixes the WARN.

Before this commit the setting of the adapter.nr was somewhat convoluted,
in the acpi_companion case it was set from dw_i2c_acpi_configure, in the
non acpi_companion case it was set from dw_i2c_set_fifo_size() based on
tx_fifo_depth not being set yet. This commit also cleans this up.

Note the "linux,use-dynamic-adapter-nr" is meant for kernel internal use
only, therefor it is NOT documented under Documents/devicetree/bindings.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1687065
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index ead5e7de3e4d..9f28159efdb2 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -86,7 +86,6 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 	struct i2c_timings *t = &dev->timings;
 	u32 ss_ht = 0, fp_ht = 0, hs_ht = 0, fs_ht = 0;
 
-	dev->adapter.nr = -1;
 	dev->tx_fifo_depth = 32;
 	dev->rx_fifo_depth = 32;
 
@@ -219,7 +218,7 @@ static void i2c_dw_configure_slave(struct dw_i2c_dev *dev)
 	dev->mode = DW_IC_SLAVE;
 }
 
-static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
+static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev)
 {
 	u32 param, tx_fifo_depth, rx_fifo_depth;
 
@@ -233,7 +232,6 @@ static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
 	if (!dev->tx_fifo_depth) {
 		dev->tx_fifo_depth = tx_fifo_depth;
 		dev->rx_fifo_depth = rx_fifo_depth;
-		dev->adapter.nr = id;
 	} else if (tx_fifo_depth >= 2) {
 		dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
 				tx_fifo_depth);
@@ -324,6 +322,13 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	if (has_acpi_companion(&pdev->dev))
 		dw_i2c_acpi_configure(pdev);
 
+	if (has_acpi_companion(&pdev->dev) ||
+	    device_property_read_bool(&pdev->dev,
+				      "linux,use-dynamic-adapter-nr"))
+		dev->adapter.nr = -1;
+	else
+		dev->adapter.nr = pdev->id;
+
 	/*
 	 * Only standard mode at 100kHz, fast mode at 400kHz,
 	 * fast mode plus at 1MHz and high speed mode at 3.4MHz are supported.
@@ -358,7 +363,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 				div_u64(clk_khz * t->sda_hold_ns + 500000, 1000000);
 	}
 
-	dw_i2c_set_fifo_size(dev, pdev->id);
+	dw_i2c_set_fifo_size(dev);
 
 	adap = &dev->adapter;
 	adap->owner = THIS_MODULE;
-- 
2.20.1

Re: [PATCH 1/2] i2c: i2c-designware-platdrv: Allow a dynamic adap. nr without an ACPI fwnode

[Andy Shevchenko]

On Mon, Mar 11, 2019 at 12:22:15PM +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 otherwise
> it will use pdev->id as adapter-nr.
> 
> On some devices e.g. the Apollo Lake using Acer TravelMate Spin B118,
> some of the LPSS i2c-adapters are enumerated through PCI and do not have
> an ACPI fwnode. These devices are handled as mfd devices so they end up
> using the i2c-designware-platdrv driver.
> 
> This results in the i2c-adapter being registered with the mfd generated
> pdev->id as adapter-nr, which conflicts with existing adapters, triggering
> a WARN(id < 0, "couldn't get idr") in i2c-core-base.c and causing the
> adapter registration to fail.
> 
> This commit adds support for setting a "linux,use-dynamic-adapter-nr"
> device property on the device to make i2c-designware-platdrv use dynamic
> adapter-nrs on devices without an ACPI fwnode, together with changes to
> drivers/mfd/intel-lpss-pci.c to set this, this fixes the WARN.
> 

> Before this commit the setting of the adapter.nr was somewhat convoluted,
> in the acpi_companion case it was set from dw_i2c_acpi_configure, in the
> non acpi_companion case it was set from dw_i2c_set_fifo_size() based on
> tx_fifo_depth not being set yet. This commit also cleans this up.

Can we split this to two patches, i.e. one is almost the same as this one,
except the second one adds a new property check to the conditional?

If you agree to do so, you may add mine Rb tag to the first one out of three.

> Note the "linux,use-dynamic-adapter-nr" is meant for kernel internal use
> only, therefor it is NOT documented under Documents/devicetree/bindings.

To the second and third ones, can we rather check if the device has fwnode
either ACPI or swnode? AFAIU now we have swnode assigned during MFD device
registration and can easily distinguish this w/o any additional properties.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] i2c: i2c-designware-platdrv: Allow a dynamic adap. nr without an ACPI fwnode

[Hans de Goede]

Hi,

On 11-03-19 13:52, Andy Shevchenko wrote:
> On Mon, Mar 11, 2019 at 12:22:15PM +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 otherwise
>> it will use pdev->id as adapter-nr.
>>
>> On some devices e.g. the Apollo Lake using Acer TravelMate Spin B118,
>> some of the LPSS i2c-adapters are enumerated through PCI and do not have
>> an ACPI fwnode. These devices are handled as mfd devices so they end up
>> using the i2c-designware-platdrv driver.
>>
>> This results in the i2c-adapter being registered with the mfd generated
>> pdev->id as adapter-nr, which conflicts with existing adapters, triggering
>> a WARN(id < 0, "couldn't get idr") in i2c-core-base.c and causing the
>> adapter registration to fail.
>>
>> This commit adds support for setting a "linux,use-dynamic-adapter-nr"
>> device property on the device to make i2c-designware-platdrv use dynamic
>> adapter-nrs on devices without an ACPI fwnode, together with changes to
>> drivers/mfd/intel-lpss-pci.c to set this, this fixes the WARN.
>>
> 
>> Before this commit the setting of the adapter.nr was somewhat convoluted,
>> in the acpi_companion case it was set from dw_i2c_acpi_configure, in the
>> non acpi_companion case it was set from dw_i2c_set_fifo_size() based on
>> tx_fifo_depth not being set yet. This commit also cleans this up.
> 
> Can we split this to two patches, i.e. one is almost the same as this one,
> except the second one adds a new property check to the conditional?

Ok, I've split the patch for v2 of the series.

> If you agree to do so, you may add mine Rb tag to the first one out of three.
> 
>> Note the "linux,use-dynamic-adapter-nr" is meant for kernel internal use
>> only, therefor it is NOT documented under Documents/devicetree/bindings.
> 
> To the second and third ones, can we rather check if the device has fwnode
> either ACPI or swnode? AFAIU now we have swnode assigned during MFD device
> registration and can easily distinguish this w/o any additional properties.

Detecting a swnode is non trivial, it may be either the primary or secondary
fwnode depending if the device already had a node before calling
device_add_properties.

More importantly deciding this based on the presence of a swnode feels like
its "wrong".

But I've come up with a better solution. I've analyzed all ways a designware_i2c
compatible platform-device can be instantiated and all cases except for:

drivers/mfd/intel_quark_i2c_gpio.c
drivers/mfd/intel-lpss-pci.c

Are always using dynamic adapter numbers already. The Quark X1000 has only
1 i2c controller, so there using dynamic adapter numbers will not make a
difference.

And the intel-lpss-pci.c case is the one which we actually want to fix,
devices using this code have many i2c busses so using a fixed bus number
leads to the bus-number conflicts this patch is trying to fix.

So we can simply always use dynamic adapter numbers, see the commit message
of the second patch in v2 of this series for the full analysis.

Regards,

Hans

Re: [PATCH 1/2] i2c: i2c-designware-platdrv: Allow a dynamic adap. nr without an ACPI fwnode

[Jarkko Nikula]

Hi

On 3/11/19 1:22 PM, 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 otherwise
> it will use pdev->id as adapter-nr.
> 
> On some devices e.g. the Apollo Lake using Acer TravelMate Spin B118,
> some of the LPSS i2c-adapters are enumerated through PCI and do not have
> an ACPI fwnode. These devices are handled as mfd devices so they end up
> using the i2c-designware-platdrv driver.
> 
> This results in the i2c-adapter being registered with the mfd generated
> pdev->id as adapter-nr, which conflicts with existing adapters, triggering
> a WARN(id < 0, "couldn't get idr") in i2c-core-base.c and causing the
> adapter registration to fail.
> 
I went thinking would we get a regression if we switch the 
i2c-designware-platdrv to dynamic numbering unconditionally?

Only drivers/mfd/intel-lpss.c and drivers/mfd/intel_quark_i2c_gpio.c 
register platform device "i2c_designware" and otherwise in the driver 
itself for known ACPI IDs and device tree bindings.

Things should be fine for ACPI cases if slave devices are also described 
in ACPI tables. As far as I've understood with device tree matching 
adapter number is irrelevant in slave device registration?

Andy: could you tell by commit 918fe70cf475 ("mfd: intel_quark_i2c_gpio: 
support devices behind i2c bus") are those devices described in ACPI or 
in some i2c_board_infos with referring to fixed adapter number either in 
or out of kernel tree code?

Then drivers/platform/chrome/chromeos_laptop.c is the only code 
searching for adapter named as "Synopsys DesignWare I2C adapter" without 
assuming any fixed adapter numbering.

What's unclear to me can there be device tree cases where i2c-designware 
probing comes with pdev->id not starting from zero or in different 
order? I.e. would it make difference do we use pdev->id or dynamic 
adapter numbering?

-- 
Jarkko

Re: [PATCH 1/2] i2c: i2c-designware-platdrv: Allow a dynamic adap. nr without an ACPI fwnode

[Hans de Goede]

Hi,

On 12-03-19 15:47, Jarkko Nikula wrote:
> Hi
> 
> On 3/11/19 1:22 PM, 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 otherwise
>> it will use pdev->id as adapter-nr.
>>
>> On some devices e.g. the Apollo Lake using Acer TravelMate Spin B118,
>> some of the LPSS i2c-adapters are enumerated through PCI and do not have
>> an ACPI fwnode. These devices are handled as mfd devices so they end up
>> using the i2c-designware-platdrv driver.
>>
>> This results in the i2c-adapter being registered with the mfd generated
>> pdev->id as adapter-nr, which conflicts with existing adapters, triggering
>> a WARN(id < 0, "couldn't get idr") in i2c-core-base.c and causing the
>> adapter registration to fail.
>>
> I went thinking would we get a regression if we switch the i2c-designware-platdrv to dynamic numbering unconditionally?
> 
> Only drivers/mfd/intel-lpss.c and drivers/mfd/intel_quark_i2c_gpio.c register platform device "i2c_designware" and otherwise in the driver itself for known ACPI IDs and device tree bindings.
> 
> Things should be fine for ACPI cases if slave devices are also described in ACPI tables. As far as I've understood with device tree matching adapter number is irrelevant in slave device registration?
> 
> Andy: could you tell by commit 918fe70cf475 ("mfd: intel_quark_i2c_gpio: support devices behind i2c bus") are those devices described in ACPI or in some i2c_board_infos with referring to fixed adapter number either in or out of kernel tree code?
> 
> Then drivers/platform/chrome/chromeos_laptop.c is the only code searching for adapter named as "Synopsys DesignWare I2C adapter" without assuming any fixed adapter numbering.
> 
> What's unclear to me can there be device tree cases where i2c-designware probing comes with pdev->id not starting from zero or in different order? I.e. would it make difference do we use pdev->id or dynamic adapter numbering?

I've just completed analyzing all ways a designware_i2c (or compatible)
platform device can be instantiated and I've come to the conclusion that
always using dynamic adapter-nrs is fine and is a proper, clean fix for this.

Also see my reply to Andy which I send about the same time you send
your reply :)

Regards,

Hans

Re: [PATCH 1/2] i2c: i2c-designware-platdrv: Allow a dynamic adap. nr without an ACPI fwnode

[Andy Shevchenko]

On Tue, Mar 12, 2019 at 04:47:48PM +0200, Jarkko Nikula wrote:
> On 3/11/19 1:22 PM, 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 otherwise
> > it will use pdev->id as adapter-nr.
> > 
> > On some devices e.g. the Apollo Lake using Acer TravelMate Spin B118,
> > some of the LPSS i2c-adapters are enumerated through PCI and do not have
> > an ACPI fwnode. These devices are handled as mfd devices so they end up
> > using the i2c-designware-platdrv driver.
> > 
> > This results in the i2c-adapter being registered with the mfd generated
> > pdev->id as adapter-nr, which conflicts with existing adapters, triggering
> > a WARN(id < 0, "couldn't get idr") in i2c-core-base.c and causing the
> > adapter registration to fail.
> > 
> I went thinking would we get a regression if we switch the
> i2c-designware-platdrv to dynamic numbering unconditionally?
> 
> Only drivers/mfd/intel-lpss.c and drivers/mfd/intel_quark_i2c_gpio.c
> register platform device "i2c_designware" and otherwise in the driver itself
> for known ACPI IDs and device tree bindings.
> 
> Things should be fine for ACPI cases if slave devices are also described in
> ACPI tables. As far as I've understood with device tree matching adapter
> number is irrelevant in slave device registration?

Seems like Hans came to the same conclusion.

> Andy: could you tell by commit 918fe70cf475 ("mfd: intel_quark_i2c_gpio:
> support devices behind i2c bus") are those devices described in ACPI or in
> some i2c_board_infos with referring to fixed adapter number either in or out
> of kernel tree code?

As far as I remember they are coming from ACPI, but you may easily check on
real hardware we have in our lab.

> Then drivers/platform/chrome/chromeos_laptop.c is the only code searching
> for adapter named as "Synopsys DesignWare I2C adapter" without assuming any
> fixed adapter numbering.

> What's unclear to me can there be device tree cases where i2c-designware
> probing comes with pdev->id not starting from zero or in different order?
> I.e. would it make difference do we use pdev->id or dynamic adapter
> numbering?

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/2] mfd: intel-lpss: Add "linux,use-dynamic-adapter-nr" device-prop to i2c-devices

[Hans de Goede]

The i2c-designware-platdrv assumes that if the pdev has an apci-companion
it should use a dynamic adapter-nr and otherwise it will use pdev->id as
adapter-nr.

On some devices e.g. the Apollo Lake using Acer TravelMate Spin B118,
some of the LPSS i2c-adapters are enumerated through PCI and do not have
an ACPI fwnode. These devices are handled as mfd devices so they end up
using the i2c-designware-platdrv driver.

This results in the i2c-adapter being registered with the mfd generated
pdev->id as adapter-nr, which conflicts with existing adapters, triggering
a WARN(id < 0, "couldn't get idr") in i2c-core-base.c and causing the
adapter registration to fail.

To fix this the i2c-designware-platdrv now also supports a
"linux,use-dynamic-adapter-nr" device-property, which tells it to use a
dynamic adapter nr even if there is no ACPI fwnode for the device.

This commit adds this device-property to the properties list for PCI
enumerated LPSS i2c-adapters.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1687065
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mfd/intel-lpss-pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c
index 0e5282fc1467..c7b84a0f090e 100644
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -69,6 +69,7 @@ static const struct intel_lpss_platform_info spt_info = {
 
 static struct property_entry spt_i2c_properties[] = {
 	PROPERTY_ENTRY_U32("i2c-sda-hold-time-ns", 230),
+	PROPERTY_ENTRY_BOOL("linux,use-dynamic-adapter-nr"),
 	{ },
 };
 
@@ -104,6 +105,7 @@ static struct property_entry bxt_i2c_properties[] = {
 	PROPERTY_ENTRY_U32("i2c-sda-hold-time-ns", 42),
 	PROPERTY_ENTRY_U32("i2c-sda-falling-time-ns", 171),
 	PROPERTY_ENTRY_U32("i2c-scl-falling-time-ns", 208),
+	PROPERTY_ENTRY_BOOL("linux,use-dynamic-adapter-nr"),
 	{ },
 };
 
@@ -116,6 +118,7 @@ static struct property_entry apl_i2c_properties[] = {
 	PROPERTY_ENTRY_U32("i2c-sda-hold-time-ns", 207),
 	PROPERTY_ENTRY_U32("i2c-sda-falling-time-ns", 171),
 	PROPERTY_ENTRY_U32("i2c-scl-falling-time-ns", 208),
+	PROPERTY_ENTRY_BOOL("linux,use-dynamic-adapter-nr"),
 	{ },
 };
 
-- 
2.20.1

Re: [PATCH 2/2] mfd: intel-lpss: Add "linux,use-dynamic-adapter-nr" device-prop to i2c-devices

[Wolfram Sang]

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

On Mon, Mar 11, 2019 at 12:22:16PM +0100, Hans de Goede wrote:
> The i2c-designware-platdrv assumes that if the pdev has an apci-companion
> it should use a dynamic adapter-nr and otherwise it will use pdev->id as
> adapter-nr.
> 
> On some devices e.g. the Apollo Lake using Acer TravelMate Spin B118,
> some of the LPSS i2c-adapters are enumerated through PCI and do not have
> an ACPI fwnode. These devices are handled as mfd devices so they end up
> using the i2c-designware-platdrv driver.
> 
> This results in the i2c-adapter being registered with the mfd generated
> pdev->id as adapter-nr, which conflicts with existing adapters, triggering
> a WARN(id < 0, "couldn't get idr") in i2c-core-base.c and causing the
> adapter registration to fail.
> 
> To fix this the i2c-designware-platdrv now also supports a
> "linux,use-dynamic-adapter-nr" device-property, which tells it to use a
> dynamic adapter nr even if there is no ACPI fwnode for the device.
> 
> This commit adds this device-property to the properties list for PCI
> enumerated LPSS i2c-adapters.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1687065
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

You need to CC the devicetree ML on such patches. However, I am afraid I
can pretty much NACK this one already. This is a Linux configuration
item and DT is only for describing HW.

Maybe the designware maintainers have a better idea how to achieve what
you need.


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