linux-kernel - Re: [PATCH v20 3/5] usb: misc: Add onboard_usb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=VJmb_P9Rnn0ooNAkf3ese-BEWZr8z-Vc0RMdF=gHmNvw@mail.gmail.com>
Date:   Tue, 15 Feb 2022 10:02:36 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Matthias Kaehlcke <mka@...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>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Peter Chen <peter.chen@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Roger Quadros <rogerq@...nel.org>,
        Michal Simek <michal.simek@...inx.com>,
        Linux USB List <linux-usb@...r.kernel.org>,
        Bastien Nocera <hadess@...ess.net>,
        Ravi Chandra Sadineni <ravisadineni@...omium.org>
Subject: Re: [PATCH v20 3/5] usb: misc: Add onboard_usb_hub driver

Hi,

On Tue, Feb 8, 2022 at 11:17 AM Matthias Kaehlcke <mka@...omium.org> wrote:
> > > +/*
> > > + * Returns the onboard_hub platform device that is associated with the USB
> > > + * device passed as parameter.
> > > + */
> > > +static struct onboard_hub *_find_onboard_hub(struct device *dev)
> > > +{
> > > +   struct platform_device *pdev;
> > > +   struct device_node *np;
> > > +   struct onboard_hub *hub;
> > > +
> > > +   pdev = of_find_device_by_node(dev->of_node);
> > > +   if (!pdev) {
> > > +           np = of_parse_phandle(dev->of_node, "companion-hub", 0);
> > > +           if (!np) {
> > > +                   dev_err(dev, "failed to find device node for companion hub\n");
> > > +                   return ERR_PTR(-EINVAL);
> > > +           }
> > > +
> > > +           pdev = of_find_device_by_node(np);
> > > +           of_node_put(np);
> > > +
> > > +           if (!pdev || !device_is_bound(&pdev->dev)) {
> >
> > I don't understand why you need to call device_is_bound() here.  What
> > are you wanting to find here?
>
> Whether the platform driver actually probed.
>
> > You found the hub device associated to this usb device, based on the
> > of_find_device_by_node() call, so why check it again?  What could have
> > happened that this isn't the correct device?
>
> It is the correct platform device, however it might not have finished
> probing (this function is called from the USB driver). It's an unlikely
> case, but it might happen, especially if the bootloader left the hub
> regulator enabled (otherwise it would only be enabled by the platform
> driver).
>
> If device_is_bound() is a no-go (as it seems) the function could check
> the drvdata instead.

So IMO the answer here is to send a v21:

1. Switch to use drvdata just to you can avoid the controversial
device_is_bound() export.

2. Add a comment here explaining _why_ you are checking the drvdata
and return -EPROBE_DEFER. In general if it's confusing to someone
during a code review it will be confusing to someone later and so
deserves a comment. Something along the lines of: "Just to be
paranoid, we check that the drvdata is set which indicates that the
platform driver has finished probing. This handles the case where
(conceivably) we could be running at the exact same time as the
platform driver's probe. If we detect the race we request probe
deferral and we'll come back and try again."


> > > +   err = onboard_hub_add_usbdev(hub, udev);
> > > +   if (err)
> > > +           return err;
> > > +
> > > +   err = sysfs_create_link(&udev->dev.kobj, &hub->dev->kobj, "onboard_hub_dev");
> >
> > What is this link for?  Messing with sysfs links is a pain and drivers
> > really shouldn't be doing them if at all possible.
>
> Alan asked me to add them. It's not strictly needed. I'm fine with removing
> them as long as there is no strong opposition to that :)

I don't personally care either way. I'd say remove them and they can
be added back later?


> > > +   if (err)
> > > +           dev_warn(&udev->dev, "failed to create symlink to platform device '%s'): %d\n",
> > > +                    dev_name(hub->dev), err);
> > > +
> > > +   return 0;
> >
> > So you ignore the error?  That's fine, just odd.
>
> Yes, the links aren't critical for the functioning of the driver.

If you keep the links in, this is another good place for a comment,
even something as simple as what you just said above about the links
not being critical for the driver to function.

-Doug