Deferred probe loop with child devices

A deferred probe loop can occur when a device returns EPROBE_DEFER after registering a bus with children:

deferred_probe_work_func()
driver_probe_device(parent)

    test_parent_probe(parent)
      device_add(child)
        (probe successful)
        driver_bound(child)
          driver_deferred_probe_trigger()
      return -EPROBE_DEFER
    driver_deferred_probe_add(parent)
    // deferred_trigger_count changed, so...
    driver_deferred_probe_trigger()

Because there was another successful probe during the parent's probe, driver_probe_device thinks we need to retry the whole probe process. But we will never make progress this way because the only thing that changed was a direct result of our own probe function.

To prevent this, add a per-device trigger_count. This allows us to determine if the global deferred_trigger_count was modified by some unrelated device or only by our own children. The read side does the work of summing children because I expect most deferred devices to be childless. The alternative is to walk up the device's parents in driver_deferred_probe_trigger.

Signed-off-by Sean Anderson <[email protected]>

---

 drivers/base/base.h    |  2 +-
 drivers/base/core.c    |  8 ++++-
 drivers/base/dd.c      | 67 ++++++++++++++++++++++++++++++++++--------
 include/linux/device.h |  3 ++
 4 files changed, 66 insertions(+), 14 deletions(-)

drivers/base/base.h +1 -1

index 123031a757d9..54263b186d1f 100644
@@ -201,7 +201,7 @@ int devres_release_all(struct device *dev);
 void device_block_probing(void);
 void device_unblock_probing(void);
 void deferred_probe_extend_timeout(void);
-void driver_deferred_probe_trigger(void);
+void driver_deferred_probe_trigger(struct device *dev);
 const char *device_get_devnode(const struct device *dev, umode_t *mode,
                               kuid_t *uid, kgid_t *gid, const char **tmp);
 

drivers/base/core.c +7 -1

index cbc0099d8ef2..8ba231ec469b 100644
@@ -1858,7 +1858,7 @@ void __init wait_for_init_devices_probe(void)
 
        pr_info("Trying to probe devices needed for running init ...\n");
        fw_devlink_best_effort = true;
-       driver_deferred_probe_trigger();
+       driver_deferred_probe_trigger(NULL);
 
        /*
         * Wait for all "best effort" probes to finish before going back to

@@ -3739,6 +3739,9 @@ int device_add(struct device *dev)
        kobject_uevent(&dev->kobj, KOBJ_REMOVE);
        glue_dir = get_glue_dir(dev);
        kobject_del(&dev->kobj);
+       if (parent)
+               atomic_add(atomic_read(&dev->trigger_count),
+                          &dev->parent->trigger_count);
  Error:
        cleanup_glue_dir(dev, glue_dir);
 parent_error:

@@ -3899,6 +3902,9 @@ void device_del(struct device *dev)
        kobject_uevent(&dev->kobj, KOBJ_REMOVE);
        glue_dir = get_glue_dir(dev);
        kobject_del(&dev->kobj);
+       if (parent)
+               atomic_add(atomic_read(&dev->trigger_count),
+                          &parent->trigger_count);
        cleanup_glue_dir(dev, glue_dir);
        memalloc_noio_restore(noio_flag);
        put_device(parent);

drivers/base/dd.c +55 -12

index b526e0e0f52d..8ce638c02275 100644
@@ -156,6 +156,7 @@ void driver_deferred_probe_del(struct device *dev)
 static bool driver_deferred_probe_enable;
 /**
  * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
+ * @dev: the successfully-bound device, or %NULL if not applicable
  *
  * This functions moves all devices from the pending list to the active
  * list and schedules the deferred probe workqueue to process them.  It

@@ -172,7 +173,7 @@ static bool driver_deferred_probe_enable;
  * changes in the midst of a probe, then deferred processing should be triggered
  * again.
  */
-void driver_deferred_probe_trigger(void)
+void driver_deferred_probe_trigger(struct device *dev)
 {
        if (!driver_deferred_probe_enable)
                return;

@@ -184,6 +185,10 @@ void driver_deferred_probe_trigger(void)
         */
        mutex_lock(&deferred_probe_mutex);
        atomic_inc(&deferred_trigger_count);
+       if (dev) {
+               smp_wmb(); /* paired with device_needs_retrigger */
+               atomic_inc(&dev->trigger_count);
+       }
        list_splice_tail_init(&deferred_probe_pending_list,
                              &deferred_probe_active_list);
        mutex_unlock(&deferred_probe_mutex);

@@ -216,7 +221,7 @@ void device_block_probing(void)
 void device_unblock_probing(void)
 {
        defer_all_probes = false;
-       driver_deferred_probe_trigger();
+       driver_deferred_probe_trigger(NULL);
 }
 
 /**

@@ -308,7 +313,7 @@ static void deferred_probe_timeout_work_func(struct work_struct *work)
        fw_devlink_drivers_done();
 
        driver_deferred_probe_timeout = 0;
-       driver_deferred_probe_trigger();
+       driver_deferred_probe_trigger(NULL);
        flush_work(&deferred_probe_work);
 
        mutex_lock(&deferred_probe_mutex);

@@ -347,7 +352,7 @@ static int deferred_probe_initcall(void)
                            &deferred_devs_fops);
 
        driver_deferred_probe_enable = true;
-       driver_deferred_probe_trigger();
+       driver_deferred_probe_trigger(NULL);
        /* Sort as many dependencies as possible before exiting initcalls */
        flush_work(&deferred_probe_work);
        initcalls_done = true;

@@ -359,7 +364,7 @@ static int deferred_probe_initcall(void)
         * Trigger deferred probe again, this time we won't defer anything
         * that is optional
         */
-       driver_deferred_probe_trigger();
+       driver_deferred_probe_trigger(NULL);
        flush_work(&deferred_probe_work);
 
        if (driver_deferred_probe_timeout > 0) {

@@ -415,7 +420,7 @@ static void driver_bound(struct device *dev)
         * kick off retrying all pending devices
         */
        driver_deferred_probe_del(dev);
-       driver_deferred_probe_trigger();
+       driver_deferred_probe_trigger(dev);
 
        bus_notify(dev, BUS_NOTIFY_BOUND_DRIVER);
        kobject_uevent(&dev->kobj, KOBJ_BIND);

@@ -806,6 +811,47 @@ static int __driver_probe_device(const struct device_driver *drv, struct device
        return ret;
 }
 
+/**
+ * dev_get_trigger_count() - Recursively read trigger_count
+ * @dev: device to read from
+ * @data: pointer to the int result; should be initialized to 0
+ *
+ * Read @dev's trigger_count, as well as all its children's trigger counts,
+ * recursively. The result is the number of times @dev or any of its
+ * (possibly-removed) children have been successfully probed.
+ *
+ * Return: 0
+ */
+static int dev_get_trigger_count(struct device *dev, void *data)
+{
+       *(int *)data += atomic_read(&dev->trigger_count);
+       return device_for_each_child(dev, dev_get_trigger_count, data);
+}
+
+/*
+ * device_needs_retrigger() - Determine if we need to re-trigger a deferred probe
+ * @dev: Device that failed to probe with %EPROBE_DEFER
+ * @old_trigger_count: Value of deferred_trigger_count before probing the device
+ *
+ * The resource @dev was looking for could have been probed between when @dev
+ * looked up the resource and when the probe process finished. If this occurred
+ * we need to retrigger deferred probing so that @dev gets another shot at
+ * probing. However, we need to ignore deferred probe triggers from @dev's own
+ * children, since that could result in an infinite probe loop.
+ *
+ * Return: %true if we should retrigger probing of deferred devices
+ */
+static bool device_needs_retrigger(struct device *dev, int old_trigger_count)
+{
+       int dev_trigger_count = 0;
+       int new_trigger_count;
+
+       dev_get_trigger_count(dev, &dev_trigger_count);
+       smp_rmb(); /* paired with driver_deferred_probe_trigger */
+       new_trigger_count = atomic_read(&deferred_trigger_count);
+       return new_trigger_count > old_trigger_count + dev_trigger_count;
+}
+
 /**
  * driver_probe_device - attempt to bind device & driver together
  * @drv: driver to bind a device to

@@ -830,12 +876,9 @@ static int driver_probe_device(const struct device_driver *drv, struct device *d
        if (ret == -EPROBE_DEFER || ret == EPROBE_DEFER) {
                driver_deferred_probe_add(dev);
 
-               /*
-                * Did a trigger occur while probing? Need to re-trigger if yes
-                */
-               if (trigger_count != atomic_read(&deferred_trigger_count) &&
-                   !defer_all_probes)
-                       driver_deferred_probe_trigger();
+               if (!defer_all_probes &&
+                   device_needs_retrigger(dev, trigger_count))
+                       driver_deferred_probe_trigger(NULL);
        }
        atomic_dec(&probe_count);
        wake_up_all(&probe_waitqueue);

include/linux/device.h +3 -1

index 4940db137fff..9c9153adb8d6 100644
@@ -486,6 +486,8 @@ struct device_physical_location {
  * @p:         Holds the private data of the driver core portions of the device.
  *             See the comment of the struct device_private for detail.
  * @kobj:      A top-level, abstract class from which other classes are derived.
+ * @trigger_count: Number of times this device (or any of its removed children)
+ *              has been successfully bound to a driver.
  * @init_name: Initial name of the device.
  * @type:      The type of device.
  *             This identifies the device type and carries type-specific

@@ -581,6 +583,7 @@ struct device_physical_location {
  */
 struct device {
        struct kobject kobj;
+       atomic_t                trigger_count;
        struct device           *parent;
 
        struct device_private   *p;
--

2.35.1.1320.gc452695387.dirty

Quoted 3 lines · Sean Anderson

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

but in those cases, the parent's probe should still do all the checks ahead of time.

Can you be more specific about the actual failure you are seeing?

Quoted 11 lines · Saravana Kannan

Quoted 3 lines · Sean Anderson

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.

Quoted 2 lines

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.

Quoted 2 lines

but in those cases, the parent's probe should still do all the checks ahead of time.

Such as what? How is the parent going to know the resource is missing without checking for it?

Quoted 1 line

Can you be more specific about the actual failure you are seeing?

MAC is looking for a PCS that's on its internal MDIO bus, but that PCS's driver isn't loaded. The PCS has to be loaded at probe time because phylink_create needs it, and phylink is necessary to register the netdev. The latter situation is not ideal, but it would be quite a bit of work to untangle.

Quoted 12 lines · Sean Anderson

Quoted 11 lines · Saravana Kannan

Quoted 3 lines · Sean Anderson

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 2 lines

but in those cases, the parent's probe should still do all the checks ahead of time.

Such as what? How is the parent going to know the resource is missing without checking for it?

Quoted 7 lines

Quoted 1 line

Can you be more specific about the actual failure you are seeing?

MAC is looking for a PCS that's on its internal MDIO bus, but that PCS's driver isn't loaded. The PCS has to be loaded at probe time because phylink_create needs it, and phylink is necessary to register the netdev. The latter situation is not ideal, but it would be quite a bit of work to untangle.

Please untangle, don't put stuff in the driver core for broken subsystems. That is just pushing the maintaince of this from the driver authors to the driver core maintainers for the next 20+ years :(

Quoted 16 lines · Greg Kroah-Hartman

Quoted 15 lines · Sean Anderson

Quoted 13 lines · Saravana Kannan

Quoted 3 lines · Sean Anderson

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

Quoted 4 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

but in those cases, the parent's probe should still do all the checks ahead of time.

Such as what? How is the parent going to know the resource is missing without checking for it?

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Can you be more specific about the actual failure you are seeing?

MAC is looking for a PCS that's on its internal MDIO bus, but that PCS's driver isn't loaded. The PCS has to be loaded at probe time because phylink_create needs it, and phylink is necessary to register the netdev. The latter situation is not ideal, but it would be quite a bit of work to untangle.

Quoted 4 lines

Please untangle, don't put stuff in the driver core for broken subsystems. That is just pushing the maintaince of this from the driver authors to the driver core maintainers for the next 20+ years :(

What makes it broken? The "mess" has already been made in netdev. The driver authors have already pushed it off onto phylink.

And by "quite a bit of work to untangle" I mean the PCS affects settings (ethtool ksettings, MII IOCTL) that are exposed to userspace as soon as the netdev is registered. So we cannot move to a "delayed" lookup without breaking reading/modifying the settings. We could of course fake it, but what happens when e.g. userspace looks at the settings and breaks because we are not reporting the right capabilities (which would have been reported in the past)?

--Sean

Quoted 21 lines · Sean Anderson

Quoted 19 lines · Greg Kroah-Hartman

Quoted 17 lines · Sean Anderson

Quoted 5 lines · Saravana Kannan

Quoted 3 lines · Sean Anderson

A deferred probe loop can occur when a device returns EPROBE_DEFER after registering a bus with children:

Quoted 8 lines

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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

but in those cases, the parent's probe should still do all the checks ahead of time.

Such as what? How is the parent going to know the resource is missing without checking for it?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Can you be more specific about the actual failure you are seeing?

MAC is looking for a PCS that's on its internal MDIO bus, but that PCS's driver isn't loaded. The PCS has to be loaded at probe time because phylink_create needs it, and phylink is necessary to register the netdev. The latter situation is not ideal, but it would be quite a bit of work to untangle.

I meant, point to a specific device in a DT and the driver. Provide logs of the failure if possible, etc. Tell me which device is failing and why, etc. That way, I can take a closer look or give you other suggestions.

Quoted 22 lines · Saravana Kannan

Quoted 21 lines · Sean Anderson

Quoted 19 lines · Greg Kroah-Hartman

Quoted 17 lines · Sean Anderson

Quoted 5 lines · Saravana Kannan

Quoted 3 lines · Sean Anderson

A deferred probe loop can occur when a device returns EPROBE_DEFER after registering a bus with children:

Quoted 8 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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...

Quoted 38 lines · Sean Anderson

Quoted 22 lines · Saravana Kannan

Quoted 21 lines · Sean Anderson

Quoted 19 lines · Greg Kroah-Hartman

Quoted 17 lines · Sean Anderson

Quoted 5 lines · Saravana Kannan

Quoted 3 lines · Sean Anderson

A deferred probe loop can occur when a device returns EPROBE_DEFER after registering a bus with children:

Quoted 8 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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).

Quoted 5 lines

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.

And if MFD doesn't work, there's always the aux-bus code, perhaps that should be used here instead?

Quoted 45 lines · Greg Kroah-Hartman

Quoted 43 lines · Sean Anderson

Quoted 22 lines · Saravana Kannan

Quoted 21 lines · Sean Anderson

Quoted 19 lines · Greg Kroah-Hartman

Quoted 17 lines · Sean Anderson

Quoted 5 lines · Saravana Kannan

Quoted 3 lines · Sean Anderson

A deferred probe loop can occur when a device returns EPROBE_DEFER after registering a bus with children:

Quoted 8 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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...

Quoted 2 lines

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.

Quoted 46 lines · Sean Anderson

Quoted 45 lines · Greg Kroah-Hartman

Quoted 43 lines · Sean Anderson

Quoted 22 lines · Saravana Kannan

Quoted 21 lines · Sean Anderson

Quoted 19 lines · Greg Kroah-Hartman

Quoted 17 lines · Sean Anderson

Quoted 5 lines · Saravana Kannan

Quoted 3 lines · Sean Anderson

A deferred probe loop can occur when a device returns EPROBE_DEFER after registering a bus with children:

Quoted 8 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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...

Quoted 5 lines

Quoted 2 lines

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 :)

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.

Quoted 50 lines · Greg Kroah-Hartman

Quoted 49 lines · Sean Anderson

Quoted 47 lines · Greg Kroah-Hartman

Quoted 38 lines · Sean Anderson

Quoted 22 lines · Saravana Kannan

Quoted 21 lines · Sean Anderson

Quoted 19 lines · Greg Kroah-Hartman

Quoted 17 lines · Sean Anderson

Quoted 5 lines · Saravana Kannan

Quoted 3 lines · Sean Anderson

A deferred probe loop can occur when a device returns EPROBE_DEFER after registering a bus with children:

Quoted 8 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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).

Quoted 5 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 2 lines

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?

Quoted 51 lines · Sean Anderson

Quoted 50 lines · Greg Kroah-Hartman

Quoted 49 lines · Sean Anderson

Quoted 47 lines · Greg Kroah-Hartman

Quoted 38 lines · Sean Anderson

Quoted 22 lines · Saravana Kannan

Quoted 21 lines · Sean Anderson

Quoted 19 lines · Greg Kroah-Hartman

Quoted 17 lines · Sean Anderson

Quoted 5 lines · Saravana Kannan

Quoted 3 lines · Sean Anderson

A deferred probe loop can occur when a device returns EPROBE_DEFER after registering a bus with children:

Quoted 8 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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).

Quoted 5 lines

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.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 65 lines · Greg Kroah-Hartman

Quoted 51 lines · Sean Anderson

Quoted 50 lines · Greg Kroah-Hartman

Quoted 49 lines · Sean Anderson

Quoted 47 lines · Greg Kroah-Hartman

Quoted 38 lines · Sean Anderson

Quoted 22 lines · Saravana Kannan

Quoted 21 lines · Sean Anderson

Quoted 19 lines · Greg Kroah-Hartman

Quoted 17 lines · Sean Anderson

Quoted 5 lines · Saravana Kannan

Quoted 3 lines · Sean Anderson

A deferred probe loop can occur when a device returns EPROBE_DEFER after
registering a bus with children:

Quoted 8 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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).

Quoted 5 lines

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.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

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.