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
