| Message ID | 20210325122926.58392-2-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: Linking ports to their Type-C connectors | expand |
On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote: > Introducing usb_for_each_port(). It works the same way as > usb_for_each_dev(), but instead of going through every USB > device in the system, it walks through the USB ports in the > system. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> This has a couple of nasty errors. > --- > drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/usb.h | 1 + > 2 files changed, 44 insertions(+) > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > index 2ce3667ec6fae..6d49db9a1b208 100644 > --- a/drivers/usb/core/usb.c > +++ b/drivers/usb/core/usb.c > @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *)) > } > EXPORT_SYMBOL_GPL(usb_for_each_dev); > > +struct each_hub_arg { > + void *data; > + int (*fn)(struct device *, void *); > +}; > + > +static int __each_hub(struct device *dev, void *data) > +{ > + struct each_hub_arg *arg = (struct each_hub_arg *)data; > + struct usb_device *hdev = to_usb_device(dev); to_usb_device() won't work properly if the struct device isn't embedded in an actual usb_device structure. And that will happen, since the USB bus type holds usb_interface structures as well as usb_devices. In fact, you should use usb_for_each_dev here; it already does what you want. > + struct usb_hub *hub; > + int ret; > + int i; > + > + hub = usb_hub_to_struct_hub(hdev); > + if (!hub) > + return 0; > + > + for (i = 0; i < hdev->maxchild; i++) { > + ret = arg->fn(&hub->ports[i]->dev, arg->data); > + if (ret) > + return ret; > + } > + > + return 0; > +} Don't you need some sort of locking or refcounting here? What would happen if this hub got removed while the routine was running? Alan Stern
On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote: > On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote: > > Introducing usb_for_each_port(). It works the same way as > > usb_for_each_dev(), but instead of going through every USB > > device in the system, it walks through the USB ports in the > > system. > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > This has a couple of nasty errors. > > > --- > > drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++ > > include/linux/usb.h | 1 + > > 2 files changed, 44 insertions(+) > > > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > > index 2ce3667ec6fae..6d49db9a1b208 100644 > > --- a/drivers/usb/core/usb.c > > +++ b/drivers/usb/core/usb.c > > @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *)) > > } > > EXPORT_SYMBOL_GPL(usb_for_each_dev); > > > > +struct each_hub_arg { > > + void *data; > > + int (*fn)(struct device *, void *); > > +}; > > + > > +static int __each_hub(struct device *dev, void *data) > > +{ > > + struct each_hub_arg *arg = (struct each_hub_arg *)data; > > + struct usb_device *hdev = to_usb_device(dev); > > to_usb_device() won't work properly if the struct device isn't embedded > in an actual usb_device structure. And that will happen, since the USB > bus type holds usb_interface structures as well as usb_devices. OK, so I need to filter them out. > In fact, you should use usb_for_each_dev here; it already does what you > want. Unfortunately I can't use usb_for_each_dev here, because it deals with struct usb_device while I need to deal with struct device in the callback. > > + struct usb_hub *hub; > > + int ret; > > + int i; > > + > > + hub = usb_hub_to_struct_hub(hdev); > > + if (!hub) > > + return 0; > > + > > + for (i = 0; i < hdev->maxchild; i++) { > > + ret = arg->fn(&hub->ports[i]->dev, arg->data); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > Don't you need some sort of locking or refcounting here? What would > happen if this hub got removed while the routine was running? I'll use a lock then. thanks,
On Thu, Mar 25, 2021 at 05:14:42PM +0200, Heikki Krogerus wrote: > On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote: > > On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote: > > > Introducing usb_for_each_port(). It works the same way as > > > usb_for_each_dev(), but instead of going through every USB > > > device in the system, it walks through the USB ports in the > > > system. > > > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > This has a couple of nasty errors. > > > > > --- > > > drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/usb.h | 1 + > > > 2 files changed, 44 insertions(+) > > > > > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > > > index 2ce3667ec6fae..6d49db9a1b208 100644 > > > --- a/drivers/usb/core/usb.c > > > +++ b/drivers/usb/core/usb.c > > > @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *)) > > > } > > > EXPORT_SYMBOL_GPL(usb_for_each_dev); > > > > > > +struct each_hub_arg { > > > + void *data; > > > + int (*fn)(struct device *, void *); > > > +}; > > > + > > > +static int __each_hub(struct device *dev, void *data) > > > +{ > > > + struct each_hub_arg *arg = (struct each_hub_arg *)data; > > > + struct usb_device *hdev = to_usb_device(dev); > > > > to_usb_device() won't work properly if the struct device isn't embedded > > in an actual usb_device structure. And that will happen, since the USB > > bus type holds usb_interface structures as well as usb_devices. > > OK, so I need to filter them out. > > > In fact, you should use usb_for_each_dev here; it already does what you > > want. > > Unfortunately I can't use usb_for_each_dev here, because it deals with > struct usb_device while I need to deal with struct device in the > callback. Why do you need 'struct device' in the callback? All you really want is the hub devices, right? > > > + struct usb_hub *hub; > > > + int ret; > > > + int i; > > > + > > > + hub = usb_hub_to_struct_hub(hdev); > > > + if (!hub) > > > + return 0; > > > + > > > + for (i = 0; i < hdev->maxchild; i++) { > > > + ret = arg->fn(&hub->ports[i]->dev, arg->data); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > > Don't you need some sort of locking or refcounting here? What would > > happen if this hub got removed while the routine was running? > > I'll use a lock then. That's not going to work to be held over a callback. Just properly increment the reference count. thanks, greg k-h
On Thu, Mar 25, 2021 at 05:14:45PM +0200, Heikki Krogerus wrote: > On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote: > > On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote: > > > Introducing usb_for_each_port(). It works the same way as > > > usb_for_each_dev(), but instead of going through every USB > > > device in the system, it walks through the USB ports in the > > > system. > > > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > This has a couple of nasty errors. > > > > > --- > > > drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/usb.h | 1 + > > > 2 files changed, 44 insertions(+) > > > > > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > > > index 2ce3667ec6fae..6d49db9a1b208 100644 > > > --- a/drivers/usb/core/usb.c > > > +++ b/drivers/usb/core/usb.c > > > @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *)) > > > } > > > EXPORT_SYMBOL_GPL(usb_for_each_dev); > > > > > > +struct each_hub_arg { > > > + void *data; > > > + int (*fn)(struct device *, void *); > > > +}; > > > + > > > +static int __each_hub(struct device *dev, void *data) > > > +{ > > > + struct each_hub_arg *arg = (struct each_hub_arg *)data; > > > + struct usb_device *hdev = to_usb_device(dev); > > > > to_usb_device() won't work properly if the struct device isn't embedded > > in an actual usb_device structure. And that will happen, since the USB > > bus type holds usb_interface structures as well as usb_devices. > > OK, so I need to filter them out. > > > In fact, you should use usb_for_each_dev here; it already does what you > > want. > > Unfortunately I can't use usb_for_each_dev here, because it deals with > struct usb_device while I need to deal with struct device in the > callback. Ah, I can use it instead of bus_for_each_dev() in usb_for_each_port(). I'll fix these in v2. For the lock I guess I can just use the peer lock (usb_port_peer_mutex)? thanks,
On Thu, Mar 25, 2021 at 05:14:42PM +0200, Heikki Krogerus wrote: > On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote: > > On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote: > > > Introducing usb_for_each_port(). It works the same way as > > > usb_for_each_dev(), but instead of going through every USB > > > device in the system, it walks through the USB ports in the > > > system. > > > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > This has a couple of nasty errors. > > > > > --- > > > drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/usb.h | 1 + > > > 2 files changed, 44 insertions(+) > > > > > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > > > index 2ce3667ec6fae..6d49db9a1b208 100644 > > > --- a/drivers/usb/core/usb.c > > > +++ b/drivers/usb/core/usb.c > > > @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *)) > > > } > > > EXPORT_SYMBOL_GPL(usb_for_each_dev); > > > > > > +struct each_hub_arg { > > > + void *data; > > > + int (*fn)(struct device *, void *); > > > +}; > > > + > > > +static int __each_hub(struct device *dev, void *data) > > > +{ > > > + struct each_hub_arg *arg = (struct each_hub_arg *)data; > > > + struct usb_device *hdev = to_usb_device(dev); > > > > to_usb_device() won't work properly if the struct device isn't embedded > > in an actual usb_device structure. And that will happen, since the USB > > bus type holds usb_interface structures as well as usb_devices. > > OK, so I need to filter them out. > > > In fact, you should use usb_for_each_dev here; it already does what you > > want. > > Unfortunately I can't use usb_for_each_dev here, because it deals with > struct usb_device while I need to deal with struct device in the > callback. I see; the prototypes of arg->fn are different. Oh well, it's a shame the code can't be reused. In any case, you should copy what usb.c:__each_dev() does. Alan Stern > > > + struct usb_hub *hub; > > > + int ret; > > > + int i; > > > + > > > + hub = usb_hub_to_struct_hub(hdev); > > > + if (!hub) > > > + return 0; > > > + > > > + for (i = 0; i < hdev->maxchild; i++) { > > > + ret = arg->fn(&hub->ports[i]->dev, arg->data); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > > Don't you need some sort of locking or refcounting here? What would > > happen if this hub got removed while the routine was running? > > I'll use a lock then. > > thanks, > > -- > heikki
On Thu, Mar 25, 2021 at 04:20:15PM +0100, Greg Kroah-Hartman wrote: > On Thu, Mar 25, 2021 at 05:14:42PM +0200, Heikki Krogerus wrote: > > On Thu, Mar 25, 2021 at 10:41:09AM -0400, Alan Stern wrote: > > > On Thu, Mar 25, 2021 at 03:29:21PM +0300, Heikki Krogerus wrote: > > > > Introducing usb_for_each_port(). It works the same way as > > > > usb_for_each_dev(), but instead of going through every USB > > > > device in the system, it walks through the USB ports in the > > > > system. > > > > > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > > > This has a couple of nasty errors. > > > > > > > --- > > > > drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++ > > > > include/linux/usb.h | 1 + > > > > 2 files changed, 44 insertions(+) > > > > > > > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > > > > index 2ce3667ec6fae..6d49db9a1b208 100644 > > > > --- a/drivers/usb/core/usb.c > > > > +++ b/drivers/usb/core/usb.c > > > > @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *)) > > > > } > > > > EXPORT_SYMBOL_GPL(usb_for_each_dev); > > > > > > > > +struct each_hub_arg { > > > > + void *data; > > > > + int (*fn)(struct device *, void *); > > > > +}; > > > > + > > > > +static int __each_hub(struct device *dev, void *data) > > > > +{ > > > > + struct each_hub_arg *arg = (struct each_hub_arg *)data; > > > > + struct usb_device *hdev = to_usb_device(dev); > > > > > > to_usb_device() won't work properly if the struct device isn't embedded > > > in an actual usb_device structure. And that will happen, since the USB > > > bus type holds usb_interface structures as well as usb_devices. > > > > OK, so I need to filter them out. > > > > > In fact, you should use usb_for_each_dev here; it already does what you > > > want. > > > > Unfortunately I can't use usb_for_each_dev here, because it deals with > > struct usb_device while I need to deal with struct device in the > > callback. > > Why do you need 'struct device' in the callback? All you really want is > the hub devices, right? I need the ports, not the hubs. > > > > + struct usb_hub *hub; > > > > + int ret; > > > > + int i; > > > > + > > > > + hub = usb_hub_to_struct_hub(hdev); > > > > + if (!hub) > > > > + return 0; > > > > + > > > > + for (i = 0; i < hdev->maxchild; i++) { > > > > + ret = arg->fn(&hub->ports[i]->dev, arg->data); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > > > Don't you need some sort of locking or refcounting here? What would > > > happen if this hub got removed while the routine was running? > > > > I'll use a lock then. > > That's not going to work to be held over a callback. Just properly > increment the reference count. I though we have done that already. Does bus_for_each_dev() do that for the device that it passes to the callback until that callback returns? thanks,
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c index 2ce3667ec6fae..6d49db9a1b208 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -398,6 +398,49 @@ int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *)) } EXPORT_SYMBOL_GPL(usb_for_each_dev); +struct each_hub_arg { + void *data; + int (*fn)(struct device *, void *); +}; + +static int __each_hub(struct device *dev, void *data) +{ + struct each_hub_arg *arg = (struct each_hub_arg *)data; + struct usb_device *hdev = to_usb_device(dev); + struct usb_hub *hub; + int ret; + int i; + + hub = usb_hub_to_struct_hub(hdev); + if (!hub) + return 0; + + for (i = 0; i < hdev->maxchild; i++) { + ret = arg->fn(&hub->ports[i]->dev, arg->data); + if (ret) + return ret; + } + + return 0; +} + +/** + * usb_for_each_port - interate over all USB ports in the system + * @data: data pointer that will be handed to the callback function + * @fn: callback function to be called for each USB port + * + * Iterate over all USB ports and call @fn for each, passing it @data. If it + * returns anything other than 0, we break the iteration prematurely and return + * that value. + */ +int usb_for_each_port(void *data, int (*fn)(struct device *, void *)) +{ + struct each_hub_arg arg = {data, fn}; + + return bus_for_each_dev(&usb_bus_type, NULL, &arg, __each_hub); +} +EXPORT_SYMBOL_GPL(usb_for_each_port); + /** * usb_release_dev - free a usb device structure when all users of it are finished. * @dev: device that's been disconnected diff --git a/include/linux/usb.h b/include/linux/usb.h index 57c1e0ce5eba4..06ed5779fb4d8 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -869,6 +869,7 @@ extern int usb_match_one_id(struct usb_interface *interface, const struct usb_device_id *id); extern int usb_for_each_dev(void *data, int (*fn)(struct usb_device *, void *)); +int usb_for_each_port(void *data, int (*fn)(struct device *, void *)); extern struct usb_interface *usb_find_interface(struct usb_driver *drv, int minor); extern struct usb_interface *usb_ifnum_to_if(const struct usb_device *dev,
Introducing usb_for_each_port(). It works the same way as usb_for_each_dev(), but instead of going through every USB device in the system, it walks through the USB ports in the system. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/usb/core/usb.c | 43 ++++++++++++++++++++++++++++++++++++++++++ include/linux/usb.h | 1 + 2 files changed, 44 insertions(+)