| Message ID | 20221222022605.v2.1.If5e7ec83b1782e4dffa6ea759416a27326c8231d@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] usb: misc: onboard_usb_hub: Don't create platform devices for DT nodes without 'vdd-supply' | expand |
On Thu, Dec 22, 2022 at 02:26:44AM +0000, Matthias Kaehlcke wrote: > The primary task of the onboard_usb_hub driver is to control the > power of an onboard USB hub. The driver gets the regulator from the > device tree property "vdd-supply" of the hub's DT node. Some boards > have device tree nodes for USB hubs supported by this driver, but > don't specify a "vdd-supply". This is not an error per se, it just > means that the onboard hub driver can't be used for these hubs, so > don't create platform devices for such nodes. > > This change doesn't completely fix the reported regression. It > should fix it for the RPi 3 B Plus and boards with similar hub > configurations (compatible DT nodes without "vdd-supply"), boards > that actually use the onboard hub driver could still be impacted > by the race conditions discussed in that thread. Not creating the > platform devices for nodes without "vdd-supply" is the right > thing to do, independently from the race condition, which will > be fixed in future patch. > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > Link: https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > Changes in v2: > - don't create platform devices when "vdd-supply" is missing, > rather than returning an error from _find_onboard_hub() > - check for "vdd-supply" not "vdd" (Johan) > - updated subject and commit message > - added 'Link' tag (regzbot) > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter>
Am 22.12.22 um 03:26 schrieb Matthias Kaehlcke: > The primary task of the onboard_usb_hub driver is to control the > power of an onboard USB hub. The driver gets the regulator from the > device tree property "vdd-supply" of the hub's DT node. Some boards > have device tree nodes for USB hubs supported by this driver, but > don't specify a "vdd-supply". This is not an error per se, it just > means that the onboard hub driver can't be used for these hubs, so > don't create platform devices for such nodes. > > This change doesn't completely fix the reported regression. It > should fix it for the RPi 3 B Plus and boards with similar hub > configurations (compatible DT nodes without "vdd-supply"), boards > that actually use the onboard hub driver could still be impacted > by the race conditions discussed in that thread. Not creating the > platform devices for nodes without "vdd-supply" is the right > thing to do, independently from the race condition, which will > be fixed in future patch. > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > Link: https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Hi, On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > The primary task of the onboard_usb_hub driver is to control the > power of an onboard USB hub. The driver gets the regulator from the > device tree property "vdd-supply" of the hub's DT node. Some boards > have device tree nodes for USB hubs supported by this driver, but > don't specify a "vdd-supply". This is not an error per se, it just > means that the onboard hub driver can't be used for these hubs, so > don't create platform devices for such nodes. > > This change doesn't completely fix the reported regression. It > should fix it for the RPi 3 B Plus and boards with similar hub > configurations (compatible DT nodes without "vdd-supply"), boards > that actually use the onboard hub driver could still be impacted > by the race conditions discussed in that thread. Not creating the > platform devices for nodes without "vdd-supply" is the right > thing to do, independently from the race condition, which will > be fixed in future patch. > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > Link: https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > Changes in v2: > - don't create platform devices when "vdd-supply" is missing, > rather than returning an error from _find_onboard_hub() > - check for "vdd-supply" not "vdd" (Johan) > - updated subject and commit message > - added 'Link' tag (regzbot) > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) I'm a tad bit skeptical. It somehow feels a bit too much like "inside knowledge" to add this here. I guess the "onboard_usb_hub_pdevs.c" is already pretty entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file keep the absolute minimum amount of stuff in it and all of the details be in the other file. If this was the only issue though, I'd be tempted to let it slide. As it is, I'm kinda worried that your patch will break Alexander Stein, who should have been CCed (I've CCed him now) or Icenowy Zheng (also CCed now). I believe those folks are using the USB hub driver primarily to drive a reset GPIO. Looking at the example in the bindings for one of them (genesys,gl850g.yaml), I even see that the reset-gpio is specified but not a vdd-supply. I think you'll break that? In general, it feels like it should actually be fine to create the USB hub driver even if vdd isn't supplied. Sure, it won't do a lot, but it shouldn't actively hurt anything. You'll just be turning off and on bogus regulators and burning a few CPU cycles. I guess the problem is some race condition that you talk about in the commit message. I'd rather see that fixed... That being said, if we want to be more efficient and not burn CPU cycles and memory in Stefan Wahren's case, maybe the USB hub driver itself could return a canonical error value from its probe when it detects that it has no useful job and then "onboard_usb_hub_pdevs" could just silently bail out?
在 2022-12-22星期四的 11:26 -0800，Doug Anderson写道： > Hi, > > On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> > wrote: > > > > The primary task of the onboard_usb_hub driver is to control the > > power of an onboard USB hub. The driver gets the regulator from the > > device tree property "vdd-supply" of the hub's DT node. Some boards > > have device tree nodes for USB hubs supported by this driver, but > > don't specify a "vdd-supply". This is not an error per se, it just > > means that the onboard hub driver can't be used for these hubs, so > > don't create platform devices for such nodes. > > > > This change doesn't completely fix the reported regression. It > > should fix it for the RPi 3 B Plus and boards with similar hub > > configurations (compatible DT nodes without "vdd-supply"), boards > > that actually use the onboard hub driver could still be impacted > > by the race conditions discussed in that thread. Not creating the > > platform devices for nodes without "vdd-supply" is the right > > thing to do, independently from the race condition, which will > > be fixed in future patch. > > > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > Link: > > https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > > > Changes in v2: > > - don't create platform devices when "vdd-supply" is missing, > > rather than returning an error from _find_onboard_hub() > > - check for "vdd-supply" not "vdd" (Johan) > > - updated subject and commit message > > - added 'Link' tag (regzbot) > > > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > I'm a tad bit skeptical. > > It somehow feels a bit too much like "inside knowledge" to add this > here. I guess the "onboard_usb_hub_pdevs.c" is already pretty > entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file > keep the absolute minimum amount of stuff in it and all of the > details > be in the other file. > > If this was the only issue though, I'd be tempted to let it slide. As > it is, I'm kinda worried that your patch will break Alexander Stein, > who should have been CCed (I've CCed him now) or Icenowy Zheng (also > CCed now). I believe those folks are using the USB hub driver > primarily to drive a reset GPIO. Looking at the example in the > bindings for one of them (genesys,gl850g.yaml), I even see that the > reset-gpio is specified but not a vdd-supply. I think you'll break > that? Well technically in my final DT a regulator is included (to have the Vbus enabled when enabling the hub), however I am still against this patch, because the driver should work w/o vdd-supply (or w/o reset- gpios), and changing this behavior is a DT binding stability breakage. In addition the kernel never fails because of a lacking regulator unless explicitly forbid dummy regulators. BTW USB is a discoverable bus, and if a hub do not need special handlement, it just does not need to appear in the DT, thus no onboard hub DT node. > > In general, it feels like it should actually be fine to create the > USB > hub driver even if vdd isn't supplied. Sure, it won't do a lot, but > it > shouldn't actively hurt anything. You'll just be turning off and on > bogus regulators and burning a few CPU cycles. I guess the problem is > some race condition that you talk about in the commit message. I'd > rather see that fixed... That being said, if we want to be more > efficient and not burn CPU cycles and memory in Stefan Wahren's case, > maybe the USB hub driver itself could return a canonical error value > from its probe when it detects that it has no useful job and then > "onboard_usb_hub_pdevs" could just silently bail out? I agree.
On Thu, Dec 22, 2022 at 02:26:44AM +0000, Matthias Kaehlcke wrote: > The primary task of the onboard_usb_hub driver is to control the > power of an onboard USB hub. The driver gets the regulator from the > device tree property "vdd-supply" of the hub's DT node. Some boards > have device tree nodes for USB hubs supported by this driver, but > don't specify a "vdd-supply". This is not an error per se, it just > means that the onboard hub driver can't be used for these hubs, so > don't create platform devices for such nodes. > > This change doesn't completely fix the reported regression. It > should fix it for the RPi 3 B Plus and boards with similar hub > configurations (compatible DT nodes without "vdd-supply"), boards > that actually use the onboard hub driver could still be impacted > by the race conditions discussed in that thread. Not creating the > platform devices for nodes without "vdd-supply" is the right > thing to do, independently from the race condition, which will > be fixed in future patch. > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > Link: https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > Changes in v2: > - don't create platform devices when "vdd-supply" is missing, > rather than returning an error from _find_onboard_hub() > - check for "vdd-supply" not "vdd" (Johan) Please try to remember to CC people providing feedback on your patches. > - updated subject and commit message > - added 'Link' tag (regzbot) > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/usb/misc/onboard_usb_hub_pdevs.c b/drivers/usb/misc/onboard_usb_hub_pdevs.c > index ed22a18f4ab7..8cea53b0907e 100644 > --- a/drivers/usb/misc/onboard_usb_hub_pdevs.c > +++ b/drivers/usb/misc/onboard_usb_hub_pdevs.c > @@ -101,6 +101,19 @@ void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *p > } > } > > + /* > + * The primary task of the onboard_usb_hub driver is to control > + * the power of an USB onboard hub. Some boards have device tree > + * nodes for USB hubs supported by this driver, but don't > + * specify a "vdd-supply", which is needed by the driver. This is > + * not a DT error per se, it just means that the onboard hub > + * driver can't be used with these nodes, so don't create a > + * a platform device for such a node. > + */ > + if (!of_get_property(np, "vdd-supply", NULL) && > + !of_get_property(npc, "vdd-supply", NULL)) > + goto node_put; So as I mentioned elsewhere, this doesn't look right. It is the responsibility of the platform driver to manage its resources and it may not even need a supply. I see now that you have already matched on the compatible property above so that you only create the platform device for the devices that (may) need it. It seems the assumptions that this driver was written under needs to be revisited. > + > pdev = of_platform_device_create(np, NULL, &parent_hub->dev); > if (!pdev) { > dev_err(&parent_hub->dev, Johan
Hi everybody, Am Freitag, 23. Dezember 2022, 08:46:45 CET schrieb Icenowy Zheng: > 在 2022-12-22星期四的 11:26 -0800，Doug Anderson写道： > > > Hi, > > > > On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> > > > > wrote: > > > The primary task of the onboard_usb_hub driver is to control the > > > power of an onboard USB hub. The driver gets the regulator from the > > > device tree property "vdd-supply" of the hub's DT node. Some boards > > > have device tree nodes for USB hubs supported by this driver, but > > > don't specify a "vdd-supply". This is not an error per se, it just > > > means that the onboard hub driver can't be used for these hubs, so > > > don't create platform devices for such nodes. > > > > > > This change doesn't completely fix the reported regression. It > > > should fix it for the RPi 3 B Plus and boards with similar hub > > > configurations (compatible DT nodes without "vdd-supply"), boards > > > that actually use the onboard hub driver could still be impacted > > > by the race conditions discussed in that thread. Not creating the > > > platform devices for nodes without "vdd-supply" is the right > > > thing to do, independently from the race condition, which will > > > be fixed in future patch. > > > > > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > > Link: > > > https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ > > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > --- > > > > > > Changes in v2: > > > - don't create platform devices when "vdd-supply" is missing, > > > rather than returning an error from _find_onboard_hub() > > > - check for "vdd-supply" not "vdd" (Johan) > > > - updated subject and commit message > > > - added 'Link' tag (regzbot) > > > > > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > I'm a tad bit skeptical. > > > > It somehow feels a bit too much like "inside knowledge" to add this > > here. I guess the "onboard_usb_hub_pdevs.c" is already pretty > > entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file > > keep the absolute minimum amount of stuff in it and all of the > > details > > be in the other file. > > > > If this was the only issue though, I'd be tempted to let it slide. As > > it is, I'm kinda worried that your patch will break Alexander Stein, > > who should have been CCed (I've CCed him now) or Icenowy Zheng (also > > CCed now). I believe those folks are using the USB hub driver > > primarily to drive a reset GPIO. Looking at the example in the > > bindings for one of them (genesys,gl850g.yaml), I even see that the > > reset-gpio is specified but not a vdd-supply. I think you'll break > > that? > > Well technically in my final DT a regulator is included (to have the > Vbus enabled when enabling the hub), however I am still against this > patch, because the driver should work w/o vdd-supply (or w/o reset- > gpios), and changing this behavior is a DT binding stability breakage. I second that. The bindings don't require neither vdd-supply nor reset-gpios. But I have to admit I lack to understand the purpose of this series in the first place. What is the benefit or fix? Best regards, Alexader > In addition the kernel never fails because of a lacking regulator > unless explicitly forbid dummy regulators. > > BTW USB is a discoverable bus, and if a hub do not need special > handlement, it just does not need to appear in the DT, thus no onboard > hub DT node. > > > In general, it feels like it should actually be fine to create the > > USB > > hub driver even if vdd isn't supplied. Sure, it won't do a lot, but > > it > > shouldn't actively hurt anything. You'll just be turning off and on > > bogus regulators and burning a few CPU cycles. I guess the problem is > > some race condition that you talk about in the commit message. I'd > > rather see that fixed... That being said, if we want to be more > > efficient and not burn CPU cycles and memory in Stefan Wahren's case, > > maybe the USB hub driver itself could return a canonical error value > > from its probe when it detects that it has no useful job and then > > "onboard_usb_hub_pdevs" could just silently bail out? > > I agree.
Hello Alexander, Am 02.01.23 um 10:20 schrieb Alexander Stein: > Hi everybody, > > Am Freitag, 23. Dezember 2022, 08:46:45 CET schrieb Icenowy Zheng: >> 在 2022-12-22星期四的 11:26 -0800，Doug Anderson写道： >> >>> Hi, >>> >>> On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> >>> >>> wrote: >>>> The primary task of the onboard_usb_hub driver is to control the >>>> power of an onboard USB hub. The driver gets the regulator from the >>>> device tree property "vdd-supply" of the hub's DT node. Some boards >>>> have device tree nodes for USB hubs supported by this driver, but >>>> don't specify a "vdd-supply". This is not an error per se, it just >>>> means that the onboard hub driver can't be used for these hubs, so >>>> don't create platform devices for such nodes. >>>> >>>> This change doesn't completely fix the reported regression. It >>>> should fix it for the RPi 3 B Plus and boards with similar hub >>>> configurations (compatible DT nodes without "vdd-supply"), boards >>>> that actually use the onboard hub driver could still be impacted >>>> by the race conditions discussed in that thread. Not creating the >>>> platform devices for nodes without "vdd-supply" is the right >>>> thing to do, independently from the race condition, which will >>>> be fixed in future patch. >>>> >>>> Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") >>>> Link: >>>> https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ >>>> Reported-by: Stefan Wahren <stefan.wahren@i2se.com> >>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> >>>> --- >>>> >>>> Changes in v2: >>>> - don't create platform devices when "vdd-supply" is missing, >>>> rather than returning an error from _find_onboard_hub() >>>> - check for "vdd-supply" not "vdd" (Johan) >>>> - updated subject and commit message >>>> - added 'Link' tag (regzbot) >>>> >>>> drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ >>>> 1 file changed, 13 insertions(+) >>> I'm a tad bit skeptical. >>> >>> It somehow feels a bit too much like "inside knowledge" to add this >>> here. I guess the "onboard_usb_hub_pdevs.c" is already pretty >>> entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file >>> keep the absolute minimum amount of stuff in it and all of the >>> details >>> be in the other file. >>> >>> If this was the only issue though, I'd be tempted to let it slide. As >>> it is, I'm kinda worried that your patch will break Alexander Stein, >>> who should have been CCed (I've CCed him now) or Icenowy Zheng (also >>> CCed now). I believe those folks are using the USB hub driver >>> primarily to drive a reset GPIO. Looking at the example in the >>> bindings for one of them (genesys,gl850g.yaml), I even see that the >>> reset-gpio is specified but not a vdd-supply. I think you'll break >>> that? >> Well technically in my final DT a regulator is included (to have the >> Vbus enabled when enabling the hub), however I am still against this >> patch, because the driver should work w/o vdd-supply (or w/o reset- >> gpios), and changing this behavior is a DT binding stability breakage. > I second that. The bindings don't require neither vdd-supply nor reset-gpios. > > But I have to admit I lack to understand the purpose of this series in the > first place. What is the benefit or fix? did you followed the provided link from the patch? Best regards > > Best regards, > Alexader > >> In addition the kernel never fails because of a lacking regulator >> unless explicitly forbid dummy regulators. >> >> BTW USB is a discoverable bus, and if a hub do not need special >> handlement, it just does not need to appear in the DT, thus no onboard >> hub DT node. >> >>> In general, it feels like it should actually be fine to create the >>> USB >>> hub driver even if vdd isn't supplied. Sure, it won't do a lot, but >>> it >>> shouldn't actively hurt anything. You'll just be turning off and on >>> bogus regulators and burning a few CPU cycles. I guess the problem is >>> some race condition that you talk about in the commit message. I'd >>> rather see that fixed... That being said, if we want to be more >>> efficient and not burn CPU cycles and memory in Stefan Wahren's case, >>> maybe the USB hub driver itself could return a canonical error value >>> from its probe when it detects that it has no useful job and then >>> "onboard_usb_hub_pdevs" could just silently bail out? >> I agree. > > >
Hi Stefan, Am Montag, 2. Januar 2023, 12:44:33 CET schrieb Stefan Wahren: > Hello Alexander, > > Am 02.01.23 um 10:20 schrieb Alexander Stein: > > Hi everybody, > > > > Am Freitag, 23. Dezember 2022, 08:46:45 CET schrieb Icenowy Zheng: > >> 在 2022-12-22星期四的 11:26 -0800，Doug Anderson写道： > >> > >>> Hi, > >>> > >>> On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> > >>> > >>> wrote: > >>>> The primary task of the onboard_usb_hub driver is to control the > >>>> power of an onboard USB hub. The driver gets the regulator from the > >>>> device tree property "vdd-supply" of the hub's DT node. Some boards > >>>> have device tree nodes for USB hubs supported by this driver, but > >>>> don't specify a "vdd-supply". This is not an error per se, it just > >>>> means that the onboard hub driver can't be used for these hubs, so > >>>> don't create platform devices for such nodes. > >>>> > >>>> This change doesn't completely fix the reported regression. It > >>>> should fix it for the RPi 3 B Plus and boards with similar hub > >>>> configurations (compatible DT nodes without "vdd-supply"), boards > >>>> that actually use the onboard hub driver could still be impacted > >>>> by the race conditions discussed in that thread. Not creating the > >>>> platform devices for nodes without "vdd-supply" is the right > >>>> thing to do, independently from the race condition, which will > >>>> be fixed in future patch. > >>>> > >>>> Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > >>>> Link: > >>>> https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com > >>>> / > >>>> Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > >>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > >>>> --- > >>>> > >>>> Changes in v2: > >>>> - don't create platform devices when "vdd-supply" is missing, > >>>> > >>>> rather than returning an error from _find_onboard_hub() > >>>> > >>>> - check for "vdd-supply" not "vdd" (Johan) > >>>> - updated subject and commit message > >>>> - added 'Link' tag (regzbot) > >>>> > >>>> drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > >>>> 1 file changed, 13 insertions(+) > >>> > >>> I'm a tad bit skeptical. > >>> > >>> It somehow feels a bit too much like "inside knowledge" to add this > >>> here. I guess the "onboard_usb_hub_pdevs.c" is already pretty > >>> entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file > >>> keep the absolute minimum amount of stuff in it and all of the > >>> details > >>> be in the other file. > >>> > >>> If this was the only issue though, I'd be tempted to let it slide. As > >>> it is, I'm kinda worried that your patch will break Alexander Stein, > >>> who should have been CCed (I've CCed him now) or Icenowy Zheng (also > >>> CCed now). I believe those folks are using the USB hub driver > >>> primarily to drive a reset GPIO. Looking at the example in the > >>> bindings for one of them (genesys,gl850g.yaml), I even see that the > >>> reset-gpio is specified but not a vdd-supply. I think you'll break > >>> that? > >> > >> Well technically in my final DT a regulator is included (to have the > >> Vbus enabled when enabling the hub), however I am still against this > >> patch, because the driver should work w/o vdd-supply (or w/o reset- > >> gpios), and changing this behavior is a DT binding stability breakage. > > > > I second that. The bindings don't require neither vdd-supply nor > > reset-gpios. > > > > But I have to admit I lack to understand the purpose of this series in the > > first place. What is the benefit or fix? > > did you followed the provided link from the patch? Ah, I didn't notice that. Thanks. Admittedly I've yet to fully understand that race condition, but Matthias already suspects this series might not be enough, even for boards which do use vdd-supply. Just for the record, this series breaks my board if, as suspected by Doug Anderson and Icenowy Zheng, there is no vdd-supply. The reset line will never be touched. Best regards, Alexander > Best regards > > > Best regards, > > Alexader > > > >> In addition the kernel never fails because of a lacking regulator > >> unless explicitly forbid dummy regulators. > >> > >> BTW USB is a discoverable bus, and if a hub do not need special > >> handlement, it just does not need to appear in the DT, thus no onboard > >> hub DT node. > >> > >>> In general, it feels like it should actually be fine to create the > >>> USB > >>> hub driver even if vdd isn't supplied. Sure, it won't do a lot, but > >>> it > >>> shouldn't actively hurt anything. You'll just be turning off and on > >>> bogus regulators and burning a few CPU cycles. I guess the problem is > >>> some race condition that you talk about in the commit message. I'd > >>> rather see that fixed... That being said, if we want to be more > >>> efficient and not burn CPU cycles and memory in Stefan Wahren's case, > >>> maybe the USB hub driver itself could return a canonical error value > >>> from its probe when it detects that it has no useful job and then > >>> "onboard_usb_hub_pdevs" could just silently bail out? > >> > >> I agree.
On Thu, Dec 22, 2022 at 11:26:26AM -0800, Doug Anderson wrote: > Hi, > > On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > The primary task of the onboard_usb_hub driver is to control the > > power of an onboard USB hub. The driver gets the regulator from the > > device tree property "vdd-supply" of the hub's DT node. Some boards > > have device tree nodes for USB hubs supported by this driver, but > > don't specify a "vdd-supply". This is not an error per se, it just > > means that the onboard hub driver can't be used for these hubs, so > > don't create platform devices for such nodes. > > > > This change doesn't completely fix the reported regression. It > > should fix it for the RPi 3 B Plus and boards with similar hub > > configurations (compatible DT nodes without "vdd-supply"), boards > > that actually use the onboard hub driver could still be impacted > > by the race conditions discussed in that thread. Not creating the > > platform devices for nodes without "vdd-supply" is the right > > thing to do, independently from the race condition, which will > > be fixed in future patch. > > > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > Link: https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > > > Changes in v2: > > - don't create platform devices when "vdd-supply" is missing, > > rather than returning an error from _find_onboard_hub() > > - check for "vdd-supply" not "vdd" (Johan) > > - updated subject and commit message > > - added 'Link' tag (regzbot) > > > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > I'm a tad bit skeptical. > > It somehow feels a bit too much like "inside knowledge" to add this > here. I guess the "onboard_usb_hub_pdevs.c" is already pretty > entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file > keep the absolute minimum amount of stuff in it and all of the details > be in the other file. > > If this was the only issue though, I'd be tempted to let it slide. As > it is, I'm kinda worried that your patch will break Alexander Stein, > who should have been CCed (I've CCed him now) or Icenowy Zheng (also > CCed now). I believe those folks are using the USB hub driver > primarily to drive a reset GPIO. Looking at the example in the > bindings for one of them (genesys,gl850g.yaml), I even see that the > reset-gpio is specified but not a vdd-supply. I think you'll break > that? Thanks for pointing that out. My assumption was that the regulator is needed for the driver to do anything useful, but you are right, the reset GPIO alone could be used in combination with an always-on regulator to 'switch the hub on and off'. > In general, it feels like it should actually be fine to create the USB > hub driver even if vdd isn't supplied. Sure, it won't do a lot, but it > shouldn't actively hurt anything. You'll just be turning off and on > bogus regulators and burning a few CPU cycles. I guess the problem is > some race condition that you talk about in the commit message. I'd > rather see that fixed... Yes, the race conditions needs to be fixed as well, I didn't have enough time to write and test a patch before taking a longer break for the holidays, so I only sent out this (supposed) partial mitigation. > That being said, if we want to be more efficient and not burn CPU cycles > and memory in Stefan Wahren's case, maybe the USB hub driver itself could > return a canonical error value from its probe when it detects that it has > no useful job and then "onboard_usb_hub_pdevs" could just silently bail > out? _probe() could return an error, however onboard_hub_create_pdevs() can't rely on that, since the actual onboard_hub driver might not have been loaded yet when the function is called. It would be nice not to instantiate the pdev and onboard_hub USB instances if the DT node has neither a 'vdd-supply' nor 'reset-gpios'. If we aren't ok with doing that in onboard_hub_create_pdevs() then at least the 'raw' platform device would be created. onboard_hub_probe() could still bail if both properties are absent, _find_onboard_hub() would have to check it again to avoid the deferred probing 'loop' for the USB instances. Alternatively we could 'just' fix the race condition involving the 'attach work' and the onboard_hub driver is fully instantiated even on (certain) boards where it does nothing. It's relatively rare that USB hub nodes are specified in the DT (unless the intention is to use this driver) and CONFIG_USB_ONBOARD_HUB needs to be set for the instances to be created, so maybe creating the useless instances is not such a big deal.
On Fri, Dec 23, 2022 at 03:46:45PM +0800, Icenowy Zheng wrote: > 在 2022-12-22星期四的 11:26 -0800，Doug Anderson写道： > > Hi, > > > > On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> > > wrote: > > > > > > The primary task of the onboard_usb_hub driver is to control the > > > power of an onboard USB hub. The driver gets the regulator from the > > > device tree property "vdd-supply" of the hub's DT node. Some boards > > > have device tree nodes for USB hubs supported by this driver, but > > > don't specify a "vdd-supply". This is not an error per se, it just > > > means that the onboard hub driver can't be used for these hubs, so > > > don't create platform devices for such nodes. > > > > > > This change doesn't completely fix the reported regression. It > > > should fix it for the RPi 3 B Plus and boards with similar hub > > > configurations (compatible DT nodes without "vdd-supply"), boards > > > that actually use the onboard hub driver could still be impacted > > > by the race conditions discussed in that thread. Not creating the > > > platform devices for nodes without "vdd-supply" is the right > > > thing to do, independently from the race condition, which will > > > be fixed in future patch. > > > > > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > > Link: > > > https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ > > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > --- > > > > > > Changes in v2: > > > - don't create platform devices when "vdd-supply" is missing, > > > rather than returning an error from _find_onboard_hub() > > > - check for "vdd-supply" not "vdd" (Johan) > > > - updated subject and commit message > > > - added 'Link' tag (regzbot) > > > > > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > I'm a tad bit skeptical. > > > > It somehow feels a bit too much like "inside knowledge" to add this > > here. I guess the "onboard_usb_hub_pdevs.c" is already pretty > > entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file > > keep the absolute minimum amount of stuff in it and all of the > > details > > be in the other file. > > > > If this was the only issue though, I'd be tempted to let it slide. As > > it is, I'm kinda worried that your patch will break Alexander Stein, > > who should have been CCed (I've CCed him now) or Icenowy Zheng (also > > CCed now). I believe those folks are using the USB hub driver > > primarily to drive a reset GPIO. Looking at the example in the > > bindings for one of them (genesys,gl850g.yaml), I even see that the > > reset-gpio is specified but not a vdd-supply. I think you'll break > > that? > > Well technically in my final DT a regulator is included (to have the > Vbus enabled when enabling the hub), however I am still against this > patch, because the driver should work w/o vdd-supply (or w/o reset- > gpios), and changing this behavior is a DT binding stability breakage. Agreed that the driver should work with either 'vdd-supply' or 'reset-gpios' missing, however it won't do anything useful if neither of them is specified. So if the driver wasn't instantiated in this case there would be no behavioral change or DT binding stability breakage. > In addition the kernel never fails because of a lacking regulator > unless explicitly forbid dummy regulators. It wouldn't be an actual failure if the driver really has nothing to do, userspace wouldn't see any differences, besides missing sysfs entries for the onboard_hub pdev and USB devices. > BTW USB is a discoverable bus, and if a hub do not need special > handlement, it just does not need to appear in the DT, thus no onboard > hub DT node. That was my assumption when writing this driver, however there are rare cases where hub nodes are specified without intention to use the onboard_hub driver: https://lore.kernel.org/all/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ > > In general, it feels like it should actually be fine to create the > > USB > > hub driver even if vdd isn't supplied. Sure, it won't do a lot, but > > it > > shouldn't actively hurt anything. You'll just be turning off and on > > bogus regulators and burning a few CPU cycles. I guess the problem is > > some race condition that you talk about in the commit message. I'd > > rather see that fixed... That being said, if we want to be more > > efficient and not burn CPU cycles and memory in Stefan Wahren's case, > > maybe the USB hub driver itself could return a canonical error value > > from its probe when it detects that it has no useful job and then > > "onboard_usb_hub_pdevs" could just silently bail out? > > I agree. >
On Mon, Jan 02, 2023 at 03:38:19PM +0100, Alexander Stein wrote: > Hi Stefan, > > Am Montag, 2. Januar 2023, 12:44:33 CET schrieb Stefan Wahren: > > Hello Alexander, > > > > Am 02.01.23 um 10:20 schrieb Alexander Stein: > > > Hi everybody, > > > > > > Am Freitag, 23. Dezember 2022, 08:46:45 CET schrieb Icenowy Zheng: > > >> 在 2022-12-22星期四的 11:26 -0800，Doug Anderson写道： > > >> > > >>> Hi, > > >>> > > >>> On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> > > >>> > > >>> wrote: > > >>>> The primary task of the onboard_usb_hub driver is to control the > > >>>> power of an onboard USB hub. The driver gets the regulator from the > > >>>> device tree property "vdd-supply" of the hub's DT node. Some boards > > >>>> have device tree nodes for USB hubs supported by this driver, but > > >>>> don't specify a "vdd-supply". This is not an error per se, it just > > >>>> means that the onboard hub driver can't be used for these hubs, so > > >>>> don't create platform devices for such nodes. > > >>>> > > >>>> This change doesn't completely fix the reported regression. It > > >>>> should fix it for the RPi 3 B Plus and boards with similar hub > > >>>> configurations (compatible DT nodes without "vdd-supply"), boards > > >>>> that actually use the onboard hub driver could still be impacted > > >>>> by the race conditions discussed in that thread. Not creating the > > >>>> platform devices for nodes without "vdd-supply" is the right > > >>>> thing to do, independently from the race condition, which will > > >>>> be fixed in future patch. > > >>>> > > >>>> Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > >>>> Link: > > >>>> https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com > > >>>> / > > >>>> Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > >>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > >>>> --- > > >>>> > > >>>> Changes in v2: > > >>>> - don't create platform devices when "vdd-supply" is missing, > > >>>> > > >>>> rather than returning an error from _find_onboard_hub() > > >>>> > > >>>> - check for "vdd-supply" not "vdd" (Johan) > > >>>> - updated subject and commit message > > >>>> - added 'Link' tag (regzbot) > > >>>> > > >>>> drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > > >>>> 1 file changed, 13 insertions(+) > > >>> > > >>> I'm a tad bit skeptical. > > >>> > > >>> It somehow feels a bit too much like "inside knowledge" to add this > > >>> here. I guess the "onboard_usb_hub_pdevs.c" is already pretty > > >>> entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file > > >>> keep the absolute minimum amount of stuff in it and all of the > > >>> details > > >>> be in the other file. > > >>> > > >>> If this was the only issue though, I'd be tempted to let it slide. As > > >>> it is, I'm kinda worried that your patch will break Alexander Stein, > > >>> who should have been CCed (I've CCed him now) or Icenowy Zheng (also > > >>> CCed now). I believe those folks are using the USB hub driver > > >>> primarily to drive a reset GPIO. Looking at the example in the > > >>> bindings for one of them (genesys,gl850g.yaml), I even see that the > > >>> reset-gpio is specified but not a vdd-supply. I think you'll break > > >>> that? > > >> > > >> Well technically in my final DT a regulator is included (to have the > > >> Vbus enabled when enabling the hub), however I am still against this > > >> patch, because the driver should work w/o vdd-supply (or w/o reset- > > >> gpios), and changing this behavior is a DT binding stability breakage. > > > > > > I second that. The bindings don't require neither vdd-supply nor > > > reset-gpios. > > > > > > But I have to admit I lack to understand the purpose of this series in the > > > first place. What is the benefit or fix? > > > > did you followed the provided link from the patch? > > Ah, I didn't notice that. Thanks. Admittedly I've yet to fully understand that > race condition, but Matthias already suspects this series might not be enough, > even for boards which do use vdd-supply. > > Just for the record, this series breaks my board if, as suspected by Doug > Anderson and Icenowy Zheng, there is no vdd-supply. The reset line will never > be touched. Yes, I missed that a reset GPIO could be used in combination with an always-on regulator (instead of specifiying 'vdd-supply') to 'switch the hub on and off'. If we proceed with the general approach of this patch then creation of the pdev should only be skipped when neither 'vdd-supply' nor 'reset-gpios' is specified.
On Fri, Dec 23, 2022 at 03:01:54PM +0100, Johan Hovold wrote: > On Thu, Dec 22, 2022 at 02:26:44AM +0000, Matthias Kaehlcke wrote: > > The primary task of the onboard_usb_hub driver is to control the > > power of an onboard USB hub. The driver gets the regulator from the > > device tree property "vdd-supply" of the hub's DT node. Some boards > > have device tree nodes for USB hubs supported by this driver, but > > don't specify a "vdd-supply". This is not an error per se, it just > > means that the onboard hub driver can't be used for these hubs, so > > don't create platform devices for such nodes. > > > > This change doesn't completely fix the reported regression. It > > should fix it for the RPi 3 B Plus and boards with similar hub > > configurations (compatible DT nodes without "vdd-supply"), boards > > that actually use the onboard hub driver could still be impacted > > by the race conditions discussed in that thread. Not creating the > > platform devices for nodes without "vdd-supply" is the right > > thing to do, independently from the race condition, which will > > be fixed in future patch. > > > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > Link: https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > > > Changes in v2: > > - don't create platform devices when "vdd-supply" is missing, > > rather than returning an error from _find_onboard_hub() > > - check for "vdd-supply" not "vdd" (Johan) > > Please try to remember to CC people providing feedback on your patches. Ack > > - updated subject and commit message > > - added 'Link' tag (regzbot) > > > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/usb/misc/onboard_usb_hub_pdevs.c b/drivers/usb/misc/onboard_usb_hub_pdevs.c > > index ed22a18f4ab7..8cea53b0907e 100644 > > --- a/drivers/usb/misc/onboard_usb_hub_pdevs.c > > +++ b/drivers/usb/misc/onboard_usb_hub_pdevs.c > > @@ -101,6 +101,19 @@ void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *p > > } > > } > > > > + /* > > + * The primary task of the onboard_usb_hub driver is to control > > + * the power of an USB onboard hub. Some boards have device tree > > + * nodes for USB hubs supported by this driver, but don't > > + * specify a "vdd-supply", which is needed by the driver. This is > > + * not a DT error per se, it just means that the onboard hub > > + * driver can't be used with these nodes, so don't create a > > + * a platform device for such a node. > > + */ > > + if (!of_get_property(np, "vdd-supply", NULL) && > > + !of_get_property(npc, "vdd-supply", NULL)) > > + goto node_put; > > So as I mentioned elsewhere, this doesn't look right. It is the > responsibility of the platform driver to manage its resources and it may > not even need a supply. > > I see now that you have already matched on the compatible property above > so that you only create the platform device for the devices that (may) > need it. > > It seems the assumptions that this driver was written under needs to be > revisited. The assumption was that the driver should always be used when the DT has nodes with the supported compatible strings. It turns out that is not entirely correct, in rare cases (like the RPi 3 B Plus) the nodes weren't added with the onboard_hub driver in mind and may lack DT properties that are needed by the driver. I see essentially two possible ways of dealing with DT nodes that don't have all the information to make the onboard_hub driver do something useful: 1) don't instantiate the driver when certain DT properties don't exist (the approach of this patch) 2) instantiate the driver regardless. Not ideal, but such DTs should be relatively rare (+ CONFIG_USB_ONBOARD_HUB needs to be enabled for instantiation to happen), so maybe it's not a big deal
Hi Matthias, Am Dienstag, 3. Januar 2023, 18:12:24 CET schrieb Matthias Kaehlcke: > On Thu, Dec 22, 2022 at 11:26:26AM -0800, Doug Anderson wrote: > > Hi, > > > > On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > The primary task of the onboard_usb_hub driver is to control the > > > power of an onboard USB hub. The driver gets the regulator from the > > > device tree property "vdd-supply" of the hub's DT node. Some boards > > > have device tree nodes for USB hubs supported by this driver, but > > > don't specify a "vdd-supply". This is not an error per se, it just > > > means that the onboard hub driver can't be used for these hubs, so > > > don't create platform devices for such nodes. > > > > > > This change doesn't completely fix the reported regression. It > > > should fix it for the RPi 3 B Plus and boards with similar hub > > > configurations (compatible DT nodes without "vdd-supply"), boards > > > that actually use the onboard hub driver could still be impacted > > > by the race conditions discussed in that thread. Not creating the > > > platform devices for nodes without "vdd-supply" is the right > > > thing to do, independently from the race condition, which will > > > be fixed in future patch. > > > > > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > > Link: > > > https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com > > > / Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > --- > > > > > > Changes in v2: > > > - don't create platform devices when "vdd-supply" is missing, > > > > > > rather than returning an error from _find_onboard_hub() > > > > > > - check for "vdd-supply" not "vdd" (Johan) > > > - updated subject and commit message > > > - added 'Link' tag (regzbot) > > > > > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > I'm a tad bit skeptical. > > > > It somehow feels a bit too much like "inside knowledge" to add this > > here. I guess the "onboard_usb_hub_pdevs.c" is already pretty > > entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file > > keep the absolute minimum amount of stuff in it and all of the details > > be in the other file. > > > > If this was the only issue though, I'd be tempted to let it slide. As > > it is, I'm kinda worried that your patch will break Alexander Stein, > > who should have been CCed (I've CCed him now) or Icenowy Zheng (also > > CCed now). I believe those folks are using the USB hub driver > > primarily to drive a reset GPIO. Looking at the example in the > > bindings for one of them (genesys,gl850g.yaml), I even see that the > > reset-gpio is specified but not a vdd-supply. I think you'll break > > that? > > Thanks for pointing that out. My assumption was that the regulator is > needed for the driver to do anything useful, but you are right, the > reset GPIO alone could be used in combination with an always-on regulator > to 'switch the hub on and off'. > > > In general, it feels like it should actually be fine to create the USB > > hub driver even if vdd isn't supplied. Sure, it won't do a lot, but it > > shouldn't actively hurt anything. You'll just be turning off and on > > bogus regulators and burning a few CPU cycles. I guess the problem is > > some race condition that you talk about in the commit message. I'd > > rather see that fixed... > > Yes, the race conditions needs to be fixed as well, I didn't have enough > time to write and test a patch before taking a longer break for the > holidays, so I only sent out this (supposed) partial mitigation. > > > That being said, if we want to be more efficient and not burn CPU cycles > > and memory in Stefan Wahren's case, maybe the USB hub driver itself could > > return a canonical error value from its probe when it detects that it has > > no useful job and then "onboard_usb_hub_pdevs" could just silently bail > > out? > > _probe() could return an error, however onboard_hub_create_pdevs() can't > rely on that, since the actual onboard_hub driver might not have been > loaded yet when the function is called. > > It would be nice not to instantiate the pdev and onboard_hub USB instances > if the DT node has neither a 'vdd-supply' nor 'reset-gpios'. If we aren't > ok with doing that in onboard_hub_create_pdevs() then at least the 'raw' > platform device would be created. onboard_hub_probe() could still > bail if both properties are absent, _find_onboard_hub() would have to > check it again to avoid the deferred probing 'loop' for the USB instances. I'm not really fond of checking for optional features like 'vdd-supply' and 'reset-gpios'. This issue will pop up again if some new optional feature is added again, e.g. power-domains. > Alternatively we could 'just' fix the race condition involving the 'attach > work' and the onboard_hub driver is fully instantiated even on (certain) > boards where it does nothing. It's relatively rare that USB hub nodes are > specified in the DT (unless the intention is to use this driver) and > CONFIG_USB_ONBOARD_HUB needs to be set for the instances to be created, > so maybe creating the useless instances is not such a big deal. IMHO creating a pdev shouldn't harm in any case. It's similar to having a DT node without a corresponding driver enabled or even existing. Also adding USB devices to DT is something which is to be expected. usb-device.yaml exists since 2020 and the txt version since 2016. Unfortunately I'm not able to reproduce this issue on a different platform where the same HUB but no reset-gpios is required. I also noticed that onboard-usb-hub raises the error > Failed to attach USB driver: -22 for each hub it is supposed to support. Best regards, Alexander
On Wed, Jan 04, 2023 at 10:00:43AM +0100, Alexander Stein wrote: > Hi Matthias, > > Am Dienstag, 3. Januar 2023, 18:12:24 CET schrieb Matthias Kaehlcke: > > On Thu, Dec 22, 2022 at 11:26:26AM -0800, Doug Anderson wrote: > > > Hi, > > > > > > On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> > wrote: > > > > The primary task of the onboard_usb_hub driver is to control the > > > > power of an onboard USB hub. The driver gets the regulator from the > > > > device tree property "vdd-supply" of the hub's DT node. Some boards > > > > have device tree nodes for USB hubs supported by this driver, but > > > > don't specify a "vdd-supply". This is not an error per se, it just > > > > means that the onboard hub driver can't be used for these hubs, so > > > > don't create platform devices for such nodes. > > > > > > > > This change doesn't completely fix the reported regression. It > > > > should fix it for the RPi 3 B Plus and boards with similar hub > > > > configurations (compatible DT nodes without "vdd-supply"), boards > > > > that actually use the onboard hub driver could still be impacted > > > > by the race conditions discussed in that thread. Not creating the > > > > platform devices for nodes without "vdd-supply" is the right > > > > thing to do, independently from the race condition, which will > > > > be fixed in future patch. > > > > > > > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > > > Link: > > > > https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com > > > > / Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > > --- > > > > > > > > Changes in v2: > > > > - don't create platform devices when "vdd-supply" is missing, > > > > > > > > rather than returning an error from _find_onboard_hub() > > > > > > > > - check for "vdd-supply" not "vdd" (Johan) > > > > - updated subject and commit message > > > > - added 'Link' tag (regzbot) > > > > > > > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > > > > 1 file changed, 13 insertions(+) > > > > > > I'm a tad bit skeptical. > > > > > > It somehow feels a bit too much like "inside knowledge" to add this > > > here. I guess the "onboard_usb_hub_pdevs.c" is already pretty > > > entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file > > > keep the absolute minimum amount of stuff in it and all of the details > > > be in the other file. > > > > > > If this was the only issue though, I'd be tempted to let it slide. As > > > it is, I'm kinda worried that your patch will break Alexander Stein, > > > who should have been CCed (I've CCed him now) or Icenowy Zheng (also > > > CCed now). I believe those folks are using the USB hub driver > > > primarily to drive a reset GPIO. Looking at the example in the > > > bindings for one of them (genesys,gl850g.yaml), I even see that the > > > reset-gpio is specified but not a vdd-supply. I think you'll break > > > that? > > > > Thanks for pointing that out. My assumption was that the regulator is > > needed for the driver to do anything useful, but you are right, the > > reset GPIO alone could be used in combination with an always-on regulator > > to 'switch the hub on and off'. > > > > > In general, it feels like it should actually be fine to create the USB > > > hub driver even if vdd isn't supplied. Sure, it won't do a lot, but it > > > shouldn't actively hurt anything. You'll just be turning off and on > > > bogus regulators and burning a few CPU cycles. I guess the problem is > > > some race condition that you talk about in the commit message. I'd > > > rather see that fixed... > > > > Yes, the race conditions needs to be fixed as well, I didn't have enough > > time to write and test a patch before taking a longer break for the > > holidays, so I only sent out this (supposed) partial mitigation. > > > > > That being said, if we want to be more efficient and not burn CPU cycles > > > and memory in Stefan Wahren's case, maybe the USB hub driver itself could > > > return a canonical error value from its probe when it detects that it has > > > no useful job and then "onboard_usb_hub_pdevs" could just silently bail > > > out? > > > > _probe() could return an error, however onboard_hub_create_pdevs() can't > > rely on that, since the actual onboard_hub driver might not have been > > loaded yet when the function is called. > > > > It would be nice not to instantiate the pdev and onboard_hub USB instances > > if the DT node has neither a 'vdd-supply' nor 'reset-gpios'. If we aren't > > ok with doing that in onboard_hub_create_pdevs() then at least the 'raw' > > platform device would be created. onboard_hub_probe() could still > > bail if both properties are absent, _find_onboard_hub() would have to > > check it again to avoid the deferred probing 'loop' for the USB instances. > > I'm not really fond of checking for optional features like 'vdd-supply' and > 'reset-gpios'. This issue will pop up again if some new optional feature is > added again, e.g. power-domains. It's not just any optional feature, it needs to be involved in controlling power. I'm not super-exited about it either, but if we prefer not to instantiate the drivers for certain DT nodes (TBD if that's a preference), we need some sort of sentinel since the compatible string alone doesn't provide enough information. > > Alternatively we could 'just' fix the race condition involving the 'attach > > work' and the onboard_hub driver is fully instantiated even on (certain) > > boards where it does nothing. It's relatively rare that USB hub nodes are > > specified in the DT (unless the intention is to use this driver) and > > CONFIG_USB_ONBOARD_HUB needs to be set for the instances to be created, > > so maybe creating the useless instances is not such a big deal. > > IMHO creating a pdev shouldn't harm in any case. It's similar to having a DT > node without a corresponding driver enabled or even existing. If we only want a 'raw' pdev (no instantiation of the onboard hub platform and USB drivers) then a similar logic will be needed in the onboard hub driver(s). So if we don't want any logic checking that at least one power related property is defined then we have to accept that the onboard hub driver will be fully instantiated even when it effectively does nothing. If we add logic to the driver it needs to be checked in both the platform and the USB driver (in the latter to avoid a deferred probe loop). It would be simpler to just skip the creation of the 'raw' platform device in the first place. > Also adding USB devices to DT is something which is to be expected. > usb-device.yaml exists since 2020 and the txt version since 2016. Yes it it perfectly legal, so we need to handle this case somehow, and we are currently discussing how to best do that :) I still don't expect the combo of supported hub in the DT + CONFIG_USB_ONBOARD_HUB=y/m to become super-popular, which could be an argument for the option "just instantiate the drivers even if they do nothing". Not my favorite option, but probably not that bad either. > Unfortunately I'm not able to reproduce this issue on a different platform > where the same HUB but no reset-gpios is required. I also noticed that > onboard-usb-hub raises the error > > Failed to attach USB driver: -22 > for each hub it is supposed to support. Interesting I also see the error with v6.2-rc1 but not a downstream v5.15 kernel which runs most of the time on my boards. It turns out that with v6.2-rc1 the 'bus' field of 'onboard_hub_usbdev_driver.drvwrap.driver' (passed to driver_attach()) is NULL, which causes driver_attach() / bus_for_each_dev() to return -EINVAL. I did some testing (unbind/bind, unloading/reloading the driver) around the 'attach work' independently from your report. I couldn't repro a situation where the onboard_hub USB devices aren't probed by the driver, which is what the 'attach work' is supposed to solve. At some point I observed issues with that in the past, which is why driver_attach() is called. The driver_attach() call was added to the onboard_hub series in early 2021, by that time I was probably testing with a v5.4 kernel, it's not unconceivable that the issue I saw back then is fixed in newer kernels. With that I was already considering to remove the 'attach work', the error you reported reinforces that, since the driver_attach() call from the onboard_hub driver does nothing in more recent kernels due to 'bus' being NULL. Thanks Matthias
On Wed, Jan 04, 2023 at 07:37:37PM +0000, Matthias Kaehlcke wrote: > On Wed, Jan 04, 2023 at 10:00:43AM +0100, Alexander Stein wrote: > > Hi Matthias, > > > > Am Dienstag, 3. Januar 2023, 18:12:24 CET schrieb Matthias Kaehlcke: > > > On Thu, Dec 22, 2022 at 11:26:26AM -0800, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> > > wrote: > > > > > The primary task of the onboard_usb_hub driver is to control the > > > > > power of an onboard USB hub. The driver gets the regulator from the > > > > > device tree property "vdd-supply" of the hub's DT node. Some boards > > > > > have device tree nodes for USB hubs supported by this driver, but > > > > > don't specify a "vdd-supply". This is not an error per se, it just > > > > > means that the onboard hub driver can't be used for these hubs, so > > > > > don't create platform devices for such nodes. > > > > > > > > > > This change doesn't completely fix the reported regression. It > > > > > should fix it for the RPi 3 B Plus and boards with similar hub > > > > > configurations (compatible DT nodes without "vdd-supply"), boards > > > > > that actually use the onboard hub driver could still be impacted > > > > > by the race conditions discussed in that thread. Not creating the > > > > > platform devices for nodes without "vdd-supply" is the right > > > > > thing to do, independently from the race condition, which will > > > > > be fixed in future patch. > > > > > > > > > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > > > > Link: > > > > > https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com > > > > > / Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > > > --- > > > > > > > > > > Changes in v2: > > > > > - don't create platform devices when "vdd-supply" is missing, > > > > > > > > > > rather than returning an error from _find_onboard_hub() > > > > > > > > > > - check for "vdd-supply" not "vdd" (Johan) > > > > > - updated subject and commit message > > > > > - added 'Link' tag (regzbot) > > > > > > > > > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > > > > > 1 file changed, 13 insertions(+) > > > > > > > > I'm a tad bit skeptical. > > > > > > > > It somehow feels a bit too much like "inside knowledge" to add this > > > > here. I guess the "onboard_usb_hub_pdevs.c" is already pretty > > > > entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file > > > > keep the absolute minimum amount of stuff in it and all of the details > > > > be in the other file. > > > > > > > > If this was the only issue though, I'd be tempted to let it slide. As > > > > it is, I'm kinda worried that your patch will break Alexander Stein, > > > > who should have been CCed (I've CCed him now) or Icenowy Zheng (also > > > > CCed now). I believe those folks are using the USB hub driver > > > > primarily to drive a reset GPIO. Looking at the example in the > > > > bindings for one of them (genesys,gl850g.yaml), I even see that the > > > > reset-gpio is specified but not a vdd-supply. I think you'll break > > > > that? > > > > > > Thanks for pointing that out. My assumption was that the regulator is > > > needed for the driver to do anything useful, but you are right, the > > > reset GPIO alone could be used in combination with an always-on regulator > > > to 'switch the hub on and off'. > > > > > > > In general, it feels like it should actually be fine to create the USB > > > > hub driver even if vdd isn't supplied. Sure, it won't do a lot, but it > > > > shouldn't actively hurt anything. You'll just be turning off and on > > > > bogus regulators and burning a few CPU cycles. I guess the problem is > > > > some race condition that you talk about in the commit message. I'd > > > > rather see that fixed... > > > > > > Yes, the race conditions needs to be fixed as well, I didn't have enough > > > time to write and test a patch before taking a longer break for the > > > holidays, so I only sent out this (supposed) partial mitigation. > > > > > > > That being said, if we want to be more efficient and not burn CPU cycles > > > > and memory in Stefan Wahren's case, maybe the USB hub driver itself could > > > > return a canonical error value from its probe when it detects that it has > > > > no useful job and then "onboard_usb_hub_pdevs" could just silently bail > > > > out? > > > > > > _probe() could return an error, however onboard_hub_create_pdevs() can't > > > rely on that, since the actual onboard_hub driver might not have been > > > loaded yet when the function is called. > > > > > > It would be nice not to instantiate the pdev and onboard_hub USB instances > > > if the DT node has neither a 'vdd-supply' nor 'reset-gpios'. If we aren't > > > ok with doing that in onboard_hub_create_pdevs() then at least the 'raw' > > > platform device would be created. onboard_hub_probe() could still > > > bail if both properties are absent, _find_onboard_hub() would have to > > > check it again to avoid the deferred probing 'loop' for the USB instances. > > > > I'm not really fond of checking for optional features like 'vdd-supply' and > > 'reset-gpios'. This issue will pop up again if some new optional feature is > > added again, e.g. power-domains. > > It's not just any optional feature, it needs to be involved in controlling > power. I'm not super-exited about it either, but if we prefer not to > instantiate the drivers for certain DT nodes (TBD if that's a preference), we > need some sort of sentinel since the compatible string alone doesn't provide > enough information. > > > > Alternatively we could 'just' fix the race condition involving the 'attach > > > work' and the onboard_hub driver is fully instantiated even on (certain) > > > boards where it does nothing. It's relatively rare that USB hub nodes are > > > specified in the DT (unless the intention is to use this driver) and > > > CONFIG_USB_ONBOARD_HUB needs to be set for the instances to be created, > > > so maybe creating the useless instances is not such a big deal. > > > > IMHO creating a pdev shouldn't harm in any case. It's similar to having a DT > > node without a corresponding driver enabled or even existing. > > If we only want a 'raw' pdev (no instantiation of the onboard hub platform and > USB drivers) then a similar logic will be needed in the onboard hub driver(s). > > So if we don't want any logic checking that at least one power related property > is defined then we have to accept that the onboard hub driver will be fully > instantiated even when it effectively does nothing. > > If we add logic to the driver it needs to be checked in both the platform and > the USB driver (in the latter to avoid a deferred probe loop). It would be > simpler to just skip the creation of the 'raw' platform device in the first > place. > > > Also adding USB devices to DT is something which is to be expected. > > usb-device.yaml exists since 2020 and the txt version since 2016. > > Yes it it perfectly legal, so we need to handle this case somehow, and we > are currently discussing how to best do that :) > > I still don't expect the combo of supported hub in the DT + > CONFIG_USB_ONBOARD_HUB=y/m to become super-popular, which could be an > argument for the option "just instantiate the drivers even if they do > nothing". Not my favorite option, but probably not that bad either. > > > Unfortunately I'm not able to reproduce this issue on a different platform > > where the same HUB but no reset-gpios is required. I also noticed that > > onboard-usb-hub raises the error > > > Failed to attach USB driver: -22 > > for each hub it is supposed to support. > > Interesting > > I also see the error with v6.2-rc1 but not a downstream v5.15 kernel which > runs most of the time on my boards. It turns out that with v6.2-rc1 the 'bus' > field of 'onboard_hub_usbdev_driver.drvwrap.driver' (passed to driver_attach()) > is NULL, which causes driver_attach() / bus_for_each_dev() to return -EINVAL. With v6.2-rc1 the platform device probes before the USB driver is registered, that's why 'bus' is NULL. The platform driver is registered first since the USB driver sorta depends on it, but it should be ok to reverse the order (the USB device defers probing when the platform dev isn't ready yet) if we were to keep the attach work. As of now I'm still leaning towards removing the attach stuff since it doesn't seem to be needed with recent-ish kernels. > I did some testing (unbind/bind, unloading/reloading the driver) around the > 'attach work' independently from your report. I couldn't repro a situation > where the onboard_hub USB devices aren't probed by the driver, which is what > the 'attach work' is supposed to solve. At some point I observed issues with > that in the past, which is why driver_attach() is called. The driver_attach() > call was added to the onboard_hub series in early 2021, by that time I was > probably testing with a v5.4 kernel, it's not unconceivable that the issue I > saw back then is fixed in newer kernels. > > With that I was already considering to remove the 'attach work', the error you > reported reinforces that, since the driver_attach() call from the onboard_hub > driver does nothing in more recent kernels due to 'bus' being NULL. > > Thanks > > Matthias
Hi Matthias, Am 04.01.23 um 20:37 schrieb Matthias Kaehlcke: > On Wed, Jan 04, 2023 at 10:00:43AM +0100, Alexander Stein wrote: >> Hi Matthias, >> >> Am Dienstag, 3. Januar 2023, 18:12:24 CET schrieb Matthias Kaehlcke: >>> On Thu, Dec 22, 2022 at 11:26:26AM -0800, Doug Anderson wrote: >>>> Hi, >>>> >>>> On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> >> wrote: >>>>> The primary task of the onboard_usb_hub driver is to control the >>>>> power of an onboard USB hub. The driver gets the regulator from the >>>>> device tree property "vdd-supply" of the hub's DT node. Some boards >>>>> have device tree nodes for USB hubs supported by this driver, but >>>>> don't specify a "vdd-supply". This is not an error per se, it just >>>>> means that the onboard hub driver can't be used for these hubs, so >>>>> don't create platform devices for such nodes. >>>>> >>>>> This change doesn't completely fix the reported regression. It >>>>> should fix it for the RPi 3 B Plus and boards with similar hub >>>>> configurations (compatible DT nodes without "vdd-supply"), boards >>>>> that actually use the onboard hub driver could still be impacted >>>>> by the race conditions discussed in that thread. Not creating the >>>>> platform devices for nodes without "vdd-supply" is the right >>>>> thing to do, independently from the race condition, which will >>>>> be fixed in future patch. >>>>> >>>>> Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") >>>>> Link: >>>>> https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com >>>>> / Reported-by: Stefan Wahren <stefan.wahren@i2se.com> >>>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> >>>>> --- >>>>> >>>>> Changes in v2: >>>>> - don't create platform devices when "vdd-supply" is missing, >>>>> >>>>> rather than returning an error from _find_onboard_hub() >>>>> >>>>> - check for "vdd-supply" not "vdd" (Johan) >>>>> - updated subject and commit message >>>>> - added 'Link' tag (regzbot) >>>>> >>>>> drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ >>>>> 1 file changed, 13 insertions(+) >>>> I'm a tad bit skeptical. >>>> >>>> It somehow feels a bit too much like "inside knowledge" to add this >>>> here. I guess the "onboard_usb_hub_pdevs.c" is already pretty >>>> entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file >>>> keep the absolute minimum amount of stuff in it and all of the details >>>> be in the other file. >>>> >>>> If this was the only issue though, I'd be tempted to let it slide. As >>>> it is, I'm kinda worried that your patch will break Alexander Stein, >>>> who should have been CCed (I've CCed him now) or Icenowy Zheng (also >>>> CCed now). I believe those folks are using the USB hub driver >>>> primarily to drive a reset GPIO. Looking at the example in the >>>> bindings for one of them (genesys,gl850g.yaml), I even see that the >>>> reset-gpio is specified but not a vdd-supply. I think you'll break >>>> that? >>> Thanks for pointing that out. My assumption was that the regulator is >>> needed for the driver to do anything useful, but you are right, the >>> reset GPIO alone could be used in combination with an always-on regulator >>> to 'switch the hub on and off'. >>> >>>> In general, it feels like it should actually be fine to create the USB >>>> hub driver even if vdd isn't supplied. Sure, it won't do a lot, but it >>>> shouldn't actively hurt anything. You'll just be turning off and on >>>> bogus regulators and burning a few CPU cycles. I guess the problem is >>>> some race condition that you talk about in the commit message. I'd >>>> rather see that fixed... >>> Yes, the race conditions needs to be fixed as well, I didn't have enough >>> time to write and test a patch before taking a longer break for the >>> holidays, so I only sent out this (supposed) partial mitigation. >>> >>>> That being said, if we want to be more efficient and not burn CPU cycles >>>> and memory in Stefan Wahren's case, maybe the USB hub driver itself could >>>> return a canonical error value from its probe when it detects that it has >>>> no useful job and then "onboard_usb_hub_pdevs" could just silently bail >>>> out? >>> _probe() could return an error, however onboard_hub_create_pdevs() can't >>> rely on that, since the actual onboard_hub driver might not have been >>> loaded yet when the function is called. >>> >>> It would be nice not to instantiate the pdev and onboard_hub USB instances >>> if the DT node has neither a 'vdd-supply' nor 'reset-gpios'. If we aren't >>> ok with doing that in onboard_hub_create_pdevs() then at least the 'raw' >>> platform device would be created. onboard_hub_probe() could still >>> bail if both properties are absent, _find_onboard_hub() would have to >>> check it again to avoid the deferred probing 'loop' for the USB instances. >> I'm not really fond of checking for optional features like 'vdd-supply' and >> 'reset-gpios'. This issue will pop up again if some new optional feature is >> added again, e.g. power-domains. > It's not just any optional feature, it needs to be involved in controlling > power. I'm not super-exited about it either, but if we prefer not to > instantiate the drivers for certain DT nodes (TBD if that's a preference), we > need some sort of sentinel since the compatible string alone doesn't provide > enough information. > >>> Alternatively we could 'just' fix the race condition involving the 'attach >>> work' and the onboard_hub driver is fully instantiated even on (certain) >>> boards where it does nothing. It's relatively rare that USB hub nodes are >>> specified in the DT (unless the intention is to use this driver) and >>> CONFIG_USB_ONBOARD_HUB needs to be set for the instances to be created, >>> so maybe creating the useless instances is not such a big deal. >> IMHO creating a pdev shouldn't harm in any case. It's similar to having a DT >> node without a corresponding driver enabled or even existing. > If we only want a 'raw' pdev (no instantiation of the onboard hub platform and > USB drivers) then a similar logic will be needed in the onboard hub driver(s). > > So if we don't want any logic checking that at least one power related property > is defined then we have to accept that the onboard hub driver will be fully > instantiated even when it effectively does nothing. > > If we add logic to the driver it needs to be checked in both the platform and > the USB driver (in the latter to avoid a deferred probe loop). It would be > simpler to just skip the creation of the 'raw' platform device in the first > place. > >> Also adding USB devices to DT is something which is to be expected. >> usb-device.yaml exists since 2020 and the txt version since 2016. > Yes it it perfectly legal, so we need to handle this case somehow, and we > are currently discussing how to best do that :) > > I still don't expect the combo of supported hub in the DT + > CONFIG_USB_ONBOARD_HUB=y/m to become super-popular, which could be an > argument for the option "just instantiate the drivers even if they do > nothing". Not my favorite option, but probably not that bad either. i disagree in this point. The driver becomes more and more popular [1] and this breaks arm64 for RPi 3B+ too. So it's only a question of time until distributions run into this problem. I willing to help in debugging the real issue, but i need a little bit guidance here. [1] - https://lore.kernel.org/linux-arm-kernel/2188024.ZfL8zNpBrT@steina-w/T/ > >> Unfortunately I'm not able to reproduce this issue on a different platform >> where the same HUB but no reset-gpios is required. I also noticed that >> onboard-usb-hub raises the error >>> Failed to attach USB driver: -22 >> for each hub it is supposed to support. > Interesting > > I also see the error with v6.2-rc1 but not a downstream v5.15 kernel which > runs most of the time on my boards. It turns out that with v6.2-rc1 the 'bus' > field of 'onboard_hub_usbdev_driver.drvwrap.driver' (passed to driver_attach()) > is NULL, which causes driver_attach() / bus_for_each_dev() to return -EINVAL. > > I did some testing (unbind/bind, unloading/reloading the driver) around the > 'attach work' independently from your report. I couldn't repro a situation > where the onboard_hub USB devices aren't probed by the driver, which is what > the 'attach work' is supposed to solve. At some point I observed issues with > that in the past, which is why driver_attach() is called. The driver_attach() > call was added to the onboard_hub series in early 2021, by that time I was > probably testing with a v5.4 kernel, it's not unconceivable that the issue I > saw back then is fixed in newer kernels. > > With that I was already considering to remove the 'attach work', the error you > reported reinforces that, since the driver_attach() call from the onboard_hub > driver does nothing in more recent kernels due to 'bus' being NULL. > > Thanks > > Matthias
On Thu, Jan 05, 2023 at 08:50:17AM +0100, Stefan Wahren wrote: > Hi Matthias, > > Am 04.01.23 um 20:37 schrieb Matthias Kaehlcke: > > On Wed, Jan 04, 2023 at 10:00:43AM +0100, Alexander Stein wrote: > > > Hi Matthias, > > > > > > Am Dienstag, 3. Januar 2023, 18:12:24 CET schrieb Matthias Kaehlcke: > > > > On Thu, Dec 22, 2022 at 11:26:26AM -0800, Doug Anderson wrote: > > > > > Hi, > > > > > > > > > > On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> > > > wrote: > > > > > > The primary task of the onboard_usb_hub driver is to control the > > > > > > power of an onboard USB hub. The driver gets the regulator from the > > > > > > device tree property "vdd-supply" of the hub's DT node. Some boards > > > > > > have device tree nodes for USB hubs supported by this driver, but > > > > > > don't specify a "vdd-supply". This is not an error per se, it just > > > > > > means that the onboard hub driver can't be used for these hubs, so > > > > > > don't create platform devices for such nodes. > > > > > > > > > > > > This change doesn't completely fix the reported regression. It > > > > > > should fix it for the RPi 3 B Plus and boards with similar hub > > > > > > configurations (compatible DT nodes without "vdd-supply"), boards > > > > > > that actually use the onboard hub driver could still be impacted > > > > > > by the race conditions discussed in that thread. Not creating the > > > > > > platform devices for nodes without "vdd-supply" is the right > > > > > > thing to do, independently from the race condition, which will > > > > > > be fixed in future patch. > > > > > > > > > > > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > > > > > Link: > > > > > > https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com > > > > > > / Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > > > > --- > > > > > > > > > > > > Changes in v2: > > > > > > - don't create platform devices when "vdd-supply" is missing, > > > > > > > > > > > > rather than returning an error from _find_onboard_hub() > > > > > > > > > > > > - check for "vdd-supply" not "vdd" (Johan) > > > > > > - updated subject and commit message > > > > > > - added 'Link' tag (regzbot) > > > > > > > > > > > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > > > > > > 1 file changed, 13 insertions(+) > > > > > I'm a tad bit skeptical. > > > > > > > > > > It somehow feels a bit too much like "inside knowledge" to add this > > > > > here. I guess the "onboard_usb_hub_pdevs.c" is already pretty > > > > > entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file > > > > > keep the absolute minimum amount of stuff in it and all of the details > > > > > be in the other file. > > > > > > > > > > If this was the only issue though, I'd be tempted to let it slide. As > > > > > it is, I'm kinda worried that your patch will break Alexander Stein, > > > > > who should have been CCed (I've CCed him now) or Icenowy Zheng (also > > > > > CCed now). I believe those folks are using the USB hub driver > > > > > primarily to drive a reset GPIO. Looking at the example in the > > > > > bindings for one of them (genesys,gl850g.yaml), I even see that the > > > > > reset-gpio is specified but not a vdd-supply. I think you'll break > > > > > that? > > > > Thanks for pointing that out. My assumption was that the regulator is > > > > needed for the driver to do anything useful, but you are right, the > > > > reset GPIO alone could be used in combination with an always-on regulator > > > > to 'switch the hub on and off'. > > > > > > > > > In general, it feels like it should actually be fine to create the USB > > > > > hub driver even if vdd isn't supplied. Sure, it won't do a lot, but it > > > > > shouldn't actively hurt anything. You'll just be turning off and on > > > > > bogus regulators and burning a few CPU cycles. I guess the problem is > > > > > some race condition that you talk about in the commit message. I'd > > > > > rather see that fixed... > > > > Yes, the race conditions needs to be fixed as well, I didn't have enough > > > > time to write and test a patch before taking a longer break for the > > > > holidays, so I only sent out this (supposed) partial mitigation. > > > > > > > > > That being said, if we want to be more efficient and not burn CPU cycles > > > > > and memory in Stefan Wahren's case, maybe the USB hub driver itself could > > > > > return a canonical error value from its probe when it detects that it has > > > > > no useful job and then "onboard_usb_hub_pdevs" could just silently bail > > > > > out? > > > > _probe() could return an error, however onboard_hub_create_pdevs() can't > > > > rely on that, since the actual onboard_hub driver might not have been > > > > loaded yet when the function is called. > > > > > > > > It would be nice not to instantiate the pdev and onboard_hub USB instances > > > > if the DT node has neither a 'vdd-supply' nor 'reset-gpios'. If we aren't > > > > ok with doing that in onboard_hub_create_pdevs() then at least the 'raw' > > > > platform device would be created. onboard_hub_probe() could still > > > > bail if both properties are absent, _find_onboard_hub() would have to > > > > check it again to avoid the deferred probing 'loop' for the USB instances. > > > I'm not really fond of checking for optional features like 'vdd-supply' and > > > 'reset-gpios'. This issue will pop up again if some new optional feature is > > > added again, e.g. power-domains. > > It's not just any optional feature, it needs to be involved in controlling > > power. I'm not super-exited about it either, but if we prefer not to > > instantiate the drivers for certain DT nodes (TBD if that's a preference), we > > need some sort of sentinel since the compatible string alone doesn't provide > > enough information. > > > > > > Alternatively we could 'just' fix the race condition involving the 'attach > > > > work' and the onboard_hub driver is fully instantiated even on (certain) > > > > boards where it does nothing. It's relatively rare that USB hub nodes are > > > > specified in the DT (unless the intention is to use this driver) and > > > > CONFIG_USB_ONBOARD_HUB needs to be set for the instances to be created, > > > > so maybe creating the useless instances is not such a big deal. > > > IMHO creating a pdev shouldn't harm in any case. It's similar to having a DT > > > node without a corresponding driver enabled or even existing. > > If we only want a 'raw' pdev (no instantiation of the onboard hub platform and > > USB drivers) then a similar logic will be needed in the onboard hub driver(s). > > > > So if we don't want any logic checking that at least one power related property > > is defined then we have to accept that the onboard hub driver will be fully > > instantiated even when it effectively does nothing. > > > > If we add logic to the driver it needs to be checked in both the platform and > > the USB driver (in the latter to avoid a deferred probe loop). It would be > > simpler to just skip the creation of the 'raw' platform device in the first > > place. > > > > > Also adding USB devices to DT is something which is to be expected. > > > usb-device.yaml exists since 2020 and the txt version since 2016. > > Yes it it perfectly legal, so we need to handle this case somehow, and we > > are currently discussing how to best do that :) > > > > I still don't expect the combo of supported hub in the DT + > > CONFIG_USB_ONBOARD_HUB=y/m to become super-popular, which could be an > > argument for the option "just instantiate the drivers even if they do > > nothing". Not my favorite option, but probably not that bad either. > > i disagree in this point. The driver becomes more and more popular [1] and > this breaks arm64 for RPi 3B+ too. So it's only a question of time until > distributions run into this problem. There seems to be a misunderstanding, the above option doesn't break anything (as long as the attach race is fixed, which needs to be done anyway). It impacts boards that specify a hub in the DT but *don't* intend to use the driver (neither specify 'vdd-supply' nor 'reset-gpios'). I expect the number of such boards to remain low, since a USB hub is usually not specified in the DT, unless the intention is to use the onboard_hub driver and a few other cases. There are two separate issues/questions: 1) fix the attach race 2) what to do with hubs for which the driver does nothing Possible options: a) instantiate the drivers regardless (current situation) b) don't create 'raw' pdev if the DT node doesn't have certain properties (a evolution of this patch) c) don't create instantiate the onboard_hub pdev and USB devices if the DT node doesn't have certain properties > I willing to help in debugging the real issue, but i need a little bit > guidance here. > > [1] - > https://lore.kernel.org/linux-arm-kernel/2188024.ZfL8zNpBrT@steina-w/T/ > > > > > > Unfortunately I'm not able to reproduce this issue on a different platform > > > where the same HUB but no reset-gpios is required. I also noticed that > > > onboard-usb-hub raises the error > > > > Failed to attach USB driver: -22 > > > for each hub it is supposed to support. > > Interesting > > > > I also see the error with v6.2-rc1 but not a downstream v5.15 kernel which > > runs most of the time on my boards. It turns out that with v6.2-rc1 the 'bus' > > field of 'onboard_hub_usbdev_driver.drvwrap.driver' (passed to driver_attach()) > > is NULL, which causes driver_attach() / bus_for_each_dev() to return -EINVAL. > > > > I did some testing (unbind/bind, unloading/reloading the driver) around the > > 'attach work' independently from your report. I couldn't repro a situation > > where the onboard_hub USB devices aren't probed by the driver, which is what > > the 'attach work' is supposed to solve. At some point I observed issues with > > that in the past, which is why driver_attach() is called. The driver_attach() > > call was added to the onboard_hub series in early 2021, by that time I was > > probably testing with a v5.4 kernel, it's not unconceivable that the issue I > > saw back then is fixed in newer kernels. > > > > With that I was already considering to remove the 'attach work', the error you > > reported reinforces that, since the driver_attach() call from the onboard_hub > > driver does nothing in more recent kernels due to 'bus' being NULL. > > > > Thanks > > > > Matthias
On Wed, Jan 04, 2023 at 07:37:37PM +0000, Matthias Kaehlcke wrote: > On Wed, Jan 04, 2023 at 10:00:43AM +0100, Alexander Stein wrote: > > Hi Matthias, > > > > Am Dienstag, 3. Januar 2023, 18:12:24 CET schrieb Matthias Kaehlcke: > > > On Thu, Dec 22, 2022 at 11:26:26AM -0800, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@chromium.org> > > wrote: > > > > > The primary task of the onboard_usb_hub driver is to control the > > > > > power of an onboard USB hub. The driver gets the regulator from the > > > > > device tree property "vdd-supply" of the hub's DT node. Some boards > > > > > have device tree nodes for USB hubs supported by this driver, but > > > > > don't specify a "vdd-supply". This is not an error per se, it just > > > > > means that the onboard hub driver can't be used for these hubs, so > > > > > don't create platform devices for such nodes. > > > > > > > > > > This change doesn't completely fix the reported regression. It > > > > > should fix it for the RPi 3 B Plus and boards with similar hub > > > > > configurations (compatible DT nodes without "vdd-supply"), boards > > > > > that actually use the onboard hub driver could still be impacted > > > > > by the race conditions discussed in that thread. Not creating the > > > > > platform devices for nodes without "vdd-supply" is the right > > > > > thing to do, independently from the race condition, which will > > > > > be fixed in future patch. > > > > > > > > > > Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") > > > > > Link: > > > > > https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com > > > > > / Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > > > --- > > > > > > > > > > Changes in v2: > > > > > - don't create platform devices when "vdd-supply" is missing, > > > > > > > > > > rather than returning an error from _find_onboard_hub() > > > > > > > > > > - check for "vdd-supply" not "vdd" (Johan) > > > > > - updated subject and commit message > > > > > - added 'Link' tag (regzbot) > > > > > > > > > > drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ > > > > > 1 file changed, 13 insertions(+) > > > > > > > > I'm a tad bit skeptical. > > > > > > > > It somehow feels a bit too much like "inside knowledge" to add this > > > > here. I guess the "onboard_usb_hub_pdevs.c" is already pretty > > > > entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file > > > > keep the absolute minimum amount of stuff in it and all of the details > > > > be in the other file. > > > > > > > > If this was the only issue though, I'd be tempted to let it slide. As > > > > it is, I'm kinda worried that your patch will break Alexander Stein, > > > > who should have been CCed (I've CCed him now) or Icenowy Zheng (also > > > > CCed now). I believe those folks are using the USB hub driver > > > > primarily to drive a reset GPIO. Looking at the example in the > > > > bindings for one of them (genesys,gl850g.yaml), I even see that the > > > > reset-gpio is specified but not a vdd-supply. I think you'll break > > > > that? > > > > > > Thanks for pointing that out. My assumption was that the regulator is > > > needed for the driver to do anything useful, but you are right, the > > > reset GPIO alone could be used in combination with an always-on regulator > > > to 'switch the hub on and off'. > > > > > > > In general, it feels like it should actually be fine to create the USB > > > > hub driver even if vdd isn't supplied. Sure, it won't do a lot, but it > > > > shouldn't actively hurt anything. You'll just be turning off and on > > > > bogus regulators and burning a few CPU cycles. I guess the problem is > > > > some race condition that you talk about in the commit message. I'd > > > > rather see that fixed... > > > > > > Yes, the race conditions needs to be fixed as well, I didn't have enough > > > time to write and test a patch before taking a longer break for the > > > holidays, so I only sent out this (supposed) partial mitigation. > > > > > > > That being said, if we want to be more efficient and not burn CPU cycles > > > > and memory in Stefan Wahren's case, maybe the USB hub driver itself could > > > > return a canonical error value from its probe when it detects that it has > > > > no useful job and then "onboard_usb_hub_pdevs" could just silently bail > > > > out? > > > > > > _probe() could return an error, however onboard_hub_create_pdevs() can't > > > rely on that, since the actual onboard_hub driver might not have been > > > loaded yet when the function is called. > > > > > > It would be nice not to instantiate the pdev and onboard_hub USB instances > > > if the DT node has neither a 'vdd-supply' nor 'reset-gpios'. If we aren't > > > ok with doing that in onboard_hub_create_pdevs() then at least the 'raw' > > > platform device would be created. onboard_hub_probe() could still > > > bail if both properties are absent, _find_onboard_hub() would have to > > > check it again to avoid the deferred probing 'loop' for the USB instances. > > > > I'm not really fond of checking for optional features like 'vdd-supply' and > > 'reset-gpios'. This issue will pop up again if some new optional feature is > > added again, e.g. power-domains. > > It's not just any optional feature, it needs to be involved in controlling > power. I'm not super-exited about it either, but if we prefer not to > instantiate the drivers for certain DT nodes (TBD if that's a preference), we > need some sort of sentinel since the compatible string alone doesn't provide > enough information. > > > > Alternatively we could 'just' fix the race condition involving the 'attach > > > work' and the onboard_hub driver is fully instantiated even on (certain) > > > boards where it does nothing. It's relatively rare that USB hub nodes are > > > specified in the DT (unless the intention is to use this driver) and > > > CONFIG_USB_ONBOARD_HUB needs to be set for the instances to be created, > > > so maybe creating the useless instances is not such a big deal. > > > > IMHO creating a pdev shouldn't harm in any case. It's similar to having a DT > > node without a corresponding driver enabled or even existing. > > If we only want a 'raw' pdev (no instantiation of the onboard hub platform and > USB drivers) then a similar logic will be needed in the onboard hub driver(s). > > So if we don't want any logic checking that at least one power related property > is defined then we have to accept that the onboard hub driver will be fully > instantiated even when it effectively does nothing. > > If we add logic to the driver it needs to be checked in both the platform and > the USB driver (in the latter to avoid a deferred probe loop). It would be > simpler to just skip the creation of the 'raw' platform device in the first > place. > > > Also adding USB devices to DT is something which is to be expected. > > usb-device.yaml exists since 2020 and the txt version since 2016. > > Yes it it perfectly legal, so we need to handle this case somehow, and we > are currently discussing how to best do that :) > > I still don't expect the combo of supported hub in the DT + > CONFIG_USB_ONBOARD_HUB=y/m to become super-popular, which could be an > argument for the option "just instantiate the drivers even if they do > nothing". Not my favorite option, but probably not that bad either. > > > Unfortunately I'm not able to reproduce this issue on a different platform > > where the same HUB but no reset-gpios is required. I also noticed that > > onboard-usb-hub raises the error > > > Failed to attach USB driver: -22 > > for each hub it is supposed to support. > > Interesting > > I also see the error with v6.2-rc1 but not a downstream v5.15 kernel which > runs most of the time on my boards. It turns out that with v6.2-rc1 the 'bus' > field of 'onboard_hub_usbdev_driver.drvwrap.driver' (passed to driver_attach()) > is NULL, which causes driver_attach() / bus_for_each_dev() to return -EINVAL. > > I did some testing (unbind/bind, unloading/reloading the driver) around the > 'attach work' independently from your report. I couldn't repro a situation > where the onboard_hub USB devices aren't probed by the driver, which is what > the 'attach work' is supposed to solve. At some point I observed issues with > that in the past, which is why driver_attach() is called. The driver_attach() > call was added to the onboard_hub series in early 2021, by that time I was > probably testing with a v5.4 kernel, it's not unconceivable that the issue I > saw back then is fixed in newer kernels. I found a configuration with which the driver_attach() call is needed: for a hub that isn't power cycled on un/rebind (e.g. because it as an 'always-on' regulator and no reset GPIO) the USB devices aren't probed by the onboard_hub driver on rebind. I saw this with a test config for nested hubs, where the secondary hub isn't actually an onboard hub, but an external hub (with DT nodes to fake an onboard hub). On an actual onboard config the hub would usually be power cycled, which should fix/mask the issue (the unbound USB devices 'disappear' on unbind and are freshly created on re-bind). The re-attach is probably not super-important for real world configs, but it would still be good to have this working for the odd/test cases.
diff --git a/drivers/usb/misc/onboard_usb_hub_pdevs.c b/drivers/usb/misc/onboard_usb_hub_pdevs.c index ed22a18f4ab7..8cea53b0907e 100644 --- a/drivers/usb/misc/onboard_usb_hub_pdevs.c +++ b/drivers/usb/misc/onboard_usb_hub_pdevs.c @@ -101,6 +101,19 @@ void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *p } } + /* + * The primary task of the onboard_usb_hub driver is to control + * the power of an USB onboard hub. Some boards have device tree + * nodes for USB hubs supported by this driver, but don't + * specify a "vdd-supply", which is needed by the driver. This is + * not a DT error per se, it just means that the onboard hub + * driver can't be used with these nodes, so don't create a + * a platform device for such a node. + */ + if (!of_get_property(np, "vdd-supply", NULL) && + !of_get_property(npc, "vdd-supply", NULL)) + goto node_put; + pdev = of_platform_device_create(np, NULL, &parent_hub->dev); if (!pdev) { dev_err(&parent_hub->dev,
The primary task of the onboard_usb_hub driver is to control the power of an onboard USB hub. The driver gets the regulator from the device tree property "vdd-supply" of the hub's DT node. Some boards have device tree nodes for USB hubs supported by this driver, but don't specify a "vdd-supply". This is not an error per se, it just means that the onboard hub driver can't be used for these hubs, so don't create platform devices for such nodes. This change doesn't completely fix the reported regression. It should fix it for the RPi 3 B Plus and boards with similar hub configurations (compatible DT nodes without "vdd-supply"), boards that actually use the onboard hub driver could still be impacted by the race conditions discussed in that thread. Not creating the platform devices for nodes without "vdd-supply" is the right thing to do, independently from the race condition, which will be fixed in future patch. Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver") Link: https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/ Reported-by: Stefan Wahren <stefan.wahren@i2se.com> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- Changes in v2: - don't create platform devices when "vdd-supply" is missing, rather than returning an error from _find_onboard_hub() - check for "vdd-supply" not "vdd" (Johan) - updated subject and commit message - added 'Link' tag (regzbot) drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)