| Message ID | X/ycQpu7NIGI969v@gerhold.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Infinite recursion in device_reorder_to_tail() due to circular device links | expand |
On Mon, Jan 11, 2021 at 7:46 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > 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. > 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. > 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. > 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? Clearly, device_add() needs to check if there is a device link between dev and dev->parent, but it is not so clear what to do if such a link is present. I would make it return an error in that case. > (In case it helps, I have attached an example kernel module (circdl.c) > that allows reproducing the exact same crash.) IMV the bug is in the code that attempts to create a device link before registering the child and it clearly shows that creating device links from unregistered consumers is not really a good idea in general. In principle, a device link creation flag to instruct device_link_add() to fail if the consumer is not registered can be added and it can be used by the code in question if that helps.
On Tue, Jan 12, 2021 at 03:32:04PM +0100, Rafael J. Wysocki wrote: > On Mon, Jan 11, 2021 at 7:46 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > 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. Now reverted, thanks. greg k-h
On Tue, Jan 12, 2021 at 3:32 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Mon, Jan 11, 2021 at 7:46 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > 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. > > > 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. > > > 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.
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 <rafael@kernel.org> wrote: > > > > On Mon, Jan 11, 2021 at 7:46 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > 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! > > > 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. > > > > > 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
On 21-01-13 12:18:35, Stephan Gerhold wrote: > > 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. > Hi Stephan, You could try to get the PHY at parent driver (drivers/usb/chipidea/ci_hdrc_msm.c) to see the difference.
Hi Peter, On Thu, Jan 14, 2021 at 08:54:54AM +0800, Peter Chen wrote: > On 21-01-13 12:18:35, Stephan Gerhold wrote: > > > > 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. > > > > You could try to get the PHY at parent driver > (drivers/usb/chipidea/ci_hdrc_msm.c) to see the difference. > Unfortunately, I don't think this works in my case because I have an ULPI PHY. It's not available until ret = ci_ulpi_init(ci); is called from the ci_hdrc.0 platform device. I tried the following diff yesterday. It prevents the circular device link, therefore also the crash and even devm_* on the parent device: diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index aa40e510b806..79f556d0c93e 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -847,6 +847,8 @@ struct platform_device *ci_hdrc_add_device(struct device *dev, } pdev->dev.parent = dev; + device_set_of_node_from_dev(&pdev->dev, dev); + pdev->driver_override = kstrdup("ci_hdrc", GFP_KERNEL); ret = platform_device_add_resources(pdev, res, nres); if (ret) @@ -1027,7 +1029,7 @@ static int ci_hdrc_probe(struct platform_device *pdev) ci->usb_phy = ci->platdata->usb_phy; } else { /* Look for a generic PHY first */ - ci->phy = devm_phy_get(dev->parent, "usb-phy"); + ci->phy = devm_phy_get(dev, "usb-phy"); if (PTR_ERR(ci->phy) == -EPROBE_DEFER) { ret = -EPROBE_DEFER; Basically my idea was to share the of_node with the ci_hdrc.0 platform device, so that it can request the PHY itself instead of going through the parent. It seems to work fine but I had to add pdev->driver_override to prevent the ci_hdrc.0 device from probing using ci_hdrc_msm (since it considers the "compatible" value on the of_node otherwise). This is a bit weird (I think driver_override is mainly intended to be written through sysfs, not from kernel code directly). That is why I did not post this as a proper patch yet... Thanks, Stephan
On Tue, 12 Jan 2021, Greg Kroah-Hartman wrote: > On Tue, Jan 12, 2021 at 03:32:04PM +0100, Rafael J. Wysocki wrote: > > On Mon, Jan 11, 2021 at 7:46 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > 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. > > Now reverted, thanks. > > greg k-h I think that there has been a misunderstanding here: although 5b6164d3465f ("driver core: Reorder devices on successful probe") has been reverted from linux-next (thank you), it has not yet been reverted from 5.11-rc, and still causing problems there (in my case, not the infinite recursion Stephan reported in this thread, but the ThinkPad rmi4 suspend failure that I reported in another thread). Thanks, Hugh
On Sat, Jan 23, 2021 at 03:37:30PM -0800, Hugh Dickins wrote: > On Tue, 12 Jan 2021, Greg Kroah-Hartman wrote: > > On Tue, Jan 12, 2021 at 03:32:04PM +0100, Rafael J. Wysocki wrote: > > > On Mon, Jan 11, 2021 at 7:46 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > > > 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. > > > > Now reverted, thanks. > > > > greg k-h > > I think that there has been a misunderstanding here: although > 5b6164d3465f ("driver core: Reorder devices on successful probe") > has been reverted from linux-next (thank you), it has not yet been > reverted from 5.11-rc, and still causing problems there (in my case, > not the infinite recursion Stephan reported in this thread, but the > ThinkPad rmi4 suspend failure that I reported in another thread). It will be sent to Linus in a few hours, thanks, so should show up in 5.11-rc5. I had other patches to go along with this to send him at the same time :) greg k-h
On Sun, 24 Jan 2021, Greg Kroah-Hartman wrote: > On Sat, Jan 23, 2021 at 03:37:30PM -0800, Hugh Dickins wrote: > > On Tue, 12 Jan 2021, Greg Kroah-Hartman wrote: > > > On Tue, Jan 12, 2021 at 03:32:04PM +0100, Rafael J. Wysocki wrote: > > > > On Mon, Jan 11, 2021 at 7:46 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > > > > > 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. > > > > > > Now reverted, thanks. > > > > > > greg k-h > > > > I think that there has been a misunderstanding here: although > > 5b6164d3465f ("driver core: Reorder devices on successful probe") > > has been reverted from linux-next (thank you), it has not yet been > > reverted from 5.11-rc, and still causing problems there (in my case, > > not the infinite recursion Stephan reported in this thread, but the > > ThinkPad rmi4 suspend failure that I reported in another thread). > > It will be sent to Linus in a few hours, thanks, so should show up in > 5.11-rc5. I had other patches to go along with this to send him at the > same time :) And indeed it's now in, thanks Greg: I'm sorry for being importunate, the misunderstanding was mine. Hugh
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);
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.: 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 #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");