Infinite recursion in device_reorder_to_tail() due to circular device links

* Infinite recursion in device_reorder_to_tail() due to circular device links
@ 2021-01-11 18:43 Stephan Gerhold
  2021-01-12 14:32 ` Rafael J. Wysocki
  0 siblings, 1 reply; 10+ messages in thread
From: Stephan Gerhold @ 2021-01-11 18:43 UTC (permalink / raw)
  To: Rafael J. Wysocki, Greg Kroah-Hartman
  Cc: linux-kernel, Peter Chen, Kishon Vijay Abraham I, linux-usb,
	Thierry Reding

[-- Attachment #1: Type: text/plain, Size: 4763 bytes --]

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. But I don't think this commit is the actual problem...

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.

Without "vbus-supply" (2) happens before (1) because then the PHY driver
requests probe deferral a few times while waiting for regulators.

I'm not really sure what to do here, any suggestions how to fix this properly?

(In case it helps, I have attached an example kernel module (circdl.c)
 that allows reproducing the exact same crash.)

Thanks in advance!
Stephan

[-- Attachment #2: circdl.c --]
[-- Type: text/plain, Size: 1998 bytes --]

#include <linux/module.h>
#include <linux/platform_device.h>

static int parent_probe(struct platform_device *pdev)
{
	struct device *dev = &pdev->dev;
	struct platform_device *middle;

	dev_info(dev, "probe\n");

	middle = platform_device_register_data(dev, "middle", PLATFORM_DEVID_AUTO, NULL, 0);
	if (IS_ERR(middle))
		return dev_err_probe(dev, PTR_ERR(middle), "failed to create middle\n");

	return 0;
}

static int middle_probe(struct platform_device *pdev)
{
	struct device *dev = &pdev->dev;
	struct platform_device *child;
	struct device_link *link;

	dev_info(dev, "probe\n");

	child = platform_device_register_data(dev, "child", PLATFORM_DEVID_AUTO, NULL, 0);
	if (IS_ERR(child))
		return dev_err_probe(dev, PTR_ERR(child), "failed to create child\n");

	dev_info(dev, "device_link_add\n");

	/*
	 * Note how the device link is created between the parent device and the
	 * child device. This is equivalent to what happens in the phy-core
	 * because ci_hdrc calls phy_get(dev->parent, "usb-phy");
	 */
	link = device_link_add(dev->parent, &child->dev, DL_FLAG_STATELESS);
	if (link)
		dev_err(dev, "Creating circular device link should have failed!\n");
	else
		dev_info(dev, "Creating device link failed (as it should)\n");

	return 0;
}

static int child_probe(struct platform_device *pdev)
{
	dev_info(&pdev->dev, "probe\n");
	return 0;
}

static struct platform_driver parent_driver = {
	.probe	= parent_probe,
	.driver	= {
		.name = "parent",
	},
};

static struct platform_driver middle_driver = {
	.probe	= middle_probe,
	.driver	= {
		.name = "middle",
	},
};

static struct platform_driver child_driver = {
	.probe	= child_probe,
	.driver	= {
		.name = "child",
	},
};

static int __init circdl_init(void)
{
	platform_driver_register(&parent_driver);
	platform_driver_register(&middle_driver);
	platform_driver_register(&child_driver);
	platform_device_register_simple("parent", PLATFORM_DEVID_AUTO, NULL, 0);
	return 0;
}

module_init(circdl_init)
MODULE_LICENSE("GPL v2");

^ permalink raw reply related	[flat|nested] 10+ messages in thread