Message-ID: <YqpprpUHmlD62YzI@google.com>
Date:   Wed, 15 Jun 2022 16:22:22 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Felipe Balbi <balbi@...nel.org>,
        Michal Simek <michal.simek@...inx.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Stephen Boyd <swboyd@...omium.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Bastien Nocera <hadess@...ess.net>,
        Peter Chen <peter.chen@...nel.org>,
        Ravi Chandra Sadineni <ravisadineni@...omium.org>,
        Roger Quadros <rogerq@...nel.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Souradeep Chowdhury <quic_schowdhu@...cinc.com>
Subject: Re: [PATCH v22 2/3] usb: misc: Add onboard_usb_hub driver

Hi Doug,

Thanks for the review!

On Wed, Jun 15, 2022 at 02:09:43PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 9, 2022 at 12:20 PM Matthias Kaehlcke <mka@...omium.org> wrote:
> >
> > @@ -11,6 +11,7 @@ usbcore-y += phy.o port.o
> >  usbcore-$(CONFIG_OF)           += of.o
> >  usbcore-$(CONFIG_USB_PCI)              += hcd-pci.o
> >  usbcore-$(CONFIG_ACPI)         += usb-acpi.o
> > +usbcore-$(CONFIG_USB_ONBOARD_HUB)      += ../misc/onboard_usb_hub_pdevs.o
> 
> I'm OK with this solution of just linking the code into the "usbcore"
> if USB folks are. Thinking about it, I guess another way to solve the
> circular dependency is to somehow create some type of generic
> notification scheme where the USB hub driver subscribes to "a hub has
> appeared" notification. ...but I don't personally have any intuition
> about whether people would like that better than your solution.

Not sure if it would be better (if feasible). In any case it would require
the hub to be powered (to be enumerated) which is precisely what the
onboard_usb_hub driver intends to do.

> > +config USB_ONBOARD_HUB
> > +       bool "Onboard USB hub support"
> > +       depends on OF || COMPILE_TEST
> > +       help
> > +         Say Y here if you want to support discrete onboard USB hubs that
> > +         don't require an additional control bus for initialization, but
> > +         need some non-trivial form of initialization, such as enabling a
> > +         power regulator. An example for such a hub is the Realtek
> > +         RTS5411.
> > +
> > +         This driver can be used as a module but its state (module vs
> > +         builtin) must match the state of the USB subsystem. Enabling
> > +         this config will enable the driver and it will automatically
> > +         match the state of the USB subsystem. If this driver is a
> > +         module it will be called onboard_usb_hub.
> > +
> > +if USB_ONBOARD_HUB
> > +config USB_ONBOARD_HUB_ACTUAL
> > +       tristate
> > +       default m if USB=m
> > +       default y if USB=y
> > +endif
> 
> Do you still need to play the games with "_ACTUAL"? The USB core no
> longer calls the hub directly. I think that means you can just "depend
> on USB" and be done with the mess. That allows USB to be builtin and
> USB_ONBOARD_HUB can be a module, right?

Good point, with the pdev creation inside the USB core it shouldn't be
needed anymore.

> > +static int onboard_hub_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct onboard_hub *hub;
> > +       int err;
> > +
> > +       hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
> > +       if (!hub)
> > +               return -ENOMEM;
> > +
> > +       hub->vdd = devm_regulator_get(dev, "vdd");
> > +       if (IS_ERR(hub->vdd))
> > +               return PTR_ERR(hub->vdd);
> > +
> > +       hub->dev = dev;
> > +       mutex_init(&hub->lock);
> > +       INIT_LIST_HEAD(&hub->udev_list);
> > +
> > +       dev_set_drvdata(dev, hub);
> > +
> > +       err = onboard_hub_power_on(hub);
> > +       if (err)
> > +               return err;
> > +
> > +       /*
> > +        * The USB driver might have been detached from the USB devices by
> > +        * onboard_hub_remove(), make sure to re-attach it if needed.
> > +        *
> > +        * This needs to be done deferred to avoid self-deadlocks on systems
> > +        * with nested onboard hubs.
> > +        */
> > +       INIT_WORK(&hub->attach_usb_driver_work, onboard_hub_attach_usb_driver);
> > +       schedule_work(&hub->attach_usb_driver_work);
> 
> I'm sure that the above is totally necessary but it's been long enough
> since I looked at this code last that I've totally forgotten why. Any
> chance you could add comments to say under what situation
> onboard_hub_remove() would have detached the USB driver? Is this
> something where you unbind the platform driver and then bind it again?

Yes, that's the scenario in which I encountered that the hub USB
devices wouldn't be bound to the onboard_hub_driver again.

> ...and why does that cause the driver to be detached?

We call device_release_driver(udev) in onboard_hub_remove() to
make sure the USB devices don't keep a reference to the platform
device after it is removed.

> > +/**
> > + * onboard_hub_create_pdevs -- create platform devices for onboard USB hubs
> > + * @parent_hub : parent hub to scan for connected onboard hubs
> > + * @pdev_list  : list of onboard hub platform devices owned by the parent hub
> > + *
> > + * Creates a platform device for each supported onboard hub that is connected to
> > + * the given parent hub. To keep track of the platform devices they are added to
> > + * a list that is owned by the parent hub.
> 
> I'm ashamed to admit how long it took me to remember why exactly we
> needed a platform device to begin with and why the normal USB devices
> weren't enough (it's because we won't enumerate the USB devices until
> we're powered and so the platform device is in charge of powering
> things up). Finally I re-read the commit message and then it made
> sense, but someone looking at the code later might not think to look
> at the commit message for a while. Maybe remind people in the comments
> for this function? Even if it's somewhere else in the code and I
> missed it, I wouldn't mind a tiny blurb here.

Ok, I can add a short note.

> > +void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *pdev_list)
> > +{
> > +       int i;
> > +       struct usb_hcd *hcd = bus_to_hcd(parent_hub->bus);
> > +       struct device_node *np, *npc;
> > +       struct platform_device *pdev = NULL;
> > +       struct pdev_list_entry *pdle;
> > +
> > +       if (!parent_hub->dev.of_node)
> > +               return;
> > +
> > +       for (i = 1; i <= parent_hub->maxchild; i++) {
> > +               np = usb_of_get_device_node(parent_hub, i);
> > +               if (!np)
> > +                       continue;
> > +
> > +               if (!of_is_onboard_usb_hub(np))
> > +                       goto node_put;
> > +
> > +               npc = of_parse_phandle(np, "companion-hub", 0);
> > +               if (npc) {
> > +                       /*
> > +                        * Hubs with companions share the same platform device.
> > +                        * Create the plaform device only for the hub that is
> > +                        * connected to the primary HCD (directly or through
> > +                        * other hubs).
> > +                        */
> > +                       if (!usb_hcd_is_primary_hcd(hcd)) {
> > +                               of_node_put(npc);
> > +                               goto node_put;
> > +                       }
> > +
> > +                       pdev = of_find_device_by_node(npc);
> > +                       of_node_put(npc);
> > +               } else {
> > +                       /*
> > +                        * For root hubs this function can be called multiple times
> > +                        * for the same root hub node (the HCD node). Make sure only
> > +                        * one platform device is created for this hub.
> > +                        */
> > +                       if (!parent_hub->parent && !usb_hcd_is_primary_hcd(hcd))
> > +                               goto node_put;
> 
> I don't understand the "else" case above. What case exactly are we
> handling again? This is when:
> * the hub is presumably just a 2.0 hub since there is no companion.
> * our parent is the root hub and the USB 2.0 hub we're looking at is
> not the primary

The 'else' case can be entered for hubs connected to a root hub or to another
hub further down in the tree, but we bail out only for first level hubs.

> ...but that doesn't make a lot of sense to me? I must have missed something...

It's not super-obvious, this bit is important: "this function can be called
multiple times for the same root hub node". For any first level hub we only
create a pdev if this function is called on behalf of the primary HCD. That
is also true of a hub connected to the secondary HCD. We only want to create
one pdev and there is supposedly always a primary HCD.

Maybe it would be slightly clearer if the function returned before the loop
if this condition is met. Unfortunately we can't just always ignore the
secondary HCD because:

> In general though, do we even need to look at the "companion-hub"
> property? If this node matches an onboard USB hub and it's the primary
> HCD then we want a platform dev. Otherwise we don't, right?

This wouldn't work for some topologies like:

3.x root hub
  <nothing connected>

2.x root hub
  2.x-only hub

no platform device would be created for the 2.x-only hub since it isn't
connected to the primary HCD

or

3.x root hub
  3.x hub

2.x root hub
  2.x hub
    2.x-only hub

The first level hub would have a platform device for being a companion hub
of the 3.x hub, however no pdev would be created for the second level hub
since the 2.x root hub isn't primary.

Powered by blists - more mailing lists