Re: [PATCH] Revert "of: property: fw_devlink: Add support for "phy-handle" property"

[Saravana Kannan]

On Fri, Sep 17, 2021 at 9:59 AM Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
>
> On 9/16/21 7:27 PM, Saravana Kannan wrote:
> > On Thu, Sep 16, 2021 at 7:21 PM Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 9/15/2021 1:19 AM, Saravana Kannan wrote:
> >>> This reverts commit cf4b94c8530d14017fbddae26aad064ddc42edd4.
> >>>
> >>> Some PHYs pointed to by "phy-handle" will never bind to a driver until a
> >>> consumer attaches to it. And when the consumer attaches to it, they get
> >>> forcefully bound to a generic PHY driver. In such cases, parsing the
> >>> phy-handle property and creating a device link will prevent the consumer
> >>> from ever probing. We don't want that. So revert support for
> >>> "phy-handle" property until we come up with a better mechanism for
> >>> binding PHYs to generic drivers before a consumer tries to attach to it.
> >>>
> >>> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> >>
> >> Thanks for getting this revert submitted, I just ran a bisection this
> >> afternoon that pointed to this offending commit. It would cause the dead
> >> lock
> >
> > Dead lock in the kernel? Or do you mean just a hang waiting forever for network?
>
> It locks up since we try to acquire __device_driver_lock() while we are
> in the main driver's probe function.

Off the top of my head that sounds weird, but I'll look into the
logs/code later. Bunch of other stuff and some LPC prep comes first :)

>
> >
> >> on boot with drivers/net/dsa/bcm_sf2.c pasted below.
> >
> > The log is too jumbled up to be readable (word wrap I suppose) and
> > maybe even multiple thread printing at the same time.
>
> Re-attached (thunderbird is not really helping me).

Thanks!

>
> >
> >> Saravana, can
> >> you CC on me on your fix or what you would want me to be testing?
> >
> > By fix, I assume you mean when I bring back phy-handle with a proper
> > fix to handle the case in the commit text? Yeah, that's going to take
> > a while. It's brewing in my head and there are some issues that's not
> > fully resolved. But I haven't had time to code it up or dig into the
> > net code to make sure it'll work. But yes, I'll CC you when I do so
> > you can test it with this case.
>
> Well by fix, I meant something that does not lock up on my system,

Hold on. Now I'm confused. Are you still hitting hangs/issues after
the phy-handle patch is reverted?

> it is
> a different problem from supporting 'phy-handle', but it should not
> regress an existing system, no matter how quirky that system behaves in
> its probe function. For history and reference, the "offending" change
> and background can be found here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=771089c2a485958e423f305e974303760167b45c

So that is the change that's interacting with the phy-handle patch to
cause the deadlock?

I'm a bit confused on what needs debugging now.

-Saravana

>
> Thanks for your patience working on the quirky MDIO/PHY subsystem :)

No worries. Thanks for your patience with me accidentally breaking stuff :)

-Saravana