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 [off-list ref] wrote:

quoted

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

quoted

+ONBOARD USB HUB DRIVER
+M:     Matthias Kaehlcke [off-list ref]
+L:     linux-usb@vger.kernel.org
+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

quoted

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

quoted

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

quoted

+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

quoted

+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

quoted

+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

quoted

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

quoted

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

quoted

+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

quoted

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

quoted

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

quoted

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

quoted

+                       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

quoted

+                       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

quoted

+                       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

quoted

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