lkml.org 
[lkml]   [2022]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 1/2] usb: misc: onboard_usb_hub: Don't create platform devices for DT nodes without 'vdd-supply'
    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?

    \
     
     \ /
      Last update: 2023-03-26 23:18    [W:4.076 / U:0.180 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site