Message-ID: <20200922004158.GC21107@google.com>
Date:   Mon, 21 Sep 2020 17:41:58 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        linux-kernel@...r.kernel.org,
        Douglas Anderson <dianders@...omium.org>,
        linux-usb@...r.kernel.org, Bastien Nocera <hadess@...ess.net>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        devicetree@...r.kernel.org,
        Ravi Chandra Sadineni <ravisadineni@...omium.org>,
        Peter Chen <peter.chen@....com>,
        Stephen Boyd <swboyd@...omium.org>,
        "Alexander A. Klimov" <grandmaster@...klimov.de>,
        Masahiro Yamada <masahiroy@...nel.org>
Subject: Re: [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@...omium.org>
> > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@...omium.org>
> > Signed-off-by: Matthias Kaehlcke <mka@...omium.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."

Powered by blists - more mailing lists