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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux