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@xxxxxxxxxxxx>
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@xxxxxxxx/
Reported-by: Stefan Wahren <stefan.wahren@xxxxxxxx>
Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
---
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.
