Re: [PATCH 1/6] usb: Iterator for ports
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 > > > > > > 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(>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, -- heikki
Re: [PATCH 1/6] usb: Iterator for ports
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 > > > > 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(>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
Re: [PATCH 1/6] usb: Iterator for ports
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 > > > > 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, -- heikki
Re: [PATCH 1/6] usb: Iterator for ports
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 > > > > 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(>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
Re: [PATCH 1/6] usb: Iterator for ports
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 > > 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(>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
Re: [PATCH 1/6] usb: Iterator for ports
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 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(>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
[PATCH 1/6] usb: Iterator for ports
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 --- 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); + 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(>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(_bus_type, NULL, , __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, -- 2.30.2