[RFC PATCH v1] driver core: Set fw_devlink to "permissive" behavior by default

[RFC PATCH v1] driver core: Set fw_devlink to "permissive" behavior by default

[Saravana Kannan]

Set fw_devlink to "permissive" behavior by default so that device links
are automatically created (with DL_FLAG_SYNC_STATE_ONLY) by scanning the
firmware.

This ensures suppliers get their sync_state() calls only after all their
consumers have probed successfully. Without this, suppliers will get
their sync_state() calls at late_initcall_sync() even if their consuer

Ideally, we'd want to set fw_devlink to "on" or "rpm" by default. But
that needs more testing as it's known to break some corner case
drivers/platforms.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Saravana Kannan <saravanak@google.com>
---

I think it's time to soak test this and see if anything fails or if
anyone complains. Definitely not ready for 5.6. But pulling it in for
5.7 and having it go through all the rc testing would be helpful.

I'm sure there'll be reports where some DT properties are ambiguously
names and is breaking downstream or even some upstream platform. For
example, a DT property like "nr-gpios" would have a dmesg log about
parsing error because it looks like a valid "-gpios" DT binding. It'll
be good to catch those case and fix them.

Also, is there no way to look up current value of early_params? It'd be
nice if there was a way to do that.

-Saravana

 drivers/base/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5e3cc1651c78..9fabf9749a06 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2345,7 +2345,7 @@ static int device_private_init(struct device *dev)
 	return 0;
 }
 
-static u32 fw_devlink_flags;
+static u32 fw_devlink_flags = DL_FLAG_SYNC_STATE_ONLY;
 static int __init fw_devlink_setup(char *arg)
 {
 	if (!arg)
-- 
2.25.1.696.g5e7596f4ac-goog

Re: [RFC PATCH v1] driver core: Set fw_devlink to "permissive" behavior by default

[Marek Szyprowski]

Hi,

On 2020-03-21 22:03, Saravana Kannan wrote:
> Set fw_devlink to "permissive" behavior by default so that device links
> are automatically created (with DL_FLAG_SYNC_STATE_ONLY) by scanning the
> firmware.
>
> This ensures suppliers get their sync_state() calls only after all their
> consumers have probed successfully. Without this, suppliers will get
> their sync_state() calls at late_initcall_sync() even if their consuer
>
> Ideally, we'd want to set fw_devlink to "on" or "rpm" by default. But
> that needs more testing as it's known to break some corner case
> drivers/platforms.
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Saravana Kannan <saravanak@google.com>

This patch has just landed in linux-next 20200326. Sadly it breaks 
booting of the Raspberry Pi3b and Pi4 boards, either in 32bit or 64bit 
mode. There is no warning nor panic message, just a silent freeze. The 
last message shown on the earlycon is:

[    0.893217] Serial: 8250/16550 driver, 1 ports, IRQ sharing enabled

> ---
>
> I think it's time to soak test this and see if anything fails or if
> anyone complains. Definitely not ready for 5.6. But pulling it in for
> 5.7 and having it go through all the rc testing would be helpful.
>
> I'm sure there'll be reports where some DT properties are ambiguously
> names and is breaking downstream or even some upstream platform. For
> example, a DT property like "nr-gpios" would have a dmesg log about
> parsing error because it looks like a valid "-gpios" DT binding. It'll
> be good to catch those case and fix them.
>
> Also, is there no way to look up current value of early_params? It'd be
> nice if there was a way to do that.
>
> -Saravana
>
>   drivers/base/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 5e3cc1651c78..9fabf9749a06 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2345,7 +2345,7 @@ static int device_private_init(struct device *dev)
>   	return 0;
>   }
>   
> -static u32 fw_devlink_flags;
> +static u32 fw_devlink_flags = DL_FLAG_SYNC_STATE_ONLY;
>   static int __init fw_devlink_setup(char *arg)
>   {
>   	if (!arg)

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [RFC PATCH v1] driver core: Set fw_devlink to "permissive" behavior by default

[Greg Kroah-Hartman]

On Fri, Mar 27, 2020 at 11:25:48AM +0100, Marek Szyprowski wrote:
> Hi,
> 
> On 2020-03-21 22:03, Saravana Kannan wrote:
> > Set fw_devlink to "permissive" behavior by default so that device links
> > are automatically created (with DL_FLAG_SYNC_STATE_ONLY) by scanning the
> > firmware.
> >
> > This ensures suppliers get their sync_state() calls only after all their
> > consumers have probed successfully. Without this, suppliers will get
> > their sync_state() calls at late_initcall_sync() even if their consuer
> >
> > Ideally, we'd want to set fw_devlink to "on" or "rpm" by default. But
> > that needs more testing as it's known to break some corner case
> > drivers/platforms.
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> 
> This patch has just landed in linux-next 20200326. Sadly it breaks 
> booting of the Raspberry Pi3b and Pi4 boards, either in 32bit or 64bit 
> mode. There is no warning nor panic message, just a silent freeze. The 
> last message shown on the earlycon is:
> 
> [    0.893217] Serial: 8250/16550 driver, 1 ports, IRQ sharing enabled

Ugh, not good.

Saravana, mind if I revert this?

thanks,

greg k-h

Re: [RFC PATCH v1] driver core: Set fw_devlink to "permissive" behavior by default

[Greg Kroah-Hartman]

On Fri, Mar 27, 2020 at 11:25:48AM +0100, Marek Szyprowski wrote:
> Hi,
> 
> On 2020-03-21 22:03, Saravana Kannan wrote:
> > Set fw_devlink to "permissive" behavior by default so that device links
> > are automatically created (with DL_FLAG_SYNC_STATE_ONLY) by scanning the
> > firmware.
> >
> > This ensures suppliers get their sync_state() calls only after all their
> > consumers have probed successfully. Without this, suppliers will get
> > their sync_state() calls at late_initcall_sync() even if their consuer
> >
> > Ideally, we'd want to set fw_devlink to "on" or "rpm" by default. But
> > that needs more testing as it's known to break some corner case
> > drivers/platforms.
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> 
> This patch has just landed in linux-next 20200326. Sadly it breaks 
> booting of the Raspberry Pi3b and Pi4 boards, either in 32bit or 64bit 
> mode. There is no warning nor panic message, just a silent freeze. The 
> last message shown on the earlycon is:
> 
> [    0.893217] Serial: 8250/16550 driver, 1 ports, IRQ sharing enabled

I've just reverted this for now.

thanks,

greg k-h

Re: [RFC PATCH v1] driver core: Set fw_devlink to "permissive" behavior by default

[Saravana Kannan]

On Fri, Mar 27, 2020 at 8:21 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Mar 27, 2020 at 11:25:48AM +0100, Marek Szyprowski wrote:
> > Hi,
> >
> > On 2020-03-21 22:03, Saravana Kannan wrote:
> > > Set fw_devlink to "permissive" behavior by default so that device links
> > > are automatically created (with DL_FLAG_SYNC_STATE_ONLY) by scanning the
> > > firmware.
> > >
> > > This ensures suppliers get their sync_state() calls only after all their
> > > consumers have probed successfully. Without this, suppliers will get
> > > their sync_state() calls at late_initcall_sync() even if their consuer
> > >
> > > Ideally, we'd want to set fw_devlink to "on" or "rpm" by default. But
> > > that needs more testing as it's known to break some corner case
> > > drivers/platforms.
> > >
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Frank Rowand <frowand.list@gmail.com>
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> >
> > This patch has just landed in linux-next 20200326. Sadly it breaks
> > booting of the Raspberry Pi3b and Pi4 boards, either in 32bit or 64bit
> > mode. There is no warning nor panic message, just a silent freeze. The
> > last message shown on the earlycon is:
> >
> > [    0.893217] Serial: 8250/16550 driver, 1 ports, IRQ sharing enabled

Marek,

Any chance you could get me a stack trace for when it's stuck? That'd
be super helpful and I'd really appreciate it. Is it working fine on
other variants of Raspberry?

>
> I've just reverted this for now.
>

Greg,

I have no problem with reverting this. If there's any other
tree/branch you can put this on where it could get more testing and
reporting of issues, that'd be great.

-Saravana

Re: [RFC PATCH v1] driver core: Set fw_devlink to "permissive" behavior by default

[Marek Szyprowski]

Hi

On 2020-03-27 19:30, Saravana Kannan wrote:
> On Fri, Mar 27, 2020 at 8:21 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Fri, Mar 27, 2020 at 11:25:48AM +0100, Marek Szyprowski wrote:
>>> On 2020-03-21 22:03, Saravana Kannan wrote:
>>>> Set fw_devlink to "permissive" behavior by default so that device links
>>>> are automatically created (with DL_FLAG_SYNC_STATE_ONLY) by scanning the
>>>> firmware.
>>>>
>>>> This ensures suppliers get their sync_state() calls only after all their
>>>> consumers have probed successfully. Without this, suppliers will get
>>>> their sync_state() calls at late_initcall_sync() even if their consuer
>>>>
>>>> Ideally, we'd want to set fw_devlink to "on" or "rpm" by default. But
>>>> that needs more testing as it's known to break some corner case
>>>> drivers/platforms.
>>>>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>>> Cc: devicetree@vger.kernel.org
>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>> This patch has just landed in linux-next 20200326. Sadly it breaks
>>> booting of the Raspberry Pi3b and Pi4 boards, either in 32bit or 64bit
>>> mode. There is no warning nor panic message, just a silent freeze. The
>>> last message shown on the earlycon is:
>>>
>>> [    0.893217] Serial: 8250/16550 driver, 1 ports, IRQ sharing enabled
> Marek,
>
> Any chance you could get me a stack trace for when it's stuck? That'd
> be super helpful and I'd really appreciate it. Is it working fine on
> other variants of Raspberry?

I have no access to other variants of Raspberry board.

The issue seems to be related to bcm2835aux_serial_driver. I've added 
"initcall_debug" and "ignore_loglevel" to kernel cmdline and I got the 
following log:

[    4.595353] calling  exar_pci_driver_init+0x0/0x30 @ 1
[    4.600597] initcall exar_pci_driver_init+0x0/0x30 returned 0 after 
44 usecs
[    4.607747] calling  bcm2835aux_serial_driver_init+0x0/0x28 @ 1

The with some debug printk calls I've found that the clock lookup fails 
with -517 (-EPROBE_DEFER) in bcm2835aux_serial_driver: 
https://elixir.bootlin.com/linux/v5.6/source/drivers/tty/serial/8250/8250_bcm2835aux.c#L52

Without this patch, the lookup works fine.

Please let me know if you need more information. The kernel cmdline I've 
use is: "8250.nr_uarts=1 console=ttyS0,115200n8 
earlycon=uart8250,mmio32,0x3f215040 root=/dev/mmcblk0p2 rootwait rw", 
kernel is compiled with bcm2835_defconfig, booted on Raspberry Pi3b+ 
with arch/arm/boot/dts/bcm2837-rpi-3-b.dtb

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [RFC PATCH v1] driver core: Set fw_devlink to "permissive" behavior by default

[Saravana Kannan]

On Sun, Mar 29, 2020 at 11:20 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi
>
> On 2020-03-27 19:30, Saravana Kannan wrote:
> > On Fri, Mar 27, 2020 at 8:21 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> >> On Fri, Mar 27, 2020 at 11:25:48AM +0100, Marek Szyprowski wrote:
> >>> On 2020-03-21 22:03, Saravana Kannan wrote:
> >>>> Set fw_devlink to "permissive" behavior by default so that device links
> >>>> are automatically created (with DL_FLAG_SYNC_STATE_ONLY) by scanning the
> >>>> firmware.
> >>>>
> >>>> This ensures suppliers get their sync_state() calls only after all their
> >>>> consumers have probed successfully. Without this, suppliers will get
> >>>> their sync_state() calls at late_initcall_sync() even if their consuer
> >>>>
> >>>> Ideally, we'd want to set fw_devlink to "on" or "rpm" by default. But
> >>>> that needs more testing as it's known to break some corner case
> >>>> drivers/platforms.
> >>>>
> >>>> Cc: Rob Herring <robh+dt@kernel.org>
> >>>> Cc: Frank Rowand <frowand.list@gmail.com>
> >>>> Cc: devicetree@vger.kernel.org
> >>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >>> This patch has just landed in linux-next 20200326. Sadly it breaks
> >>> booting of the Raspberry Pi3b and Pi4 boards, either in 32bit or 64bit
> >>> mode. There is no warning nor panic message, just a silent freeze. The
> >>> last message shown on the earlycon is:
> >>>
> >>> [    0.893217] Serial: 8250/16550 driver, 1 ports, IRQ sharing enabled
> > Marek,
> >
> > Any chance you could get me a stack trace for when it's stuck? That'd
> > be super helpful and I'd really appreciate it. Is it working fine on
> > other variants of Raspberry?
>
> I have no access to other variants of Raspberry board.
>
> The issue seems to be related to bcm2835aux_serial_driver. I've added
> "initcall_debug" and "ignore_loglevel" to kernel cmdline and I got the
> following log:
>
> [    4.595353] calling  exar_pci_driver_init+0x0/0x30 @ 1
> [    4.600597] initcall exar_pci_driver_init+0x0/0x30 returned 0 after
> 44 usecs
> [    4.607747] calling  bcm2835aux_serial_driver_init+0x0/0x28 @ 1
>
> The with some debug printk calls I've found that the clock lookup fails
> with -517 (-EPROBE_DEFER) in bcm2835aux_serial_driver:
> https://elixir.bootlin.com/linux/v5.6/source/drivers/tty/serial/8250/8250_bcm2835aux.c#L52
>
> Without this patch, the lookup works fine.
>
> Please let me know if you need more information. The kernel cmdline I've
> use is: "8250.nr_uarts=1 console=ttyS0,115200n8
> earlycon=uart8250,mmio32,0x3f215040 root=/dev/mmcblk0p2 rootwait rw",
> kernel is compiled with bcm2835_defconfig, booted on Raspberry Pi3b+
> with arch/arm/boot/dts/bcm2837-rpi-3-b.dtb

Thanks for the details! I think it gave me an idea of what might be
going wrong here.

-Saravana