Re: [PATCH] driver core: Prevent deferred probe loops

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Jun 19, 2025 at 12:19:23PM -0400, Sean Anderson wrote:
> On 6/19/25 04:21, Greg Kroah-Hartman wrote:
> > On Tue, Jun 17, 2025 at 01:14:31PM -0400, Sean Anderson wrote:
> >> On 6/17/25 11:49, Greg Kroah-Hartman wrote:
> >> > On Tue, Jun 17, 2025 at 11:35:04AM -0400, Sean Anderson wrote:
> >> >> On 6/17/25 04:50, Greg Kroah-Hartman wrote:
> >> >> > On Thu, Jun 12, 2025 at 04:40:48PM -0400, Sean Anderson wrote:
> >> >> >> On 6/12/25 13:56, Saravana Kannan wrote:
> >> >> >> > On Thu, Jun 12, 2025 at 8:53 AM Sean Anderson <sean.anderson@linux.dev> wrote:
> >> >> >> >>
> >> >> >> >> On 6/11/25 08:23, Greg Kroah-Hartman wrote:
> >> >> >> >> > On Tue, Jun 10, 2025 at 07:44:27PM -0400, Sean Anderson wrote:
> >> >> >> >> >> On 6/10/25 19:32, Saravana Kannan wrote:
> >> >> >> >> >> > On Tue, Jun 10, 2025 at 11:35 AM Sean Anderson <sean.anderson@linux.dev> wrote:
> >> >> >> >> >> >>
> >> >> >> >> >> >> A deferred probe loop can occur when a device returns EPROBE_DEFER after
> >> >> >> >> >> >> registering a bus with children:
> >> >> >> >> >> >
> >> >> >> >> >> > This is a broken driver. A parent device shouldn't register child
> >> >> >> >> >> > devices unless it is fully read itself. It's not logical to say the
> >> >> >> >> >> > child devices are available, if the parent itself isn't fully ready.
> >> >> >> >> >> > So, adding child devices/the bus should be the last thing done in the
> >> >> >> >> >> > parent's probe function.
> >> >> >> >> >> >
> >> >> >> >> >> > I know there are odd exceptions where the parent depends on the child,
> >> >> >> >> >> > so they might add the child a bit earlier in the probe
> >> >> >> >> >>
> >> >> >> >> >> This is exactly the case here. So the bus probing cannot happen any
> >> >> >> >> >> later than it already does.
> >> >> >> >> >
> >> >> >> >> > Please fix the driver not to do this.
> >> >> >> >>
> >> >> >> >> How? The driver needs the PCS to work. And the PCS can live on the MDIO
> >> >> >> >> bus.
> >> >> >> > 
> >> >> >> > Obviously I don't know the full details, but you could implement it as
> >> >> >> > MFD. So the bus part would not get removed even if the PCS fails to
> >> >> >> > probe. Then the PCS can probe when whatever it needs ends up probing.
> >> >> >> 
> >> >> >> I was thinking about making the MDIO bus a separate device. But I think
> >> >> >> it will be tricky to get suspend/resume working correctly. And this
> >> >> >> makes conversions more difficult because you cannot just add some
> >> >> >> pcs_get/pcs_put calls, you have to split out the MDIO bus too (which is
> >> >> >> invariably created as a child of the MAC).
> >> >> >> 
> >> >> >> And what happens if a developer doesn't realize they have to split off
> >> >> >> the MDIO bus before converting? Everything works fine, except if there
> >> >> >> is some problem loading the PCS driver, which they may not test. Is this
> >> >> >> prohibition against failing after creating a bus documented anywhere? I
> >> >> >> don't recall seeing it...
> >> >> > 
> >> >> > What do you mean "failing after creating a bus"?  If a bus is failed to
> >> >> > be created, you fail like normal, no difference here.
> >> >> 
> >> >> Creating the bus is successful, but there's an EPROBE_DEFER failure after
> >> >> that. Which induces the probe loop as described in my initial email.
> >> > 
> >> > Then don't allow a defer to happen :)
> >> 
> >> Well, I could require all PCS drivers to be built-in I guess. But I suspect
> >> users will want them to be modules to reduce kernel size.
> > 
> > True, then just auto-load them as needed like all other busses do.
> > 
> >> > Or better yet, just succeed and spin up a new thread for the new bus to
> >> > attach it's devices to.  That's what many other busses do today.
> >> 
> >> Sorry, I'm not sure I follow. How can you attach a device to a thread? Do
> >> you have an example for this?
> > 
> > Busses discover their devices in a thread, which then calls probe for
> > them when needed.  A device isn't being attached to a thread, sorry for
> > the confusion.
> 
> OK, just to clarify, the subsystem I linked above is not a bus, it's an
> internal API. Think GPIO or PWM. The devices typically live on an MDIO
> bus (although a platform bus wouldn't be out of the question). So it's
> "not our job" to load the module; that should be done by the bus. From
> our perspective, if we look up a device and it's not there we don't
> really know if it's ever going to show up.

That's fine, it will show up when it shows up, don't wait around for it :)

> Regarding the auxiliary bus, I tried it out and it works. The conversion
> is around +50 lines, which is not ideal. Ideally I would like to push
> complexity into subsystem code rather than making drivers deal with it,
> but I don't really see a good way to do this in the subsystem. There are
> just a lot of assumptions along the line of "when you register the
> device you must know what capabilities it supports." For example, fixed
> links (MAC to MAC) are validated when the phylink is created and the
> whole process fails if there's an incompatibility. Which can occur if
> the late-binding component is the thing that adds support for the fixed
> link.

50+ lines is not much, let's see the patch!

thanks,

greg k-h