| Message ID | 20210329084426.78138-6-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: Linking ports to their Type-C connectors | expand |
On Mon, Mar 29, 2021 at 11:44:25AM +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> > --- > drivers/usb/core/usb.c | 46 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/usb.h | 1 + > 2 files changed, 47 insertions(+) > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > index 2ce3667ec6fae..62368c4ed37af 100644 > --- a/drivers/usb/core/usb.c > +++ b/drivers/usb/core/usb.c > @@ -398,6 +398,52 @@ 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 usb_device *hdev, void *data) > +{ > + struct each_hub_arg *arg = (struct each_hub_arg *)data; > + struct usb_hub *hub; > + int ret = 0; > + int i; > + > + hub = usb_hub_to_struct_hub(hdev); > + if (!hub) > + return 0; What happens if the hub is removed exactly now? Although hdev is reference-counted (and the loop iterator does take a reference to it), usb_hub_to_struct_hub doesn't take a reference to hub. And hub->ports isn't refcounted at all. > + > + mutex_lock(&usb_port_peer_mutex); > + > + for (i = 0; i < hdev->maxchild; i++) { > + ret = arg->fn(&hub->ports[i]->dev, arg->data); > + if (ret) > + break; > + } > + > + mutex_unlock(&usb_port_peer_mutex); I have a feeling that it would be better to take and release this mutex in usb_for_each_port (or its caller), so that it is held over the whole loop. Alan Stern > + > + return ret; > +} > + > +/** > + * 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 usb_for_each_dev(&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 ddd2f5b2a2827..e4d2eb703cf89 100644 > --- a/include/linux/usb.h > +++ b/include/linux/usb.h > @@ -871,6 +871,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 >
On Mon, Mar 29, 2021 at 02:49:46PM -0400, Alan Stern wrote: > On Mon, Mar 29, 2021 at 11:44:25AM +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> > > --- > > drivers/usb/core/usb.c | 46 ++++++++++++++++++++++++++++++++++++++++++ > > include/linux/usb.h | 1 + > > 2 files changed, 47 insertions(+) > > > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > > index 2ce3667ec6fae..62368c4ed37af 100644 > > --- a/drivers/usb/core/usb.c > > +++ b/drivers/usb/core/usb.c > > @@ -398,6 +398,52 @@ 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 usb_device *hdev, void *data) > > +{ > > + struct each_hub_arg *arg = (struct each_hub_arg *)data; > > + struct usb_hub *hub; > > + int ret = 0; > > + int i; > > + > > + hub = usb_hub_to_struct_hub(hdev); > > + if (!hub) > > + return 0; > > What happens if the hub is removed exactly now? Although hdev is > reference-counted (and the loop iterator does take a reference to it), > usb_hub_to_struct_hub doesn't take a reference to hub. And hub->ports > isn't refcounted at all. If the hub is removed right now, and if hub_disconnect() also manages to remove the ports before we have time to take the lock below, then hdev->maxchild will be 0 by the time we can take the lock. In that case nothing happens here. If on the other hand we manage to acquire the usb_port_peer_mutex before hub_disconnect(), then hub_disconnect() will simply have to wait until we are done, and only after that remove the ports. > > + mutex_lock(&usb_port_peer_mutex); > > + > > + for (i = 0; i < hdev->maxchild; i++) { > > + ret = arg->fn(&hub->ports[i]->dev, arg->data); > > + if (ret) > > + break; > > + } > > + > > + mutex_unlock(&usb_port_peer_mutex); > > I have a feeling that it would be better to take and release this mutex > in usb_for_each_port (or its caller), so that it is held over the whole > loop. I disagree. The lock is for the ports, not the hubs. We should take the lock when we are going through the ports of a hub, but release it between the hubs. Otherwise we will be only keeping things on hold for a long period of time for no good reason (I for example have to evaluate the _PLD of every single port which takes a lot of time). We don't need to prevent other things from happening to the hubs at the same time. thanks,
On Tue, Mar 30, 2021 at 12:07:35PM +0300, Heikki Krogerus wrote: > On Mon, Mar 29, 2021 at 02:49:46PM -0400, Alan Stern wrote: > > On Mon, Mar 29, 2021 at 11:44:25AM +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> > > > --- > > > drivers/usb/core/usb.c | 46 ++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/usb.h | 1 + > > > 2 files changed, 47 insertions(+) > > > > > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > > > index 2ce3667ec6fae..62368c4ed37af 100644 > > > --- a/drivers/usb/core/usb.c > > > +++ b/drivers/usb/core/usb.c > > > @@ -398,6 +398,52 @@ 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 usb_device *hdev, void *data) > > > +{ > > > + struct each_hub_arg *arg = (struct each_hub_arg *)data; > > > + struct usb_hub *hub; > > > + int ret = 0; > > > + int i; > > > + > > > + hub = usb_hub_to_struct_hub(hdev); > > > + if (!hub) > > > + return 0; > > > > What happens if the hub is removed exactly now? Although hdev is > > reference-counted (and the loop iterator does take a reference to it), > > usb_hub_to_struct_hub doesn't take a reference to hub. And hub->ports > > isn't refcounted at all. > > If the hub is removed right now, and if hub_disconnect() also manages > to remove the ports before we have time to take the lock below, then > hdev->maxchild will be 0 by the time we can take the lock. In that > case nothing happens here. Okay, good. > If on the other hand we manage to acquire the usb_port_peer_mutex > before hub_disconnect(), then hub_disconnect() will simply have to > wait until we are done, and only after that remove the ports. > > > > + mutex_lock(&usb_port_peer_mutex); > > > + > > > + for (i = 0; i < hdev->maxchild; i++) { > > > + ret = arg->fn(&hub->ports[i]->dev, arg->data); > > > + if (ret) > > > + break; > > > + } > > > + > > > + mutex_unlock(&usb_port_peer_mutex); > > > > I have a feeling that it would be better to take and release this mutex > > in usb_for_each_port (or its caller), so that it is held over the whole > > loop. > > I disagree. The lock is for the ports, not the hubs. We should take > the lock when we are going through the ports of a hub, but release it > between the hubs. Otherwise we will be only keeping things on hold for > a long period of time for no good reason (I for example have to > evaluate the _PLD of every single port which takes a lot of time). We > don't need to prevent other things from happening to the hubs at the > same time. All right, you convinced me. Acked-by: Alan Stern <stern@rowland.harvard.edu> Alan Stern
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c index 2ce3667ec6fae..62368c4ed37af 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -398,6 +398,52 @@ 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 usb_device *hdev, void *data) +{ + struct each_hub_arg *arg = (struct each_hub_arg *)data; + struct usb_hub *hub; + int ret = 0; + int i; + + hub = usb_hub_to_struct_hub(hdev); + if (!hub) + return 0; + + mutex_lock(&usb_port_peer_mutex); + + for (i = 0; i < hdev->maxchild; i++) { + ret = arg->fn(&hub->ports[i]->dev, arg->data); + if (ret) + break; + } + + mutex_unlock(&usb_port_peer_mutex); + + return ret; +} + +/** + * 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 usb_for_each_dev(&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 ddd2f5b2a2827..e4d2eb703cf89 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -871,6 +871,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 | 46 ++++++++++++++++++++++++++++++++++++++++++ include/linux/usb.h | 1 + 2 files changed, 47 insertions(+)