arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Uwe Kleine-König]

[adding lakml and rmk to Cc]

Hello Guenter,

On Sun, Feb 14, 2016 at 08:50:10AM -0800, Guenter Roeck wrote:
> Uwe,
> 
> Your patch 'driver-core: platform: probe of-devices only using list of
> compatibles' causes the following qemu tests to crash in -next.
> 
> arm:vexpress-a9:vexpress_defconfig:vexpress-v2p-ca9
> arm:vexpress-a15:vexpress_defconfig:vexpress-v2p-ca15-tc1
> arm:vexpress-a9:multi_v7_defconfig:vexpress-v2p-ca9
> arm:vexpress-a15:multi_v7_defconfig:vexpress-v2p-ca15-tc1
> 
> Crash log:
> 
> VFS: Cannot open root device "mmcblk0" or unknown-block(0,0): error -6
> Please append a correct "root=" boot option; here are the available partitions:
> 1f00          131072 mtdblock0  (driver?)
> 1f01           32768 mtdblock1  (driver?)
> Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
> 
> ie the mmc driver no longer instantiates. Reverting the patch fixes the problem.

The driver is drivers/mmc/host/mmci.c, right? and the relevant device
tree snippet is:

	mmci at 05000 {
		compatible = "arm,pl180", "arm,primecell";
		...
	};

? So the unexpected abnormality here is that even though this device is
instantiated by dt, the driver doesn't provide any compatibles.
Either my expectation is wrong, then 67d02a1bbb33455 should be reverted
(or handle this case in a different way), or the mmci driver should
declare compatibles (but then it needs to be a platform driver and not
an amba driver?).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Russell King - ARM Linux]

On Sun, Feb 14, 2016 at 08:55:01PM +0100, Uwe Kleine-K?nig wrote:
> So the unexpected abnormality here is that even though this device is
> instantiated by dt, the driver doesn't provide any compatibles.
> Either my expectation is wrong, then 67d02a1bbb33455 should be reverted

Your expectation is wrong.  AMBA primecell devices have hardware IDs
and are matched to their drivers by those IDs.  Just like PCI.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Uwe Kleine-König]

Hello Russell,

On Sun, Feb 14, 2016 at 08:07:55PM +0000, Russell King - ARM Linux wrote:
> On Sun, Feb 14, 2016 at 08:55:01PM +0100, Uwe Kleine-K?nig wrote:
> > So the unexpected abnormality here is that even though this device is
> > instantiated by dt, the driver doesn't provide any compatibles.
> > Either my expectation is wrong, then 67d02a1bbb33455 should be reverted
> 
> Your expectation is wrong.  AMBA primecell devices have hardware IDs
> and are matched to their drivers by those IDs.  Just like PCI.

pci devices don't appear in dt, do they? I don't see the connection
between amba devices and platform devices, see my other mail in this
thread for some more details.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Russell King - ARM Linux]

On Mon, Feb 15, 2016 at 09:17:50AM +0100, Uwe Kleine-K?nig wrote:
> Hello Russell,
> 
> On Sun, Feb 14, 2016 at 08:07:55PM +0000, Russell King - ARM Linux wrote:
> > On Sun, Feb 14, 2016 at 08:55:01PM +0100, Uwe Kleine-K?nig wrote:
> > > So the unexpected abnormality here is that even though this device is
> > > instantiated by dt, the driver doesn't provide any compatibles.
> > > Either my expectation is wrong, then 67d02a1bbb33455 should be reverted
> > 
> > Your expectation is wrong.  AMBA primecell devices have hardware IDs
> > and are matched to their drivers by those IDs.  Just like PCI.
> 
> pci devices don't appear in dt, do they? I don't see the connection
> between amba devices and platform devices, see my other mail in this
> thread for some more details.

They both have hardware IDs, and they are both matched via those hardware
IDs.

Your change has introduced a regression and is therefore wrong.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Uwe Kleine-König]

Hello Russell,

On Mon, Feb 15, 2016 at 08:58:18AM +0000, Russell King - ARM Linux wrote:
> On Mon, Feb 15, 2016 at 09:17:50AM +0100, Uwe Kleine-K?nig wrote:
> > On Sun, Feb 14, 2016 at 08:07:55PM +0000, Russell King - ARM Linux wrote:
> > > On Sun, Feb 14, 2016 at 08:55:01PM +0100, Uwe Kleine-K?nig wrote:
> > > > So the unexpected abnormality here is that even though this device is
> > > > instantiated by dt, the driver doesn't provide any compatibles.
> > > > Either my expectation is wrong, then 67d02a1bbb33455 should be reverted
> > > 
> > > Your expectation is wrong.  AMBA primecell devices have hardware IDs
> > > and are matched to their drivers by those IDs.  Just like PCI.
> > 
> > pci devices don't appear in dt, do they? I don't see the connection
> > between amba devices and platform devices, see my other mail in this
> > thread for some more details.
> 
> They both have hardware IDs, and they are both matched via those hardware
> IDs.

I changed platform_match which is about matching by dt compatible, acpi
and/or device name. I don't see how this can affect an amba device given
they match to a driver by a hardware id.

> Your change has introduced a regression and is therefore wrong.

I'd like to understand though why and how my commit is wrong to be able
to fix it instead of getting it reverted.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Russell King - ARM Linux]

On Mon, Feb 15, 2016 at 10:14:06AM +0100, Uwe Kleine-K?nig wrote:
> Hello Russell,
> 
> On Mon, Feb 15, 2016 at 08:58:18AM +0000, Russell King - ARM Linux wrote:
> > On Mon, Feb 15, 2016 at 09:17:50AM +0100, Uwe Kleine-K?nig wrote:
> > > On Sun, Feb 14, 2016 at 08:07:55PM +0000, Russell King - ARM Linux wrote:
> > > > On Sun, Feb 14, 2016 at 08:55:01PM +0100, Uwe Kleine-K?nig wrote:
> > > > > So the unexpected abnormality here is that even though this device is
> > > > > instantiated by dt, the driver doesn't provide any compatibles.
> > > > > Either my expectation is wrong, then 67d02a1bbb33455 should be reverted
> > > > 
> > > > Your expectation is wrong.  AMBA primecell devices have hardware IDs
> > > > and are matched to their drivers by those IDs.  Just like PCI.
> > > 
> > > pci devices don't appear in dt, do they? I don't see the connection
> > > between amba devices and platform devices, see my other mail in this
> > > thread for some more details.
> > 
> > They both have hardware IDs, and they are both matched via those hardware
> > IDs.
> 
> I changed platform_match which is about matching by dt compatible, acpi
> and/or device name. I don't see how this can affect an amba device given
> they match to a driver by a hardware id.
> 
> > Your change has introduced a regression and is therefore wrong.
> 
> I'd like to understand though why and how my commit is wrong to be able
> to fix it instead of getting it reverted.

I don't have the commit, and I haven't seen the patch so I can't
comment further, sorry.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Uwe Kleine-König]

Hello Russell,

On Mon, Feb 15, 2016 at 10:04:15AM +0000, Russell King - ARM Linux wrote:
> On Mon, Feb 15, 2016 at 10:14:06AM +0100, Uwe Kleine-K?nig wrote:
> > On Mon, Feb 15, 2016 at 08:58:18AM +0000, Russell King - ARM Linux wrote:
> > > On Mon, Feb 15, 2016 at 09:17:50AM +0100, Uwe Kleine-K?nig wrote:
> > > > On Sun, Feb 14, 2016 at 08:07:55PM +0000, Russell King - ARM Linux wrote:
> > > > > On Sun, Feb 14, 2016 at 08:55:01PM +0100, Uwe Kleine-K?nig wrote:
> > > > > > So the unexpected abnormality here is that even though this device is
> > > > > > instantiated by dt, the driver doesn't provide any compatibles.
> > > > > > Either my expectation is wrong, then 67d02a1bbb33455 should be reverted
> > > > > 
> > > > > Your expectation is wrong.  AMBA primecell devices have hardware IDs
> > > > > and are matched to their drivers by those IDs.  Just like PCI.
> > > > 
> > > > pci devices don't appear in dt, do they? I don't see the connection
> > > > between amba devices and platform devices, see my other mail in this
> > > > thread for some more details.
> > > 
> > > They both have hardware IDs, and they are both matched via those hardware
> > > IDs.
> > 
> > I changed platform_match which is about matching by dt compatible, acpi
> > and/or device name. I don't see how this can affect an amba device given
> > they match to a driver by a hardware id.
> > 
> > > Your change has introduced a regression and is therefore wrong.
> > 
> > I'd like to understand though why and how my commit is wrong to be able
> > to fix it instead of getting it reverted.
> 
> I don't have the commit, and I haven't seen the patch so I can't
> comment further, sorry.

It's in -next. For a quick look:

	https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=67d02a1bbb33455

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Russell King - ARM Linux]

On Mon, Feb 15, 2016 at 11:10:14AM +0100, Uwe Kleine-K?nig wrote:
> Hello Russell,
> 
> On Mon, Feb 15, 2016 at 10:04:15AM +0000, Russell King - ARM Linux wrote:
> > On Mon, Feb 15, 2016 at 10:14:06AM +0100, Uwe Kleine-K?nig wrote:
> > > On Mon, Feb 15, 2016 at 08:58:18AM +0000, Russell King - ARM Linux wrote:
> > > > On Mon, Feb 15, 2016 at 09:17:50AM +0100, Uwe Kleine-K?nig wrote:
> > > > > On Sun, Feb 14, 2016 at 08:07:55PM +0000, Russell King - ARM Linux wrote:
> > > > > > On Sun, Feb 14, 2016 at 08:55:01PM +0100, Uwe Kleine-K?nig wrote:
> > > > > > > So the unexpected abnormality here is that even though this device is
> > > > > > > instantiated by dt, the driver doesn't provide any compatibles.
> > > > > > > Either my expectation is wrong, then 67d02a1bbb33455 should be reverted
> > > > > > 
> > > > > > Your expectation is wrong.  AMBA primecell devices have hardware IDs
> > > > > > and are matched to their drivers by those IDs.  Just like PCI.
> > > > > 
> > > > > pci devices don't appear in dt, do they? I don't see the connection
> > > > > between amba devices and platform devices, see my other mail in this
> > > > > thread for some more details.
> > > > 
> > > > They both have hardware IDs, and they are both matched via those hardware
> > > > IDs.
> > > 
> > > I changed platform_match which is about matching by dt compatible, acpi
> > > and/or device name. I don't see how this can affect an amba device given
> > > they match to a driver by a hardware id.
> > > 
> > > > Your change has introduced a regression and is therefore wrong.
> > > 
> > > I'd like to understand though why and how my commit is wrong to be able
> > > to fix it instead of getting it reverted.
> > 
> > I don't have the commit, and I haven't seen the patch so I can't
> > comment further, sorry.
> 
> It's in -next. For a quick look:
> 
> 	https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=67d02a1bbb33455

Well, if that's only touching the platform device matching, it can't have
any effect on AMBA bus matching, which uses completely different code.
The AMBA bus code is entirely separate from platform devices.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Guenter Roeck]

On 02/14/2016 11:55 AM, Uwe Kleine-K?nig wrote:
> [adding lakml and rmk to Cc]
>
> Hello Guenter,
>
> On Sun, Feb 14, 2016 at 08:50:10AM -0800, Guenter Roeck wrote:
>> Uwe,
>>
>> Your patch 'driver-core: platform: probe of-devices only using list of
>> compatibles' causes the following qemu tests to crash in -next.
>>
>> arm:vexpress-a9:vexpress_defconfig:vexpress-v2p-ca9
>> arm:vexpress-a15:vexpress_defconfig:vexpress-v2p-ca15-tc1
>> arm:vexpress-a9:multi_v7_defconfig:vexpress-v2p-ca9
>> arm:vexpress-a15:multi_v7_defconfig:vexpress-v2p-ca15-tc1
>>
>> Crash log:
>>
>> VFS: Cannot open root device "mmcblk0" or unknown-block(0,0): error -6
>> Please append a correct "root=" boot option; here are the available partitions:
>> 1f00          131072 mtdblock0  (driver?)
>> 1f01           32768 mtdblock1  (driver?)
>> Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
>>
>> ie the mmc driver no longer instantiates. Reverting the patch fixes the problem.
>
> The driver is drivers/mmc/host/mmci.c, right? and the relevant device
> tree snippet is:
>
> 	mmci at 05000 {
> 		compatible = "arm,pl180", "arm,primecell";
> 		...
> 	};
>

Yes, I think so, or one of the many other similar mmc entries.

> ? So the unexpected abnormality here is that even though this device is
> instantiated by dt, the driver doesn't provide any compatibles.
> Either my expectation is wrong, then 67d02a1bbb33455 should be reverted
> (or handle this case in a different way), or the mmci driver should
> declare compatibles (but then it needs to be a platform driver and not
> an amba driver?).
>

No idea what the correct solution would be. I do see

         if (of_device_is_compatible(bus, "arm,primecell")) {
                 /*
                  * Don't return an error here to keep compatibility with older
                  * device tree files.
                  */
                 of_amba_device_create(bus, bus_id, platform_data, parent);
                 return 0;
         }

in drivers/of/platform.c, which suggests some special handling for amba
devices. No idea if and how that is related, but I do have some concern
that fixing the problem for mmc alone might not fix it for all the other
devices instantiated with "arm,primecell". After all, my boot tests are
really rudimentary (it boots, therefore it works).

Thanks,
Guenter

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Uwe Kleine-König]

Hello Guenter,

On Sun, Feb 14, 2016 at 01:08:42PM -0800, Guenter Roeck wrote:
> On 02/14/2016 11:55 AM, Uwe Kleine-K?nig wrote:
> >[adding lakml and rmk to Cc]
[adding some more people to Cc]

> >On Sun, Feb 14, 2016 at 08:50:10AM -0800, Guenter Roeck wrote:
> >>Your patch 'driver-core: platform: probe of-devices only using list of
> >>compatibles' causes the following qemu tests to crash in -next.

For the new readers, that is 67d02a1bbb334558e9380409a3cd426b36d4578b.

The original idea of this commit was to not bind a device created from
device tree when its name matches the driver name but none of the
driver's compatibles which might yield some surprises.

> >>arm:vexpress-a9:vexpress_defconfig:vexpress-v2p-ca9
> >>arm:vexpress-a15:vexpress_defconfig:vexpress-v2p-ca15-tc1
> >>arm:vexpress-a9:multi_v7_defconfig:vexpress-v2p-ca9
> >>arm:vexpress-a15:multi_v7_defconfig:vexpress-v2p-ca15-tc1
> >>
> >>Crash log:
> >>
> >>VFS: Cannot open root device "mmcblk0" or unknown-block(0,0): error -6
> >>Please append a correct "root=" boot option; here are the available partitions:
> >>1f00          131072 mtdblock0  (driver?)
> >>1f01           32768 mtdblock1  (driver?)
> >>Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
> >>
> >>ie the mmc driver no longer instantiates. Reverting the patch fixes the problem.
> >
> >The driver is drivers/mmc/host/mmci.c, right? and the relevant device
> >tree snippet is:
> >
> >	mmci at 05000 {
> >		compatible = "arm,pl180", "arm,primecell";
> >		...
> >	};
> >
> 
> Yes, I think so, or one of the many other similar mmc entries.

So the driver in question is an amba_driver and it fails to bind because

	static int platform_match(struct device *dev, struct device_driver *drv)
	
was changed. This is the platform bus type's match function. Why is this
called for amba devices (that I would expect to use amba_bustype and so
amba_match)?

The driver isn't matched by of_driver_match_device, so the
following code must yield 1 for the mmci device:

        /* Then try ACPI style match */
        if (acpi_driver_match_device(dev, drv))
                return 1;

        /* Then try to match against the id table */
        if (pdrv->id_table)
                return platform_match_id(pdrv->id_table, pdev) != NULL;

        /* fall-back to driver name match */
        return (strcmp(pdev->name, drv->name) == 0);

acpi seems unlikely, and the other two match by the device's name which
feels wrong. And I also wonder, what drv is here, because platform_match
assumes it is a platform_driver, not an amba_driver.

> >? So the unexpected abnormality here is that even though this device is
> >instantiated by dt, the driver doesn't provide any compatibles.
> >Either my expectation is wrong, then 67d02a1bbb33455 should be reverted
> >(or handle this case in a different way), or the mmci driver should
> >declare compatibles (but then it needs to be a platform driver and not
> >an amba driver?).
> 
> No idea what the correct solution would be. I do see
> 
>         if (of_device_is_compatible(bus, "arm,primecell")) {
>                 /*
>                  * Don't return an error here to keep compatibility with older
>                  * device tree files.
>                  */
>                 of_amba_device_create(bus, bus_id, platform_data, parent);
>                 return 0;
>         }

So there is a new (and better?) way to instantiate amba devices?

> in drivers/of/platform.c, which suggests some special handling for amba
> devices. No idea if and how that is related, but I do have some concern
> that fixing the problem for mmc alone might not fix it for all the other
> devices instantiated with "arm,primecell". After all, my boot tests are
> really rudimentary (it boots, therefore it works).

I don't see the right thing to do either. Maybe someone else can shed
some light on this issue?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Uwe Kleine-König]

Hello Guenter,

On Sun, Feb 14, 2016 at 08:50:10AM -0800, Guenter Roeck wrote:
> Uwe,
> 
> Your patch 'driver-core: platform: probe of-devices only using list of
> compatibles' causes the following qemu tests to crash in -next.
> 
> arm:vexpress-a9:vexpress_defconfig:vexpress-v2p-ca9
> arm:vexpress-a15:vexpress_defconfig:vexpress-v2p-ca15-tc1
> arm:vexpress-a9:multi_v7_defconfig:vexpress-v2p-ca9
> arm:vexpress-a15:multi_v7_defconfig:vexpress-v2p-ca15-tc1
> 
> Crash log:
> 
> VFS: Cannot open root device "mmcblk0" or unknown-block(0,0): error -6
> Please append a correct "root=" boot option; here are the available partitions:
> 1f00          131072 mtdblock0  (driver?)
> 1f01           32768 mtdblock1  (driver?)
> Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)

Can you provide a complete boot log? This might already reveal which
device is failing. It might not be the mmci device but something it
depends on (clock, bus parent, irq).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Robin Murphy]

On 15/02/16 10:59, Uwe Kleine-K?nig wrote:
> Hello Guenter,
>
> On Sun, Feb 14, 2016 at 08:50:10AM -0800, Guenter Roeck wrote:
>> Uwe,
>>
>> Your patch 'driver-core: platform: probe of-devices only using list of
>> compatibles' causes the following qemu tests to crash in -next.
>>
>> arm:vexpress-a9:vexpress_defconfig:vexpress-v2p-ca9
>> arm:vexpress-a15:vexpress_defconfig:vexpress-v2p-ca15-tc1
>> arm:vexpress-a9:multi_v7_defconfig:vexpress-v2p-ca9
>> arm:vexpress-a15:multi_v7_defconfig:vexpress-v2p-ca15-tc1
>>
>> Crash log:
>>
>> VFS: Cannot open root device "mmcblk0" or unknown-block(0,0): error -6
>> Please append a correct "root=" boot option; here are the available partitions:
>> 1f00          131072 mtdblock0  (driver?)
>> 1f01           32768 mtdblock1  (driver?)
>> Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
>
> Can you provide a complete boot log? This might already reveal which
> device is failing. It might not be the mmci device but something it
> depends on (clock, bus parent, irq).

FWIW the PL180 on my Juno still works fine with this patch picked on top 
of -rc3, so the issue would seem to be something else - From a quick 
comparison between the DTs I see a slight difference in compatible 
strings for the clocks, but the more likely-looking suspect is that the 
VExpress DT references some GPIOs where the Juno DT doesn't.

Robin.

>
> Best regards
> Uwe
>

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Russell King - ARM Linux]

On Mon, Feb 15, 2016 at 01:11:49PM +0000, Robin Murphy wrote:
> FWIW the PL180 on my Juno still works fine with this patch picked on top of
> -rc3, so the issue would seem to be something else - From a quick comparison
> between the DTs I see a slight difference in compatible strings for the
> clocks, but the more likely-looking suspect is that the VExpress DT
> references some GPIOs where the Juno DT doesn't.

Maybe it would be a good idea that Uwe creates a patch which initially
warns when a DT platform device falls back to matching via the platform
strings?

It's likely that the "basic subsystem" platform drivers are silent when
they probe, so having notification of a fallback would at least put
something into the kernel log when that happens - and then later change
that to be a hard failure (as Uwe is trying to do with his patch.)

However, I have to bring up another point: is what Uwe is trying to do
actually the right thing?  The DT platform device code has the ability
to create standard platform devices from DT, with an of_node, but with
standard names, and platform data.  It's there for compatibility with
older systems, and is there to allow systems to be transitioned over.

This patch breaks all that: despite the DT code changing the platform
device bus_id from the address.nodename format to the standard format
(thus allowing unconverted platform drivers to match), this patch
means that because the platform device has a of_node attached, this
will now fail.

Therefore, I think Uwe's patch is just wrong - or, if it's something we
want, the auxdata table support code needs to _also_ be ripped out of
the drivers/of/platform.c code, but that then means anyone who wants to
go through the conversion has a big flag-day change to go through.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Uwe Kleine-König]

Hello Russell,

On Mon, Feb 15, 2016 at 02:43:44PM +0000, Russell King - ARM Linux wrote:
> On Mon, Feb 15, 2016 at 01:11:49PM +0000, Robin Murphy wrote:
> > FWIW the PL180 on my Juno still works fine with this patch picked on top of
> > -rc3, so the issue would seem to be something else - From a quick comparison
> > between the DTs I see a slight difference in compatible strings for the
> > clocks, but the more likely-looking suspect is that the VExpress DT
> > references some GPIOs where the Juno DT doesn't.
> 
> Maybe it would be a good idea that Uwe creates a patch which initially
> warns when a DT platform device falls back to matching via the platform
> strings?
> 
> It's likely that the "basic subsystem" platform drivers are silent when
> they probe, so having notification of a fallback would at least put
> something into the kernel log when that happens - and then later change
> that to be a hard failure (as Uwe is trying to do with his patch.)
> 
> However, I have to bring up another point: is what Uwe is trying to do
> actually the right thing?  The DT platform device code has the ability
> to create standard platform devices from DT, with an of_node, but with
> standard names, and platform data.  It's there for compatibility with
> older systems, and is there to allow systems to be transitioned over.
> 
> This patch breaks all that: despite the DT code changing the platform
> device bus_id from the address.nodename format to the standard format
> (thus allowing unconverted platform drivers to match), this patch
> means that because the platform device has a of_node attached, this
> will now fail.
> 
> Therefore, I think Uwe's patch is just wrong - or, if it's something we
> want, the auxdata table support code needs to _also_ be ripped out of
> the drivers/of/platform.c code, but that then means anyone who wants to
> go through the conversion has a big flag-day change to go through.

That's a valid concern I wasn't aware of when I created the patch.

So maybe just emitting a warning as you suggested is a good idea. And
additionally only emit it when the driver is dt aware, too.

Greg, can you drop this patch, or do you need a proper changelog for a
revert? On top of that I'd then create a new patch which is more
conservative.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Greg Kroah-Hartman]

On Mon, Feb 15, 2016 at 05:27:53PM +0100, Uwe Kleine-K?nig wrote:
> Hello Russell,
> 
> On Mon, Feb 15, 2016 at 02:43:44PM +0000, Russell King - ARM Linux wrote:
> > On Mon, Feb 15, 2016 at 01:11:49PM +0000, Robin Murphy wrote:
> > > FWIW the PL180 on my Juno still works fine with this patch picked on top of
> > > -rc3, so the issue would seem to be something else - From a quick comparison
> > > between the DTs I see a slight difference in compatible strings for the
> > > clocks, but the more likely-looking suspect is that the VExpress DT
> > > references some GPIOs where the Juno DT doesn't.
> > 
> > Maybe it would be a good idea that Uwe creates a patch which initially
> > warns when a DT platform device falls back to matching via the platform
> > strings?
> > 
> > It's likely that the "basic subsystem" platform drivers are silent when
> > they probe, so having notification of a fallback would at least put
> > something into the kernel log when that happens - and then later change
> > that to be a hard failure (as Uwe is trying to do with his patch.)
> > 
> > However, I have to bring up another point: is what Uwe is trying to do
> > actually the right thing?  The DT platform device code has the ability
> > to create standard platform devices from DT, with an of_node, but with
> > standard names, and platform data.  It's there for compatibility with
> > older systems, and is there to allow systems to be transitioned over.
> > 
> > This patch breaks all that: despite the DT code changing the platform
> > device bus_id from the address.nodename format to the standard format
> > (thus allowing unconverted platform drivers to match), this patch
> > means that because the platform device has a of_node attached, this
> > will now fail.
> > 
> > Therefore, I think Uwe's patch is just wrong - or, if it's something we
> > want, the auxdata table support code needs to _also_ be ripped out of
> > the drivers/of/platform.c code, but that then means anyone who wants to
> > go through the conversion has a big flag-day change to go through.
> 
> That's a valid concern I wasn't aware of when I created the patch.
> 
> So maybe just emitting a warning as you suggested is a good idea. And
> additionally only emit it when the driver is dt aware, too.
> 
> Greg, can you drop this patch, or do you need a proper changelog for a
> revert? On top of that I'd then create a new patch which is more
> conservative.

A hint as to what the git commit id was would be helpful, I can just
revert it based on that.

thanks,

greg k-h

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Uwe Kleine-König]

Hello Greg,

On Mon, Feb 15, 2016 at 08:49:37AM -0800, Greg Kroah-Hartman wrote:
> On Mon, Feb 15, 2016 at 05:27:53PM +0100, Uwe Kleine-K?nig wrote:
> > Greg, can you drop this patch, or do you need a proper changelog for a
> > revert? On top of that I'd then create a new patch which is more
> > conservative.
> 
> A hint as to what the git commit id was would be helpful, I can just
> revert it based on that.

This is 67d02a1bbb33 ("driver-core: platform: probe of-devices only
using list of compatibles")

If you need a log, something like:

	Reallow binding of of-devices by name

	It turned out that there are valid reasons (e.g. step by step
	conversion to device tree probing using auxdata) to bind
	of-instantiated devices to drivers by name. So revert to the
	original logic.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Greg Kroah-Hartman]

On Mon, Feb 15, 2016 at 06:12:01PM +0100, Uwe Kleine-K?nig wrote:
> Hello Greg,
> 
> On Mon, Feb 15, 2016 at 08:49:37AM -0800, Greg Kroah-Hartman wrote:
> > On Mon, Feb 15, 2016 at 05:27:53PM +0100, Uwe Kleine-K?nig wrote:
> > > Greg, can you drop this patch, or do you need a proper changelog for a
> > > revert? On top of that I'd then create a new patch which is more
> > > conservative.
> > 
> > A hint as to what the git commit id was would be helpful, I can just
> > revert it based on that.
> 
> This is 67d02a1bbb33 ("driver-core: platform: probe of-devices only
> using list of compatibles")
> 
> If you need a log, something like:
> 
> 	Reallow binding of of-devices by name
> 
> 	It turned out that there are valid reasons (e.g. step by step
> 	conversion to device tree probing using auxdata) to bind
> 	of-instantiated devices to drivers by name. So revert to the
> 	original logic.

Now reverted, thanks for the text.

greg k-h

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Guenter Roeck]

On 02/15/2016 02:59 AM, Uwe Kleine-K?nig wrote:
> Hello Guenter,
>
> On Sun, Feb 14, 2016 at 08:50:10AM -0800, Guenter Roeck wrote:
>> Uwe,
>>
>> Your patch 'driver-core: platform: probe of-devices only using list of
>> compatibles' causes the following qemu tests to crash in -next.
>>
>> arm:vexpress-a9:vexpress_defconfig:vexpress-v2p-ca9
>> arm:vexpress-a15:vexpress_defconfig:vexpress-v2p-ca15-tc1
>> arm:vexpress-a9:multi_v7_defconfig:vexpress-v2p-ca9
>> arm:vexpress-a15:multi_v7_defconfig:vexpress-v2p-ca15-tc1
>>
>> Crash log:
>>
>> VFS: Cannot open root device "mmcblk0" or unknown-block(0,0): error -6
>> Please append a correct "root=" boot option; here are the available partitions:
>> 1f00          131072 mtdblock0  (driver?)
>> 1f01           32768 mtdblock1  (driver?)
>> Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
>
> Can you provide a complete boot log? This might already reveal which
> device is failing. It might not be the mmci device but something it
> depends on (clock, bus parent, irq).
>

Sure, something else may be failing, but why does reverting your patch
fix the problem ?

Anyway, complete logs are at http://kerneltests.org/builders.

http://kerneltests.org/builders/qemu-arm-next/builds/376/steps/qemubuildcommand/logs/stdio

is the most recent log (next-20120215). Look for the vexpress crashes; the overo
crash bisected to to 'PM / OPP: Disable OPPs that aren't supported by the regulator',
in next-20160212, which I have not fully analyzed yet, and the beagle crashes
as well as the 'new' overo crash are brand new.

Guenter

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Russell King - ARM Linux]

On Mon, Feb 15, 2016 at 07:41:19AM -0800, Guenter Roeck wrote:
> On 02/15/2016 02:59 AM, Uwe Kleine-K?nig wrote:
> >Hello Guenter,
> >
> >On Sun, Feb 14, 2016 at 08:50:10AM -0800, Guenter Roeck wrote:
> >>Uwe,
> >>
> >>Your patch 'driver-core: platform: probe of-devices only using list of
> >>compatibles' causes the following qemu tests to crash in -next.
> >>
> >>arm:vexpress-a9:vexpress_defconfig:vexpress-v2p-ca9
> >>arm:vexpress-a15:vexpress_defconfig:vexpress-v2p-ca15-tc1
> >>arm:vexpress-a9:multi_v7_defconfig:vexpress-v2p-ca9
> >>arm:vexpress-a15:multi_v7_defconfig:vexpress-v2p-ca15-tc1
> >>
> >>Crash log:
> >>
> >>VFS: Cannot open root device "mmcblk0" or unknown-block(0,0): error -6
> >>Please append a correct "root=" boot option; here are the available partitions:
> >>1f00          131072 mtdblock0  (driver?)
> >>1f01           32768 mtdblock1  (driver?)
> >>Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
> >
> >Can you provide a complete boot log? This might already reveal which
> >device is failing. It might not be the mmci device but something it
> >depends on (clock, bus parent, irq).
> >
> 
> Sure, something else may be failing, but why does reverting your patch
> fix the problem ?
> 
> Anyway, complete logs are at http://kerneltests.org/builders.
> 
> http://kerneltests.org/builders/qemu-arm-next/builds/376/steps/qemubuildcommand/logs/stdio
> 
> is the most recent log (next-20120215). Look for the vexpress crashes; the overo
> crash bisected to to 'PM / OPP: Disable OPPs that aren't supported by the regulator',
> in next-20160212, which I have not fully analyzed yet, and the beagle crashes
> as well as the 'new' overo crash are brand new.

Looking at the vexpress-ca9 one, nothing stands out to me apart from the
lack of messages about a MMC driver.  I don't see anything there which
indicates why that would be.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Uwe Kleine-König]

On Mon, Feb 15, 2016 at 07:41:19AM -0800, Guenter Roeck wrote:
> On 02/15/2016 02:59 AM, Uwe Kleine-K?nig wrote:
> >Hello Guenter,
> >
> >On Sun, Feb 14, 2016 at 08:50:10AM -0800, Guenter Roeck wrote:
> >>Uwe,
> >>
> >>Your patch 'driver-core: platform: probe of-devices only using list of
> >>compatibles' causes the following qemu tests to crash in -next.
> >>
> >>arm:vexpress-a9:vexpress_defconfig:vexpress-v2p-ca9
> >>arm:vexpress-a15:vexpress_defconfig:vexpress-v2p-ca15-tc1
> >>arm:vexpress-a9:multi_v7_defconfig:vexpress-v2p-ca9
> >>arm:vexpress-a15:multi_v7_defconfig:vexpress-v2p-ca15-tc1
> >>
> >>Crash log:
> >>
> >>VFS: Cannot open root device "mmcblk0" or unknown-block(0,0): error -6
> >>Please append a correct "root=" boot option; here are the available partitions:
> >>1f00          131072 mtdblock0  (driver?)
> >>1f01           32768 mtdblock1  (driver?)
> >>Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
> >
> >Can you provide a complete boot log? This might already reveal which
> >device is failing. It might not be the mmci device but something it
> >depends on (clock, bus parent, irq).
> >
> 
> Sure, something else may be failing, but why does reverting your patch
> fix the problem ?

Well, my patch made matching of platform devices to platform drivers
more strict. Your machine relies on the respective binding though. So of
course reverting my patch "repairs" your machine, but that doesn't
necessarily mean that my patch is wrong. Even though I'm convinced in
the meantime by Russell that there are false positives it doesn't
necessarily imply that your case is such a false positive, too.

One example of a combination of driver + device I intended to break with
my patch was drivers/mtd/nand/mxc_nand.c and devices that got bound to
that by name. The driver does:

	const struct of_device_id *of_id =
		of_match_device(mxcnd_dt_ids, host->dev);

and doesn't handle of_id being NULL after that. Some people argued (also
for other drivers in similar situations) that this cannot happen because
all compatibles had a non-NULL device_id. That is an error that is easy
to make and so the idea was to just not bind in such a case and safe the
user from the surprise.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Guenter Roeck]

On 02/15/2016 09:00 AM, Uwe Kleine-K?nig wrote:
> On Mon, Feb 15, 2016 at 07:41:19AM -0800, Guenter Roeck wrote:
>> On 02/15/2016 02:59 AM, Uwe Kleine-K?nig wrote:
>>> Hello Guenter,
>>>
>>> On Sun, Feb 14, 2016 at 08:50:10AM -0800, Guenter Roeck wrote:
>>>> Uwe,
>>>>
>>>> Your patch 'driver-core: platform: probe of-devices only using list of
>>>> compatibles' causes the following qemu tests to crash in -next.
>>>>
>>>> arm:vexpress-a9:vexpress_defconfig:vexpress-v2p-ca9
>>>> arm:vexpress-a15:vexpress_defconfig:vexpress-v2p-ca15-tc1
>>>> arm:vexpress-a9:multi_v7_defconfig:vexpress-v2p-ca9
>>>> arm:vexpress-a15:multi_v7_defconfig:vexpress-v2p-ca15-tc1
>>>>
>>>> Crash log:
>>>>
>>>> VFS: Cannot open root device "mmcblk0" or unknown-block(0,0): error -6
>>>> Please append a correct "root=" boot option; here are the available partitions:
>>>> 1f00          131072 mtdblock0  (driver?)
>>>> 1f01           32768 mtdblock1  (driver?)
>>>> Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
>>>
>>> Can you provide a complete boot log? This might already reveal which
>>> device is failing. It might not be the mmci device but something it
>>> depends on (clock, bus parent, irq).
>>>
>>
>> Sure, something else may be failing, but why does reverting your patch
>> fix the problem ?
>
> Well, my patch made matching of platform devices to platform drivers
> more strict. Your machine relies on the respective binding though. So of
> course reverting my patch "repairs" your machine, but that doesn't
> necessarily mean that my patch is wrong. Even though I'm convinced in
> the meantime by Russell that there are false positives it doesn't
> necessarily imply that your case is such a false positive, too.
>
> One example of a combination of driver + device I intended to break with
> my patch was drivers/mtd/nand/mxc_nand.c and devices that got bound to
> that by name. The driver does:
>
> 	const struct of_device_id *of_id =
> 		of_match_device(mxcnd_dt_ids, host->dev);
>
> and doesn't handle of_id being NULL after that. Some people argued (also
> for other drivers in similar situations) that this cannot happen because
> all compatibles had a non-NULL device_id. That is an error that is easy
> to make and so the idea was to just not bind in such a case and safe the
> user from the surprise.
>

I added some debugging on top of your patch, and get:

platform basic-mmio-gpio.1.auto: Device node exists [/smb/motherboard/iofpga at 7,00000000/sysreg at 00000/sys_led at 08], of_driver_match_device() failed
platform basic-mmio-gpio.1.auto: platform_match_id() succeeded
platform basic-mmio-gpio.2.auto: Device node exists [/smb/motherboard/iofpga at 7,00000000/sysreg at 00000/sys_mci at 48], of_driver_match_device() failed
platform basic-mmio-gpio.2.auto: platform_match_id() succeeded
platform basic-mmio-gpio.3.auto: Device node exists [/smb/motherboard/iofpga at 7,00000000/sysreg at 00000/sys_flash at 4c], of_driver_match_device() failed
platform basic-mmio-gpio.3.auto: platform_match_id() succeeded

So it isn't the mmc driver failing to instantiate directly,
but (I think) vexpress-sysreg.

Guenter

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Sudeep Holla]

On 15/02/16 18:12, Guenter Roeck wrote:

[...]

>
> I added some debugging on top of your patch, and get:
>
> platform basic-mmio-gpio.1.auto: Device node exists
> [/smb/motherboard/iofpga at 7,00000000/sysreg at 00000/sys_led at 08],
> of_driver_match_device() failed
> platform basic-mmio-gpio.1.auto: platform_match_id() succeeded
> platform basic-mmio-gpio.2.auto: Device node exists
> [/smb/motherboard/iofpga at 7,00000000/sysreg at 00000/sys_mci at 48],
> of_driver_match_device() failed
> platform basic-mmio-gpio.2.auto: platform_match_id() succeeded
> platform basic-mmio-gpio.3.auto: Device node exists
> [/smb/motherboard/iofpga at 7,00000000/sysreg at 00000/sys_flash at 4c],
> of_driver_match_device() failed
> platform basic-mmio-gpio.3.auto: platform_match_id() succeeded
>
> So it isn't the mmc driver failing to instantiate directly,
> but (I think) vexpress-sysreg.
>

That's correct, I could reproduce this issue and reported with similar
analysis earlier [1]

--
Regards,
Sudeep

[1] http://www.spinics.net/lists/arm-kernel/msg483107.html

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Sudeep Holla]

On 15/02/16 15:41, Guenter Roeck wrote:
> On 02/15/2016 02:59 AM, Uwe Kleine-K?nig wrote:
>> Hello Guenter,
>>
>> On Sun, Feb 14, 2016 at 08:50:10AM -0800, Guenter Roeck wrote:
>>> Uwe,
>>>
>>> Your patch 'driver-core: platform: probe of-devices only using list of
>>> compatibles' causes the following qemu tests to crash in -next.
>>>
>>> arm:vexpress-a9:vexpress_defconfig:vexpress-v2p-ca9
>>> arm:vexpress-a15:vexpress_defconfig:vexpress-v2p-ca15-tc1
>>> arm:vexpress-a9:multi_v7_defconfig:vexpress-v2p-ca9
>>> arm:vexpress-a15:multi_v7_defconfig:vexpress-v2p-ca15-tc1
>>>
>>> Crash log:
>>>
>>> VFS: Cannot open root device "mmcblk0" or unknown-block(0,0): error -6
>>> Please append a correct "root=" boot option; here are the available
>>> partitions:
>>> 1f00          131072 mtdblock0  (driver?)
>>> 1f01           32768 mtdblock1  (driver?)
>>> Kernel panic - not syncing: VFS: Unable to mount root fs on
>>> unknown-block(0,0)
>>
>> Can you provide a complete boot log? This might already reveal which
>> device is failing. It might not be the mmci device but something it
>> depends on (clock, bus parent, irq).
>>
>
> Sure, something else may be failing, but why does reverting your patch
> fix the problem ?
>
> Anyway, complete logs are at http://kerneltests.org/builders.
>
> http://kerneltests.org/builders/qemu-arm-next/builds/376/steps/qemubuildcommand/logs/stdio
>
>

Sorry for missing this earlier, I could reproduce this on my TC2.
The issue is with card-detect gpio probing. It's not related to AMBA
probing as discussed on the mail thread.

mfd_add_device adds devices with of_node when cell->of_compatible is
matched, but the device created is expected to be matched based on name
which the patch under discussion clearly breaks.

One other option I see is to set driver_override for mfd devices
(something like below) but I am not sure that can be generic.

-- 
Regards,
Sudeep

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Russell King - ARM Linux]

On Mon, Feb 15, 2016 at 05:41:42PM +0000, Sudeep Holla wrote:
> Sorry for missing this earlier, I could reproduce this on my TC2.
> The issue is with card-detect gpio probing. It's not related to AMBA
> probing as discussed on the mail thread.
> 
> mfd_add_device adds devices with of_node when cell->of_compatible is
> matched, but the device created is expected to be matched based on name
> which the patch under discussion clearly breaks.

If I'm understanding you correctly, you're saying that MFD re-adds
platform devices with the of_node of a new platform device pointing
to an existing of_node, but expects this new platform device to
match a _different_ driver?

Sounds like MFD needs fixing.  I've said this before: of_node's must
_never_ be copied between different device structures, especially
when they are on the _same_ bus - quite simply because the driver
core _can_ match using the DT compatible.

For example... let's say you have a platform device called "1234.foo"
created by DT with compatible of "example,foo".  You have two platform
drivers.  One of them matches compatible "example,foo" and the other
matches against platform devices with a name of "bar".

The "example,foo" device driver is matched against "1234.foo".  It
creates a platform device with a name of "bar" and bus ID "bar.0",
setting the of_node to the same as "1234.foo".

When scanning for a matching driver, if the "example,foo" driver is
found first, "bar.0" will be matched to this driver, and its probe
method called.  If it accepts this device, it will repeat its action,
creating "bar.1".  Repeat until you run out of memory.

If it instead finds the "bar" driver first, this driver will be used
and everything appears to work correctly.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

arm qemu test failures due to 'driver-core: platform: probe of-devices only using list of compatibles'

[Sudeep Holla]

On 15/02/16 18:03, Russell King - ARM Linux wrote:
> On Mon, Feb 15, 2016 at 05:41:42PM +0000, Sudeep Holla wrote:
>> Sorry for missing this earlier, I could reproduce this on my TC2.
>> The issue is with card-detect gpio probing. It's not related to AMBA
>> probing as discussed on the mail thread.
>>
>> mfd_add_device adds devices with of_node when cell->of_compatible is
>> matched, but the device created is expected to be matched based on name
>> which the patch under discussion clearly breaks.
>
> If I'm understanding you correctly, you're saying that MFD re-adds
> platform devices with the of_node of a new platform device pointing
> to an existing of_node, but expects this new platform device to
> match a _different_ driver?
>

Sorry if I was not clear.

I don't think it re-adds. IIUC mfd cells are specified inside the
mfd device DT node. MFD adds devices for it's child nodes with the
associated device node but with the name specified by MFD cells
matching the compatible.

> Sounds like MFD needs fixing.  I've said this before: of_node's must
> _never_ be copied between different device structures, especially
> when they are on the _same_ bus - quite simply because the driver
> core _can_ match using the DT compatible.
>

I don't think that's happening in this case at-least. For example:

Device node compatible: arm,vexpress-sysreg
Child node compatible: arm,vexpress-sysreg,sys_mci

MFD device is created for above child node with name
"basic-mmio-gpio.<id>.auto" as it matched the MFD cell of_compatible

Since there's no driver to match "arm,vexpress-sysreg,sys_mci", it fails
with $subject patch applied which otherwise would do normal name matching

-- 
Regards,
Sudeep