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

[Doug Anderson]

Hi,

On Mon, Sep 28, 2020 at 10:14 AM Matthias Kaehlcke <mka@xxxxxxxxxxxx> 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 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.


> +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.


> +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.

The above is pretty much nits, though, so:

Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>