Messages in this thread | | | From | Doug Anderson <> | Date | Thu, 11 Nov 2021 15:31:31 -0800 | Subject | Re: [PATCH v16 1/7] usb: misc: Add onboard_usb_hub driver |
| |
Hi,
On Fri, Aug 13, 2021 at 12:52 PM Matthias Kaehlcke <mka@chromium.org> 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.
> +ONBOARD USB HUB DRIVER > +M: Matthias Kaehlcke <mka@chromium.org> > +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?
| |