lkml.org 
[lkml]   [2020]   [Sep]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 2/2] USB: misc: Add onboard_usb_hub driver
    Hi Alan,

    thanks for taking time to review!

    On Thu, Sep 17, 2020 at 03:54:16PM -0400, Alan Stern wrote:
    > On Thu, Sep 17, 2020 at 11:46:22AM -0700, Matthias Kaehlcke wrote:
    > > The main issue this driver addresses is that a USB hub needs to be
    > > powered before it can be discovered. For onboard hubs this is often
    > > solved by supplying the hub with an 'always-on' regulator, which is
    > > kind of a hack. Some onboard hubs may require further initialization
    > > steps, like changing the state of a GPIO or enabling a clock, which
    > > requires further hacks. This driver creates a platform device
    > > representing the hub which performs the necessary initialization.
    > > Currently it only supports switching on a single regulator, support
    > > for multiple regulators or other actions can be added as needed.
    > > Different initialization sequences can be supported based on the
    > > compatible string.
    > >
    > > Besides performing the initialization the driver can be configured
    > > to power the hub off during system suspend. This can help to extend
    > > battery life on battery powered devices which have no requirements
    > > to keep the hub powered during suspend. The driver can also be
    > > configured to leave the hub powered when a wakeup capable USB device
    > > is connected when suspending, and power it off otherwise.
    > >
    > > Technically the driver consists of two drivers, the platform driver
    > > described above and a very thin USB driver that subclasses the
    > > generic driver. The purpose of this driver is to provide the platform
    > > driver with the USB devices corresponding to the hub(s) (a hub
    > > controller may provide multiple 'logical' hubs, e.g. one to support
    > > USB 2.0 and another for USB 3.x).
    > >
    > > Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org>
    > > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org>
    > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
    >
    > > +config USB_ONBOARD_HUB
    > > + tristate "Onboard USB hub support"
    > > + depends on OF
    > > + help
    > > + Say Y here if you want to support onboard USB hubs. The driver
    > > + powers supported hubs on and may perform other initialization
    > > + steps.
    >
    > I have a nagging feeling that this description may be too vague for a
    > lot of people to understand. Does everybody know what an "onboard"
    > USB hub is?
    >
    > Consider for example that Intel's current EHCI host controllers all
    > come with a USB hub built into the chipset. That built-in hub
    > certainly could be considered "onboard", but it doesn't need this
    > driver.
    >
    > Maybe also give some examples of devices that require this driver, to
    > help make the idea clear to readers.

    Ok, I'll try to come up with a better description.

    > > +static int __maybe_unused onboard_hub_suspend(struct device *dev)
    > > +{
    > > + struct onboard_hub *hub = dev_get_drvdata(dev);
    > > + struct udev_node *node;
    > > + int rc = 0;
    > > +
    > > + hub->has_wakeup_capable_descendants = false;
    > > +
    > > + if (!hub->power_off_in_suspend)
    > > + return 0;
    > > +
    > > + mutex_lock(&hub->lock);
    > > +
    > > + list_for_each_entry(node, &hub->udev_list, list) {
    > > + if (!device_may_wakeup(node->udev->bus->controller))
    > > + break;
    >
    > You're assuming that node->udev->bus->controller is going to be the
    > same for the nodes on the list, right?

    Yes, that is the assumption, although you have a point that this isn't
    necessarily the case. It's probably true in the vast majority of cases,
    but a hub could be wired up to multiple controllers. I'll change the
    loop to set the flag without breaking, it's a micro-optimization
    anyway.

    > > +
    > > + if (usb_wakeup_enabled_descendants(node->udev)) {
    > > + hub->has_wakeup_capable_descendants = true;
    > > + break;
    > > + }
    > > + }
    > > +
    > > + mutex_unlock(&hub->lock);
    > > +
    > > + if (!hub->has_wakeup_capable_descendants)
    > > + rc = onboard_hub_power_off(hub);
    > > +
    > > + return rc;
    > > +}
    > > +
    > > +static int __maybe_unused onboard_hub_resume(struct device *dev)
    > > +{
    > > + struct onboard_hub *hub = dev_get_drvdata(dev);
    > > + int rc = 0;
    > > +
    > > + if (hub->power_off_in_suspend && !hub->has_wakeup_capable_descendants)
    >
    > Instead of this cumbersome two-condition test, how about simply
    > having a hub->is_powered_on flag? Then
    > hub->has_wakeup_capable_descendants wouldn't be needed.

    Ok, less cumbersome code is always good :)

    > > + rc = onboard_hub_power_on(hub);
    > > +
    > > + return rc;
    > > +}
    >
    > > +static int onboard_hub_remove_usbdev(struct onboard_hub *hub, struct usb_device *udev)
    > > +{
    > > + struct udev_node *node;
    > > +
    > > + mutex_lock(&hub->lock);
    > > +
    > > + list_for_each_entry(node, &hub->udev_list, list) {
    > > + if (node->udev == udev) {
    > > + list_del(&node->list);
    > > + devm_kfree(hub->dev, node);
    >
    > Why have an explicit kfree here but not anywhere else? And if you do
    > have an explicit kfree, why use devm_kzalloc rather than plain kzalloc?

    The motivation of the explicit kfree was to avoid hogging memory if the
    USB device disappears and reappears repeatedly. However this doesn't seem
    to be a very common scenario so maybe we can ignore it.

    > > + break;
    > > + }
    > > + }
    > > +
    > > + mutex_unlock(&hub->lock);
    > > +
    > > + if (node == NULL)
    > > + return -EINVAL;
    >
    > This test is wrong. Look at the definition of list_for_each_entry;
    > node will never be NULL. Probably the best approach is to use a local
    > "ret" variable.

    Ack, thanks for catching!

    > > +
    > > + return 0;
    > > +}
    >
    > > +static int onboard_hub_remove(struct platform_device *pdev)
    > > +{
    > > + struct onboard_hub *hub = dev_get_drvdata(&pdev->dev);
    > > +
    > > + sysfs_remove_file(&pdev->dev.kobj, &dev_attr_power_off_in_suspend.attr);
    > > +
    > > + return onboard_hub_power_off(hub);
    > > +}
    >
    > Shouldn't this routine unbind the onboard_hub_usbdev driver from all
    > the associated devices? Otherwise you end up with more-or-less
    > dangling references to hub (I say more-or-less because with the devm
    > allocations, the structures will hang around as zombies for a while).

    True, the dangling references aren't a good idea. Initially I thought
    that the USB devices holding a reference of the hub device would prevent
    this, but apparently that was wishful thinking. IIUC unbinding would be
    done through device_driver_detach().

    > Relying on the onboard_hub_power_off call to do this for you isn't a
    > great idea, because the effect won't happen immediately.
    >
    > > +static int onboard_hub_usbdev_probe(struct usb_device *udev)
    > > +{
    > > + struct device *dev = &udev->dev;
    > > + struct onboard_hub *hub;
    > > +
    > > + /* ignore supported hubs without device tree node */
    > > + if (!dev->of_node)
    > > + return -ENODEV;
    > > +
    > > + hub = _find_onboard_hub(dev);
    > > + if (IS_ERR(hub))
    > > + return PTR_ERR(dev);
    > > +
    > > + dev_set_drvdata(dev, hub);
    > > +
    > > + onboard_hub_add_usbdev(hub, udev);
    >
    > Ignoring the return code? Then why does that routine return int rather
    > than void?

    Ok, will abort if the function returns an error.

    > > +
    > > + return 0;
    > > +}
    > > +
    > > +static void onboard_hub_usbdev_disconnect(struct usb_device *udev)
    > > +{
    > > + struct onboard_hub *hub = dev_get_drvdata(&udev->dev);
    > > +
    > > + onboard_hub_remove_usbdev(hub, udev);
    >
    > Ditto.

    In this case it's probably better to change the return type to void, since
    there is not really an alternative course of action.

    > > +
    > > + put_device(hub->dev);
    >
    > Is there a matching get_device somewhere (like in _find_onboard_hub)?
    > If so, I didn't see it. And I don't see any reason for it.

    Yes, implicitly, of_find_device_by_node() "takes a reference to the
    embedded struct device which needs to be dropped after use."

    \
     
     \ /
      Last update: 2020-09-22 02:42    [W:3.757 / U:0.228 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site