Re: [PATCH v2 1/2] usb: misc: onboard_usb_hub: Don't create platform devices for DT nodes without 'vdd-supply'

[Matthias Kaehlcke]

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@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)
> 
> 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