Re: [PATCH v4 2/2] USB: misc: Add onboard_usb_hub driver

[Matthias Kaehlcke <mka@chromium.org>]

Hi Doug,

On Mon, Sep 28, 2020 at 03:03:20PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Sep 28, 2020 at 10:14 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > +static ssize_t power_off_in_suspend_show(struct device *dev, struct device_attribute *attr,
> > +                          char *buf)
> > +{
> > +       struct onboard_hub *hub = dev_get_drvdata(dev);
> > +
> > +       return sprintf(buf, "%d\n", hub->power_off_in_suspend);
> > +}
> > +
> > +static ssize_t power_off_in_suspend_store(struct device *dev, struct device_attribute *attr,
> > +                           const char *buf, size_t count)
> > +{
> > +       struct onboard_hub *hub = dev_get_drvdata(dev);
> > +       bool val;
> > +       int ret;
> > +
> > +       ret = kstrtobool(buf, &val);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       hub->power_off_in_suspend = val;
> > +
> > +       return count;
> > +}
> > +static DEVICE_ATTR_RW(power_off_in_suspend);
> 
> I wish there was a short name that meant "try to power off in suspend
> unless there's an active wakeup source underneath you".  The name here
> is a bit misleading since we might keep this powered if there's an
> active wakeup source even if "power_off_in_suspend" is true...  I
> wonder if it's easier to describe the opposite, like
> "always_power_in_suspend".  Then, if that's false, it'll be in
> "automatic" mode and if it's true it'll always keep powered.

I agree that the name is somewhat misleading and it's hard find something
concise. 'always_power_in_suspend' would certainly be more correct, it
would make it slightly harder to configure the 'always power off' case
though, since you would have to make sure that USB wakeup is disabled. IIUC
this should be the default though (unless explicitly enabled), so probably
it's not so bad. I'm somewhat undecided between 'always_power_in_suspend'
and 'keep_powered_in_suspend'.

> I guess you can also argue about what the default should be.  I guess
> if you just left it as zero-initted then we'd (by default) power off
> in suspend.  To me that seems like a saner default, but I'm probably
> biased.

I tend to agree, though yes, you could make a valid argument for either
value.

> > +static int onboard_hub_remove(struct platform_device *pdev)
> > +{
> > +       struct onboard_hub *hub = dev_get_drvdata(&pdev->dev);
> > +       struct udev_node *node;
> > +       struct usb_device *udev;
> > +
> > +       hub->going_away = true;
> > +
> > +       mutex_lock(&hub->lock);
> > +
> > +       /* unbind the USB devices to avoid dangling references to this device */
> > +       while (!list_empty(&hub->udev_list)) {
> > +               node = list_first_entry(&hub->udev_list, struct udev_node, list);
> > +               udev = node->udev;
> > +
> > +               /*
> > +                * Unbinding the driver will call onboard_hub_remove_usbdev(),
> > +                * which acquires hub->lock.  We must release the lock first.
> > +                */
> > +               get_device(&udev->dev);
> > +               mutex_unlock(&hub->lock);
> > +               device_release_driver(&udev->dev);
> > +               put_device(&udev->dev);
> > +               mutex_lock(&hub->lock);
> 
> I didn't try to grok all the removal corner cases since it seems like
> you and Alan have been going over that.  If you feel like this needs
> extra attention then yell and I'll look closer.

Thanks, I think we are good, especially after the additional testing
I did today.

> > +static const struct of_device_id onboard_hub_match[] = {
> > +       { .compatible = "onboard-usb-hub" },
> > +       { .compatible = "realtek,rts5411" },
> 
> You only need "onboard-usb-hub" here.  The bindings still have
> "realtek,rts5411" in them in case we later have to do something
> different/special on that device, but the whole idea of including the
> generic is that we don't need to add every specific instance to this
> table.

right, I'll remove the realtek string in the next version.

> The above is pretty much nits, though, so:
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks!