On Thu, 2014-01-16 at 16:18 -0500, Alan Stern wrote:
> On Thu, 16 Jan 2014, Sarah Sharp wrote:
>
> > > > What do you think about the rest of the patchset?
> > >
> > > I regret that I haven't taken the time to look through it yet. :-(
> > > I'll do my best to go through it soon.
> >
> > No worries! I'm most interested in your comments about the changes to
> > khubd, so if you could find time to look at just patches 7 and 9, that
> > would be appreciated.
>
> Okay, I have looked at them (and patch 8 too). In short, I disapprove
> of the design. It's an ad-hoc approach that ignores the larger issues.
>
> The real problem is that we need to guarantee mutual exclusion for
> access to a particular port on the hub. The things we can do to a port
> include:
>
> khubd's normal processing:
> turning off various port status-change flags,
> handling remote wakeups,
> taking care of overcurrent events,
> issuing USB-3 warm resets,
> and handling connect/disconnect events;
>
> Issuing a port reset, which can happen:
> when a USB-3 port goes into SS.Inactive,
> when a driver resets a device,
> or when a reset-resume is needed;
>
> Resuming a port (perhaps suspending it too);
>
> Turning port power on or off.
>
> These four somewhat overlapping actions need to be mutually exclusive.
> (Actually, the first of khubd's activities -- turning off status-change
> flags -- probably doesn't need to be exclusive with anything else. But
> we might as well include it with the rest.)
>
> The natural solution is use a per-port mutex, instead of messing around
> with atomic busy_bits stuff or PM barriers.
>
> It turns out that we will require a particular locking order. It will
> sometimes be necessary to acquire the port's mutex while holding the
> child device's lock. This can happen when we are binding or unbinding
> a driver to the child device; usb_probe_interface() is called with the
> child locked, and it calls usb_autoresume_device() which will need to
> lock the upstream port if the device is in runtime suspend.
>
> As far as I can tell, we don't actually need to acquire the locks in
> the opposite order, although to make that work means we will have to
> drop the port lock in various parts of hub_port_connect_change() where
> the child device gets locked.
>
> One possibility is to use the port's own embedded device lock as the
> port mutex. But this would preclude ever moving the child USB device
> directly under the port device in the device tree, because the locking
> order would be wrong. It seems safer to embed a new mutex in struct
> usb_port.
>
> Dan, what do you think of this approach?
Maybe I need to consider it a bit longer, but introducing a per-port
mutex adds at least 2 new lock ordering constraints. A replacement of
hub->busy_bits with port_dev->lock now introduces a constraint with not
only the child lock, but also synchronous invocations of rpm_{suspend|
resume} for the port.
Patch 7 would look something like this:
---
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index d86548edcc36..da12d07da127 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4740,6 +4740,13 @@ static void hub_events(void)
/* deal with port status changes */
for (i = 1; i <= hdev->maxchild; i++) {
+ mutex_lock(&port_dev->lock);
+ if (port_dev->power_is_on)
+ do_hub_event(...);
+ else
+ hub_clear_misc_change(..);
+ mutex_unlock(&port_dev->lock);
+
[..]
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 97e4939fee1a..a7f32f200d90 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -88,7 +88,7 @@ static int usb_port_runtime_resume(struct device *dev)
pm_runtime_get_sync(&peer->dev);
usb_autopm_get_interface(intf);
- set_bit(port1, hub->busy_bits);
+ mutex_lock(&port_dev->lock);
retval = usb_hub_set_port_power(hdev, hub, port1, true);
if (port_dev->child && !retval) {
@@ -105,7 +105,7 @@ static int usb_port_runtime_resume(struct device *dev)
retval = 0;
}
- clear_bit(port1, hub->busy_bits);
+ mutex_unlock(&port_dev->lock);
usb_autopm_put_interface(intf);
if (!hub_is_superspeed(hdev) && peer)
@@ -138,12 +138,12 @@ static int usb_port_runtime_suspend(struct device *dev)
return -EBUSY;
usb_autopm_get_interface(intf);
- set_bit(port1, hub->busy_bits);
+ mutex_lock(&port_dev->lock);
retval = usb_hub_set_port_power(hdev, hub, port1, false);
usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_CONNECTION);
if (!hub_is_superspeed(hdev))
usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE);
- clear_bit(port1, hub->busy_bits);
+ mutex_unlock(&port_dev->lock);
usb_autopm_put_interface(intf);
/* bounce our peer now that we are down */
---
...not too bad, although I don't really like ->power_is_on compared to
just reading the pm runtime state. If we use pm_runtime_active() then
this begins to look a lot like patch 7 again.
However, patch 9 wants khubd held off until both the port and any child
devices have had a chance to resume. It would expand the scope of the
lock from protecting concurrent port access to ordering port vs child
device resume. Patch 9 as coded does so without adding a locking
constraint (beyond flushing resume work). We certainly can't wrap a
port mutex around usb_autoresume_device(port_dev->child) given the child
synchronously resumes the port. What is the alternative I am missing?
Some nice benefits fall out from forcing a port+child resume cycle on
port resume:
1/ prevents usb_port_runtime_resume() from growing recovery logic that
usb_port_resume() implements, like reset and disconnect.
2/ similarly, if usb_port_resume() grows warm reset recovery logic (as
it seems to need) that is applicable to port power recovery as well.
Leaning on pm_runtime synchronization to implement new hub_events()
requirements of "port pm active" and "flushes pending usb_device
recovery" is a tighter implementation. Specifically, it is tighter than
adding a new lock and its ordering constraints between suspend, resume,
child and potentially peer port events.
--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
