Re: [PATCH v2 4/4] usb: hub: rename khubd to hub_wq in documentation and comments
On Thu, 18 Sep 2014, Tejun Heo wrote: Hello, Alan, Petr. On Wed, Sep 17, 2014 at 01:36:26PM -0400, Alan Stern wrote: - /* If khubd ever becomes multithreaded, this will need a lock */ + /* If hub_wq ever becomes multithreaded, this will need a lock */ if (udev-wusb) { devnum = udev-portnum + 1; BUG_ON(test_bit(devnum, bus-devmap.devicemap)); You probably didn't notice when changing this comment. But in fact, workqueues _are_ multithreaded. Therefore you need to add a lock to this routine. Haven't read the code but if this function is called from a single work_struct, workqueue guarantees that there's only single thread of execution at any given time. A work item is never executed concurrently no matter what. This routine can be called from multiple work_structs, because a USB bus can have multiple hubs. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] usb: hub: rename khubd to hub_wq in documentation and comments
On Thu 18-09-14 10:24:23, Alan Stern wrote: On Thu, 18 Sep 2014, Tejun Heo wrote: Hello, Alan, Petr. On Wed, Sep 17, 2014 at 01:36:26PM -0400, Alan Stern wrote: - /* If khubd ever becomes multithreaded, this will need a lock */ + /* If hub_wq ever becomes multithreaded, this will need a lock */ if (udev-wusb) { devnum = udev-portnum + 1; BUG_ON(test_bit(devnum, bus-devmap.devicemap)); You probably didn't notice when changing this comment. But in fact, workqueues _are_ multithreaded. Therefore you need to add a lock to this routine. Haven't read the code but if this function is called from a single work_struct, workqueue guarantees that there's only single thread of execution at any given time. A work item is never executed concurrently no matter what. This routine can be called from multiple work_structs, because a USB bus can have multiple hubs. The easiest solution would be to allocate the work queue with the flag WQ_UNBOUND and max_active = 1. It will force serialization of all work items. Alternatively, we might add the locking. But to be honest, I am not sure that I am brave enough to do so. First, I am not sure if this is the only location that might be affected by the parallel execution of hub_event(). Second, there are used so many locks and the code is so complex that I would need many days and maybe weeks to understand it. Well, if we assume that this is the only problematic location, here are the ideas how to prevent the parallel execution: 1. Use some brand new lock, e.g. call it hub_devnum_lock, and do: static void choose_devnum(struct usb_device *udev) { spin_lock_irq(hub_devnum_lock); [...] spin_unlock_irq(hub_event_lock); } This looks clean but it creates another lock. 2. Alternatively, we could use an existing global lock the same way, for example usb_bus_list_lock. But this looks like a hack and I do not like it much. 3. Alternatively, it seems the the function affects one struct usb_device and one struct usb_bus. It might be enough to take the appropriate locks for these structures. This would mean to take two locks. It would be slower and we would need to make sure that it does not cause a dead lock. Best Regards, Petr -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] usb: hub: rename khubd to hub_wq in documentation and comments
On Thu, Sep 18, 2014 at 06:15:02PM +0200, Petr Mládek wrote: The easiest solution would be to allocate the work queue with the flag WQ_UNBOUND and max_active = 1. It will force serialization of all work items. Please use alloc_ordered_workqueue() for that purpose. WQ_UNBOUND + max_active == 1 no longer guarantees single thread of execution. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] usb: hub: rename khubd to hub_wq in documentation and comments
On Thu, 18 Sep 2014, Petr [iso-8859-1] Ml�dek wrote: This routine can be called from multiple work_structs, because a USB bus can have multiple hubs. The easiest solution would be to allocate the work queue with the flag WQ_UNBOUND and max_active = 1. It will force serialization of all work items. Alternatively, we might add the locking. But to be honest, I am not sure that I am brave enough to do so. First, I am not sure if this is the only location that might be affected by the parallel execution of hub_event(). Second, there are used so many locks and the code is so complex that I would need many days and maybe weeks to understand it. In the past I have considered making khubd multithreaded. But it didn't seem especially important. Apart from initial device discovery while the system is starting up (which works out well enough now, even if it could be faster) there usually is activity on only one USB hub and port at a time. On the whole, I would be happy if we can simply guarantee that max_active never gets higher than 1. As Tejun recommended, alloc_ordered_workqueue should be enough. Well, if we assume that this is the only problematic location, here are the ideas how to prevent the parallel execution: 1. Use some brand new lock, e.g. call it hub_devnum_lock, and do: static void choose_devnum(struct usb_device *udev) { spin_lock_irq(hub_devnum_lock); [...] spin_unlock_irq(hub_event_lock); } This looks clean but it creates another lock. That would be okay. It shouldn't be a spinlock; a mutex would be better. And adding a new lock doesn't hurt if it is private to this one routine, since this is a leaf routine. Or if you want, you could reuse udev-bus-usb_address0_mutex. That would make sense, because that mutex is already associated with device address assignment. 2. Alternatively, we could use an existing global lock the same way, for example usb_bus_list_lock. But this looks like a hack and I do not like it much. 3. Alternatively, it seems the the function affects one struct usb_device and one struct usb_bus. It might be enough to take the appropriate locks for these structures. This would mean to take two locks. It would be slower and we would need to make sure that it does not cause a dead lock. The right answer is to use either a private mutex or the bus-specific usb_address0_mutex. As far as I know, this is the only place that relies on khubd being a single thread. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/4] usb: hub: rename khubd to hub_wq in documentation and comments
USB hub has started to use a workqueue instead of kthread. Let's update the documentation and comments here and there. This patch mostly just replaces khubd with hub_wq. There are only few exceptions where the whole sentence was updated. These more complicated changes can be found in the following files: Documentation/usb/hotplug.txt drivers/net/usb/usbnet.c drivers/usb/core/hcd.c drivers/usb/host/ohci-hcd.c drivers/usb/host/xhci.c Signed-off-by: Petr Mladek pmla...@suse.cz --- Documentation/DocBook/usb.tmpl | 2 +- Documentation/usb/WUSB-Design-overview.txt | 6 ++-- Documentation/usb/hotplug.txt | 2 +- drivers/net/usb/usbnet.c | 14 ++ drivers/usb/README | 2 +- drivers/usb/core/hcd.c | 10 +++ drivers/usb/core/hub.c | 44 +++--- drivers/usb/host/ehci-fsl.c| 2 +- drivers/usb/host/ehci-hcd.c| 2 +- drivers/usb/host/ehci-hub.c| 8 +++--- drivers/usb/host/fhci-hcd.c| 6 ++-- drivers/usb/host/fotg210-hcd.c | 8 +++--- drivers/usb/host/fusbh200-hcd.c| 8 +++--- drivers/usb/host/isp1760-hcd.c | 6 ++-- drivers/usb/host/ohci-hcd.c| 6 ++-- drivers/usb/host/ohci-hub.c| 4 +-- drivers/usb/host/ohci-omap.c | 2 +- drivers/usb/host/oxu210hp-hcd.c| 10 +++ drivers/usb/host/sl811-hcd.c | 8 +++--- drivers/usb/host/xhci-hub.c| 2 +- drivers/usb/host/xhci.c| 4 +-- drivers/usb/misc/usbtest.c | 2 +- drivers/usb/musb/am35x.c | 1 + drivers/usb/musb/tusb6010.c| 2 +- drivers/usb/phy/phy-fsl-usb.c | 2 +- drivers/usb/phy/phy-isp1301-omap.c | 2 +- drivers/usb/wusbcore/devconnect.c | 6 ++-- drivers/usb/wusbcore/wa-hc.h | 2 +- sound/usb/midi.c | 2 +- 29 files changed, 89 insertions(+), 86 deletions(-) diff --git a/Documentation/DocBook/usb.tmpl b/Documentation/DocBook/usb.tmpl index 85fc0e28576f..4cd5b2cd0f3d 100644 --- a/Documentation/DocBook/usb.tmpl +++ b/Documentation/DocBook/usb.tmpl @@ -593,7 +593,7 @@ for (;;) { Each device has one control endpoint (endpoint zero) which supports a limited RPC style RPC access. Devices are configured - by khubd (in the kernel) setting a device-wide + by hub_wq (in the kernel) setting a device-wide emphasisconfiguration/emphasis that affects things like power consumption and basic functionality. The endpoints are part of USB emphasisinterfaces/emphasis, diff --git a/Documentation/usb/WUSB-Design-overview.txt b/Documentation/usb/WUSB-Design-overview.txt index 1cd07c017cf6..9d08f179a7ca 100644 --- a/Documentation/usb/WUSB-Design-overview.txt +++ b/Documentation/usb/WUSB-Design-overview.txt @@ -317,7 +317,7 @@ HC picks the /DN_Connect/ out (nep module sends to notif.c for delivery into /devconnect/). This process starts the authentication process for the device. First we allocate a /fake port/ and assign an unauthenticated address (128 to 255--what we really do is -0x80 | fake_port_idx). We fiddle with the fake port status and /khubd/ +0x80 | fake_port_idx). We fiddle with the fake port status and /hub_wq/ sees a new connection, so he moves on to enable the fake port with a reset. So now we are in the reset path -- we know we have a non-yet enumerated @@ -326,7 +326,7 @@ device with an unauthorized address; we ask user space to authenticate exchange (FIXME: not yet done) and issue a /set address 0/ to bring the device to the default state. Device is authenticated. -From here, the USB stack takes control through the usb_hcd ops. khubd +From here, the USB stack takes control through the usb_hcd ops. hub_wq has seen the port status changes, as we have been toggling them. It will start enumerating and doing transfers through usb_hcd-urb_enqueue() to read descriptors and move our data. @@ -340,7 +340,7 @@ Keep Alive IE; it responds with a /DN_Alive/ pong during the DNTS (this arrives to us as a notification through devconnect.c:wusb_handle_dn_alive(). If a device times out, we disconnect it from the system (cleaning up internal information and -toggling the bits in the fake hub port, which kicks khubd into removing +toggling the bits in the fake hub port, which kicks hub_wq into removing the rest of the stuff). This is done through devconnect:__wusb_check_devs(), which will scan the diff --git a/Documentation/usb/hotplug.txt b/Documentation/usb/hotplug.txt index a80b0e9a7a0b..5b243f315b2c 100644 --- a/Documentation/usb/hotplug.txt +++ b/Documentation/usb/hotplug.txt @@ -58,7 +58,7 @@ USB POLICY AGENT The USB subsystem
Re: [PATCH v2 4/4] usb: hub: rename khubd to hub_wq in documentation and comments
On Wed, 17 Sep 2014, Petr Mladek wrote: USB hub has started to use a workqueue instead of kthread. Let's update the documentation and comments here and there. This patch mostly just replaces khubd with hub_wq. There are only few exceptions where the whole sentence was updated. These more complicated changes can be found in the following files: Documentation/usb/hotplug.txt drivers/net/usb/usbnet.c drivers/usb/core/hcd.c drivers/usb/host/ohci-hcd.c drivers/usb/host/xhci.c Okay, here's a real issue. @@ -2042,7 +2042,7 @@ static void choose_devnum(struct usb_device *udev) int devnum; struct usb_bus *bus = udev-bus; - /* If khubd ever becomes multithreaded, this will need a lock */ + /* If hub_wq ever becomes multithreaded, this will need a lock */ if (udev-wusb) { devnum = udev-portnum + 1; BUG_ON(test_bit(devnum, bus-devmap.devicemap)); You probably didn't notice when changing this comment. But in fact, workqueues _are_ multithreaded. Therefore you need to add a lock to this routine. Still, apart from these relatively minor issues, the series looks good. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] usb: hub: rename khubd to hub_wq in documentation and comments
Hello, Alan, Petr. On Wed, Sep 17, 2014 at 01:36:26PM -0400, Alan Stern wrote: - /* If khubd ever becomes multithreaded, this will need a lock */ + /* If hub_wq ever becomes multithreaded, this will need a lock */ if (udev-wusb) { devnum = udev-portnum + 1; BUG_ON(test_bit(devnum, bus-devmap.devicemap)); You probably didn't notice when changing this comment. But in fact, workqueues _are_ multithreaded. Therefore you need to add a lock to this routine. Haven't read the code but if this function is called from a single work_struct, workqueue guarantees that there's only single thread of execution at any given time. A work item is never executed concurrently no matter what. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html