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.