Hi,

On Fri, Aug 13, 2021 at 12:52 PM Matthias Kaehlcke [off-list ref] wrote:

I dunno how stuff like this is usually managed, but March 2021 and
5.13 is no longer correct.

+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. I guess this should be updated to:

F: Documentation/devicetree/bindings/usb/realtek,rts5411.yaml

Also: should this have:

F: Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub

+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` ?

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

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

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

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

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

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

+static struct usb_device_driver onboard_hub_usbdev_driver = {
+
+       .name = "onboard-usb-hub",

Remove the extra blank line at the start of the structure?

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

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

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.

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

+                       if (!pdle)
+                               goto node_put;

If your memory allocation fails here, don't you need to
of_platform_device_destroy() ?

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

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

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