| Message ID | 20200917114600.v2.1.I248292623d3d0f6a4f0c5bc58478ca3c0062b49a@changeid |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/2] dt-bindings: usb: Add binding for onboard USB hubs | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
On Thu, Sep 17, 2020 at 11:46:22AM -0700, Matthias Kaehlcke wrote: > The main issue this driver addresses is that a USB hub needs to be > powered before it can be discovered. For onboard hubs this is often > solved by supplying the hub with an 'always-on' regulator, which is > kind of a hack. Some onboard hubs may require further initialization > steps, like changing the state of a GPIO or enabling a clock, which > requires further hacks. This driver creates a platform device > representing the hub which performs the necessary initialization. > Currently it only supports switching on a single regulator, support > for multiple regulators or other actions can be added as needed. > Different initialization sequences can be supported based on the > compatible string. > > Besides performing the initialization the driver can be configured > to power the hub off during system suspend. This can help to extend > battery life on battery powered devices which have no requirements > to keep the hub powered during suspend. The driver can also be > configured to leave the hub powered when a wakeup capable USB device > is connected when suspending, and power it off otherwise. > > Technically the driver consists of two drivers, the platform driver > described above and a very thin USB driver that subclasses the > generic driver. The purpose of this driver is to provide the platform > driver with the USB devices corresponding to the hub(s) (a hub > controller may provide multiple 'logical' hubs, e.g. one to support > USB 2.0 and another for USB 3.x). > > Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > +config USB_ONBOARD_HUB > + tristate "Onboard USB hub support" > + depends on OF > + help > + Say Y here if you want to support onboard USB hubs. The driver > + powers supported hubs on and may perform other initialization > + steps. I have a nagging feeling that this description may be too vague for a lot of people to understand. Does everybody know what an "onboard" USB hub is? Consider for example that Intel's current EHCI host controllers all come with a USB hub built into the chipset. That built-in hub certainly could be considered "onboard", but it doesn't need this driver. Maybe also give some examples of devices that require this driver, to help make the idea clear to readers. > +static int __maybe_unused onboard_hub_suspend(struct device *dev) > +{ > + struct onboard_hub *hub = dev_get_drvdata(dev); > + struct udev_node *node; > + int rc = 0; > + > + hub->has_wakeup_capable_descendants = false; > + > + if (!hub->power_off_in_suspend) > + return 0; > + > + mutex_lock(&hub->lock); > + > + list_for_each_entry(node, &hub->udev_list, list) { > + if (!device_may_wakeup(node->udev->bus->controller)) > + break; You're assuming that node->udev->bus->controller is going to be the same for the nodes on the list, right? > + > + if (usb_wakeup_enabled_descendants(node->udev)) { > + hub->has_wakeup_capable_descendants = true; > + break; > + } > + } > + > + mutex_unlock(&hub->lock); > + > + if (!hub->has_wakeup_capable_descendants) > + rc = onboard_hub_power_off(hub); > + > + return rc; > +} > + > +static int __maybe_unused onboard_hub_resume(struct device *dev) > +{ > + struct onboard_hub *hub = dev_get_drvdata(dev); > + int rc = 0; > + > + if (hub->power_off_in_suspend && !hub->has_wakeup_capable_descendants) Instead of this cumbersome two-condition test, how about simply having a hub->is_powered_on flag? Then hub->has_wakeup_capable_descendants wouldn't be needed. > + rc = onboard_hub_power_on(hub); > + > + return rc; > +} > +static int onboard_hub_remove_usbdev(struct onboard_hub *hub, struct usb_device *udev) > +{ > + struct udev_node *node; > + > + mutex_lock(&hub->lock); > + > + list_for_each_entry(node, &hub->udev_list, list) { > + if (node->udev == udev) { > + list_del(&node->list); > + devm_kfree(hub->dev, node); Why have an explicit kfree here but not anywhere else? And if you do have an explicit kfree, why use devm_kzalloc rather than plain kzalloc? > + break; > + } > + } > + > + mutex_unlock(&hub->lock); > + > + if (node == NULL) > + return -EINVAL; This test is wrong. Look at the definition of list_for_each_entry; node will never be NULL. Probably the best approach is to use a local "ret" variable. > + > + return 0; > +} > +static int onboard_hub_remove(struct platform_device *pdev) > +{ > + struct onboard_hub *hub = dev_get_drvdata(&pdev->dev); > + > + sysfs_remove_file(&pdev->dev.kobj, &dev_attr_power_off_in_suspend.attr); > + > + return onboard_hub_power_off(hub); > +} Shouldn't this routine unbind the onboard_hub_usbdev driver from all the associated devices? Otherwise you end up with more-or-less dangling references to hub (I say more-or-less because with the devm allocations, the structures will hang around as zombies for a while). Relying on the onboard_hub_power_off call to do this for you isn't a great idea, because the effect won't happen immediately. > +static int onboard_hub_usbdev_probe(struct usb_device *udev) > +{ > + struct device *dev = &udev->dev; > + struct onboard_hub *hub; > + > + /* ignore supported hubs without device tree node */ > + if (!dev->of_node) > + return -ENODEV; > + > + hub = _find_onboard_hub(dev); > + if (IS_ERR(hub)) > + return PTR_ERR(dev); > + > + dev_set_drvdata(dev, hub); > + > + onboard_hub_add_usbdev(hub, udev); Ignoring the return code? Then why does that routine return int rather than void? > + > + return 0; > +} > + > +static void onboard_hub_usbdev_disconnect(struct usb_device *udev) > +{ > + struct onboard_hub *hub = dev_get_drvdata(&udev->dev); > + > + onboard_hub_remove_usbdev(hub, udev); Ditto. > + > + put_device(hub->dev); Is there a matching get_device somewhere (like in _find_onboard_hub)? If so, I didn't see it. And I don't see any reason for it. Alan Stern
On 20-09-17 11:46:22, Matthias Kaehlcke wrote: > The main issue this driver addresses is that a USB hub needs to be > powered before it can be discovered. For onboard hubs this is often > solved by supplying the hub with an 'always-on' regulator, which is > kind of a hack. Some onboard hubs may require further initialization > steps, like changing the state of a GPIO or enabling a clock, which > requires further hacks. This driver creates a platform device > representing the hub which performs the necessary initialization. > Currently it only supports switching on a single regulator, support > for multiple regulators or other actions can be added as needed. > Different initialization sequences can be supported based on the > compatible string. > > Besides performing the initialization the driver can be configured > to power the hub off during system suspend. This can help to extend > battery life on battery powered devices which have no requirements > to keep the hub powered during suspend. The driver can also be > configured to leave the hub powered when a wakeup capable USB device > is connected when suspending, and power it off otherwise. > > Technically the driver consists of two drivers, the platform driver > described above and a very thin USB driver that subclasses the > generic driver. The purpose of this driver is to provide the platform > driver with the USB devices corresponding to the hub(s) (a hub > controller may provide multiple 'logical' hubs, e.g. one to support > USB 2.0 and another for USB 3.x). > > Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > Changes in v2: > - check wakeup enabled state of the USB controller instead of > using 'wakeup-source' property > - use sysfs attribute instead of DT property to determine if > the hub should be powered off at all during system suspend > - added missing brace in onboard_hub_suspend() > - updated commit message > - use pm_ptr for pm_ops as suggested by Alan > > Changes in v1: > - renamed the driver to 'onboard_usb_hub' > - single file for platform and USB driver > - USB hub devices register with the platform device > - the DT includes a phandle of the platform device > - the platform device now controls when power is turned off > - the USB driver became a very thin subclass of the generic USB > driver > - enabled autosuspend support > > drivers/usb/misc/Kconfig | 15 ++ > drivers/usb/misc/Makefile | 1 + > drivers/usb/misc/onboard_usb_hub.c | 329 +++++++++++++++++++++++++++++ > 3 files changed, 345 insertions(+) > create mode 100644 drivers/usb/misc/onboard_usb_hub.c > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig > index 6818ea689cd9..e941244e24e5 100644 > --- a/drivers/usb/misc/Kconfig > +++ b/drivers/usb/misc/Kconfig > @@ -275,3 +275,18 @@ config USB_CHAOSKEY > > To compile this driver as a module, choose M here: the > module will be called chaoskey. > + > +config USB_ONBOARD_HUB > + tristate "Onboard USB hub support" On board HUB belongs to HUB, this driver is just for possible power and initialization requirements for HUB which is hard-wired on board. The configuration name USB_HUB_POWER_SUPPLY may more suitable, and at the menu and help, you could indicate it is special for HUBs which are hard-wired on board. > +static ssize_t power_off_in_suspend_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct onboard_hub *hub = dev_get_drvdata(dev); > + > + return sprintf(buf, "%d\n", hub->power_off_in_suspend); > +} > + > +static ssize_t power_off_in_suspend_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct onboard_hub *hub = dev_get_drvdata(dev); > + bool val; > + int ret; > + > + ret = strtobool(buf, &val); > + if (ret < 0) > + return ret; > + > + hub->power_off_in_suspend = val; > + > + return count; > +} > +static DEVICE_ATTR_RW(power_off_in_suspend); > + > +static int onboard_hub_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct onboard_hub *hub; > + int rc; > + > + hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL); > + if (!hub) > + return -ENOMEM; > + > + hub->vdd = devm_regulator_get(dev, "vdd"); > + if (IS_ERR(hub->vdd)) > + return PTR_ERR(hub->vdd); > + > + hub->dev = dev; > + mutex_init(&hub->lock); > + INIT_LIST_HEAD(&hub->udev_list); > + > + dev_set_drvdata(dev, hub); > + > + rc = sysfs_create_file(&dev->kobj, &dev_attr_power_off_in_suspend.attr); > + if (rc) > + return rc; You could use dev_groups for sysfs entry management. > +/************************** USB driver **************************/ > + > +#define VENDOR_ID_REALTEK 0x0bda > + > +static struct onboard_hub *_find_onboard_hub(struct device *dev) > +{ > + const phandle *ph; > + struct device_node *np; > + struct platform_device *pdev; > + > + ph = of_get_property(dev->of_node, "hub", NULL); > + if (!ph) { > + dev_err(dev, "failed to read 'hub' property\n"); > + return ERR_PTR(-EINVAL); > + } > + > + np = of_find_node_by_phandle(be32_to_cpu(*ph)); > + if (!np) { > + dev_err(dev, "failed find device node for onboard hub\n"); > + return ERR_PTR(-EINVAL); > + } > + > + pdev = of_find_device_by_node(np); > + of_node_put(np); > + if (!pdev) > + return ERR_PTR(-EPROBE_DEFER); > + > + return dev_get_drvdata(&pdev->dev); > +} > + > +static int onboard_hub_usbdev_probe(struct usb_device *udev) > +{ > + struct device *dev = &udev->dev; > + struct onboard_hub *hub; > + > + /* ignore supported hubs without device tree node */ > + if (!dev->of_node) > + return -ENODEV; > + > + hub = _find_onboard_hub(dev); > + if (IS_ERR(hub)) > + return PTR_ERR(dev); > + > + dev_set_drvdata(dev, hub); > + > + onboard_hub_add_usbdev(hub, udev); > + > + return 0; > +} > + > +static void onboard_hub_usbdev_disconnect(struct usb_device *udev) > +{ > + struct onboard_hub *hub = dev_get_drvdata(&udev->dev); > + > + onboard_hub_remove_usbdev(hub, udev); > + > + put_device(hub->dev); > +} > + > +static const struct usb_device_id onboard_hub_id_table[] = { > + { .idVendor = VENDOR_ID_REALTEK, > + .idProduct = 0x0411, /* RTS5411 USB 3.0 */ > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > + { .idVendor = VENDOR_ID_REALTEK, > + .idProduct = 0x5411, /* RTS5411 USB 2.0 */ > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > + {}, > +}; > + > +MODULE_DEVICE_TABLE(usb, onboard_hub_id_table); > + > +static struct usb_device_driver onboard_hub_usbdev_driver = { > + > + .name = "onboard-usb-hub", > + .probe = onboard_hub_usbdev_probe, > + .disconnect = onboard_hub_usbdev_disconnect, > + .generic_subclass = 1, > + .supports_autosuspend = 1, > + .id_table = onboard_hub_id_table, > +}; > + > +/************************** Driver (de)registration **************************/ > + > +static int __init onboard_hub_init(void) > +{ > + int rc; > + > + rc = platform_driver_register(&onboard_hub_driver); > + if (rc) > + return rc; > + > + return usb_register_device_driver(&onboard_hub_usbdev_driver, THIS_MODULE); > +} > +device_initcall(onboard_hub_init); > + > +static void __exit onboard_hub_exit(void) > +{ > + usb_deregister_device_driver(&onboard_hub_usbdev_driver); > + platform_driver_unregister(&onboard_hub_driver); > +} > +module_exit(onboard_hub_exit); > + > +MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>"); > +MODULE_DESCRIPTION("Onboard USB Hub driver"); Improve the description like mentioned above.
On Thu, 17 Sep 2020 11:46:21 -0700, Matthias Kaehlcke wrote: > Onboard USB hubs need to be powered and may require initiaization of > other resources (like GPIOs or clocks) to work properly. This adds > a device tree binding for these hubs. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > Changes in v2: > - removed 'wakeup-source' and 'power-off-in-suspend' properties > - consistently use spaces for indentation in example > > .../bindings/usb/onboard_usb_hub.yaml | 56 +++++++++++++++++++ > 1 file changed, 56 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml > My bot found errors running 'make dt_binding_check' on your patch: Error: Documentation/devicetree/bindings/usb/onboard_usb_hub.example.dts:24.9-20 syntax error FATAL ERROR: Unable to parse input tree make[1]: *** [scripts/Makefile.lib:342: Documentation/devicetree/bindings/usb/onboard_usb_hub.example.dt.yaml] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1366: dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1366361 If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure dt-schema is up to date: pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade Please check and re-submit.
On Thu, Sep 17, 2020 at 11:46:22AM -0700, Matthias Kaehlcke wrote: > The main issue this driver addresses is that a USB hub needs to be > powered before it can be discovered. For onboard hubs this is often > solved by supplying the hub with an 'always-on' regulator, which is > kind of a hack. Some onboard hubs may require further initialization > steps, like changing the state of a GPIO or enabling a clock, which > requires further hacks. This driver creates a platform device > representing the hub which performs the necessary initialization. > Currently it only supports switching on a single regulator, support > for multiple regulators or other actions can be added as needed. > Different initialization sequences can be supported based on the > compatible string. > > Besides performing the initialization the driver can be configured > to power the hub off during system suspend. This can help to extend > battery life on battery powered devices which have no requirements > to keep the hub powered during suspend. The driver can also be > configured to leave the hub powered when a wakeup capable USB device > is connected when suspending, and power it off otherwise. > > Technically the driver consists of two drivers, the platform driver > described above and a very thin USB driver that subclasses the > generic driver. The purpose of this driver is to provide the platform > driver with the USB devices corresponding to the hub(s) (a hub > controller may provide multiple 'logical' hubs, e.g. one to support > USB 2.0 and another for USB 3.x). > > Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > Changes in v2: > - check wakeup enabled state of the USB controller instead of > using 'wakeup-source' property > - use sysfs attribute instead of DT property to determine if > the hub should be powered off at all during system suspend > - added missing brace in onboard_hub_suspend() > - updated commit message > - use pm_ptr for pm_ops as suggested by Alan > > Changes in v1: > - renamed the driver to 'onboard_usb_hub' > - single file for platform and USB driver > - USB hub devices register with the platform device > - the DT includes a phandle of the platform device > - the platform device now controls when power is turned off > - the USB driver became a very thin subclass of the generic USB > driver > - enabled autosuspend support > > drivers/usb/misc/Kconfig | 15 ++ > drivers/usb/misc/Makefile | 1 + > drivers/usb/misc/onboard_usb_hub.c | 329 +++++++++++++++++++++++++++++ > 3 files changed, 345 insertions(+) > create mode 100644 drivers/usb/misc/onboard_usb_hub.c > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig > index 6818ea689cd9..e941244e24e5 100644 > --- a/drivers/usb/misc/Kconfig > +++ b/drivers/usb/misc/Kconfig > @@ -275,3 +275,18 @@ config USB_CHAOSKEY > > To compile this driver as a module, choose M here: the > module will be called chaoskey. > + > +config USB_ONBOARD_HUB > + tristate "Onboard USB hub support" > + depends on OF What about COMPILE_TEST as well? > + help > + Say Y here if you want to support onboard USB hubs. The driver > + powers supported hubs on and may perform other initialization > + steps. > + > + The driver can also switch off the power of the hub during > + system suspend if it is configured accordingly. This may > + reduce power consumption while the system is suspended. > + > + To compile this driver as a module, choose M here: the > + module will be called onboard_usb_hub. > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile > index da39bddb0604..6f10a1c6f7e9 100644 > --- a/drivers/usb/misc/Makefile > +++ b/drivers/usb/misc/Makefile > @@ -31,3 +31,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o > > obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/ > obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o > +obj-$(CONFIG_USB_ONBOARD_HUB) += onboard_usb_hub.o > diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c > new file mode 100644 > index 000000000000..206798029041 > --- /dev/null > +++ b/drivers/usb/misc/onboard_usb_hub.c > @@ -0,0 +1,329 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Driver for onboard USB hubs > + * > + * Copyright (c) 2020, Google LLC > + */ > + > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > +#include <linux/suspend.h> > +#include <linux/usb.h> > +#include <linux/usb/hcd.h> > +#include "../core/usb.h" Why do you need private usb core functions? > + > +/************************** Platform driver **************************/ > + > +struct udev_node { > + struct usb_device *udev; > + struct list_head list; > +}; > + > +struct onboard_hub { > + struct regulator *vdd; > + struct device *dev; > + bool power_off_in_suspend; > + struct list_head udev_list; > + struct mutex lock; > + bool has_wakeup_capable_descendants; > +}; > + > +static int onboard_hub_power_on(struct onboard_hub *hub) > +{ > + int err; > + > + err = regulator_enable(hub->vdd); > + if (err) { > + dev_err(hub->dev, "failed to enable regulator: %d\n", err); > + return err; > + } Nit, no need for { } or return err here, just return err one line below. > + > + return 0; > +} > + > +static int onboard_hub_power_off(struct onboard_hub *hub) > +{ > + int err; > + > + err = regulator_disable(hub->vdd); > + if (err) { > + dev_err(hub->dev, "failed to enable regulator: %d\n", err); > + return err; > + } > + > + return 0; Same here. > +} > + > +static int __maybe_unused onboard_hub_suspend(struct device *dev) > +{ > + struct onboard_hub *hub = dev_get_drvdata(dev); > + struct udev_node *node; > + int rc = 0; > + > + hub->has_wakeup_capable_descendants = false; > + > + if (!hub->power_off_in_suspend) > + return 0; > + > + mutex_lock(&hub->lock); > + > + list_for_each_entry(node, &hub->udev_list, list) { > + if (!device_may_wakeup(node->udev->bus->controller)) > + break; > + > + if (usb_wakeup_enabled_descendants(node->udev)) { > + hub->has_wakeup_capable_descendants = true; > + break; > + } > + } > + > + mutex_unlock(&hub->lock); > + > + if (!hub->has_wakeup_capable_descendants) > + rc = onboard_hub_power_off(hub); > + > + return rc; > +} > + > +static int __maybe_unused onboard_hub_resume(struct device *dev) > +{ > + struct onboard_hub *hub = dev_get_drvdata(dev); > + int rc = 0; > + > + if (hub->power_off_in_suspend && !hub->has_wakeup_capable_descendants) > + rc = onboard_hub_power_on(hub); > + > + return rc; > +} > + > +static int onboard_hub_add_usbdev(struct onboard_hub *hub, struct usb_device *udev) > +{ > + struct udev_node *node; > + > + node = devm_kzalloc(hub->dev, sizeof(*node), GFP_KERNEL); > + if (!node) > + return -ENOMEM; > + > + node->udev = udev; No reference counting? Are you sure about this? > + > + mutex_lock(&hub->lock); > + list_add(&node->list, &hub->udev_list); > + mutex_unlock(&hub->lock); > + > + return 0; > +} > + > +static int onboard_hub_remove_usbdev(struct onboard_hub *hub, struct usb_device *udev) > +{ > + struct udev_node *node; > + > + mutex_lock(&hub->lock); > + > + list_for_each_entry(node, &hub->udev_list, list) { list_for_each_entry_safe()? > + if (node->udev == udev) { > + list_del(&node->list); > + devm_kfree(hub->dev, node); > + break; > + } > + } > + > + mutex_unlock(&hub->lock); > + > + if (node == NULL) > + return -EINVAL; > + > + return 0; > +} > + > +static ssize_t power_off_in_suspend_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct onboard_hub *hub = dev_get_drvdata(dev); > + > + return sprintf(buf, "%d\n", hub->power_off_in_suspend); > +} > + > +static ssize_t power_off_in_suspend_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct onboard_hub *hub = dev_get_drvdata(dev); > + bool val; > + int ret; > + > + ret = strtobool(buf, &val); You should use kstrtobool() instead, right? > + if (ret < 0) > + return ret; > + > + hub->power_off_in_suspend = val; > + > + return count; > +} > +static DEVICE_ATTR_RW(power_off_in_suspend); > + > +static int onboard_hub_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct onboard_hub *hub; > + int rc; > + > + hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL); > + if (!hub) > + return -ENOMEM; > + > + hub->vdd = devm_regulator_get(dev, "vdd"); > + if (IS_ERR(hub->vdd)) > + return PTR_ERR(hub->vdd); > + > + hub->dev = dev; > + mutex_init(&hub->lock); > + INIT_LIST_HEAD(&hub->udev_list); > + > + dev_set_drvdata(dev, hub); > + > + rc = sysfs_create_file(&dev->kobj, &dev_attr_power_off_in_suspend.attr); Use the default platform device files group, never create/add your own sysfs files "by hand", otherwise it could go easily wrong. > + if (rc) > + return rc; > + > + return onboard_hub_power_on(hub); > +} > + > +static int onboard_hub_remove(struct platform_device *pdev) > +{ > + struct onboard_hub *hub = dev_get_drvdata(&pdev->dev); > + > + sysfs_remove_file(&pdev->dev.kobj, &dev_attr_power_off_in_suspend.attr); If you do the above, no need to remove this here. > + > + return onboard_hub_power_off(hub); > +} > + > +static const struct of_device_id onboard_hub_match[] = { > + { .compatible = "onboard-usb-hub" }, > + { .compatible = "realtek,rts5411" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, onboard_hub_match); > + > +static SIMPLE_DEV_PM_OPS(onboard_hub_pm_ops, onboard_hub_suspend, onboard_hub_resume); > + > +static struct platform_driver onboard_hub_driver = { > + .probe = onboard_hub_probe, > + .remove = onboard_hub_remove, > + > + .driver = { > + .name = "onboard-usb-hub", > + .of_match_table = onboard_hub_match, > + .pm = pm_ptr(&onboard_hub_pm_ops), > + }, > +}; > + > +/************************** USB driver **************************/ > + > +#define VENDOR_ID_REALTEK 0x0bda > + > +static struct onboard_hub *_find_onboard_hub(struct device *dev) > +{ > + const phandle *ph; > + struct device_node *np; > + struct platform_device *pdev; > + > + ph = of_get_property(dev->of_node, "hub", NULL); > + if (!ph) { > + dev_err(dev, "failed to read 'hub' property\n"); > + return ERR_PTR(-EINVAL); > + } > + > + np = of_find_node_by_phandle(be32_to_cpu(*ph)); > + if (!np) { > + dev_err(dev, "failed find device node for onboard hub\n"); > + return ERR_PTR(-EINVAL); > + } > + > + pdev = of_find_device_by_node(np); > + of_node_put(np); > + if (!pdev) > + return ERR_PTR(-EPROBE_DEFER); Why can you defer here? > + > + return dev_get_drvdata(&pdev->dev); > +} > + > +static int onboard_hub_usbdev_probe(struct usb_device *udev) > +{ > + struct device *dev = &udev->dev; > + struct onboard_hub *hub; > + > + /* ignore supported hubs without device tree node */ > + if (!dev->of_node) > + return -ENODEV; > + > + hub = _find_onboard_hub(dev); > + if (IS_ERR(hub)) > + return PTR_ERR(dev); > + > + dev_set_drvdata(dev, hub); > + > + onboard_hub_add_usbdev(hub, udev); > + > + return 0; > +} > + > +static void onboard_hub_usbdev_disconnect(struct usb_device *udev) > +{ > + struct onboard_hub *hub = dev_get_drvdata(&udev->dev); > + > + onboard_hub_remove_usbdev(hub, udev); > + > + put_device(hub->dev); > +} > + > +static const struct usb_device_id onboard_hub_id_table[] = { > + { .idVendor = VENDOR_ID_REALTEK, > + .idProduct = 0x0411, /* RTS5411 USB 3.0 */ > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > + { .idVendor = VENDOR_ID_REALTEK, > + .idProduct = 0x5411, /* RTS5411 USB 2.0 */ > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, USB_DEVICE() should be used here instead for both of these, right? > + {}, > +}; > + > +MODULE_DEVICE_TABLE(usb, onboard_hub_id_table); > + > +static struct usb_device_driver onboard_hub_usbdev_driver = { > + > + .name = "onboard-usb-hub", > + .probe = onboard_hub_usbdev_probe, > + .disconnect = onboard_hub_usbdev_disconnect, > + .generic_subclass = 1, > + .supports_autosuspend = 1, > + .id_table = onboard_hub_id_table, > +}; > + > +/************************** Driver (de)registration **************************/ > + > +static int __init onboard_hub_init(void) > +{ > + int rc; > + > + rc = platform_driver_register(&onboard_hub_driver); > + if (rc) > + return rc; > + > + return usb_register_device_driver(&onboard_hub_usbdev_driver, THIS_MODULE); No unwinding of the platform driver register if this fails? And THIS_MODULE should not be needed, did we get the api wrong here? > +} > +device_initcall(onboard_hub_init); Why device_initcall() if this could be a module? Why not a normal module_init()? thanks, greg k-h
Hi Alan, thanks for taking time to review! On Thu, Sep 17, 2020 at 03:54:16PM -0400, Alan Stern wrote: > On Thu, Sep 17, 2020 at 11:46:22AM -0700, Matthias Kaehlcke wrote: > > The main issue this driver addresses is that a USB hub needs to be > > powered before it can be discovered. For onboard hubs this is often > > solved by supplying the hub with an 'always-on' regulator, which is > > kind of a hack. Some onboard hubs may require further initialization > > steps, like changing the state of a GPIO or enabling a clock, which > > requires further hacks. This driver creates a platform device > > representing the hub which performs the necessary initialization. > > Currently it only supports switching on a single regulator, support > > for multiple regulators or other actions can be added as needed. > > Different initialization sequences can be supported based on the > > compatible string. > > > > Besides performing the initialization the driver can be configured > > to power the hub off during system suspend. This can help to extend > > battery life on battery powered devices which have no requirements > > to keep the hub powered during suspend. The driver can also be > > configured to leave the hub powered when a wakeup capable USB device > > is connected when suspending, and power it off otherwise. > > > > Technically the driver consists of two drivers, the platform driver > > described above and a very thin USB driver that subclasses the > > generic driver. The purpose of this driver is to provide the platform > > driver with the USB devices corresponding to the hub(s) (a hub > > controller may provide multiple 'logical' hubs, e.g. one to support > > USB 2.0 and another for USB 3.x). > > > > Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > +config USB_ONBOARD_HUB > > + tristate "Onboard USB hub support" > > + depends on OF > > + help > > + Say Y here if you want to support onboard USB hubs. The driver > > + powers supported hubs on and may perform other initialization > > + steps. > > I have a nagging feeling that this description may be too vague for a > lot of people to understand. Does everybody know what an "onboard" > USB hub is? > > Consider for example that Intel's current EHCI host controllers all > come with a USB hub built into the chipset. That built-in hub > certainly could be considered "onboard", but it doesn't need this > driver. > > Maybe also give some examples of devices that require this driver, to > help make the idea clear to readers. Ok, I'll try to come up with a better description. > > +static int __maybe_unused onboard_hub_suspend(struct device *dev) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(dev); > > + struct udev_node *node; > > + int rc = 0; > > + > > + hub->has_wakeup_capable_descendants = false; > > + > > + if (!hub->power_off_in_suspend) > > + return 0; > > + > > + mutex_lock(&hub->lock); > > + > > + list_for_each_entry(node, &hub->udev_list, list) { > > + if (!device_may_wakeup(node->udev->bus->controller)) > > + break; > > You're assuming that node->udev->bus->controller is going to be the > same for the nodes on the list, right? Yes, that is the assumption, although you have a point that this isn't necessarily the case. It's probably true in the vast majority of cases, but a hub could be wired up to multiple controllers. I'll change the loop to set the flag without breaking, it's a micro-optimization anyway. > > + > > + if (usb_wakeup_enabled_descendants(node->udev)) { > > + hub->has_wakeup_capable_descendants = true; > > + break; > > + } > > + } > > + > > + mutex_unlock(&hub->lock); > > + > > + if (!hub->has_wakeup_capable_descendants) > > + rc = onboard_hub_power_off(hub); > > + > > + return rc; > > +} > > + > > +static int __maybe_unused onboard_hub_resume(struct device *dev) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(dev); > > + int rc = 0; > > + > > + if (hub->power_off_in_suspend && !hub->has_wakeup_capable_descendants) > > Instead of this cumbersome two-condition test, how about simply > having a hub->is_powered_on flag? Then > hub->has_wakeup_capable_descendants wouldn't be needed. Ok, less cumbersome code is always good :) > > + rc = onboard_hub_power_on(hub); > > + > > + return rc; > > +} > > > +static int onboard_hub_remove_usbdev(struct onboard_hub *hub, struct usb_device *udev) > > +{ > > + struct udev_node *node; > > + > > + mutex_lock(&hub->lock); > > + > > + list_for_each_entry(node, &hub->udev_list, list) { > > + if (node->udev == udev) { > > + list_del(&node->list); > > + devm_kfree(hub->dev, node); > > Why have an explicit kfree here but not anywhere else? And if you do > have an explicit kfree, why use devm_kzalloc rather than plain kzalloc? The motivation of the explicit kfree was to avoid hogging memory if the USB device disappears and reappears repeatedly. However this doesn't seem to be a very common scenario so maybe we can ignore it. > > + break; > > + } > > + } > > + > > + mutex_unlock(&hub->lock); > > + > > + if (node == NULL) > > + return -EINVAL; > > This test is wrong. Look at the definition of list_for_each_entry; > node will never be NULL. Probably the best approach is to use a local > "ret" variable. Ack, thanks for catching! > > + > > + return 0; > > +} > > > +static int onboard_hub_remove(struct platform_device *pdev) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(&pdev->dev); > > + > > + sysfs_remove_file(&pdev->dev.kobj, &dev_attr_power_off_in_suspend.attr); > > + > > + return onboard_hub_power_off(hub); > > +} > > Shouldn't this routine unbind the onboard_hub_usbdev driver from all > the associated devices? Otherwise you end up with more-or-less > dangling references to hub (I say more-or-less because with the devm > allocations, the structures will hang around as zombies for a while). True, the dangling references aren't a good idea. Initially I thought that the USB devices holding a reference of the hub device would prevent this, but apparently that was wishful thinking. IIUC unbinding would be done through device_driver_detach(). > Relying on the onboard_hub_power_off call to do this for you isn't a > great idea, because the effect won't happen immediately. > > > +static int onboard_hub_usbdev_probe(struct usb_device *udev) > > +{ > > + struct device *dev = &udev->dev; > > + struct onboard_hub *hub; > > + > > + /* ignore supported hubs without device tree node */ > > + if (!dev->of_node) > > + return -ENODEV; > > + > > + hub = _find_onboard_hub(dev); > > + if (IS_ERR(hub)) > > + return PTR_ERR(dev); > > + > > + dev_set_drvdata(dev, hub); > > + > > + onboard_hub_add_usbdev(hub, udev); > > Ignoring the return code? Then why does that routine return int rather > than void? Ok, will abort if the function returns an error. > > + > > + return 0; > > +} > > + > > +static void onboard_hub_usbdev_disconnect(struct usb_device *udev) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(&udev->dev); > > + > > + onboard_hub_remove_usbdev(hub, udev); > > Ditto. In this case it's probably better to change the return type to void, since there is not really an alternative course of action. > > + > > + put_device(hub->dev); > > Is there a matching get_device somewhere (like in _find_onboard_hub)? > If so, I didn't see it. And I don't see any reason for it. Yes, implicitly, of_find_device_by_node() "takes a reference to the embedded struct device which needs to be dropped after use."
Hi Peter, On Fri, Sep 18, 2020 at 01:30:20AM +0000, Peter Chen wrote: > On 20-09-17 11:46:22, Matthias Kaehlcke wrote: > > The main issue this driver addresses is that a USB hub needs to be > > powered before it can be discovered. For onboard hubs this is often > > solved by supplying the hub with an 'always-on' regulator, which is > > kind of a hack. Some onboard hubs may require further initialization > > steps, like changing the state of a GPIO or enabling a clock, which > > requires further hacks. This driver creates a platform device > > representing the hub which performs the necessary initialization. > > Currently it only supports switching on a single regulator, support > > for multiple regulators or other actions can be added as needed. > > Different initialization sequences can be supported based on the > > compatible string. > > > > Besides performing the initialization the driver can be configured > > to power the hub off during system suspend. This can help to extend > > battery life on battery powered devices which have no requirements > > to keep the hub powered during suspend. The driver can also be > > configured to leave the hub powered when a wakeup capable USB device > > is connected when suspending, and power it off otherwise. > > > > Technically the driver consists of two drivers, the platform driver > > described above and a very thin USB driver that subclasses the > > generic driver. The purpose of this driver is to provide the platform > > driver with the USB devices corresponding to the hub(s) (a hub > > controller may provide multiple 'logical' hubs, e.g. one to support > > USB 2.0 and another for USB 3.x). > > > > Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > > > Changes in v2: > > - check wakeup enabled state of the USB controller instead of > > using 'wakeup-source' property > > - use sysfs attribute instead of DT property to determine if > > the hub should be powered off at all during system suspend > > - added missing brace in onboard_hub_suspend() > > - updated commit message > > - use pm_ptr for pm_ops as suggested by Alan > > > > Changes in v1: > > - renamed the driver to 'onboard_usb_hub' > > - single file for platform and USB driver > > - USB hub devices register with the platform device > > - the DT includes a phandle of the platform device > > - the platform device now controls when power is turned off > > - the USB driver became a very thin subclass of the generic USB > > driver > > - enabled autosuspend support > > > > drivers/usb/misc/Kconfig | 15 ++ > > drivers/usb/misc/Makefile | 1 + > > drivers/usb/misc/onboard_usb_hub.c | 329 +++++++++++++++++++++++++++++ > > 3 files changed, 345 insertions(+) > > create mode 100644 drivers/usb/misc/onboard_usb_hub.c > > > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig > > index 6818ea689cd9..e941244e24e5 100644 > > --- a/drivers/usb/misc/Kconfig > > +++ b/drivers/usb/misc/Kconfig > > @@ -275,3 +275,18 @@ config USB_CHAOSKEY > > > > To compile this driver as a module, choose M here: the > > module will be called chaoskey. > > + > > +config USB_ONBOARD_HUB > > + tristate "Onboard USB hub support" > > On board HUB belongs to HUB, this driver is just for possible power and > initialization requirements for HUB which is hard-wired on board. The > configuration name USB_HUB_POWER_SUPPLY may more suitable, and at the > menu and help, you could indicate it is special for HUBs which are > hard-wired on board. I'm not convinced about the 'power supply' naming, since as you say there can be more initialization besides switching on a regulator and the driver 'extends' the hub driver to support switching the hub power off during system suspend. So far neither Alan nor Greg have raised concerns about the naming (though that's still an option ;-). I'm open to change it if we can come up with a name that clearly describes the driver better than the current name. > > +static ssize_t power_off_in_suspend_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(dev); > > + > > + return sprintf(buf, "%d\n", hub->power_off_in_suspend); > > +} > > + > > +static ssize_t power_off_in_suspend_store(struct device *dev, struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(dev); > > + bool val; > > + int ret; > > + > > + ret = strtobool(buf, &val); > > + if (ret < 0) > > + return ret; > > + > > + hub->power_off_in_suspend = val; > > + > > + return count; > > +} > > +static DEVICE_ATTR_RW(power_off_in_suspend); > > + > > +static int onboard_hub_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct onboard_hub *hub; > > + int rc; > > + > > + hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL); > > + if (!hub) > > + return -ENOMEM; > > + > > + hub->vdd = devm_regulator_get(dev, "vdd"); > > + if (IS_ERR(hub->vdd)) > > + return PTR_ERR(hub->vdd); > > + > > + hub->dev = dev; > > + mutex_init(&hub->lock); > > + INIT_LIST_HEAD(&hub->udev_list); > > + > > + dev_set_drvdata(dev, hub); > > + > > + rc = sysfs_create_file(&dev->kobj, &dev_attr_power_off_in_suspend.attr); > > + if (rc) > > + return rc; > > You could use dev_groups for sysfs entry management. Thanks, will do, Greg also pointed that out. > > +/************************** USB driver **************************/ > > + > > +#define VENDOR_ID_REALTEK 0x0bda > > + > > +static struct onboard_hub *_find_onboard_hub(struct device *dev) > > +{ > > + const phandle *ph; > > + struct device_node *np; > > + struct platform_device *pdev; > > + > > + ph = of_get_property(dev->of_node, "hub", NULL); > > + if (!ph) { > > + dev_err(dev, "failed to read 'hub' property\n"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + np = of_find_node_by_phandle(be32_to_cpu(*ph)); > > + if (!np) { > > + dev_err(dev, "failed find device node for onboard hub\n"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + pdev = of_find_device_by_node(np); > > + of_node_put(np); > > + if (!pdev) > > + return ERR_PTR(-EPROBE_DEFER); > > + > > + return dev_get_drvdata(&pdev->dev); > > +} > > + > > +static int onboard_hub_usbdev_probe(struct usb_device *udev) > > +{ > > + struct device *dev = &udev->dev; > > + struct onboard_hub *hub; > > + > > + /* ignore supported hubs without device tree node */ > > + if (!dev->of_node) > > + return -ENODEV; > > + > > + hub = _find_onboard_hub(dev); > > + if (IS_ERR(hub)) > > + return PTR_ERR(dev); > > + > > + dev_set_drvdata(dev, hub); > > + > > + onboard_hub_add_usbdev(hub, udev); > > + > > + return 0; > > +} > > + > > +static void onboard_hub_usbdev_disconnect(struct usb_device *udev) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(&udev->dev); > > + > > + onboard_hub_remove_usbdev(hub, udev); > > + > > + put_device(hub->dev); > > +} > > + > > +static const struct usb_device_id onboard_hub_id_table[] = { > > + { .idVendor = VENDOR_ID_REALTEK, > > + .idProduct = 0x0411, /* RTS5411 USB 3.0 */ > > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > > + { .idVendor = VENDOR_ID_REALTEK, > > + .idProduct = 0x5411, /* RTS5411 USB 2.0 */ > > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > > + {}, > > +}; > > + > > +MODULE_DEVICE_TABLE(usb, onboard_hub_id_table); > > + > > +static struct usb_device_driver onboard_hub_usbdev_driver = { > > + > > + .name = "onboard-usb-hub", > > + .probe = onboard_hub_usbdev_probe, > > + .disconnect = onboard_hub_usbdev_disconnect, > > + .generic_subclass = 1, > > + .supports_autosuspend = 1, > > + .id_table = onboard_hub_id_table, > > +}; > > + > > +/************************** Driver (de)registration **************************/ > > + > > +static int __init onboard_hub_init(void) > > +{ > > + int rc; > > + > > + rc = platform_driver_register(&onboard_hub_driver); > > + if (rc) > > + return rc; > > + > > + return usb_register_device_driver(&onboard_hub_usbdev_driver, THIS_MODULE); > > +} > > +device_initcall(onboard_hub_init); > > + > > +static void __exit onboard_hub_exit(void) > > +{ > > + usb_deregister_device_driver(&onboard_hub_usbdev_driver); > > + platform_driver_unregister(&onboard_hub_driver); > > +} > > +module_exit(onboard_hub_exit); > > + > > +MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>"); > > +MODULE_DESCRIPTION("Onboard USB Hub driver"); > > Improve the description like mentioned above. Will have to see if I can come up with something that provides more information but is at the same time short. I might leave it as is in the next version if my inspiration fails and be open to specific suggestions.
On Mon, Sep 21, 2020 at 05:41:58PM -0700, Matthias Kaehlcke wrote: > > > + put_device(hub->dev); > > > > Is there a matching get_device somewhere (like in _find_onboard_hub)? > > If so, I didn't see it. And I don't see any reason for it. > > Yes, implicitly, of_find_device_by_node() "takes a reference to the > embedded struct device which needs to be dropped after use." Okay. In that case it probably would be better to do the put_device() right away, at the end of _find_onboard_hub(). There would be no danger of the platform device getting freed too soon if you make onboard_hub_remove unbind the associated USB hub devices. But there would still be a danger of those devices somehow getting rebound again at the wrong time; this suggests that you should add a flag to the onboard_hub structure saying that the platform device is about to go away. Alan Stern
Hi Greg, thanks for the review! On Sun, Sep 20, 2020 at 04:17:20PM +0200, Greg Kroah-Hartman wrote: > On Thu, Sep 17, 2020 at 11:46:22AM -0700, Matthias Kaehlcke wrote: > > The main issue this driver addresses is that a USB hub needs to be > > powered before it can be discovered. For onboard hubs this is often > > solved by supplying the hub with an 'always-on' regulator, which is > > kind of a hack. Some onboard hubs may require further initialization > > steps, like changing the state of a GPIO or enabling a clock, which > > requires further hacks. This driver creates a platform device > > representing the hub which performs the necessary initialization. > > Currently it only supports switching on a single regulator, support > > for multiple regulators or other actions can be added as needed. > > Different initialization sequences can be supported based on the > > compatible string. > > > > Besides performing the initialization the driver can be configured > > to power the hub off during system suspend. This can help to extend > > battery life on battery powered devices which have no requirements > > to keep the hub powered during suspend. The driver can also be > > configured to leave the hub powered when a wakeup capable USB device > > is connected when suspending, and power it off otherwise. > > > > Technically the driver consists of two drivers, the platform driver > > described above and a very thin USB driver that subclasses the > > generic driver. The purpose of this driver is to provide the platform > > driver with the USB devices corresponding to the hub(s) (a hub > > controller may provide multiple 'logical' hubs, e.g. one to support > > USB 2.0 and another for USB 3.x). > > > > Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > > > Changes in v2: > > - check wakeup enabled state of the USB controller instead of > > using 'wakeup-source' property > > - use sysfs attribute instead of DT property to determine if > > the hub should be powered off at all during system suspend > > - added missing brace in onboard_hub_suspend() > > - updated commit message > > - use pm_ptr for pm_ops as suggested by Alan > > > > Changes in v1: > > - renamed the driver to 'onboard_usb_hub' > > - single file for platform and USB driver > > - USB hub devices register with the platform device > > - the DT includes a phandle of the platform device > > - the platform device now controls when power is turned off > > - the USB driver became a very thin subclass of the generic USB > > driver > > - enabled autosuspend support > > > > drivers/usb/misc/Kconfig | 15 ++ > > drivers/usb/misc/Makefile | 1 + > > drivers/usb/misc/onboard_usb_hub.c | 329 +++++++++++++++++++++++++++++ > > 3 files changed, 345 insertions(+) > > create mode 100644 drivers/usb/misc/onboard_usb_hub.c > > > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig > > index 6818ea689cd9..e941244e24e5 100644 > > --- a/drivers/usb/misc/Kconfig > > +++ b/drivers/usb/misc/Kconfig > > @@ -275,3 +275,18 @@ config USB_CHAOSKEY > > > > To compile this driver as a module, choose M here: the > > module will be called chaoskey. > > + > > +config USB_ONBOARD_HUB > > + tristate "Onboard USB hub support" > > + depends on OF > > What about COMPILE_TEST as well? ok > > + help > > + Say Y here if you want to support onboard USB hubs. The driver > > + powers supported hubs on and may perform other initialization > > + steps. > > + > > + The driver can also switch off the power of the hub during > > + system suspend if it is configured accordingly. This may > > + reduce power consumption while the system is suspended. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called onboard_usb_hub. > > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile > > index da39bddb0604..6f10a1c6f7e9 100644 > > --- a/drivers/usb/misc/Makefile > > +++ b/drivers/usb/misc/Makefile > > @@ -31,3 +31,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o > > > > obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/ > > obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o > > +obj-$(CONFIG_USB_ONBOARD_HUB) += onboard_usb_hub.o > > diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c > > new file mode 100644 > > index 000000000000..206798029041 > > --- /dev/null > > +++ b/drivers/usb/misc/onboard_usb_hub.c > > @@ -0,0 +1,329 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Driver for onboard USB hubs > > + * > > + * Copyright (c) 2020, Google LLC > > + */ > > + > > +#include <linux/init.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/of.h> > > +#include <linux/of_platform.h> > > +#include <linux/platform_device.h> > > +#include <linux/regulator/consumer.h> > > +#include <linux/suspend.h> > > +#include <linux/usb.h> > > +#include <linux/usb/hcd.h> > > +#include "../core/usb.h" > > Why do you need private usb core functions? An earlier version of the driver used usb_port_disable(), but that isn't needed anymore, I'll remove the include. > > + > > +/************************** Platform driver **************************/ > > + > > +struct udev_node { > > + struct usb_device *udev; > > + struct list_head list; > > +}; > > + > > +struct onboard_hub { > > + struct regulator *vdd; > > + struct device *dev; > > + bool power_off_in_suspend; > > + struct list_head udev_list; > > + struct mutex lock; > > + bool has_wakeup_capable_descendants; > > +}; > > + > > +static int onboard_hub_power_on(struct onboard_hub *hub) > > +{ > > + int err; > > + > > + err = regulator_enable(hub->vdd); > > + if (err) { > > + dev_err(hub->dev, "failed to enable regulator: %d\n", err); > > + return err; > > + } > > Nit, no need for { } or return err here, just return err one line below. ack > > + > > + return 0; > > +} > > + > > +static int onboard_hub_power_off(struct onboard_hub *hub) > > +{ > > + int err; > > + > > + err = regulator_disable(hub->vdd); > > + if (err) { > > + dev_err(hub->dev, "failed to enable regulator: %d\n", err); > > + return err; > > + } > > + > > + return 0; > > Same here. ack > > +} > > + > > +static int __maybe_unused onboard_hub_suspend(struct device *dev) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(dev); > > + struct udev_node *node; > > + int rc = 0; > > + > > + hub->has_wakeup_capable_descendants = false; > > + > > + if (!hub->power_off_in_suspend) > > + return 0; > > + > > + mutex_lock(&hub->lock); > > + > > + list_for_each_entry(node, &hub->udev_list, list) { > > + if (!device_may_wakeup(node->udev->bus->controller)) > > + break; > > + > > + if (usb_wakeup_enabled_descendants(node->udev)) { > > + hub->has_wakeup_capable_descendants = true; > > + break; > > + } > > + } > > + > > + mutex_unlock(&hub->lock); > > + > > + if (!hub->has_wakeup_capable_descendants) > > + rc = onboard_hub_power_off(hub); > > + > > + return rc; > > +} > > + > > +static int __maybe_unused onboard_hub_resume(struct device *dev) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(dev); > > + int rc = 0; > > + > > + if (hub->power_off_in_suspend && !hub->has_wakeup_capable_descendants) > > + rc = onboard_hub_power_on(hub); > > + > > + return rc; > > +} > > + > > +static int onboard_hub_add_usbdev(struct onboard_hub *hub, struct usb_device *udev) > > +{ > > + struct udev_node *node; > > + > > + node = devm_kzalloc(hub->dev, sizeof(*node), GFP_KERNEL); > > + if (!node) > > + return -ENOMEM; > > + > > + node->udev = udev; > > No reference counting? Are you sure about this? I thought it isn't strictly needed, since this function is only called by the onboard_hub_usbdev driver, which also calls onboard_hub_remove_usbdev() on disconnect. So my thinking was that the driver trusts itself, like the kernel. Am I missing a case? > > + > > + mutex_lock(&hub->lock); > > + list_add(&node->list, &hub->udev_list); > > + mutex_unlock(&hub->lock); > > + > > + return 0; > > +} > > + > > +static int onboard_hub_remove_usbdev(struct onboard_hub *hub, struct usb_device *udev) > > +{ > > + struct udev_node *node; > > + > > + mutex_lock(&hub->lock); > > + > > + list_for_each_entry(node, &hub->udev_list, list) { > > list_for_each_entry_safe()? I can change it, but IIUC it shouldn't be really necessary, since the loop is aborted after removing the node. > > + if (node->udev == udev) { > > + list_del(&node->list); > > + devm_kfree(hub->dev, node); > > + break; > > + } > > + } > > + > > + mutex_unlock(&hub->lock); > > + > > + if (node == NULL) > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > +static ssize_t power_off_in_suspend_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(dev); > > + > > + return sprintf(buf, "%d\n", hub->power_off_in_suspend); > > +} > > + > > +static ssize_t power_off_in_suspend_store(struct device *dev, struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(dev); > > + bool val; > > + int ret; > > + > > + ret = strtobool(buf, &val); > > You should use kstrtobool() instead, right? Will change > > + if (ret < 0) > > + return ret; > > + > > + hub->power_off_in_suspend = val; > > + > > + return count; > > +} > > +static DEVICE_ATTR_RW(power_off_in_suspend); > > + > > +static int onboard_hub_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct onboard_hub *hub; > > + int rc; > > + > > + hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL); > > + if (!hub) > > + return -ENOMEM; > > + > > + hub->vdd = devm_regulator_get(dev, "vdd"); > > + if (IS_ERR(hub->vdd)) > > + return PTR_ERR(hub->vdd); > > + > > + hub->dev = dev; > > + mutex_init(&hub->lock); > > + INIT_LIST_HEAD(&hub->udev_list); > > + > > + dev_set_drvdata(dev, hub); > > + > > + rc = sysfs_create_file(&dev->kobj, &dev_attr_power_off_in_suspend.attr); > > Use the default platform device files group, never create/add your own > sysfs files "by hand", otherwise it could go easily wrong. Ok > > + if (rc) > > + return rc; > > + > > + return onboard_hub_power_on(hub); > > +} > > + > > +static int onboard_hub_remove(struct platform_device *pdev) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(&pdev->dev); > > + > > + sysfs_remove_file(&pdev->dev.kobj, &dev_attr_power_off_in_suspend.attr); > > If you do the above, no need to remove this here. > > > + > > + return onboard_hub_power_off(hub); > > +} > > + > > +static const struct of_device_id onboard_hub_match[] = { > > + { .compatible = "onboard-usb-hub" }, > > + { .compatible = "realtek,rts5411" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, onboard_hub_match); > > + > > +static SIMPLE_DEV_PM_OPS(onboard_hub_pm_ops, onboard_hub_suspend, onboard_hub_resume); > > + > > +static struct platform_driver onboard_hub_driver = { > > + .probe = onboard_hub_probe, > > + .remove = onboard_hub_remove, > > + > > + .driver = { > > + .name = "onboard-usb-hub", > > + .of_match_table = onboard_hub_match, > > + .pm = pm_ptr(&onboard_hub_pm_ops), > > + }, > > +}; > > + > > +/************************** USB driver **************************/ > > + > > +#define VENDOR_ID_REALTEK 0x0bda > > + > > +static struct onboard_hub *_find_onboard_hub(struct device *dev) > > +{ > > + const phandle *ph; > > + struct device_node *np; > > + struct platform_device *pdev; > > + > > + ph = of_get_property(dev->of_node, "hub", NULL); > > + if (!ph) { > > + dev_err(dev, "failed to read 'hub' property\n"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + np = of_find_node_by_phandle(be32_to_cpu(*ph)); > > + if (!np) { > > + dev_err(dev, "failed find device node for onboard hub\n"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + pdev = of_find_device_by_node(np); > > + of_node_put(np); > > + if (!pdev) > > + return ERR_PTR(-EPROBE_DEFER); > > Why can you defer here? We know there is a node, so if the device can not be found it probably hasn't been probed yet? > > + > > + return dev_get_drvdata(&pdev->dev); > > +} > > + > > +static int onboard_hub_usbdev_probe(struct usb_device *udev) > > +{ > > + struct device *dev = &udev->dev; > > + struct onboard_hub *hub; > > + > > + /* ignore supported hubs without device tree node */ > > + if (!dev->of_node) > > + return -ENODEV; > > + > > + hub = _find_onboard_hub(dev); > > + if (IS_ERR(hub)) > > + return PTR_ERR(dev); > > + > > + dev_set_drvdata(dev, hub); > > + > > + onboard_hub_add_usbdev(hub, udev); > > + > > + return 0; > > +} > > + > > +static void onboard_hub_usbdev_disconnect(struct usb_device *udev) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(&udev->dev); > > + > > + onboard_hub_remove_usbdev(hub, udev); > > + > > + put_device(hub->dev); > > +} > > + > > +static const struct usb_device_id onboard_hub_id_table[] = { > > + { .idVendor = VENDOR_ID_REALTEK, > > + .idProduct = 0x0411, /* RTS5411 USB 3.0 */ > > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > > + { .idVendor = VENDOR_ID_REALTEK, > > + .idProduct = 0x5411, /* RTS5411 USB 2.0 */ > > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > > USB_DEVICE() should be used here instead for both of these, right? ack > > + {}, > > +}; > > + > > +MODULE_DEVICE_TABLE(usb, onboard_hub_id_table); > > + > > +static struct usb_device_driver onboard_hub_usbdev_driver = { > > + > > + .name = "onboard-usb-hub", > > + .probe = onboard_hub_usbdev_probe, > > + .disconnect = onboard_hub_usbdev_disconnect, > > + .generic_subclass = 1, > > + .supports_autosuspend = 1, > > + .id_table = onboard_hub_id_table, > > +}; > > + > > +/************************** Driver (de)registration **************************/ > > + > > +static int __init onboard_hub_init(void) > > +{ > > + int rc; > > + > > + rc = platform_driver_register(&onboard_hub_driver); > > + if (rc) > > + return rc; > > + > > + return usb_register_device_driver(&onboard_hub_usbdev_driver, THIS_MODULE); > > No unwinding of the platform driver register if this fails? Right, will add unwinding. > And THIS_MODULE should not be needed, did we get the api wrong here? It seems you suggest to use usb_register() instead, SGTM > > +} > > +device_initcall(onboard_hub_init); > > Why device_initcall() if this could be a module? Why not a normal > module_init()? Ok, will change it to module_init() Thanks Matthias
On Mon, Sep 21, 2020 at 09:08:12PM -0400, Alan Stern wrote: > On Mon, Sep 21, 2020 at 05:41:58PM -0700, Matthias Kaehlcke wrote: > > > > + put_device(hub->dev); > > > > > > Is there a matching get_device somewhere (like in _find_onboard_hub)? > > > If so, I didn't see it. And I don't see any reason for it. > > > > Yes, implicitly, of_find_device_by_node() "takes a reference to the > > embedded struct device which needs to be dropped after use." > > Okay. In that case it probably would be better to do the put_device() > right away, at the end of _find_onboard_hub(). ok > There would be no danger of the platform device getting freed too soon > if you make onboard_hub_remove unbind the associated USB hub devices. Yes, I'll add the unbinding as you suggested earlier > But there would still be a danger of those devices somehow getting > rebound again at the wrong time; this suggests that you should add a > flag to the onboard_hub structure saying that the platform device is > about to go away. Indeed, we want to avoid that. I'll add a flag to the struct as you suggested.
On Mon, Sep 21, 2020 at 06:18:37PM -0700, Matthias Kaehlcke wrote: > On Sun, Sep 20, 2020 at 04:17:20PM +0200, Greg Kroah-Hartman wrote: > > On Thu, Sep 17, 2020 at 11:46:22AM -0700, Matthias Kaehlcke wrote: > > > > > > ... > > > > > > +static int __init onboard_hub_init(void) > > > +{ > > > + int rc; > > > + > > > + rc = platform_driver_register(&onboard_hub_driver); > > > + if (rc) > > > + return rc; > > > + > > > + return usb_register_device_driver(&onboard_hub_usbdev_driver, THIS_MODULE); > > > > No unwinding of the platform driver register if this fails? > > Right, will add unwinding. > > > And THIS_MODULE should not be needed, did we get the api wrong here? > > It seems you suggest to use usb_register() instead, SGTM Actually usb_register() is for registering a struct usb_driver, however this is a struct usb_device_driver, there doesn't seem to be a registration function/macro that doesn't require THIS_MODULE. Please provide a pointer if I'm wrong.
On Wed, Sep 23, 2020 at 03:25:45PM -0700, Matthias Kaehlcke wrote: > On Mon, Sep 21, 2020 at 06:18:37PM -0700, Matthias Kaehlcke wrote: > > On Sun, Sep 20, 2020 at 04:17:20PM +0200, Greg Kroah-Hartman wrote: > > > On Thu, Sep 17, 2020 at 11:46:22AM -0700, Matthias Kaehlcke wrote: > > > > > > > > ... > > > > > > > > +static int __init onboard_hub_init(void) > > > > +{ > > > > + int rc; > > > > + > > > > + rc = platform_driver_register(&onboard_hub_driver); > > > > + if (rc) > > > > + return rc; > > > > + > > > > + return usb_register_device_driver(&onboard_hub_usbdev_driver, THIS_MODULE); > > > > > > No unwinding of the platform driver register if this fails? > > > > Right, will add unwinding. > > > > > And THIS_MODULE should not be needed, did we get the api wrong here? > > > > It seems you suggest to use usb_register() instead, SGTM > > Actually usb_register() is for registering a struct usb_driver, however > this is a struct usb_device_driver, there doesn't seem to be a > registration function/macro that doesn't require THIS_MODULE. Please > provide a pointer if I'm wrong. You are correct, I was just making a meta-comment that we got this api wrong when adding it to the kernel and need to fix it up so that you do not have to manually pass in the module owner. i.e. make it much like usb_register() does. thanks, greg k-h
diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml new file mode 100644 index 000000000000..66d67aa64e3f --- /dev/null +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml @@ -0,0 +1,56 @@ +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/usb/onboard_usb_hub.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Binding for onboard USB hubs + +maintainers: + - Matthias Kaehlcke <mka@chromium.org> + +allOf: + - $ref: /schemas/usb/onboard_usb_hub.yaml# + +properties: + compatible: + enum: + - onboard-usb-hub + - realtek,rts5411 + + vdd-supply: + description: + phandle to the regulator that provides power to the hub. + +required: + - compatible + - vdd-supply + +examples: + - | + usb_hub: usb-hub { + compatible = "realtek,rts5411", "onboard-usb-hub"; + vdd-supply = <&pp3300_hub>; + }; + + &usb_1_dwc3 { + dr_mode = "host"; + #address-cells = <1>; + #size-cells = <0>; + + /* 2.0 hub on port 1 */ + hub@1 { + compatible = "usbbda,5411"; + reg = <1>; + hub = <&usb_hub>; + }; + + /* 3.0 hub on port 2 */ + hub@2 { + compatible = "usbbda,411"; + reg = <2>; + hub = <&usb_hub>; + }; + }; + +...
Onboard USB hubs need to be powered and may require initiaization of other resources (like GPIOs or clocks) to work properly. This adds a device tree binding for these hubs. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- Changes in v2: - removed 'wakeup-source' and 'power-off-in-suspend' properties - consistently use spaces for indentation in example .../bindings/usb/onboard_usb_hub.yaml | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml