Message-ID: <Y7Rh+EkKKN5Gu8sz@google.com>
Date:   Tue, 3 Jan 2023 17:12:24 +0000
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        Stefan Wahren <stefan.wahren@...e.com>, stable@...r.kernel.org,
        Ravi Chandra Sadineni <ravisadineni@...omium.org>,
        Alexander Stein <alexander.stein@...tq-group.com>,
        Icenowy Zheng <uwu@...nowy.me>
Subject: Re: [PATCH v2 1/2] usb: misc: onboard_usb_hub: Don't create platform
 devices for DT nodes without 'vdd-supply'

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@...omium.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@...e.com>
> > Signed-off-by: Matthias Kaehlcke <mka@...omium.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.

Powered by blists - more mailing lists