by Matthias Kaehlcke

[permalink] [raw]

Subject: Re: [PATCH v16 1/7] usb: misc: Add onboard_usb_hub driver

Hi Doug,

thanks for the thorough review!

On Thu, Nov 11, 2021 at 03:31:31PM -0800, Doug Anderson wrote:
> Hi,
>
> On Fri, Aug 13, 2021 at 12:52 PM Matthias Kaehlcke <[email protected]> wrote:
> >
> > +++ b/Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub
> > @@ -0,0 +1,8 @@
> > +What: /sys/bus/platform/devices/<dev>/always_powered_in_suspend
> > +Date: March 2021
> > +KernelVersion: 5.13
>
> I dunno how stuff like this is usually managed, but March 2021 and
> 5.13 is no longer correct.

will update, though it's not unlikely it will go stale again before this
series lands.

> > +ONBOARD USB HUB DRIVER
> > +M: Matthias Kaehlcke <[email protected]>
> > +L: [email protected]
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
>
> I'm confused. Where is this .yaml file? It doesn't look landed and it
> doesn't look to be in your series.

It's a leftover from the early days of the series, when the driver had
it's own binding, I'll remove it.

> I guess this should be updated to:
>
> F: Documentation/devicetree/bindings/usb/realtek,rts5411.yaml

Not sure about that, the rts5411 binding could exist without this driver.

> Also: should this have:
>
> F: Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub

ack

> > +struct udev_node {
> > + struct usb_device *udev;
> > + struct list_head list;
> > +};
>
> nit: 'udev' has a whole different connotation to me. Maybe just go
> with `usbdev_node` ?

Will change to 'usbdev_dev' node as suggested, I think it's ok to keep
'udev' for the pointer to the USB device itself, since that abbreviation
is used commonly in USB kernel land.

> > +static int __maybe_unused onboard_hub_suspend(struct device *dev)
> > +{
> > + struct onboard_hub *hub = dev_get_drvdata(dev);
> > + struct udev_node *node;
> > + bool power_off;
> > + int rc = 0;
> > +
> > + if (hub->always_powered_in_suspend)
> > + return 0;
> > +
> > + power_off = true;
> > +
> > + mutex_lock(&hub->lock);
> > +
> > + list_for_each_entry(node, &hub->udev_list, list) {
> > + if (!device_may_wakeup(node->udev->bus->controller))
> > + continue;
> > +
> > + if (usb_wakeup_enabled_descendants(node->udev)) {
> > + power_off = false;
> > + break;
> > + }
> > + }
> > +
> > + mutex_unlock(&hub->lock);
> > +
> > + if (power_off)
> > + rc = onboard_hub_power_off(hub);
> > +
> > + return rc;
>
> optional nit: get rid of "rc" and write the above as:
>
> if (power_off)
> return onboard_hub_power_off(hub);
>
> return 0;

ok, I plan to revert the suggested logic though and bail out 'early' if there
is nothing to do.

> > +static int __maybe_unused onboard_hub_resume(struct device *dev)
> > +{
> > + struct onboard_hub *hub = dev_get_drvdata(dev);
> > + int rc = 0;
> > +
> > + if (!hub->is_powered_on)
> > + rc = onboard_hub_power_on(hub);
> > +
> > + return rc;
>
> optional nit: get rid of "rc" and write the above as:
>
> if (!hub->is_powered_on)
> return onboard_hub_power_on(hub);
>
> return 0;

ok, same as above

> > +static void onboard_hub_remove_usbdev(struct onboard_hub *hub, struct usb_device *udev)
> > +{
> > + struct udev_node *node;
> > + char link_name[64];
> > +
> > + snprintf(link_name, sizeof(link_name), "usb_dev.%s", dev_name(&udev->dev));
> > + sysfs_remove_link(&hub->dev->kobj, link_name);
>
> I would be at least moderately worried about the duplicate snprintf
> between here and the add function. Any way that could be a helper?

I'll add a helper

> > +static struct onboard_hub *_find_onboard_hub(struct device *dev)
> > +{
> > + struct platform_device *pdev;
> > + struct device_node *np;
> > + phandle ph;
> > +
> > + pdev = of_find_device_by_node(dev->of_node);
> > + if (!pdev) {
> > + if (of_property_read_u32(dev->of_node, "companion-hub", &ph)) {
> > + dev_err(dev, "failed to read 'companion-hub' property\n");
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + np = of_find_node_by_phandle(ph);
> > + if (!np) {
> > + dev_err(dev, "failed to find device node for companion hub\n");
> > + return ERR_PTR(-EINVAL);
> > + }
>
> Aren't the above two calls equivalent to this?
>
> npc = of_parse_phandle(dev->of_node, "companion-hub", 0)

Indeed, will use of_parse_phandle() instead

> > +
> > + pdev = of_find_device_by_node(np);
> > + of_node_put(np);
> > +
> > + if (!pdev)
> > + return ERR_PTR(-EPROBE_DEFER);
>
> Shouldn't you also defer if the dev_get_drvdata() returns NULL? What
> if you're racing the probe of the platform device?

Yeah, it seems that race could happen. IIUC we could use device_is_bound()
to check if probing completed, really_probe() calls driver_bound() only
after successfully probing the device.

> > + }
> > +
> > + put_device(&pdev->dev);
> > +
> > + return dev_get_drvdata(&pdev->dev);
>
> It feels like it would be safer to call dev_get_drvdata() before
> putting the device? ...and actually, are you sure you should even be
> putting the device? Maybe we should wait to put it until
> onboard_hub_usbdev_disconnect()

It shouldn't be necessary, when the platform device is destroyed it
unbinds the associated USB devices (see onboard_hub_remove()), hence
they don't keep using the drvdata. There was a related discussion in
the early days of this series: https://lkml.org/lkml/2020/9/21/2153

> > +static struct usb_device_driver onboard_hub_usbdev_driver = {
> > +
> > + .name = "onboard-usb-hub",
>
> Remove the extra blank line at the start of the structure?

ok

> > +void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *pdev_list)
> > +{
> > + int i;
> > + phandle ph;
> > + struct device_node *np, *npc;
> > + struct platform_device *pdev;
> > + struct pdev_list_entry *pdle;
>
> Should the `INIT_LIST_HEAD(pdev_list);` go here? Is there any reason
> why we need to push this into the caller?

That would limit pdev_list to a single entry, which is not what we want. A
parent hub might have multiple compatible onboard hubs connected to it.

> > + for (i = 1; i <= parent_hub->maxchild; i++) {
> > + np = usb_of_get_device_node(parent_hub, i);
> > + if (!np)
> > + continue;
> > +
> > + if (!of_is_onboard_usb_hub(np))
> > + goto node_put;
> > +
> > + if (of_property_read_u32(np, "companion-hub", &ph))
> > + goto node_put;
> > +
> > + npc = of_find_node_by_phandle(ph);
> > + if (!npc)
> > + goto node_put;
>
> Aren't the above two calls equivalent to this?
>
> npc = of_parse_phandle(np, "companion-hub", 0)

yes, will change to of_parse_phandle()

> I'm also curious why a companion-hub is a _required_ property.
> Couldn't you support USB 2.0 hubs better by just allowing
> companion-hub to be optional? I guess that could be a future
> improvement, but it also seems trivial to support from the start.

The evolution of this driver somewhat tied it to xHCI, however that
isn't strictly necessary. In a sense it is nice when 'companion-hub'
is mandatory, since things can get messy if it is forgotten when it
should be there.

The property should be mandatory in the bindings of the USB >= 3.0
hubs that are supported by this driver, but it could be optional
for USB 2.0 hubs. Instead of doing the enforcement in the driver
it could be limited to checking a DT against the bindings in .yaml.
It's also an option to make it mandatory in the driver through a
list of compatible strings / VIDs/PIDs.

> > + pdev = of_find_device_by_node(npc);
> > + of_node_put(npc);
> > +
> > + if (pdev) {
> > + /* the companion hub already has a platform device, nothing to do here */
> > + put_device(&pdev->dev);
> > + goto node_put;
> > + }
> > +
> > + pdev = of_platform_device_create(np, NULL, &parent_hub->dev);
> > + if (pdev) {
> > + pdle = kzalloc(sizeof(*pdle), GFP_KERNEL);
>
> Maybe devm_kzalloc(&pdev->dev, GFP_KERNEL) ? Then you can get rid of
> the free in the destroy function?

it feels a bit sneaky to do it after creation instead of probe(), but I guess
it's fine.

> > + if (!pdle)
> > + goto node_put;
>
> If your memory allocation fails here, don't you need to
> of_platform_device_destroy() ?

right, will call of_platform_device_destroy() in case of failure

> > + INIT_LIST_HEAD(&pdle->node);
>
> I don't believe that the INIT_LIST_HEAD() does anything useful here.
> &pdle->node is not a list head--it's a list element. Adding it to the
> end of the existing list will fully initialize its ->next and ->prev
> pointers but won't look at what they were.

indeed, will remove

> > + pdle->pdev = pdev;
> > + list_add(&pdle->node, pdev_list);
> > + } else {
> > + dev_err(&parent_hub->dev,
> > + "failed to create platform device for onboard hub '%s'\n",
> > + of_node_full_name(np));
>
> Use "%pOF" instead of open-coding.

ack

> > +void onboard_hub_destroy_pdevs(struct list_head *pdev_list)
> > +{
> > + struct pdev_list_entry *pdle, *tmp;
> > +
> > + list_for_each_entry_safe(pdle, tmp, pdev_list, node) {
> > + of_platform_device_destroy(&pdle->pdev->dev, NULL);
> > + kfree(pdle);
>
> It feels like you should be removing the node from the list too,
> right? Otherwise if you unbind / bind the USB driver you'll still have
> garbage in your list the 2nd time?

Could catch, it seems I limited testing to a single removal ...

2021-11-16 18:07:35

by Matthias Kaehlcke

[permalink] [raw]

Subject: Re: [PATCH v16 5/7] usb: Specify dependencies on USB_XHCI_PLATFORM with 'depends on'

On Thu, Nov 11, 2021 at 03:48:06PM -0800, Doug Anderson wrote:
> Hi,
>
> On Fri, Aug 13, 2021 at 12:52 PM Matthias Kaehlcke <[email protected]> wrote:
> >
> > config USB_DWC3
> > tristate "DesignWare USB3 DRD Core Support"
> > - depends on (USB || USB_GADGET) && HAS_DMA
> > - select USB_XHCI_PLATFORM if USB_XHCI_HCD
> > + depends on ((USB && USB_XHCI_PLATFORM) || USB_GADGET) && HAS_DMA
>
> Technically you don't need the "USB &&", right? Since
> USB_XHCI_PLATFORM is defined in 'usb/host/Kconfig' and that's only
> even included if USB is defined. So it can be just:
>
> depends on (USB_XHCI_PLATFORM || USB_GADGET) && HAS_DMA

True, the dependency on USB isn't strictly needed.

> That's not terribly important, though, so:
>
> Reviewed-by: Douglas Anderson <[email protected]>

Thanks!