* [PATCH] usb: misc: onboard_usb_hub: Don't defer probing for 'incomplete' DT nodes @ 2022-12-20 0:45 Matthias Kaehlcke 2022-12-20 6:24 ` Thorsten Leemhuis ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Matthias Kaehlcke @ 2022-12-20 0:45 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Douglas Anderson, linux-kernel, linux-usb, Stefan Wahren, Matthias Kaehlcke, Ravi Chandra Sadineni Some boards have device tree nodes for USB hubs supported by the onboard_usb_hub driver, but the nodes don't have all properties needed for the driver to work properly (which is not necessarily an error in the DT). Currently _find_onboard_hub() returns -EPROBE_DEFER in such cases, which results in an unusable USB hub, since successive probes fail in the same way. Use the absence of the "vdd" supply as an indicator of such 'incomplete' DT nodes and return -ENODEV. Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") Reported-by: Stefan Wahren <stefan.wahren@i2se.com> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- drivers/usb/misc/onboard_usb_hub.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c index d63c63942af1..2968da515016 100644 --- a/drivers/usb/misc/onboard_usb_hub.c +++ b/drivers/usb/misc/onboard_usb_hub.c @@ -363,6 +363,15 @@ static struct onboard_hub *_find_onboard_hub(struct device *dev) hub = dev_get_drvdata(&pdev->dev); put_device(&pdev->dev); + /* + * Some boards have device tree nodes for USB hubs supported by this + * driver, but the nodes don't have all properties needed for the driver + * to work properly. Use the absence of the "vdd" supply as an indicator + * of such nodes. + */ + if (!of_get_property(pdev->dev.of_node, "vdd", NULL)) + return ERR_PTR(-ENODEV); + /* * The presence of drvdata ('hub') indicates that the platform driver * finished probing. This handles the case where (conceivably) we could -- 2.39.0.314.g84b9a713c41-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: misc: onboard_usb_hub: Don't defer probing for 'incomplete' DT nodes 2022-12-20 0:45 [PATCH] usb: misc: onboard_usb_hub: Don't defer probing for 'incomplete' DT nodes Matthias Kaehlcke @ 2022-12-20 6:24 ` Thorsten Leemhuis 2022-12-20 6:58 ` Greg Kroah-Hartman 2022-12-20 7:55 ` Johan Hovold 2 siblings, 0 replies; 6+ messages in thread From: Thorsten Leemhuis @ 2022-12-20 6:24 UTC (permalink / raw) To: Matthias Kaehlcke, Greg Kroah-Hartman Cc: Douglas Anderson, linux-kernel, linux-usb, Stefan Wahren, Ravi Chandra Sadineni Hi, this is your Linux kernel regression tracker. On 20.12.22 01:45, Matthias Kaehlcke wrote: > Some boards have device tree nodes for USB hubs supported by the > onboard_usb_hub driver, but the nodes don't have all properties > needed for the driver to work properly (which is not necessarily > an error in the DT). Currently _find_onboard_hub() returns > -EPROBE_DEFER in such cases, which results in an unusable USB hub, > since successive probes fail in the same way. Use the absence of > the "vdd" supply as an indicator of such 'incomplete' DT nodes > and return -ENODEV. > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> Many thx for taking care of this. There is one small thing to improve, please add the following tag here to make things easier for future code archaeologists: Link: https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ Basically: every time you use "Reported-by:" use a "Link:" to the report. To explain: Linus[1] and others considered proper link tags important in cases like this, as they allow anyone to look into the backstory of a commit weeks or years later. That's nothing new, the documentation[2] for some time says to place tags in cases like this. I care personally (and made it a bit more explicit in the docs a while ago), because these tags make my regression tracking efforts a whole lot easier, as they allow my tracking bot 'regzbot' to automatically connect reports with patches posted or committed to fix tracked regressions. Apropos regzbot, let me tell regzbot to monitor this thread: #regzbot ^backmonitor: https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ #regzbot fix: usb: misc: onboard_usb_hub: Don't defer probing for 'incomplete' DT nodes Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) P.S.: As the Linux kernel's regression tracker I deal with a lot of reports and sometimes miss something important when writing mails like this. If that's the case here, don't hesitate to tell me in a public reply, it's in everyone's interest to set the public record straight. [1] for details, see: https://lore.kernel.org/all/CAHk-=wjMmSZzMJ3Xnskdg4+GGz=5p5p+GSYyFBTh0f-DgvdBWg@mail.gmail.com/ https://lore.kernel.org/all/CAHk-=wgs38ZrfPvy=nOwVkVzjpM3VFU1zobP37Fwd_h9iAD5JQ@mail.gmail.com/ https://lore.kernel.org/all/CAHk-=wjxzafG-=J8oT30s7upn4RhBs6TX-uVFZ5rME+L5_DoJA@mail.gmail.com/ [2] see Documentation/process/submitting-patches.rst (http://docs.kernel.org/process/submitting-patches.html) and Documentation/process/5.Posting.rst (https://docs.kernel.org/process/5.Posting.html) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: misc: onboard_usb_hub: Don't defer probing for 'incomplete' DT nodes 2022-12-20 0:45 [PATCH] usb: misc: onboard_usb_hub: Don't defer probing for 'incomplete' DT nodes Matthias Kaehlcke 2022-12-20 6:24 ` Thorsten Leemhuis @ 2022-12-20 6:58 ` Greg Kroah-Hartman 2022-12-20 7:55 ` Johan Hovold 2 siblings, 0 replies; 6+ messages in thread From: Greg Kroah-Hartman @ 2022-12-20 6:58 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Douglas Anderson, linux-kernel, linux-usb, Stefan Wahren, Ravi Chandra Sadineni On Tue, Dec 20, 2022 at 12:45:01AM +0000, Matthias Kaehlcke wrote: > Some boards have device tree nodes for USB hubs supported by the > onboard_usb_hub driver, but the nodes don't have all properties > needed for the driver to work properly (which is not necessarily > an error in the DT). Currently _find_onboard_hub() returns > -EPROBE_DEFER in such cases, which results in an unusable USB hub, > since successive probes fail in the same way. Use the absence of > the "vdd" supply as an indicator of such 'incomplete' DT nodes > and return -ENODEV. > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> No cc: stable? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: misc: onboard_usb_hub: Don't defer probing for 'incomplete' DT nodes 2022-12-20 0:45 [PATCH] usb: misc: onboard_usb_hub: Don't defer probing for 'incomplete' DT nodes Matthias Kaehlcke 2022-12-20 6:24 ` Thorsten Leemhuis 2022-12-20 6:58 ` Greg Kroah-Hartman @ 2022-12-20 7:55 ` Johan Hovold 2022-12-20 14:15 ` Matthias Kaehlcke 2 siblings, 1 reply; 6+ messages in thread From: Johan Hovold @ 2022-12-20 7:55 UTC (permalink / raw) To: Matthias Kaehlcke, Rob Herring Cc: Greg Kroah-Hartman, Douglas Anderson, linux-kernel, linux-usb, Stefan Wahren, Ravi Chandra Sadineni On Tue, Dec 20, 2022 at 12:45:01AM +0000, Matthias Kaehlcke wrote: > Some boards have device tree nodes for USB hubs supported by the > onboard_usb_hub driver, but the nodes don't have all properties > needed for the driver to work properly (which is not necessarily > an error in the DT). Currently _find_onboard_hub() returns > -EPROBE_DEFER in such cases, which results in an unusable USB hub, > since successive probes fail in the same way. Use the absence of > the "vdd" supply as an indicator of such 'incomplete' DT nodes > and return -ENODEV. > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > drivers/usb/misc/onboard_usb_hub.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c > index d63c63942af1..2968da515016 100644 > --- a/drivers/usb/misc/onboard_usb_hub.c > +++ b/drivers/usb/misc/onboard_usb_hub.c > @@ -363,6 +363,15 @@ static struct onboard_hub *_find_onboard_hub(struct device *dev) > hub = dev_get_drvdata(&pdev->dev); > put_device(&pdev->dev); > > + /* > + * Some boards have device tree nodes for USB hubs supported by this > + * driver, but the nodes don't have all properties needed for the driver > + * to work properly. Use the absence of the "vdd" supply as an indicator > + * of such nodes. > + */ > + if (!of_get_property(pdev->dev.of_node, "vdd", NULL)) > + return ERR_PTR(-ENODEV); Does this not break your original use case? Don't you want "vdd-supply" here? That said, this seems like the wrong property to look for both in principle and as it is described as optional by the binding: Documentation/devicetree/bindings/usb/realtek,rts5411.yaml It seems that you should use the compatible property and check that it holds one of the expected values: - usbbda,5411 - usbbda,411 rather than treat every hub node as describing a realtek hub (AFAIK, there is no generic binding for this yet). > + > /* > * The presence of drvdata ('hub') indicates that the platform driver > * finished probing. This handles the case where (conceivably) we could Johan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: misc: onboard_usb_hub: Don't defer probing for 'incomplete' DT nodes 2022-12-20 7:55 ` Johan Hovold @ 2022-12-20 14:15 ` Matthias Kaehlcke 2022-12-23 13:42 ` Johan Hovold 0 siblings, 1 reply; 6+ messages in thread From: Matthias Kaehlcke @ 2022-12-20 14:15 UTC (permalink / raw) To: Johan Hovold Cc: Rob Herring, Greg Kroah-Hartman, Douglas Anderson, linux-kernel, linux-usb, Stefan Wahren, Ravi Chandra Sadineni Hi Johan, On Tue, Dec 20, 2022 at 08:55:52AM +0100, Johan Hovold wrote: > On Tue, Dec 20, 2022 at 12:45:01AM +0000, Matthias Kaehlcke wrote: > > Some boards have device tree nodes for USB hubs supported by the > > onboard_usb_hub driver, but the nodes don't have all properties > > needed for the driver to work properly (which is not necessarily > > an error in the DT). Currently _find_onboard_hub() returns > > -EPROBE_DEFER in such cases, which results in an unusable USB hub, > > since successive probes fail in the same way. Use the absence of > > the "vdd" supply as an indicator of such 'incomplete' DT nodes > > and return -ENODEV. > > > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > > > drivers/usb/misc/onboard_usb_hub.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c > > index d63c63942af1..2968da515016 100644 > > --- a/drivers/usb/misc/onboard_usb_hub.c > > +++ b/drivers/usb/misc/onboard_usb_hub.c > > @@ -363,6 +363,15 @@ static struct onboard_hub *_find_onboard_hub(struct device *dev) > > hub = dev_get_drvdata(&pdev->dev); > > put_device(&pdev->dev); > > > > + /* > > + * Some boards have device tree nodes for USB hubs supported by this > > + * driver, but the nodes don't have all properties needed for the driver > > + * to work properly. Use the absence of the "vdd" supply as an indicator > > + * of such nodes. > > + */ > > + if (!of_get_property(pdev->dev.of_node, "vdd", NULL)) > > + return ERR_PTR(-ENODEV); > > Does this not break your original use case? Don't you want "vdd-supply" > here? Ouch, yes it does (to a certain degree). Thanks for pointing it out. My sanity check didn't catch this because the platform driver still probes successfully and powers the hub on. > That said, this seems like the wrong property to look for both in > principle and as it is described as optional by the binding: > > Documentation/devicetree/bindings/usb/realtek,rts5411.yaml > > It seems that you should use the compatible property and check that it > holds one of the expected values: > > - usbbda,5411 > - usbbda,411 > > rather than treat every hub node as describing a realtek hub (AFAIK, > there is no generic binding for this yet). The driver only probes for specific hub models, among them the Microchip USB2514B hub with which Stefan encountered the regression [1]. My initial assumption when writing this driver was that the existence of a node for a supported hub means that the driver should be used. However the regression encountered by Stefan makes clear that this assumption is incorrect. It's not common, but a device tree may have nodes for onboard USB devices, among them hubs (which might become more common with this driver). Not in all instances the hub nodes were added with the intention of using this driver for power sequencing the hub (e.g. [2]). The compatible string alone doesn't indicate that the onboard_hub driver should be instantiated for a given hub, which is why I'm using the existence of "vdd-supply" as indicator. Thanks m. [1] https://lore.kernel.org/linux-usb/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ [2] https://elixir.bootlin.com/linux/v6.1/source/arch/arm/boot/dts/bcm283x-rpi-lan7515.dtsi ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: misc: onboard_usb_hub: Don't defer probing for 'incomplete' DT nodes 2022-12-20 14:15 ` Matthias Kaehlcke @ 2022-12-23 13:42 ` Johan Hovold 0 siblings, 0 replies; 6+ messages in thread From: Johan Hovold @ 2022-12-23 13:42 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Rob Herring, Greg Kroah-Hartman, Douglas Anderson, linux-kernel, linux-usb, Stefan Wahren, Ravi Chandra Sadineni On Tue, Dec 20, 2022 at 02:15:19PM +0000, Matthias Kaehlcke wrote: > Hi Johan, > > On Tue, Dec 20, 2022 at 08:55:52AM +0100, Johan Hovold wrote: > > On Tue, Dec 20, 2022 at 12:45:01AM +0000, Matthias Kaehlcke wrote: > > > Some boards have device tree nodes for USB hubs supported by the > > > onboard_usb_hub driver, but the nodes don't have all properties > > > needed for the driver to work properly (which is not necessarily > > > an error in the DT). Currently _find_onboard_hub() returns > > > -EPROBE_DEFER in such cases, which results in an unusable USB hub, > > > since successive probes fail in the same way. Use the absence of > > > the "vdd" supply as an indicator of such 'incomplete' DT nodes > > > and return -ENODEV. > > > > > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > --- > > > > > > drivers/usb/misc/onboard_usb_hub.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c > > > index d63c63942af1..2968da515016 100644 > > > --- a/drivers/usb/misc/onboard_usb_hub.c > > > +++ b/drivers/usb/misc/onboard_usb_hub.c > > > @@ -363,6 +363,15 @@ static struct onboard_hub *_find_onboard_hub(struct device *dev) > > > hub = dev_get_drvdata(&pdev->dev); > > > put_device(&pdev->dev); > > > > > > + /* > > > + * Some boards have device tree nodes for USB hubs supported by this > > > + * driver, but the nodes don't have all properties needed for the driver > > > + * to work properly. Use the absence of the "vdd" supply as an indicator > > > + * of such nodes. > > > + */ > > > + if (!of_get_property(pdev->dev.of_node, "vdd", NULL)) > > > + return ERR_PTR(-ENODEV); > > > > Does this not break your original use case? Don't you want "vdd-supply" > > here? > > Ouch, yes it does (to a certain degree). Thanks for pointing it out. My > sanity check didn't catch this because the platform driver still probes > successfully and powers the hub on. > > > That said, this seems like the wrong property to look for both in > > principle and as it is described as optional by the binding: > > > > Documentation/devicetree/bindings/usb/realtek,rts5411.yaml > > > > It seems that you should use the compatible property and check that it > > holds one of the expected values: > > > > - usbbda,5411 > > - usbbda,411 > > > > rather than treat every hub node as describing a realtek hub (AFAIK, > > there is no generic binding for this yet). > > The driver only probes for specific hub models, among them the Microchip > USB2514B hub with which Stefan encountered the regression [1]. > > My initial assumption when writing this driver was that the existence of > a node for a supported hub means that the driver should be used. However > the regression encountered by Stefan makes clear that this assumption is > incorrect. It's not common, but a device tree may have nodes for onboard > USB devices, among them hubs (which might become more common with this > driver). Not in all instances the hub nodes were added with the intention > of using this driver for power sequencing the hub (e.g. [2]). Yeah, you can't assume that. The DT bindings for USB has been around since before your onboard-hub driver. > The > compatible string alone doesn't indicate that the onboard_hub driver > should be instantiated for a given hub, which is why I'm using the > existence of "vdd-supply" as indicator. I don't have time to review this in details, but checking for a specific supply like this just seems wrong (as other have since also pointed out in comments to your v2). There could be hubs which just need to deassert a reset pin for example, and some of the bindings do not even mandate a regulator as I mentioned above. Johan ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-23 13:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-20 0:45 [PATCH] usb: misc: onboard_usb_hub: Don't defer probing for 'incomplete' DT nodes Matthias Kaehlcke 2022-12-20 6:24 ` Thorsten Leemhuis 2022-12-20 6:58 ` Greg Kroah-Hartman 2022-12-20 7:55 ` Johan Hovold 2022-12-20 14:15 ` Matthias Kaehlcke 2022-12-23 13:42 ` Johan Hovold
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).