On Thu, 16 Jan 2014, Dan Williams wrote:
> 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.
That second constraint is very simple: Since the resume operation will
acquire the port lock, you have to make sure you don't hold the port
lock when performing a resume.
> 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);
> +
> [..]
I wouldn't split out hub_clear_misc_change in quite that way, but never
mind.
Also, you left out the places in hub_events where the port lock needs
to be dropped: around the calls to usb_reset_device and
hub_port_connect_change. And you left out the places in the resume and
reset routines where the port lock needs to be acquired.
> 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 */
> ---
Yes, something like that.
> ...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?
For one thing, like I mentioned above, usb_port_resume needs to hold
the port lock. (And so does usb_reset_device, after it calls
usb_autoresume_device.)
But maybe I'm not getting your point.
> 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.
If you mean that we should avoid duplicating code between
usb_port_resume/finish_port_resume and the resume routines in port.c, I
agree.
The basic idea of using runtime PM synchronization to prevent khubd and
port power operations from interfering sounds good, and it is simpler
than adding a new mutex. And I think we can also use it to prevent
port power operations and port resets from interfering. But that
leaves open the question of how to prevent usb_device_reset from
interfering with khubd.
Alan Stern
--
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
