On Tue, Jan 12, 2021 at 07:24:24PM +0100, Rafael J. Wysocki wrote:

On Tue, Jan 12, 2021 at 3:32 PM Rafael J. Wysocki [off-list ref] wrote:

quoted

On Mon, Jan 11, 2021 at 7:46 PM Stephan Gerhold [off-list ref] wrote:

quoted

Hi,

since 5.11-rc1 I get kernel crashes with infinite recursion in
device_reorder_to_tail() in some situations... It's a bit complicated to
explain so I want to apologize in advance for the long mail. :)

  Kernel panic - not syncing: kernel stack overflow
  CPU: 1 PID: 33 Comm: kworker/1:1 Not tainted 5.11.0-rc3 #1
  Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
  Call trace:
   ...
   device_reorder_to_tail+0x4c/0xf0
   device_reorder_to_tail+0x98/0xf0
   device_reorder_to_tail+0x60/0xf0
   device_reorder_to_tail+0x60/0xf0
   device_reorder_to_tail+0x60/0xf0
   ...

The crash happens only in 5.11 with commit 5b6164d3465f ("driver core:
Reorder devices on successful probe"). It stops happening when I revert
this commit.

Thanks for the report!

Greg, please revert commit 5b6164d3465f, it clearly is not an
improvement, at least at this point.

Thanks a lot for the quick reply and for reverting the patch!

quoted

quoted

But I don't think this commit is the actual problem...

Well, it may not be the root cause, but it is a change in behavior
that exposes the breakage and this is not the only problem introduced
by it.

quoted

It's easy to reproduce on any device based on the Qualcomm MSM8916 SoC
by adding a random regulator to the USB node, e.g.:

diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
index 3a9538e1ec97..9f43fce9e6e3 100644
--- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi

@@ -372,6 +372,7 @@ codec {

 &usb {
        status = "okay";
+       vbus-supply = <&pm8916_l5>;
        extcon = <&usb_id>, <&usb_id>;

        pinctrl-names = "default", "device";

I searched for problems in the regulator core but the problem actually
has nothing to do with regulators: The additional regulator supply just
delays probing of the USB driver long enough to trigger the issue.

Adding some debug output to device_reorder_to_tail() reveals that it
keeps recursing over the same 4 devices:

  msm_hsusb 78d9000.usb: device_reorder_to_tail()
  ci_hdrc ci_hdrc.0: device_reorder_to_tail()
  qcom_usb_hs_phy ci_hdrc.0.ulpi: device_reorder_to_tail()
  phy phy-ci_hdrc.0.ulpi.0: device_reorder_to_tail()
  msm_hsusb 78d9000.usb: device_reorder_to_tail()
  ...

The device hierarchy of these is (children devices):

  78d9000.usb -> ci_hdrc.0 -> ci_hdrc.0.ulpi -> phy-ci_hdrc.0.ulpi.0

ci_hdrc.0 calls phy_get(dev->parent, "usb-phy"). In phy_get(),
the phy-core then attempts to add the following device link:

  phy-ci_hdrc.0.ulpi.0 -> 78d9000.usb

The device link creation in phy-core is optional (see commit 1d7cb11e1090
("phy: core: Fix phy_get() to not return error on link creation failure"))
because this device link is circular in case of ULPI PHYs (like here).

And indeed, creating this device link usually fails (as it should).
However, adding the "vbus-supply" changes probe order in some way that
this circular device link ends up being created:
  /sys/class/devlink/phy-ci_hdrc.0.ulpi.0--78d9000.usb/ exists only when
I add the "vbus-supply" as in the diff above.

Apparently, there is a special situation where device_is_dependent()
does not work properly, and therefore the driver core allows creating
the circular device link.

To show the problem, I enabled some debug messages and added the
following log message:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 25e08e5f40bd..ff1344eabb31 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c

@@ -3089,9 +3089,11 @@ int device_add(struct device *dev)
        }

        bus_probe_device(dev);
-       if (parent)
+       if (parent) {
+               dev_info(dev, "add to parent %s\n", dev_name(parent));
                klist_add_tail(&dev->p->knode_parent,
                               &parent->p->klist_children);
+       }

        if (dev->class) {
                mutex_lock(&dev->class->p->mutex);

Running this with "vbus-supply" (where it crashes) produces:

     bus: 'platform': probing driver msm_hsusb with device 78d9000.usb
       <some probe deferrals while waiting for the regulator>
     bus: 'platform': probing driver msm_hsusb with device 78d9000.usb
     bus: 'platform': probing driver ci_hdrc with device ci_hdrc.0
     bus: 'ulpi': probing driver qcom_usb_hs_phy with device ci_hdrc.0.ulpi
     phy phy-ci_hdrc.0.ulpi.0: add to parent ci_hdrc.0.ulpi
     qcom_usb_hs_phy ci_hdrc.0.ulpi: add to parent ci_hdrc.0
 (1) msm_hsusb 78d9000.usb: Linked as a consumer to phy-ci_hdrc.0.ulpi.0
 (2) ci_hdrc ci_hdrc.0: add to parent 78d9000.usb
     Kernel panic - not syncing: kernel stack overflow
      ...

Note how ci_hdrc is added to the children list of 78d9000.usb (2) after
the device link was already created in (1). This is why device_is_dependent()
does not realize the devices will eventually be dependent on each other.

Well, it cannot know beforehand that the consumer is going to be
registered as a child of the supplier.

I've got it the other way around, so it should have been:

"it cannot know beforehand that the supplier is going to be registered
as a child of the consumer."

Sorry.

That said, device_is_dependend() doesn't really check for "family
connections", so to speak, which it should do in principle.

That is, it should return "true" if "target" or any direct ancestor of
it is any of the devices that depend on "dev", not just when "target"
itself is any of those devices.

Let me cut a patch for that.

Thanks!

While initially debugging the crash I naively(!) tried the following
two diffs:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 14f165816742..3864af018834 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c

@@ -3088,10 +3088,10 @@ int device_add(struct device *dev)
 		fw_devlink_link_device(dev);
 	}
 
-	bus_probe_device(dev);
 	if (parent)
 		klist_add_tail(&dev->p->knode_parent,
 			       &parent->p->klist_children);
+	bus_probe_device(dev);
 
 	if (dev->class) {
 		mutex_lock(&dev->class->p->mutex);

or alternatively

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 14f165816742..268b88ce3df7 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c

@@ -218,12 +218,16 @@ int device_links_read_lock_held(void)
  */
 int device_is_dependent(struct device *dev, void *target)
 {
+	struct device *target_dev = target;
 	struct device_link *link;
 	int ret;
 
 	if (dev == target)
 		return 1;
 
+	if (target_dev->parent && device_is_dependent(dev, target_dev->parent))
+		return 1;
+
 	ret = device_for_each_child(dev, target, device_is_dependent);
 	if (ret)
 		return ret;

Both prevent the circular device link and therefore also the crash.
I didn't mention it because it was really just a naive thought and
I'm sure they might cause other problems. :-)

Also, on a completely different note I looked again at the chipidea USB
driver that produces this situation. To request the PHY (which ends up
in the circular device link) it does:

	/* Look for a generic PHY first */
	ci->phy = devm_phy_get(dev->parent, "usb-phy");

To me it doesn't really seem great to use the devm_* helpers on the
parent device either, so I will check if I can refactor this somehow.
Perhaps this situation can be prevented entirely.

Thanks!
Stephan