Re: [PATCH v2 0/9] deferred_probe_timeout logic clean up

Re: [PATCH v2 0/9] deferred_probe_timeout logic clean up

[Geert Uytterhoeven]

Hi Saravana,

On Wed, Jun 1, 2022 at 12:46 PM Saravana Kannan <saravanak@google.com> wrote:
> This series is based on linux-next + these 2 small patches applies on top:
> https://lore.kernel.org/lkml/20220526034609.480766-1-saravanak@google.com/
>
> A lot of the deferred_probe_timeout logic is redundant with
> fw_devlink=on.  Also, enabling deferred_probe_timeout by default breaks
> a few cases.
>
> This series tries to delete the redundant logic, simplify the frameworks
> that use driver_deferred_probe_check_state(), enable
> deferred_probe_timeout=10 by default, and fixes the nfsroot failure
> case.
>
> The overall idea of this series is to replace the global behavior of
> driver_deferred_probe_check_state() where all devices give up waiting on
> supplier at the same time with a more granular behavior:
>
> 1. Devices with all their suppliers successfully probed by late_initcall
>    probe as usual and avoid unnecessary deferred probe attempts.
>
> 2. At or after late_initcall, in cases where boot would break because of
>    fw_devlink=on being strict about the ordering, we
>
>    a. Temporarily relax the enforcement to probe any unprobed devices
>       that can probe successfully in the current state of the system.
>       For example, when we boot with a NFS rootfs and no network device
>       has probed.
>    b. Go back to enforcing the ordering for any devices that haven't
>       probed.
>
> 3. After deferred probe timeout expires, we permanently give up waiting
>    on supplier devices without drivers. At this point, whatever devices
>    can probe without some of their optional suppliers end up probing.
>
> In the case where module support is disabled, it's fairly
> straightforward and all device probes are completed before the initcalls
> are done.
>
> Patches 1 to 3 are fairly straightforward and can probably be applied
> right away.
>
> Patches 4 to 6 are for fixing the NFS rootfs issue and setting the
> default deferred_probe_timeout back to 10 seconds when modules are
> enabled.
>
> Patches 7 to 9 are further clean up of the deferred_probe_timeout logic
> so that no framework has to know/care about deferred_probe_timeout.
>
> Yoshihiro/Geert,
>
> If you can test this patch series and confirm that the NFS root case
> works, I'd really appreciate that.

Thanks, I gave this a try on various boards I have access to.
The results were quite positive. E.g. the compile error I saw on v1
(implicit declation of fw_devlink_unblock_may_probe(), which is no longer
 used in v2) is gone.

However, I'm seeing a weird error when userspace (Debian9 nfsroot) is
starting:

    [  OK  ] Started D-Bus System Message Bus.
    Unable to handle kernel NULL pointer dereference at virtual
address 0000000000000000
    Unable to handle kernel NULL pointer dereference at virtual
address 0000000000000000
    Mem abort info:
      ESR = 0x0000000096000004
    Mem abort info:
      ESR = 0x0000000096000004
      EC = 0x25: DABT (current EL), IL = 32 bits
      SET = 0, FnV = 0
      EC = 0x25: DABT (current EL), IL = 32 bits
      EA = 0, S1PTW = 0
      FSC = 0x04: level 0 translation fault
      SET = 0, FnV = 0
    Data abort info:
      ISV = 0, ISS = 0x00000004
      EA = 0, S1PTW = 0
      FSC = 0x04: level 0 translation fault
      CM = 0, WnR = 0
    user pgtable: 4k pages, 48-bit VAs, pgdp=000000004ec45000
    [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
    Data abort info:
    Internal error: Oops: 96000004 [#1] PREEMPT SMP
    CPU: 0 PID: 374 Comm: v4l_id Tainted: G        W
5.19.0-rc1-arm64-renesas-00799-gc13c3e49e8bd #1660
      ISV = 0, ISS = 0x00000004
    Hardware name: Renesas Ebisu-4D board based on r8a77990 (DT)
    pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
      CM = 0, WnR = 0
    pc : subdev_open+0x8c/0x128
    lr : subdev_open+0x78/0x128
    sp : ffff80000aadba60
    x29: ffff80000aadba60 x28: 0000000000000000 x27: ffff80000aadbc58
    x26: 0000000000020000 x25: ffff00000b3aaf00 x24: 0000000000000000
    x23: ffff00000c331c00 x22: ffff000009aa61b8 x21: ffff000009aa6000
    x20: ffff000008bae3e8 x19: ffff00000c3fe200 x18: 0000000000000000
    x17: ffff800076945000 x16: ffff800008004000 x15: 00008cc6bf550c7c
    x14: 000000000000038f x13: 000000000000001a x12: ffff00007fba8618
    x11: 0000000000000001 x10: 0000000000000000 x9 : ffff800009253954
    x8 : ffff00000b3aaf00 x7 : 0000000000000004 x6 : 000000000000001a
    x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000001
    x2 : 0000000100000001 x1 : 0000000000000000 x0 : 0000000000000000
    Call trace:
     subdev_open+0x8c/0x128
     v4l2_open+0xa4/0x120
     chrdev_open+0x78/0x178
     do_dentry_open+0xfc/0x398
     vfs_open+0x28/0x30
     path_openat+0x584/0x9c8
     do_filp_open+0x80/0x108
     do_sys_openat2+0x20c/0x2d8
    user pgtable: 4k pages, 48-bit VAs, pgdp=000000004ec53000
     do_sys_open+0x54/0xa0
     __arm64_sys_openat+0x20/0x28
     invoke_syscall+0x40/0xf8
     el0_svc_common.constprop.0+0xf0/0x110
     do_el0_svc+0x20/0x78
     el0_svc+0x48/0xd0
     el0t_64_sync_handler+0xb0/0xb8
     el0t_64_sync+0x148/0x14c
    Code: f9405280 f9400400 b40000e0 f9400280 (f9400000)
    ---[ end trace 0000000000000000 ]---

This only happens on the Ebisu-4D board (r8a77990-ebisu.dts).
I do not see this on the Salvator-X(S) boards.

Bisection shows this starts to happen with "[PATCH v2 7/9] driver core:
Set fw_devlink.strict=1 by default".

Adding more debug info:

    subdev_open:54: file v4l-subdev1
    Unable to handle kernel NULL pointer dereference at virtual
address 0000000000000000
    subdev_open:54: file v4l-subdev2
    Unable to handle kernel NULL pointer dereference at virtual
address 0000000000000000

Matching the subdev using sysfs gives:

    /sys/devices/platform/soc/e6500000.i2c/i2c-0/0-0070/video4linux/v4l-subdev1
    /sys/devices/platform/soc/e6500000.i2c/i2c-0/0-0070/video4linux/v4l-subdev2

The i2c device is the adi,adv7482 at address 0x70.
But now I'm lost...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/9] deferred_probe_timeout logic clean up

[Saravana Kannan]

-Hideaki -- their email keeps bouncing.

On Tue, Jun 7, 2022 at 11:13 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Wed, Jun 1, 2022 at 12:46 PM Saravana Kannan <saravanak@google.com> wrote:
> > This series is based on linux-next + these 2 small patches applies on top:
> > https://lore.kernel.org/lkml/20220526034609.480766-1-saravanak@google.com/
> >
> > A lot of the deferred_probe_timeout logic is redundant with
> > fw_devlink=on.  Also, enabling deferred_probe_timeout by default breaks
> > a few cases.
> >
> > This series tries to delete the redundant logic, simplify the frameworks
> > that use driver_deferred_probe_check_state(), enable
> > deferred_probe_timeout=10 by default, and fixes the nfsroot failure
> > case.
> >
> > The overall idea of this series is to replace the global behavior of
> > driver_deferred_probe_check_state() where all devices give up waiting on
> > supplier at the same time with a more granular behavior:
> >
> > 1. Devices with all their suppliers successfully probed by late_initcall
> >    probe as usual and avoid unnecessary deferred probe attempts.
> >
> > 2. At or after late_initcall, in cases where boot would break because of
> >    fw_devlink=on being strict about the ordering, we
> >
> >    a. Temporarily relax the enforcement to probe any unprobed devices
> >       that can probe successfully in the current state of the system.
> >       For example, when we boot with a NFS rootfs and no network device
> >       has probed.
> >    b. Go back to enforcing the ordering for any devices that haven't
> >       probed.
> >
> > 3. After deferred probe timeout expires, we permanently give up waiting
> >    on supplier devices without drivers. At this point, whatever devices
> >    can probe without some of their optional suppliers end up probing.
> >
> > In the case where module support is disabled, it's fairly
> > straightforward and all device probes are completed before the initcalls
> > are done.
> >
> > Patches 1 to 3 are fairly straightforward and can probably be applied
> > right away.
> >
> > Patches 4 to 6 are for fixing the NFS rootfs issue and setting the
> > default deferred_probe_timeout back to 10 seconds when modules are
> > enabled.
> >
> > Patches 7 to 9 are further clean up of the deferred_probe_timeout logic
> > so that no framework has to know/care about deferred_probe_timeout.
> >
> > Yoshihiro/Geert,
> >
> > If you can test this patch series and confirm that the NFS root case
> > works, I'd really appreciate that.
>
> Thanks, I gave this a try on various boards I have access to.
> The results were quite positive. E.g. the compile error I saw on v1
> (implicit declation of fw_devlink_unblock_may_probe(), which is no longer
>  used in v2) is gone.

Thanks a lot for testing these.

> However, I'm seeing a weird error when userspace (Debian9 nfsroot) is
> starting:
>
>     [  OK  ] Started D-Bus System Message Bus.
>     Unable to handle kernel NULL pointer dereference at virtual
> address 0000000000000000
>     Unable to handle kernel NULL pointer dereference at virtual
> address 0000000000000000
>     Mem abort info:
>       ESR = 0x0000000096000004
>     Mem abort info:
>       ESR = 0x0000000096000004
>       EC = 0x25: DABT (current EL), IL = 32 bits
>       SET = 0, FnV = 0
>       EC = 0x25: DABT (current EL), IL = 32 bits
>       EA = 0, S1PTW = 0
>       FSC = 0x04: level 0 translation fault
>       SET = 0, FnV = 0
>     Data abort info:
>       ISV = 0, ISS = 0x00000004
>       EA = 0, S1PTW = 0
>       FSC = 0x04: level 0 translation fault
>       CM = 0, WnR = 0
>     user pgtable: 4k pages, 48-bit VAs, pgdp=000000004ec45000
>     [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
>     Data abort info:
>     Internal error: Oops: 96000004 [#1] PREEMPT SMP
>     CPU: 0 PID: 374 Comm: v4l_id Tainted: G        W
> 5.19.0-rc1-arm64-renesas-00799-gc13c3e49e8bd #1660
>       ISV = 0, ISS = 0x00000004
>     Hardware name: Renesas Ebisu-4D board based on r8a77990 (DT)
>     pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>       CM = 0, WnR = 0
>     pc : subdev_open+0x8c/0x128
>     lr : subdev_open+0x78/0x128
>     sp : ffff80000aadba60
>     x29: ffff80000aadba60 x28: 0000000000000000 x27: ffff80000aadbc58
>     x26: 0000000000020000 x25: ffff00000b3aaf00 x24: 0000000000000000
>     x23: ffff00000c331c00 x22: ffff000009aa61b8 x21: ffff000009aa6000
>     x20: ffff000008bae3e8 x19: ffff00000c3fe200 x18: 0000000000000000
>     x17: ffff800076945000 x16: ffff800008004000 x15: 00008cc6bf550c7c
>     x14: 000000000000038f x13: 000000000000001a x12: ffff00007fba8618
>     x11: 0000000000000001 x10: 0000000000000000 x9 : ffff800009253954
>     x8 : ffff00000b3aaf00 x7 : 0000000000000004 x6 : 000000000000001a
>     x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000001
>     x2 : 0000000100000001 x1 : 0000000000000000 x0 : 0000000000000000
>     Call trace:
>      subdev_open+0x8c/0x128
>      v4l2_open+0xa4/0x120
>      chrdev_open+0x78/0x178
>      do_dentry_open+0xfc/0x398
>      vfs_open+0x28/0x30
>      path_openat+0x584/0x9c8
>      do_filp_open+0x80/0x108
>      do_sys_openat2+0x20c/0x2d8
>     user pgtable: 4k pages, 48-bit VAs, pgdp=000000004ec53000
>      do_sys_open+0x54/0xa0
>      __arm64_sys_openat+0x20/0x28
>      invoke_syscall+0x40/0xf8
>      el0_svc_common.constprop.0+0xf0/0x110
>      do_el0_svc+0x20/0x78
>      el0_svc+0x48/0xd0
>      el0t_64_sync_handler+0xb0/0xb8
>      el0t_64_sync+0x148/0x14c
>     Code: f9405280 f9400400 b40000e0 f9400280 (f9400000)
>     ---[ end trace 0000000000000000 ]---
>
> This only happens on the Ebisu-4D board (r8a77990-ebisu.dts).
> I do not see this on the Salvator-X(S) boards.

Ok. I don't know much about either of these boards. Are they supposed
to be very similar?

> Bisection shows this starts to happen with "[PATCH v2 7/9] driver core:
> Set fw_devlink.strict=1 by default".

So in the series, by this point, the previous patches would have
deferred probe timeout set to 10s (it can get extended on new driver
additions of course) and once the timer expires suppliers without
drivers will no longer block any consumers. The only difference
fw_devlink.strict=1 should cause is iommus and dmas dependency being
treated as mandatory till the timeout expires.

In this instance, do you have iommu drivers and dma drivers compiled
in or loaded as modules or not available at all? In all these case,
the list of devices that would end up probing eventually should be the
same with or without fw_devlink.strict=1. The only difference would be
some reordering of probes.

So this looks to me like improper error handling/assumption in the
driver for this subdev device. I'm guessing one of the suppliers to
this subdev has a direct/indirect dependency on iommus and this subdev
driver is assuming that the supplier would have probed by the time
it's probed.

>
> Adding more debug info:
>
>     subdev_open:54: file v4l-subdev1
>     Unable to handle kernel NULL pointer dereference at virtual
> address 0000000000000000
>     subdev_open:54: file v4l-subdev2
>     Unable to handle kernel NULL pointer dereference at virtual
> address 0000000000000000
>
> Matching the subdev using sysfs gives:
>
>     /sys/devices/platform/soc/e6500000.i2c/i2c-0/0-0070/video4linux/v4l-subdev1
>     /sys/devices/platform/soc/e6500000.i2c/i2c-0/0-0070/video4linux/v4l-subdev2
>
> The i2c device is the adi,adv7482 at address 0x70.

I'm guessing the fix would be somewhere in this driver, but I haven't
dug into it. Any guesses on which of its suppliers might have a
direct/indirect dependency on an iommu/dma? You could also enable the
debug log in fw_devlink_relax_link() and see if it relaxes any link
where the supplier is an iommu/dma device. That might give us some
hints.

-Saravana

> But now I'm lost...
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v2 0/9] deferred_probe_timeout logic clean up

[Saravana Kannan]

On Tue, Jun 7, 2022 at 5:55 PM Saravana Kannan <saravanak@google.com> wrote:
>
> -Hideaki -- their email keeps bouncing.
>
> On Tue, Jun 7, 2022 at 11:13 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > Hi Saravana,
> >
> > On Wed, Jun 1, 2022 at 12:46 PM Saravana Kannan <saravanak@google.com> wrote:
> > > This series is based on linux-next + these 2 small patches applies on top:
> > > https://lore.kernel.org/lkml/20220526034609.480766-1-saravanak@google.com/
> > >
> > > A lot of the deferred_probe_timeout logic is redundant with
> > > fw_devlink=on.  Also, enabling deferred_probe_timeout by default breaks
> > > a few cases.
> > >
> > > This series tries to delete the redundant logic, simplify the frameworks
> > > that use driver_deferred_probe_check_state(), enable
> > > deferred_probe_timeout=10 by default, and fixes the nfsroot failure
> > > case.
> > >
> > > The overall idea of this series is to replace the global behavior of
> > > driver_deferred_probe_check_state() where all devices give up waiting on
> > > supplier at the same time with a more granular behavior:
> > >
> > > 1. Devices with all their suppliers successfully probed by late_initcall
> > >    probe as usual and avoid unnecessary deferred probe attempts.
> > >
> > > 2. At or after late_initcall, in cases where boot would break because of
> > >    fw_devlink=on being strict about the ordering, we
> > >
> > >    a. Temporarily relax the enforcement to probe any unprobed devices
> > >       that can probe successfully in the current state of the system.
> > >       For example, when we boot with a NFS rootfs and no network device
> > >       has probed.
> > >    b. Go back to enforcing the ordering for any devices that haven't
> > >       probed.
> > >
> > > 3. After deferred probe timeout expires, we permanently give up waiting
> > >    on supplier devices without drivers. At this point, whatever devices
> > >    can probe without some of their optional suppliers end up probing.
> > >
> > > In the case where module support is disabled, it's fairly
> > > straightforward and all device probes are completed before the initcalls
> > > are done.
> > >
> > > Patches 1 to 3 are fairly straightforward and can probably be applied
> > > right away.
> > >
> > > Patches 4 to 6 are for fixing the NFS rootfs issue and setting the
> > > default deferred_probe_timeout back to 10 seconds when modules are
> > > enabled.
> > >
> > > Patches 7 to 9 are further clean up of the deferred_probe_timeout logic
> > > so that no framework has to know/care about deferred_probe_timeout.
> > >
> > > Yoshihiro/Geert,
> > >
> > > If you can test this patch series and confirm that the NFS root case
> > > works, I'd really appreciate that.
> >
> > Thanks, I gave this a try on various boards I have access to.
> > The results were quite positive. E.g. the compile error I saw on v1
> > (implicit declation of fw_devlink_unblock_may_probe(), which is no longer
> >  used in v2) is gone.
>
> Thanks a lot for testing these.
>
> > However, I'm seeing a weird error when userspace (Debian9 nfsroot) is
> > starting:
> >
> >     [  OK  ] Started D-Bus System Message Bus.
> >     Unable to handle kernel NULL pointer dereference at virtual
> > address 0000000000000000
> >     Unable to handle kernel NULL pointer dereference at virtual
> > address 0000000000000000
> >     Mem abort info:
> >       ESR = 0x0000000096000004
> >     Mem abort info:
> >       ESR = 0x0000000096000004
> >       EC = 0x25: DABT (current EL), IL = 32 bits
> >       SET = 0, FnV = 0
> >       EC = 0x25: DABT (current EL), IL = 32 bits
> >       EA = 0, S1PTW = 0
> >       FSC = 0x04: level 0 translation fault
> >       SET = 0, FnV = 0
> >     Data abort info:
> >       ISV = 0, ISS = 0x00000004
> >       EA = 0, S1PTW = 0
> >       FSC = 0x04: level 0 translation fault
> >       CM = 0, WnR = 0
> >     user pgtable: 4k pages, 48-bit VAs, pgdp=000000004ec45000
> >     [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
> >     Data abort info:
> >     Internal error: Oops: 96000004 [#1] PREEMPT SMP
> >     CPU: 0 PID: 374 Comm: v4l_id Tainted: G        W
> > 5.19.0-rc1-arm64-renesas-00799-gc13c3e49e8bd #1660
> >       ISV = 0, ISS = 0x00000004
> >     Hardware name: Renesas Ebisu-4D board based on r8a77990 (DT)
> >     pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >       CM = 0, WnR = 0
> >     pc : subdev_open+0x8c/0x128
> >     lr : subdev_open+0x78/0x128
> >     sp : ffff80000aadba60
> >     x29: ffff80000aadba60 x28: 0000000000000000 x27: ffff80000aadbc58
> >     x26: 0000000000020000 x25: ffff00000b3aaf00 x24: 0000000000000000
> >     x23: ffff00000c331c00 x22: ffff000009aa61b8 x21: ffff000009aa6000
> >     x20: ffff000008bae3e8 x19: ffff00000c3fe200 x18: 0000000000000000
> >     x17: ffff800076945000 x16: ffff800008004000 x15: 00008cc6bf550c7c
> >     x14: 000000000000038f x13: 000000000000001a x12: ffff00007fba8618
> >     x11: 0000000000000001 x10: 0000000000000000 x9 : ffff800009253954
> >     x8 : ffff00000b3aaf00 x7 : 0000000000000004 x6 : 000000000000001a
> >     x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000001
> >     x2 : 0000000100000001 x1 : 0000000000000000 x0 : 0000000000000000
> >     Call trace:
> >      subdev_open+0x8c/0x128

After disassembling the code on my end (with slightly different
config) and looking at 0x8c from the start of the function, I'm pretty
sure the NULL deref is happening here inside subdev_open()

        if (sd->v4l2_dev->mdev && sd->entity.graph_obj.mdev->dev) {

sd->entity.graph_obj.mdev == NULL.

And going by the field names, I'm guessing these are suppliers pointed
to by "remote-endpoint". Sadly fw_devlink can't extract any dependency
info from remote-endpoint because the devices generally point to each
other so a cycle is detected and the probe ordering isn't enforced
between the endpoints. We still need to parse remote-endpoint to
detect cycles created by a combination of endpoints/other properties
(there's a real world case in upstream).

> >      v4l2_open+0xa4/0x120
> >      chrdev_open+0x78/0x178
> >      do_dentry_open+0xfc/0x398
> >      vfs_open+0x28/0x30
> >      path_openat+0x584/0x9c8
> >      do_filp_open+0x80/0x108
> >      do_sys_openat2+0x20c/0x2d8
> >     user pgtable: 4k pages, 48-bit VAs, pgdp=000000004ec53000
> >      do_sys_open+0x54/0xa0
> >      __arm64_sys_openat+0x20/0x28
> >      invoke_syscall+0x40/0xf8
> >      el0_svc_common.constprop.0+0xf0/0x110
> >      do_el0_svc+0x20/0x78
> >      el0_svc+0x48/0xd0
> >      el0t_64_sync_handler+0xb0/0xb8
> >      el0t_64_sync+0x148/0x14c
> >     Code: f9405280 f9400400 b40000e0 f9400280 (f9400000)
> >     ---[ end trace 0000000000000000 ]---
> >
> > This only happens on the Ebisu-4D board (r8a77990-ebisu.dts).
> > I do not see this on the Salvator-X(S) boards.
>
> Ok. I don't know much about either of these boards. Are they supposed
> to be very similar?
>
> > Bisection shows this starts to happen with "[PATCH v2 7/9] driver core:
> > Set fw_devlink.strict=1 by default".
>
> So in the series, by this point, the previous patches would have
> deferred probe timeout set to 10s (it can get extended on new driver
> additions of course) and once the timer expires suppliers without
> drivers will no longer block any consumers. The only difference
> fw_devlink.strict=1 should cause is iommus and dmas dependency being
> treated as mandatory till the timeout expires.
>
> In this instance, do you have iommu drivers and dma drivers compiled
> in or loaded as modules or not available at all? In all these case,
> the list of devices that would end up probing eventually should be the
> same with or without fw_devlink.strict=1. The only difference would be
> some reordering of probes.
>
> So this looks to me like improper error handling/assumption in the
> driver for this subdev device. I'm guessing one of the suppliers to
> this subdev has a direct/indirect dependency on iommus and this subdev
> driver is assuming that the supplier would have probed by the time
> it's probed.
>
> >
> > Adding more debug info:
> >
> >     subdev_open:54: file v4l-subdev1
> >     Unable to handle kernel NULL pointer dereference at virtual
> > address 0000000000000000
> >     subdev_open:54: file v4l-subdev2
> >     Unable to handle kernel NULL pointer dereference at virtual
> > address 0000000000000000

How did you get these two "subdev_open" strings? And how/why the NULL
deref there?

> >
> > Matching the subdev using sysfs gives:
> >
> >     /sys/devices/platform/soc/e6500000.i2c/i2c-0/0-0070/video4linux/v4l-subdev1
> >     /sys/devices/platform/soc/e6500000.i2c/i2c-0/0-0070/video4linux/v4l-subdev2
> >
> > The i2c device is the adi,adv7482 at address 0x70.
>
> I'm guessing the fix would be somewhere in this driver, but I haven't
> dug into it. Any guesses on which of its suppliers might have a
> direct/indirect dependency on an iommu/dma? You could also enable the
> debug log in fw_devlink_relax_link() and see if it relaxes any link
> where the supplier is an iommu/dma device. That might give us some
> hints.

After spending way too much time on this looking at
drivers/media/v4l2-core, drivers/media/mc and
drivers/media/i2c/adv748x/ code, I'm guessing the ordering issue is
probably between "csi40:" device and the video-receiver@70 (the
"adi,adv7482") device.

Based on your points about the sysfs, I was initially digging into
drivers/media/i2c/adv748x/adv748x-core.c. But then the parent of
video-receiver@70 is an i2c0 that has dmas dependencies. The csi40:
(referred to from video-controller) doesn't seem to have any iommu or
dmas dependency. So my guess is the csi40 gets probed first and then
assumes the video-controller is already available.

Can you use this info to take a stab at debugging this further?

TL;DR is that I think this is some driver issue where it's not
checking for one of its suppliers to be ready yet.

-Saravana

>
> -Saravana
>
> > But now I'm lost...
> >
> > Gr{oetje,eeting}s,
> >
> >                         Geert
> >
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> >
> > In personal conversations with technical people, I call myself a hacker. But
> > when I'm talking to journalists I just say "programmer" or something like that.
> >                                 -- Linus Torvalds

Re: [PATCH v2 0/9] deferred_probe_timeout logic clean up

[Geert Uytterhoeven]

Hi Saravana,

On Wed, Jun 8, 2022 at 6:17 AM Saravana Kannan <saravanak@google.com> wrote:
> On Tue, Jun 7, 2022 at 5:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > On Tue, Jun 7, 2022 at 11:13 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, Jun 1, 2022 at 12:46 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > This series is based on linux-next + these 2 small patches applies on top:
> > > > https://lore.kernel.org/lkml/20220526034609.480766-1-saravanak@google.com/
> > > >
> > > > A lot of the deferred_probe_timeout logic is redundant with
> > > > fw_devlink=on.  Also, enabling deferred_probe_timeout by default breaks
> > > > a few cases.
> > > >
> > > > This series tries to delete the redundant logic, simplify the frameworks
> > > > that use driver_deferred_probe_check_state(), enable
> > > > deferred_probe_timeout=10 by default, and fixes the nfsroot failure
> > > > case.
> > > >
> > > > The overall idea of this series is to replace the global behavior of
> > > > driver_deferred_probe_check_state() where all devices give up waiting on
> > > > supplier at the same time with a more granular behavior:
> > > >
> > > > 1. Devices with all their suppliers successfully probed by late_initcall
> > > >    probe as usual and avoid unnecessary deferred probe attempts.
> > > >
> > > > 2. At or after late_initcall, in cases where boot would break because of
> > > >    fw_devlink=on being strict about the ordering, we
> > > >
> > > >    a. Temporarily relax the enforcement to probe any unprobed devices
> > > >       that can probe successfully in the current state of the system.
> > > >       For example, when we boot with a NFS rootfs and no network device
> > > >       has probed.
> > > >    b. Go back to enforcing the ordering for any devices that haven't
> > > >       probed.
> > > >
> > > > 3. After deferred probe timeout expires, we permanently give up waiting
> > > >    on supplier devices without drivers. At this point, whatever devices
> > > >    can probe without some of their optional suppliers end up probing.
> > > >
> > > > In the case where module support is disabled, it's fairly
> > > > straightforward and all device probes are completed before the initcalls
> > > > are done.
> > > >
> > > > Patches 1 to 3 are fairly straightforward and can probably be applied
> > > > right away.
> > > >
> > > > Patches 4 to 6 are for fixing the NFS rootfs issue and setting the
> > > > default deferred_probe_timeout back to 10 seconds when modules are
> > > > enabled.
> > > >
> > > > Patches 7 to 9 are further clean up of the deferred_probe_timeout logic
> > > > so that no framework has to know/care about deferred_probe_timeout.
> > > >
> > > > Yoshihiro/Geert,
> > > >
> > > > If you can test this patch series and confirm that the NFS root case
> > > > works, I'd really appreciate that.
> > >
> > > Thanks, I gave this a try on various boards I have access to.
> > > The results were quite positive. E.g. the compile error I saw on v1
> > > (implicit declation of fw_devlink_unblock_may_probe(), which is no longer
> > >  used in v2) is gone.
> >
> > Thanks a lot for testing these.
> >
> > > However, I'm seeing a weird error when userspace (Debian9 nfsroot) is
> > > starting:
> > >
> > >     [  OK  ] Started D-Bus System Message Bus.
> > >     Unable to handle kernel NULL pointer dereference at virtual
> > > address 0000000000000000
> > >     Unable to handle kernel NULL pointer dereference at virtual
> > > address 0000000000000000
> > >     Mem abort info:
> > >       ESR = 0x0000000096000004
> > >     Mem abort info:
> > >       ESR = 0x0000000096000004
> > >       EC = 0x25: DABT (current EL), IL = 32 bits
> > >       SET = 0, FnV = 0
> > >       EC = 0x25: DABT (current EL), IL = 32 bits
> > >       EA = 0, S1PTW = 0
> > >       FSC = 0x04: level 0 translation fault
> > >       SET = 0, FnV = 0
> > >     Data abort info:
> > >       ISV = 0, ISS = 0x00000004
> > >       EA = 0, S1PTW = 0
> > >       FSC = 0x04: level 0 translation fault
> > >       CM = 0, WnR = 0
> > >     user pgtable: 4k pages, 48-bit VAs, pgdp=000000004ec45000
> > >     [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
> > >     Data abort info:
> > >     Internal error: Oops: 96000004 [#1] PREEMPT SMP
> > >     CPU: 0 PID: 374 Comm: v4l_id Tainted: G        W
> > > 5.19.0-rc1-arm64-renesas-00799-gc13c3e49e8bd #1660
> > >       ISV = 0, ISS = 0x00000004
> > >     Hardware name: Renesas Ebisu-4D board based on r8a77990 (DT)
> > >     pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > >       CM = 0, WnR = 0
> > >     pc : subdev_open+0x8c/0x128
> > >     lr : subdev_open+0x78/0x128
> > >     sp : ffff80000aadba60
> > >     x29: ffff80000aadba60 x28: 0000000000000000 x27: ffff80000aadbc58
> > >     x26: 0000000000020000 x25: ffff00000b3aaf00 x24: 0000000000000000
> > >     x23: ffff00000c331c00 x22: ffff000009aa61b8 x21: ffff000009aa6000
> > >     x20: ffff000008bae3e8 x19: ffff00000c3fe200 x18: 0000000000000000
> > >     x17: ffff800076945000 x16: ffff800008004000 x15: 00008cc6bf550c7c
> > >     x14: 000000000000038f x13: 000000000000001a x12: ffff00007fba8618
> > >     x11: 0000000000000001 x10: 0000000000000000 x9 : ffff800009253954
> > >     x8 : ffff00000b3aaf00 x7 : 0000000000000004 x6 : 000000000000001a
> > >     x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000001
> > >     x2 : 0000000100000001 x1 : 0000000000000000 x0 : 0000000000000000
> > >     Call trace:
> > >      subdev_open+0x8c/0x128
>
> After disassembling the code on my end (with slightly different
> config) and looking at 0x8c from the start of the function, I'm pretty
> sure the NULL deref is happening here inside subdev_open()
>
>         if (sd->v4l2_dev->mdev && sd->entity.graph_obj.mdev->dev) {
>
> sd->entity.graph_obj.mdev == NULL.
>
> And going by the field names, I'm guessing these are suppliers pointed
> to by "remote-endpoint". Sadly fw_devlink can't extract any dependency
> info from remote-endpoint because the devices generally point to each
> other so a cycle is detected and the probe ordering isn't enforced
> between the endpoints. We still need to parse remote-endpoint to
> detect cycles created by a combination of endpoints/other properties
> (there's a real world case in upstream).
>
> > >      v4l2_open+0xa4/0x120
> > >      chrdev_open+0x78/0x178
> > >      do_dentry_open+0xfc/0x398
> > >      vfs_open+0x28/0x30
> > >      path_openat+0x584/0x9c8
> > >      do_filp_open+0x80/0x108
> > >      do_sys_openat2+0x20c/0x2d8
> > >     user pgtable: 4k pages, 48-bit VAs, pgdp=000000004ec53000
> > >      do_sys_open+0x54/0xa0
> > >      __arm64_sys_openat+0x20/0x28
> > >      invoke_syscall+0x40/0xf8
> > >      el0_svc_common.constprop.0+0xf0/0x110
> > >      do_el0_svc+0x20/0x78
> > >      el0_svc+0x48/0xd0
> > >      el0t_64_sync_handler+0xb0/0xb8
> > >      el0t_64_sync+0x148/0x14c
> > >     Code: f9405280 f9400400 b40000e0 f9400280 (f9400000)
> > >     ---[ end trace 0000000000000000 ]---
> > >
> > > This only happens on the Ebisu-4D board (r8a77990-ebisu.dts).
> > > I do not see this on the Salvator-X(S) boards.
> >
> > Ok. I don't know much about either of these boards. Are they supposed
> > to be very similar?
> >
> > > Bisection shows this starts to happen with "[PATCH v2 7/9] driver core:
> > > Set fw_devlink.strict=1 by default".
> >
> > So in the series, by this point, the previous patches would have
> > deferred probe timeout set to 10s (it can get extended on new driver
> > additions of course) and once the timer expires suppliers without
> > drivers will no longer block any consumers. The only difference
> > fw_devlink.strict=1 should cause is iommus and dmas dependency being
> > treated as mandatory till the timeout expires.
> >
> > In this instance, do you have iommu drivers and dma drivers compiled
> > in or loaded as modules or not available at all? In all these case,
> > the list of devices that would end up probing eventually should be the
> > same with or without fw_devlink.strict=1. The only difference would be
> > some reordering of probes.
> >
> > So this looks to me like improper error handling/assumption in the
> > driver for this subdev device. I'm guessing one of the suppliers to
> > this subdev has a direct/indirect dependency on iommus and this subdev
> > driver is assuming that the supplier would have probed by the time
> > it's probed.
> >
> > >
> > > Adding more debug info:
> > >
> > >     subdev_open:54: file v4l-subdev1
> > >     Unable to handle kernel NULL pointer dereference at virtual
> > > address 0000000000000000
> > >     subdev_open:54: file v4l-subdev2
> > >     Unable to handle kernel NULL pointer dereference at virtual
> > > address 0000000000000000
>
> How did you get these two "subdev_open" strings? And how/why the NULL
> deref there?

I added a debug print at the top of subdev_open():

    pr_info("%s:%u: file %pD\n", __func__, __LINE__, file);

The NULL deref is the actual issue.

> > > Matching the subdev using sysfs gives:
> > >
> > >     /sys/devices/platform/soc/e6500000.i2c/i2c-0/0-0070/video4linux/v4l-subdev1
> > >     /sys/devices/platform/soc/e6500000.i2c/i2c-0/0-0070/video4linux/v4l-subdev2
> > >
> > > The i2c device is the adi,adv7482 at address 0x70.
> >
> > I'm guessing the fix would be somewhere in this driver, but I haven't
> > dug into it. Any guesses on which of its suppliers might have a
> > direct/indirect dependency on an iommu/dma? You could also enable the
> > debug log in fw_devlink_relax_link() and see if it relaxes any link
> > where the supplier is an iommu/dma device. That might give us some
> > hints.
>
> After spending way too much time on this looking at
> drivers/media/v4l2-core, drivers/media/mc and
> drivers/media/i2c/adv748x/ code, I'm guessing the ordering issue is
> probably between "csi40:" device and the video-receiver@70 (the
> "adi,adv7482") device.
>
> Based on your points about the sysfs, I was initially digging into
> drivers/media/i2c/adv748x/adv748x-core.c. But then the parent of
> video-receiver@70 is an i2c0 that has dmas dependencies. The csi40:
> (referred to from video-controller) doesn't seem to have any iommu or
> dmas dependency. So my guess is the csi40 gets probed first and then
> assumes the video-controller is already available.
>
> Can you use this info to take a stab at debugging this further?

Thanks for looking into this, there is indeed a cyclic dependency:

    i2c 0-0070: Fixing up cyclic dependency with feaa0000.csi2
    i2c 0-0070: Fixing up cyclic dependency with hdmi-in
    i2c 0-0070: Fixing up cyclic dependency with cvbs-in

> TL;DR is that I think this is some driver issue where it's not
> checking for one of its suppliers to be ready yet.

Setting fw_devlink_strict to true vs. false seems to influence which of
two different failures will happen:
  - rcar-csi2: probe of feaa0000.csi2 failed with error -22
  - rcar-vin: probe of e6ef5000.video failed with error -22
The former causes the NULL pointer dereference later.
The latter existed before, but I hadn't noticed it, and bisection
led to the real culprit (commit 3e52419ec04f9769 ("media: rcar-{csi2,vin}:
Move to full Virtual Channel routing per CSI-2 IP").

I am bringing it up with the multi-media guys in
https://lore.kernel.org/all/20220124124858.571363-4-niklas.soderlund+renesas@ragnatech.se...

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/9] deferred_probe_timeout logic clean up

[Saravana Kannan]

On Wed, Jun 8, 2022 at 3:26 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Wed, Jun 8, 2022 at 6:17 AM Saravana Kannan <saravanak@google.com> wrote:
> > On Tue, Jun 7, 2022 at 5:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > > On Tue, Jun 7, 2022 at 11:13 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Wed, Jun 1, 2022 at 12:46 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > > This series is based on linux-next + these 2 small patches applies on top:
> > > > > https://lore.kernel.org/lkml/20220526034609.480766-1-saravanak@google.com/
> > > > >
> > > > > A lot of the deferred_probe_timeout logic is redundant with
> > > > > fw_devlink=on.  Also, enabling deferred_probe_timeout by default breaks
> > > > > a few cases.
> > > > >
> > > > > This series tries to delete the redundant logic, simplify the frameworks
> > > > > that use driver_deferred_probe_check_state(), enable
> > > > > deferred_probe_timeout=10 by default, and fixes the nfsroot failure
> > > > > case.
> > > > >
> > > > > The overall idea of this series is to replace the global behavior of
> > > > > driver_deferred_probe_check_state() where all devices give up waiting on
> > > > > supplier at the same time with a more granular behavior:
> > > > >
> > > > > 1. Devices with all their suppliers successfully probed by late_initcall
> > > > >    probe as usual and avoid unnecessary deferred probe attempts.
> > > > >
> > > > > 2. At or after late_initcall, in cases where boot would break because of
> > > > >    fw_devlink=on being strict about the ordering, we
> > > > >
> > > > >    a. Temporarily relax the enforcement to probe any unprobed devices
> > > > >       that can probe successfully in the current state of the system.
> > > > >       For example, when we boot with a NFS rootfs and no network device
> > > > >       has probed.
> > > > >    b. Go back to enforcing the ordering for any devices that haven't
> > > > >       probed.
> > > > >
> > > > > 3. After deferred probe timeout expires, we permanently give up waiting
> > > > >    on supplier devices without drivers. At this point, whatever devices
> > > > >    can probe without some of their optional suppliers end up probing.
> > > > >
> > > > > In the case where module support is disabled, it's fairly
> > > > > straightforward and all device probes are completed before the initcalls
> > > > > are done.
> > > > >
> > > > > Patches 1 to 3 are fairly straightforward and can probably be applied
> > > > > right away.
> > > > >
> > > > > Patches 4 to 6 are for fixing the NFS rootfs issue and setting the
> > > > > default deferred_probe_timeout back to 10 seconds when modules are
> > > > > enabled.
> > > > >
> > > > > Patches 7 to 9 are further clean up of the deferred_probe_timeout logic
> > > > > so that no framework has to know/care about deferred_probe_timeout.
> > > > >
> > > > > Yoshihiro/Geert,
> > > > >
> > > > > If you can test this patch series and confirm that the NFS root case
> > > > > works, I'd really appreciate that.
> > > >
> > > > Thanks, I gave this a try on various boards I have access to.
> > > > The results were quite positive. E.g. the compile error I saw on v1
> > > > (implicit declation of fw_devlink_unblock_may_probe(), which is no longer
> > > >  used in v2) is gone.
> > >
> > > Thanks a lot for testing these.
> > >
> > > > However, I'm seeing a weird error when userspace (Debian9 nfsroot) is
> > > > starting:
> > > >
> > > >     [  OK  ] Started D-Bus System Message Bus.
> > > >     Unable to handle kernel NULL pointer dereference at virtual
> > > > address 0000000000000000
> > > >     Unable to handle kernel NULL pointer dereference at virtual
> > > > address 0000000000000000
> > > >     Mem abort info:
> > > >       ESR = 0x0000000096000004
> > > >     Mem abort info:
> > > >       ESR = 0x0000000096000004
> > > >       EC = 0x25: DABT (current EL), IL = 32 bits
> > > >       SET = 0, FnV = 0
> > > >       EC = 0x25: DABT (current EL), IL = 32 bits
> > > >       EA = 0, S1PTW = 0
> > > >       FSC = 0x04: level 0 translation fault
> > > >       SET = 0, FnV = 0
> > > >     Data abort info:
> > > >       ISV = 0, ISS = 0x00000004
> > > >       EA = 0, S1PTW = 0
> > > >       FSC = 0x04: level 0 translation fault
> > > >       CM = 0, WnR = 0
> > > >     user pgtable: 4k pages, 48-bit VAs, pgdp=000000004ec45000
> > > >     [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
> > > >     Data abort info:
> > > >     Internal error: Oops: 96000004 [#1] PREEMPT SMP
> > > >     CPU: 0 PID: 374 Comm: v4l_id Tainted: G        W
> > > > 5.19.0-rc1-arm64-renesas-00799-gc13c3e49e8bd #1660
> > > >       ISV = 0, ISS = 0x00000004
> > > >     Hardware name: Renesas Ebisu-4D board based on r8a77990 (DT)
> > > >     pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > >       CM = 0, WnR = 0
> > > >     pc : subdev_open+0x8c/0x128
> > > >     lr : subdev_open+0x78/0x128
> > > >     sp : ffff80000aadba60
> > > >     x29: ffff80000aadba60 x28: 0000000000000000 x27: ffff80000aadbc58
> > > >     x26: 0000000000020000 x25: ffff00000b3aaf00 x24: 0000000000000000
> > > >     x23: ffff00000c331c00 x22: ffff000009aa61b8 x21: ffff000009aa6000
> > > >     x20: ffff000008bae3e8 x19: ffff00000c3fe200 x18: 0000000000000000
> > > >     x17: ffff800076945000 x16: ffff800008004000 x15: 00008cc6bf550c7c
> > > >     x14: 000000000000038f x13: 000000000000001a x12: ffff00007fba8618
> > > >     x11: 0000000000000001 x10: 0000000000000000 x9 : ffff800009253954
> > > >     x8 : ffff00000b3aaf00 x7 : 0000000000000004 x6 : 000000000000001a
> > > >     x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000001
> > > >     x2 : 0000000100000001 x1 : 0000000000000000 x0 : 0000000000000000
> > > >     Call trace:
> > > >      subdev_open+0x8c/0x128
> >
> > After disassembling the code on my end (with slightly different
> > config) and looking at 0x8c from the start of the function, I'm pretty
> > sure the NULL deref is happening here inside subdev_open()
> >
> >         if (sd->v4l2_dev->mdev && sd->entity.graph_obj.mdev->dev) {
> >
> > sd->entity.graph_obj.mdev == NULL.
> >
> > And going by the field names, I'm guessing these are suppliers pointed
> > to by "remote-endpoint". Sadly fw_devlink can't extract any dependency
> > info from remote-endpoint because the devices generally point to each
> > other so a cycle is detected and the probe ordering isn't enforced
> > between the endpoints. We still need to parse remote-endpoint to
> > detect cycles created by a combination of endpoints/other properties
> > (there's a real world case in upstream).
> >
> > > >      v4l2_open+0xa4/0x120
> > > >      chrdev_open+0x78/0x178
> > > >      do_dentry_open+0xfc/0x398
> > > >      vfs_open+0x28/0x30
> > > >      path_openat+0x584/0x9c8
> > > >      do_filp_open+0x80/0x108
> > > >      do_sys_openat2+0x20c/0x2d8
> > > >     user pgtable: 4k pages, 48-bit VAs, pgdp=000000004ec53000
> > > >      do_sys_open+0x54/0xa0
> > > >      __arm64_sys_openat+0x20/0x28
> > > >      invoke_syscall+0x40/0xf8
> > > >      el0_svc_common.constprop.0+0xf0/0x110
> > > >      do_el0_svc+0x20/0x78
> > > >      el0_svc+0x48/0xd0
> > > >      el0t_64_sync_handler+0xb0/0xb8
> > > >      el0t_64_sync+0x148/0x14c
> > > >     Code: f9405280 f9400400 b40000e0 f9400280 (f9400000)
> > > >     ---[ end trace 0000000000000000 ]---
> > > >
> > > > This only happens on the Ebisu-4D board (r8a77990-ebisu.dts).
> > > > I do not see this on the Salvator-X(S) boards.
> > >
> > > Ok. I don't know much about either of these boards. Are they supposed
> > > to be very similar?
> > >
> > > > Bisection shows this starts to happen with "[PATCH v2 7/9] driver core:
> > > > Set fw_devlink.strict=1 by default".
> > >
> > > So in the series, by this point, the previous patches would have
> > > deferred probe timeout set to 10s (it can get extended on new driver
> > > additions of course) and once the timer expires suppliers without
> > > drivers will no longer block any consumers. The only difference
> > > fw_devlink.strict=1 should cause is iommus and dmas dependency being
> > > treated as mandatory till the timeout expires.
> > >
> > > In this instance, do you have iommu drivers and dma drivers compiled
> > > in or loaded as modules or not available at all? In all these case,
> > > the list of devices that would end up probing eventually should be the
> > > same with or without fw_devlink.strict=1. The only difference would be
> > > some reordering of probes.
> > >
> > > So this looks to me like improper error handling/assumption in the
> > > driver for this subdev device. I'm guessing one of the suppliers to
> > > this subdev has a direct/indirect dependency on iommus and this subdev
> > > driver is assuming that the supplier would have probed by the time
> > > it's probed.
> > >
> > > >
> > > > Adding more debug info:
> > > >
> > > >     subdev_open:54: file v4l-subdev1
> > > >     Unable to handle kernel NULL pointer dereference at virtual
> > > > address 0000000000000000
> > > >     subdev_open:54: file v4l-subdev2
> > > >     Unable to handle kernel NULL pointer dereference at virtual
> > > > address 0000000000000000
> >
> > How did you get these two "subdev_open" strings? And how/why the NULL
> > deref there?
>
> I added a debug print at the top of subdev_open():
>
>     pr_info("%s:%u: file %pD\n", __func__, __LINE__, file);
>
> The NULL deref is the actual issue.
>
> > > > Matching the subdev using sysfs gives:
> > > >
> > > >     /sys/devices/platform/soc/e6500000.i2c/i2c-0/0-0070/video4linux/v4l-subdev1
> > > >     /sys/devices/platform/soc/e6500000.i2c/i2c-0/0-0070/video4linux/v4l-subdev2
> > > >
> > > > The i2c device is the adi,adv7482 at address 0x70.
> > >
> > > I'm guessing the fix would be somewhere in this driver, but I haven't
> > > dug into it. Any guesses on which of its suppliers might have a
> > > direct/indirect dependency on an iommu/dma? You could also enable the
> > > debug log in fw_devlink_relax_link() and see if it relaxes any link
> > > where the supplier is an iommu/dma device. That might give us some
> > > hints.
> >
> > After spending way too much time on this looking at
> > drivers/media/v4l2-core, drivers/media/mc and
> > drivers/media/i2c/adv748x/ code, I'm guessing the ordering issue is
> > probably between "csi40:" device and the video-receiver@70 (the
> > "adi,adv7482") device.
> >
> > Based on your points about the sysfs, I was initially digging into
> > drivers/media/i2c/adv748x/adv748x-core.c. But then the parent of
> > video-receiver@70 is an i2c0 that has dmas dependencies. The csi40:
> > (referred to from video-controller) doesn't seem to have any iommu or
> > dmas dependency. So my guess is the csi40 gets probed first and then
> > assumes the video-controller is already available.
> >
> > Can you use this info to take a stab at debugging this further?
>
> Thanks for looking into this, there is indeed a cyclic dependency:
>
>     i2c 0-0070: Fixing up cyclic dependency with feaa0000.csi2
>     i2c 0-0070: Fixing up cyclic dependency with hdmi-in
>     i2c 0-0070: Fixing up cyclic dependency with cvbs-in
>
> > TL;DR is that I think this is some driver issue where it's not
> > checking for one of its suppliers to be ready yet.
>
> Setting fw_devlink_strict to true vs. false seems to influence which of
> two different failures will happen:
>   - rcar-csi2: probe of feaa0000.csi2 failed with error -22
>   - rcar-vin: probe of e6ef5000.video failed with error -22
> The former causes the NULL pointer dereference later.
> The latter existed before, but I hadn't noticed it, and bisection
> led to the real culprit (commit 3e52419ec04f9769 ("media: rcar-{csi2,vin}:
> Move to full Virtual Channel routing per CSI-2 IP").

If you revert that patch, does this series work fine? If yes, are you
happy with giving this a Tested-by?

-Saravana

>
> I am bringing it up with the multi-media guys in
> https://lore.kernel.org/all/20220124124858.571363-4-niklas.soderlund+renesas@ragnatech.se...
>
> Thanks!
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v2 0/9] deferred_probe_timeout logic clean up

[Geert Uytterhoeven]

Hi Saravana,

On Wed, Jun 8, 2022 at 8:13 PM Saravana Kannan <saravanak@google.com> wrote:
> On Wed, Jun 8, 2022 at 3:26 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Jun 8, 2022 at 6:17 AM Saravana Kannan <saravanak@google.com> wrote:
> > > On Tue, Jun 7, 2022 at 5:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > On Tue, Jun 7, 2022 at 11:13 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Wed, Jun 1, 2022 at 12:46 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > > > This series is based on linux-next + these 2 small patches applies on top:
> > > > > > https://lore.kernel.org/lkml/20220526034609.480766-1-saravanak@google.com/
> > > > > >
> > > > > > A lot of the deferred_probe_timeout logic is redundant with
> > > > > > fw_devlink=on.  Also, enabling deferred_probe_timeout by default breaks
> > > > > > a few cases.
> > > > > >
> > > > > > This series tries to delete the redundant logic, simplify the frameworks
> > > > > > that use driver_deferred_probe_check_state(), enable
> > > > > > deferred_probe_timeout=10 by default, and fixes the nfsroot failure
> > > > > > case.
> > > > > >
> > > > > > The overall idea of this series is to replace the global behavior of
> > > > > > driver_deferred_probe_check_state() where all devices give up waiting on
> > > > > > supplier at the same time with a more granular behavior:
> > > > > >
> > > > > > 1. Devices with all their suppliers successfully probed by late_initcall
> > > > > >    probe as usual and avoid unnecessary deferred probe attempts.
> > > > > >
> > > > > > 2. At or after late_initcall, in cases where boot would break because of
> > > > > >    fw_devlink=on being strict about the ordering, we
> > > > > >
> > > > > >    a. Temporarily relax the enforcement to probe any unprobed devices
> > > > > >       that can probe successfully in the current state of the system.
> > > > > >       For example, when we boot with a NFS rootfs and no network device
> > > > > >       has probed.
> > > > > >    b. Go back to enforcing the ordering for any devices that haven't
> > > > > >       probed.
> > > > > >
> > > > > > 3. After deferred probe timeout expires, we permanently give up waiting
> > > > > >    on supplier devices without drivers. At this point, whatever devices
> > > > > >    can probe without some of their optional suppliers end up probing.
> > > > > >
> > > > > > In the case where module support is disabled, it's fairly
> > > > > > straightforward and all device probes are completed before the initcalls
> > > > > > are done.
> > > > > >
> > > > > > Patches 1 to 3 are fairly straightforward and can probably be applied
> > > > > > right away.
> > > > > >
> > > > > > Patches 4 to 6 are for fixing the NFS rootfs issue and setting the
> > > > > > default deferred_probe_timeout back to 10 seconds when modules are
> > > > > > enabled.
> > > > > >
> > > > > > Patches 7 to 9 are further clean up of the deferred_probe_timeout logic
> > > > > > so that no framework has to know/care about deferred_probe_timeout.
> > > > > >
> > > > > > Yoshihiro/Geert,
> > > > > >
> > > > > > If you can test this patch series and confirm that the NFS root case
> > > > > > works, I'd really appreciate that.
> > > > >
> > > > > Thanks, I gave this a try on various boards I have access to.
> > > > > The results were quite positive. E.g. the compile error I saw on v1
> > > > > (implicit declation of fw_devlink_unblock_may_probe(), which is no longer
> > > > >  used in v2) is gone.
> > > >
> > > > Thanks a lot for testing these.
> > > >
> > > > > However, I'm seeing a weird error when userspace (Debian9 nfsroot) is
> > > > > starting:

> > Setting fw_devlink_strict to true vs. false seems to influence which of
> > two different failures will happen:
> >   - rcar-csi2: probe of feaa0000.csi2 failed with error -22
> >   - rcar-vin: probe of e6ef5000.video failed with error -22
> > The former causes the NULL pointer dereference later.
> > The latter existed before, but I hadn't noticed it, and bisection
> > led to the real culprit (commit 3e52419ec04f9769 ("media: rcar-{csi2,vin}:
> > Move to full Virtual Channel routing per CSI-2 IP").
>
> If you revert that patch, does this series work fine? If yes, are you
> happy with giving this a Tested-by?

Sure, sorry for forgetting that ;-)

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/9] deferred_probe_timeout logic clean up

[Saravana Kannan]

On Wed, Jun 8, 2022 at 11:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Wed, Jun 8, 2022 at 8:13 PM Saravana Kannan <saravanak@google.com> wrote:
> > On Wed, Jun 8, 2022 at 3:26 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, Jun 8, 2022 at 6:17 AM Saravana Kannan <saravanak@google.com> wrote:
> > > > On Tue, Jun 7, 2022 at 5:55 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > > On Tue, Jun 7, 2022 at 11:13 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > On Wed, Jun 1, 2022 at 12:46 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > > > > This series is based on linux-next + these 2 small patches applies on top:
> > > > > > > https://lore.kernel.org/lkml/20220526034609.480766-1-saravanak@google.com/
> > > > > > >
> > > > > > > A lot of the deferred_probe_timeout logic is redundant with
> > > > > > > fw_devlink=on.  Also, enabling deferred_probe_timeout by default breaks
> > > > > > > a few cases.
> > > > > > >
> > > > > > > This series tries to delete the redundant logic, simplify the frameworks
> > > > > > > that use driver_deferred_probe_check_state(), enable
> > > > > > > deferred_probe_timeout=10 by default, and fixes the nfsroot failure
> > > > > > > case.
> > > > > > >
> > > > > > > The overall idea of this series is to replace the global behavior of
> > > > > > > driver_deferred_probe_check_state() where all devices give up waiting on
> > > > > > > supplier at the same time with a more granular behavior:
> > > > > > >
> > > > > > > 1. Devices with all their suppliers successfully probed by late_initcall
> > > > > > >    probe as usual and avoid unnecessary deferred probe attempts.
> > > > > > >
> > > > > > > 2. At or after late_initcall, in cases where boot would break because of
> > > > > > >    fw_devlink=on being strict about the ordering, we
> > > > > > >
> > > > > > >    a. Temporarily relax the enforcement to probe any unprobed devices
> > > > > > >       that can probe successfully in the current state of the system.
> > > > > > >       For example, when we boot with a NFS rootfs and no network device
> > > > > > >       has probed.
> > > > > > >    b. Go back to enforcing the ordering for any devices that haven't
> > > > > > >       probed.
> > > > > > >
> > > > > > > 3. After deferred probe timeout expires, we permanently give up waiting
> > > > > > >    on supplier devices without drivers. At this point, whatever devices
> > > > > > >    can probe without some of their optional suppliers end up probing.
> > > > > > >
> > > > > > > In the case where module support is disabled, it's fairly
> > > > > > > straightforward and all device probes are completed before the initcalls
> > > > > > > are done.
> > > > > > >
> > > > > > > Patches 1 to 3 are fairly straightforward and can probably be applied
> > > > > > > right away.
> > > > > > >
> > > > > > > Patches 4 to 6 are for fixing the NFS rootfs issue and setting the
> > > > > > > default deferred_probe_timeout back to 10 seconds when modules are
> > > > > > > enabled.
> > > > > > >
> > > > > > > Patches 7 to 9 are further clean up of the deferred_probe_timeout logic
> > > > > > > so that no framework has to know/care about deferred_probe_timeout.
> > > > > > >
> > > > > > > Yoshihiro/Geert,
> > > > > > >
> > > > > > > If you can test this patch series and confirm that the NFS root case
> > > > > > > works, I'd really appreciate that.
> > > > > >
> > > > > > Thanks, I gave this a try on various boards I have access to.
> > > > > > The results were quite positive. E.g. the compile error I saw on v1
> > > > > > (implicit declation of fw_devlink_unblock_may_probe(), which is no longer
> > > > > >  used in v2) is gone.
> > > > >
> > > > > Thanks a lot for testing these.
> > > > >
> > > > > > However, I'm seeing a weird error when userspace (Debian9 nfsroot) is
> > > > > > starting:
>
> > > Setting fw_devlink_strict to true vs. false seems to influence which of
> > > two different failures will happen:
> > >   - rcar-csi2: probe of feaa0000.csi2 failed with error -22
> > >   - rcar-vin: probe of e6ef5000.video failed with error -22
> > > The former causes the NULL pointer dereference later.
> > > The latter existed before, but I hadn't noticed it, and bisection
> > > led to the real culprit (commit 3e52419ec04f9769 ("media: rcar-{csi2,vin}:
> > > Move to full Virtual Channel routing per CSI-2 IP").
> >
> > If you revert that patch, does this series work fine? If yes, are you
> > happy with giving this a Tested-by?
>
> Sure, sorry for forgetting that ;-)
>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

+few folks who I forgot to add.

Geert,

Thanks for the extensive testing!

Linus W, Ulf, Kevin, Will, Rob, Vladimir,

Can I get your reviews for the deletion of
driver_deferred_probe_check_state() please? We can finally remove it
and have frameworks not needing to know about it.

Greg, Rafael,

Can you review the wait_for_init_devices_probe() patch and the other
trivial driver core changes please?

David/Jakub,

Do the IP4 autoconfig changes look reasonable to you?

Thanks,
Saravana


>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v2 0/9] deferred_probe_timeout logic clean up

[Jakub Kicinski]

On Wed, 8 Jun 2022 14:07:44 -0700 Saravana Kannan wrote:
> David/Jakub,
> 
> Do the IP4 autoconfig changes look reasonable to you?

I'm no expert in this area, I'd trust the opinion of the embedded folks
(adding Florian as well) more than myself. It's unclear to me why we'd
wait_for_init_devices_probe() after the first failed iteration, sleep,
and then allow 11 more iterations with wait_for_device_probe().

Let me also add Thomas since he wrote e2ffe3ff6f5e ("net: ipconfig:
Wait for deferred device probes").

Re: [PATCH v2 0/9] deferred_probe_timeout logic clean up

[Saravana Kannan]

On Wed, Jun 8, 2022 at 3:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 8 Jun 2022 14:07:44 -0700 Saravana Kannan wrote:
> > David/Jakub,
> >
> > Do the IP4 autoconfig changes look reasonable to you?
>
> I'm no expert in this area, I'd trust the opinion of the embedded folks
> (adding Florian as well) more than myself.

Thanks.

> It's unclear to me why we'd
> wait_for_init_devices_probe() after the first failed iteration,

wait_for_init_devices_probe() relaxes ordering rules for all devices
and it's not something we want to do unless we really need it. That's
why we are doing that only if we can't find any network device in the
first iteration.

> sleep,
> and then allow 11 more iterations with wait_for_device_probe().
> Let me also add Thomas since he wrote e2ffe3ff6f5e ("net: ipconfig:
> Wait for deferred device probes").

Even without this change, I'm not sure the wait_for_device_probe()
needs to be within the loop. It's probably sufficient to just do it
once in the beginning, but it's already there and I'm not sure if I'm
missing some scenarios, so I left that part as is.

-Saravana

Re: [PATCH v2 3/9] net: mdio: Delete usage of driver_deferred_probe_check_state()

[Geert Uytterhoeven]

Hi Saravana,

On Wed, Jun 1, 2022 at 2:44 PM Saravana Kannan <saravanak@google.com> wrote:
> Now that fw_devlink=on by default and fw_devlink supports interrupt
> properties, the execution will never get to the point where
> driver_deferred_probe_check_state() is called before the supplier has
> probed successfully or before deferred probe timeout has expired.
>
> So, delete the call and replace it with -ENODEV.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>

Thanks for your patch, which is now commit f8217275b57aa48d ("net:
mdio: Delete usage of driver_deferred_probe_check_state()") in
driver-core/driver-core-next.

Seems like I missed something when providing my T-b for this series,
sorry for that.

arch/arm/boot/dts/r8a7791-koelsch.dts has:

    &ether {
            pinctrl-0 = <&ether_pins>, <&phy1_pins>;
            pinctrl-names = "default";

            phy-handle = <&phy1>;
            renesas,ether-link-active-low;
            status = "okay";

            phy1: ethernet-phy@1 {
                    compatible = "ethernet-phy-id0022.1537",
                                 "ethernet-phy-ieee802.3-c22";
                    reg = <1>;
                    interrupt-parent = <&irqc0>;
                    interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
                    micrel,led-mode = <1>;
                    reset-gpios = <&gpio5 22 GPIO_ACTIVE_LOW>;
            };
    };

Despite the interrupts property, &ether is now probed before irqc0
(interrupt-controller@e61c0000 in arch/arm/boot/dts/r8a7791.dtsi),
causing the PHY not finding its interrupt, and resorting to polling:

    -Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY
driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=185)
    +Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY
driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=POLL)

Reverting this commit, and commit 9cbffc7a59561be9 ("driver core:
Delete driver_deferred_probe_check_state()") fixes that.

> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -47,9 +47,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
>          * just fall back to poll mode
>          */
>         if (rc == -EPROBE_DEFER)
> -               rc = driver_deferred_probe_check_state(&phy->mdio.dev);
> -       if (rc == -EPROBE_DEFER)
> -               return rc;
> +               rc = -ENODEV;
>
>         if (rc > 0) {
>                 phy->irq = rc;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/9] net: mdio: Delete usage of driver_deferred_probe_check_state()

[Saravana Kannan]

On Tue, Jul 5, 2022 at 2:11 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Wed, Jun 1, 2022 at 2:44 PM Saravana Kannan <saravanak@google.com> wrote:
> > Now that fw_devlink=on by default and fw_devlink supports interrupt
> > properties, the execution will never get to the point where
> > driver_deferred_probe_check_state() is called before the supplier has
> > probed successfully or before deferred probe timeout has expired.
> >
> > So, delete the call and replace it with -ENODEV.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> Thanks for your patch, which is now commit f8217275b57aa48d ("net:
> mdio: Delete usage of driver_deferred_probe_check_state()") in
> driver-core/driver-core-next.
>
> Seems like I missed something when providing my T-b for this series,
> sorry for that.

No worries. Appreciate any testing help.

>
> arch/arm/boot/dts/r8a7791-koelsch.dts has:
>
>     &ether {
>             pinctrl-0 = <&ether_pins>, <&phy1_pins>;
>             pinctrl-names = "default";
>
>             phy-handle = <&phy1>;
>             renesas,ether-link-active-low;
>             status = "okay";
>
>             phy1: ethernet-phy@1 {
>                     compatible = "ethernet-phy-id0022.1537",
>                                  "ethernet-phy-ieee802.3-c22";
>                     reg = <1>;
>                     interrupt-parent = <&irqc0>;
>                     interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
>                     micrel,led-mode = <1>;
>                     reset-gpios = <&gpio5 22 GPIO_ACTIVE_LOW>;
>             };
>     };
>
> Despite the interrupts property, &ether is now probed before irqc0
> (interrupt-controller@e61c0000 in arch/arm/boot/dts/r8a7791.dtsi),
> causing the PHY not finding its interrupt, and resorting to polling:

I'd still expect the device link to have been created properly for
this phy device. Could you enable the logging in device_link_add() to
check the link is created between the phy and the IRQ?

My guess is that this probably has something to do with phys being
attached to drivers differently.

>
>     -Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY
> driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=185)
>     +Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY
> driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=POLL)

Can you drop a WARN() where this is printed to get the stack trace to
check my hypothesis?

-Saravana

>
> Reverting this commit, and commit 9cbffc7a59561be9 ("driver core:
> Delete driver_deferred_probe_check_state()") fixes that.
>
> > --- a/drivers/net/mdio/fwnode_mdio.c
> > +++ b/drivers/net/mdio/fwnode_mdio.c
> > @@ -47,9 +47,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
> >          * just fall back to poll mode
> >          */
> >         if (rc == -EPROBE_DEFER)
> > -               rc = driver_deferred_probe_check_state(&phy->mdio.dev);
> > -       if (rc == -EPROBE_DEFER)
> > -               return rc;
> > +               rc = -ENODEV;
> >
> >         if (rc > 0) {
> >                 phy->irq = rc;
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v2 3/9] net: mdio: Delete usage of driver_deferred_probe_check_state()

[Geert Uytterhoeven]

Hi Saravana,

On Wed, Jul 13, 2022 at 3:40 AM Saravana Kannan <saravanak@google.com> wrote:
> On Tue, Jul 5, 2022 at 2:11 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Jun 1, 2022 at 2:44 PM Saravana Kannan <saravanak@google.com> wrote:
> > > Now that fw_devlink=on by default and fw_devlink supports interrupt
> > > properties, the execution will never get to the point where
> > > driver_deferred_probe_check_state() is called before the supplier has
> > > probed successfully or before deferred probe timeout has expired.
> > >
> > > So, delete the call and replace it with -ENODEV.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> >
> > Thanks for your patch, which is now commit f8217275b57aa48d ("net:
> > mdio: Delete usage of driver_deferred_probe_check_state()") in
> > driver-core/driver-core-next.
> >
> > Seems like I missed something when providing my T-b for this series,
> > sorry for that.
>
> > arch/arm/boot/dts/r8a7791-koelsch.dts has:
> >
> >     &ether {
> >             pinctrl-0 = <&ether_pins>, <&phy1_pins>;
> >             pinctrl-names = "default";
> >
> >             phy-handle = <&phy1>;
> >             renesas,ether-link-active-low;
> >             status = "okay";
> >
> >             phy1: ethernet-phy@1 {
> >                     compatible = "ethernet-phy-id0022.1537",
> >                                  "ethernet-phy-ieee802.3-c22";
> >                     reg = <1>;
> >                     interrupt-parent = <&irqc0>;
> >                     interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> >                     micrel,led-mode = <1>;
> >                     reset-gpios = <&gpio5 22 GPIO_ACTIVE_LOW>;
> >             };
> >     };
> >
> > Despite the interrupts property, &ether is now probed before irqc0
> > (interrupt-controller@e61c0000 in arch/arm/boot/dts/r8a7791.dtsi),
> > causing the PHY not finding its interrupt, and resorting to polling:
>
> I'd still expect the device link to have been created properly for
> this phy device. Could you enable the logging in device_link_add() to
> check the link is created between the phy and the IRQ?
>
> My guess is that this probably has something to do with phys being
> attached to drivers differently.

Comparison of dmesg before/after enabling debugging, for
related nodes:

    +interrupt-controller@e61c0000 Linked as a fwnode consumer to
clock-controller@e6150000

    +pmic@58 Linked as a fwnode consumer to interrupt-controller@e61c0000
    +regulator@68 Linked as a fwnode consumer to interrupt-controller@e61c0000

Other user of irqc

    +ethernet@ee700000 Linked as a fwnode consumer to clock-controller@e6150000
    +ethernet@ee700000 Linked as a fwnode consumer to pinctrl@e6060000
    +ethernet-phy@1 Linked as a fwnode consumer to interrupt-controller@e61c0000
    +ethernet-phy@1 Linked as a fwnode consumer to gpio@e6055000

PHY linked correctly to consumers

    +device: 'e61c0000.interrupt-controller': device_add
    +device: 'platform:e6150000.clock-controller--platform:e61c0000.interrupt-controller':
device_add
    +devices_kset: Moving e61c0000.interrupt-controller to end of list
    +platform e61c0000.interrupt-controller: Linked as a consumer to
e6150000.clock-controller
    +interrupt-controller@e61c0000 Dropping the fwnode link to
clock-controller@e6150000
    +platform e61c0000.interrupt-controller: error -EPROBE_DEFER:
supplier e6150000.clock-controller not ready

Tried to probe irqc (why? consumer not ready), deferred.

    +device: 'platform:e61c0000.interrupt-controller--platform:e60b0000.i2c':
device_add
    +platform e60b0000.i2c: Linked as a sync state only consumer to
e61c0000.interrupt-controller

I guess sync state means through other (child) consumers (pmic,
regulator) above?

    +device: 'ee700000.ethernet': device_add
    +device: 'platform:e6060000.pinctrl--platform:ee700000.ethernet': device_add
    +devices_kset: Moving ee700000.ethernet to end of list
    +platform ee700000.ethernet: Linked as a consumer to e6060000.pinctrl
    +ethernet@ee700000 Dropping the fwnode link to pinctrl@e6060000
    +device: 'platform:e6150000.clock-controller--platform:ee700000.ethernet':
device_add
    +devices_kset: Moving ee700000.ethernet to end of list
    +platform ee700000.ethernet: Linked as a consumer to
e6150000.clock-controller
    +ethernet@ee700000 Dropping the fwnode link to clock-controller@e6150000
    +device: 'platform:e6055000.gpio--platform:ee700000.ethernet': device_add
    +platform ee700000.ethernet: Linked as a sync state only consumer
to e6055000.gpio
    +device: 'platform:e61c0000.interrupt-controller--platform:ee700000.ethernet':
device_add
    +platform ee700000.ethernet: Linked as a sync state only consumer
to e61c0000.interrupt-controller

Hence linking ethernet to child (phy) consumers.

    +device: 'ee700000.ethernet-ffffffff': device_add

Probing ethernet...

     libphy: fwnode_get_phy_id: fwnode
/soc/ethernet@ee700000/ethernet-phy@1 phy_id = 0x00221537
     libphy: fwnode_get_phy_id: fwnode
/soc/ethernet@ee700000/ethernet-phy@1 phy_id = 0x00221537
    +fwnode_mdiobus_phy_device_register: fwnode_irq_get() returned -517
    +fwnode_mdiobus_phy_device_register: ignoring -EPROBE_DEFER

This is the part that got changed by this patch.

    +device: 'ee700000.ethernet-ffffffff:01': device_add
    +device: 'platform:e6055000.gpio--mdio_bus:ee700000.ethernet-ffffffff:01':
device_add
    +devices_kset: Moving ee700000.ethernet-ffffffff:01 to end of list
    +mdio_bus ee700000.ethernet-ffffffff:01: Linked as a consumer to
e6055000.gpio
    +ethernet-phy@1 Dropping the fwnode link to gpio@e6055000
    +device: 'platform:e61c0000.interrupt-controller--mdio_bus:ee700000.ethernet-ffffffff:01':
device_add
    +devices_kset: Moving ee700000.ethernet-ffffffff:01 to end of list
    +mdio_bus ee700000.ethernet-ffffffff:01: Linked as a consumer to
e61c0000.interrupt-controller
    +ethernet-phy@1 Dropping the fwnode link to interrupt-controller@e61c0000
    +mdio_bus ee700000.ethernet-ffffffff:01: error -EPROBE_DEFER:
supplier e61c0000.interrupt-controller not ready

Why was ethernet probed this early?
We knew the supplier of the phy was still missing?

    +device: 'eth1': device_add
     sh-eth ee700000.ethernet eth1: Base address at 0xee700000,
2e:09:0a:00:6d:85, IRQ 104.
    +sh-eth ee700000.ethernet: Dropping the link to e6055000.gpio
    +device: 'platform:e6055000.gpio--platform:ee700000.ethernet':
device_unregister
    +sh-eth ee700000.ethernet: Dropping the link to
e61c0000.interrupt-controller
    +device: 'platform:e61c0000.interrupt-controller--platform:ee700000.ethernet':
device_unregister

    +devices_kset: Moving e61c0000.interrupt-controller to end of list
    +devices_kset: Moving ee700000.ethernet-ffffffff:01 to end of list
     renesas_irqc e61c0000.interrupt-controller: driving 10 irqs

Finally, irqc is probed.

    +device: '6-0058': device_add
    +device: 'platform:e61c0000.interrupt-controller--i2c:6-0058': device_add
    +devices_kset: Moving 6-0058 to end of list
    +i2c 6-0058: Linked as a consumer to e61c0000.interrupt-controller
    +pmic@58 Dropping the fwnode link to interrupt-controller@e61c0000

    +device: '6-0068': device_add
    +device: 'platform:e61c0000.interrupt-controller--i2c:6-0068': device_add
    +devices_kset: Moving 6-0068 to end of list
    +i2c 6-0068: Linked as a consumer to e61c0000.interrupt-controller
    +regulator@68 Dropping the fwnode link to interrupt-controller@e61c0000

Propagating other irqc suppliers to the parent of their consumers

    +i2c-sh_mobile e60b0000.i2c: Dropping the link to
e61c0000.interrupt-controller
    +device: 'platform:e61c0000.interrupt-controller--platform:e60b0000.i2c':
device_unregister

    +devices_kset: Moving ee700000.ethernet-ffffffff:01 to end of list

     Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY
driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=POLL)
     sh-eth ee700000.ethernet eth1: Link is Up - 100Mbps/Full - flow control off
     Sending DHCP requests ., OK

> >     -Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY
> > driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=185)
> >     +Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY
> > driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=POLL)
>
> Can you drop a WARN() where this is printed to get the stack trace to
> check my hypothesis?

That didn't help much, as this is the messenger, not the cause.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 6/9] Revert "driver core: Set default deferred_probe_timeout back to 0."

[Geert Uytterhoeven]

Hi Saravana,

On Wed, Jun 1, 2022 at 9:45 AM Saravana Kannan <saravanak@google.com> wrote:
> This reverts commit 11f7e7ef553b6b93ac1aa74a3c2011b9cc8aeb61.
>
> Let's take another shot at getting deferred_probe_timeout=10 to work.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>

Thanks for your patch, which is now commit f516d01b9df2782b
("Revert "driver core: Set default deferred_probe_timeout
back to 0."") in driver-core/driver-core-next.

Wolfram found an issue on a Renesas board where disabling the IOMMU
driver (CONFIG_IPMMU_VMSA=n) causes the system to fail to boot,
and bisected this to a merge of driver-core/driver-core-next.
After some trials, I managed to reproduce the issue, and bisected it
further to commit f516d01b9df2782b.

The affected config has:
    CONFIG_MODULES=y
    CONFIG_RCAR_DMAC=y
    CONFIG_IPMMU_VMSA=n

In arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb,
e6e88000.serial links to a dmac, and the dmac links to an iommu,
for which no driver is available.

Playing with deferred_probe_timeout values doesn't help.

However, the above options do not seem to be sufficient to trigger
the issue, as I had other configs with those three options that do
boot fine.

After bisecting configs, I found the culprit: CONFIG_IP_PNP.
As Wolfram was using an initramfs, CONFIG_IP_PNP was not needed.
If CONFIG_IP_PNP=n, booting fails.
If CONFIG_IP_PNP=y, booting succeeds.
In fact, just disabling late_initcall(ip_auto_config) makes it fail,
too.
Reducing ip_auto_config(), it turns out the call to
wait_for_init_devices_probe() is what is needed to unblock booting.

So I guess wait_for_init_devices_probe() needs to be called (where?)
if CONFIG_IP_PNP=n, too?

> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -256,7 +256,12 @@ static int deferred_devs_show(struct seq_file *s, void *data)
>  }
>  DEFINE_SHOW_ATTRIBUTE(deferred_devs);
>
> +#ifdef CONFIG_MODULES
> +int driver_deferred_probe_timeout = 10;
> +#else
>  int driver_deferred_probe_timeout;
> +#endif
> +
>  EXPORT_SYMBOL_GPL(driver_deferred_probe_timeout);
>
>  static int __init deferred_probe_timeout_setup(char *str)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 6/9] Revert "driver core: Set default deferred_probe_timeout back to 0."

[Saravana Kannan]

On Wed, Jul 20, 2022 at 10:31 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Wed, Jun 1, 2022 at 9:45 AM Saravana Kannan <saravanak@google.com> wrote:
> > This reverts commit 11f7e7ef553b6b93ac1aa74a3c2011b9cc8aeb61.
> >
> > Let's take another shot at getting deferred_probe_timeout=10 to work.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> Thanks for your patch, which is now commit f516d01b9df2782b
> ("Revert "driver core: Set default deferred_probe_timeout
> back to 0."") in driver-core/driver-core-next.
>
> Wolfram found an issue on a Renesas board where disabling the IOMMU
> driver (CONFIG_IPMMU_VMSA=n) causes the system to fail to boot,
> and bisected this to a merge of driver-core/driver-core-next.
> After some trials, I managed to reproduce the issue, and bisected it
> further to commit f516d01b9df2782b.
>
> The affected config has:
>     CONFIG_MODULES=y
>     CONFIG_RCAR_DMAC=y
>     CONFIG_IPMMU_VMSA=n
>
> In arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb,
> e6e88000.serial links to a dmac, and the dmac links to an iommu,
> for which no driver is available.

Thanks for digging into this and giving more details.

Is e6e88000.serial being blocked the reason for the boot failure?

If so, can you give this a shot?
https://lore.kernel.org/lkml/20220701012647.2007122-1-saravanak@google.com/

> Playing with deferred_probe_timeout values doesn't help.

This part is strange though. If you set deferred_probe_timeout=1,
fw_devlink will stop blocking all probes 1 second after
late_initcall()s finish. So, similar to the ip autoconfig issue, is
the issue caused by something that needs to be finished before we hit
late_initcall() but is getting blocked?

> However, the above options do not seem to be sufficient to trigger
> the issue, as I had other configs with those three options that do
> boot fine.
>
> After bisecting configs, I found the culprit: CONFIG_IP_PNP.
> As Wolfram was using an initramfs, CONFIG_IP_PNP was not needed.
> If CONFIG_IP_PNP=n, booting fails.
> If CONFIG_IP_PNP=y, booting succeeds.
> In fact, just disabling late_initcall(ip_auto_config) makes it fail,
> too.
> Reducing ip_auto_config(), it turns out the call to
> wait_for_init_devices_probe() is what is needed to unblock booting.
>
> So I guess wait_for_init_devices_probe() needs to be called (where?)
> if CONFIG_IP_PNP=n, too?

That function just unblocks all devices and allows them to try and
probe and then waits for all possible probes to finish before
returning. They problem with call it randomly/every time is that it
breaks functionality where an optional supplier will probe after a few
modules are loaded in the future.

I guess one possible issue with the timeout not helping is that once
the timeout expires, things are still being probed and nothing is
being blocked till they finish probing.

I'm trying to have the default config (in terms of fw_devlink,
deferred probe behavior, timeouts, etc) be the same between a fully
static kernel (but CONFIG_MODULES still enabled) and a fully modular
kernel (like GKI). But it might end up being an untenable problem.

I'll wait to see what specifically is the issue in this case and then
I'll go from there.

-Saravana

> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -256,7 +256,12 @@ static int deferred_devs_show(struct seq_file *s, void *data)
> >  }
> >  DEFINE_SHOW_ATTRIBUTE(deferred_devs);
> >
> > +#ifdef CONFIG_MODULES
> > +int driver_deferred_probe_timeout = 10;
> > +#else
> >  int driver_deferred_probe_timeout;
> > +#endif
> > +
> >  EXPORT_SYMBOL_GPL(driver_deferred_probe_timeout);
> >
> >  static int __init deferred_probe_timeout_setup(char *str)
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH v2 6/9] Revert "driver core: Set default deferred_probe_timeout back to 0."

[Geert Uytterhoeven]

Hi Saravana,

On Wed, Jul 20, 2022 at 9:02 PM Saravana Kannan <saravanak@google.com> wrote:
> On Wed, Jul 20, 2022 at 10:31 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Wed, Jun 1, 2022 at 9:45 AM Saravana Kannan <saravanak@google.com> wrote:
> > > This reverts commit 11f7e7ef553b6b93ac1aa74a3c2011b9cc8aeb61.
> > >
> > > Let's take another shot at getting deferred_probe_timeout=10 to work.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> >
> > Thanks for your patch, which is now commit f516d01b9df2782b
> > ("Revert "driver core: Set default deferred_probe_timeout
> > back to 0."") in driver-core/driver-core-next.
> >
> > Wolfram found an issue on a Renesas board where disabling the IOMMU
> > driver (CONFIG_IPMMU_VMSA=n) causes the system to fail to boot,
> > and bisected this to a merge of driver-core/driver-core-next.
> > After some trials, I managed to reproduce the issue, and bisected it
> > further to commit f516d01b9df2782b.
> >
> > The affected config has:
> >     CONFIG_MODULES=y
> >     CONFIG_RCAR_DMAC=y
> >     CONFIG_IPMMU_VMSA=n
> >
> > In arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dtb,
> > e6e88000.serial links to a dmac, and the dmac links to an iommu,
> > for which no driver is available.
>
> Thanks for digging into this and giving more details.
>
> Is e6e88000.serial being blocked the reason for the boot failure?

It doesn't seem to be.

> If so, can you give this a shot?
> https://lore.kernel.org/lkml/20220701012647.2007122-1-saravanak@google.com/

Thanks, but it doesn't make a difference.

> > After bisecting configs, I found the culprit: CONFIG_IP_PNP.
> > As Wolfram was using an initramfs, CONFIG_IP_PNP was not needed.
> > If CONFIG_IP_PNP=n, booting fails.
> > If CONFIG_IP_PNP=y, booting succeeds.
> > In fact, just disabling late_initcall(ip_auto_config) makes it fail,
> > too.
> > Reducing ip_auto_config(), it turns out the call to
> > wait_for_init_devices_probe() is what is needed to unblock booting.
> >
> > So I guess wait_for_init_devices_probe() needs to be called (where?)
> > if CONFIG_IP_PNP=n, too?
>
> That function just unblocks all devices and allows them to try and
> probe and then waits for all possible probes to finish before
> returning. They problem with call it randomly/every time is that it
> breaks functionality where an optional supplier will probe after a few
> modules are loaded in the future.
>
> I guess one possible issue with the timeout not helping is that once
> the timeout expires, things are still being probed and nothing is
> being blocked till they finish probing.

I'm not sure that it's a device that's missing.

Calling wait_for_init_devices_probe() or not changes lots of little
things in the probing order. But when comparing the sorted boot logs,
there does not seem to be any difference in the list of devices that
was probed successfully.
It looks like the system is just blocked on something else?...

I tried getting a list of all locks held using Magic SysRq + d,
but Magic SysRq on the serial console does not work at this point
(it does work in the booted kernel with CONFIG_IP_PNP=y).


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/9] net: mdio: Delete usage of driver_deferred_probe_check_state()

[Geert Uytterhoeven]

Hi Saravana,

On Tue, Jul 5, 2022 at 11:11 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Jun 1, 2022 at 2:44 PM Saravana Kannan <saravanak@google.com> wrote:
> > Now that fw_devlink=on by default and fw_devlink supports interrupt
> > properties, the execution will never get to the point where
> > driver_deferred_probe_check_state() is called before the supplier has
> > probed successfully or before deferred probe timeout has expired.
> >
> > So, delete the call and replace it with -ENODEV.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> Thanks for your patch, which is now commit f8217275b57aa48d ("net:
> mdio: Delete usage of driver_deferred_probe_check_state()") in
> driver-core/driver-core-next.
>
> Seems like I missed something when providing my T-b for this series,
> sorry for that.
>
> arch/arm/boot/dts/r8a7791-koelsch.dts has:
>
>     &ether {
>             pinctrl-0 = <&ether_pins>, <&phy1_pins>;
>             pinctrl-names = "default";
>
>             phy-handle = <&phy1>;
>             renesas,ether-link-active-low;
>             status = "okay";
>
>             phy1: ethernet-phy@1 {
>                     compatible = "ethernet-phy-id0022.1537",
>                                  "ethernet-phy-ieee802.3-c22";
>                     reg = <1>;
>                     interrupt-parent = <&irqc0>;
>                     interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
>                     micrel,led-mode = <1>;
>                     reset-gpios = <&gpio5 22 GPIO_ACTIVE_LOW>;
>             };
>     };
>
> Despite the interrupts property, &ether is now probed before irqc0
> (interrupt-controller@e61c0000 in arch/arm/boot/dts/r8a7791.dtsi),
> causing the PHY not finding its interrupt, and resorting to polling:
>
>     -Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY
> driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=185)
>     +Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY
> driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=POLL)
>
> Reverting this commit, and commit 9cbffc7a59561be9 ("driver core:
> Delete driver_deferred_probe_check_state()") fixes that.

FTR, this issue is now present in v6.0-rc1.
I haven't tried your newest series yet.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds