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

[Matthias Kaehlcke]

On Mon, Sep 28, 2020 at 02:47:59PM -0400, Alan Stern wrote:
> On Mon, Sep 28, 2020 at 10:13:55AM -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 discrete onboard hubs (an
> > example for such a hub is the Realtek RTS5411) 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 even more 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@xxxxxxxxxxxx>
> > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@xxxxxxxxxxxx>
> > Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> > ---
> 
> Minor cut & paste error:
> 
> > +static int onboard_hub_power_off(struct onboard_hub *hub)
> > +{
> > +	int err;
> > +
> > +	err = regulator_disable(hub->vdd);
> > +	if (err) {
> > +		dev_err(hub->dev, "failed to enable regulator: %d\n", err);
> 
> s/enable/disable/

yup, will fix

> Have you tried manually unbinding and rebinding the two drivers a few
> times to make sure they will still work?

I went through a few dozen bund/unbind cycles for both drivers and things
looked good overall, but then last minute I found that determining whether
wakeup capable devices are connected doesn't always work as (I) expected.
I didn't see this earlier, it seems to be reproduce more easily after
unbinding and rebinding the platform driver.

During development I already noticed that usb_wakeup_enabled_descendants()
returns a cached value, which was a problem for an earlier version of the
driver. The values are updated by hub_suspend(), my (flawed) assumption
was that the USB driver would always suspend before the platform driver.
This generally seems to be the case on my development platform after boot,
but not necessarily after unbinding and rebinding the driver. Using the
_suspend_late hook instead of _suspend seems to be a reliable workaround.

> I'm a little concerned about  all the devm_* stuff in here; does that
> get released when the driver is unbound from the device or when the device
> is unregistered?  And if the latter, what happens if you have multiple
> sysfs attribute groups going at the same time?

The memory gets released when the device is unbound:

device_release_driver
  device_release_driver_internal
    __device_release_driver
      devres_release_all

Anyway, if you prefer I can change the driver to use kmalloc/kfree.

> Apart from those worries and the typo, this looks pretty good to me.
> 
> Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>

Great, thanks for taking the time to review!