Re: [PATCH v20 3/5] usb: misc: Add onboard_usb_hub driver

[Doug Anderson]

Hi,

On Tue, Feb 8, 2022 at 11:17 AM Matthias Kaehlcke <mka@xxxxxxxxxxxx> 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