Hi,
On Fri, Aug 13, 2021 at 12:52 PM Matthias Kaehlcke <mka@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> +L: linux-usb@xxxxxxxxxxxxxxx
> +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?
