Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220609192000.990763-1-mka@chromium.org> <20220609121838.v22.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid>
 <CAD=FV=W6erE8ByabmYSL_OWJPKYGqysDMGYQX6j7_PSEYGZ4YQ@mail.gmail.com> <YqpprpUHmlD62YzI@google.com>
In-Reply-To: <YqpprpUHmlD62YzI@google.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 16 Jun 2022 13:12:32 -0700
Message-ID: <CAD=FV=VNDamV4+j07TrnX3cUs2-D5ySbeQ-zfU=Eef8+WagGig@mail.gmail.com>
Subject: Re: [PATCH v22 2/3] usb: misc: Add onboard_usb_hub driver
To:     Matthias Kaehlcke <mka@chromium.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Alan Stern <stern@rowland.harvard.edu>,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>,
        Mathias Nyman <mathias.nyman@intel.com>,
        Felipe Balbi <balbi@kernel.org>,
        Michal Simek <michal.simek@xilinx.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Stephen Boyd <swboyd@chromium.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, Bastien Nocera <hadess@hadess.net>,
        Peter Chen <peter.chen@kernel.org>,
        Ravi Chandra Sadineni <ravisadineni@chromium.org>,
        Roger Quadros <rogerq@kernel.org>,
        Linux USB List <linux-usb@vger.kernel.org>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Souradeep Chowdhury <quic_schowdhu@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Wed, Jun 15, 2022 at 4:22 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> > > +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.

I guess I'm still pretty confused. You say "For root hubs this
function can be called multiple times for the same root hub node".
Does that mean that the function will be called multiple times with
the same "parent_hub", or something else.

Unless it's called with the same "parent_hub" then it seems like if
the USB device has a device tree node and that device tree node is for
a onboard_usb_hub and there's no companion node then we _always_ want
to create the platform device, don't we? If it is called with the same
"parent_hub" then I'm confused how your test does something different
the first time the function is called vs. the 2nd.

-Doug