Re: [PATCH v3 03/10] usb: find internal hub tier mismatch via acpi
On Tue, 21 Jan 2014, Dan Williams wrote: I think we agree that khubd needs to not look at the portstatus while the port is down. pm runtime synchronization takes care of that and prevents khubd from starting while the port is inactive. With that in place we can now make requests in usb_port_runtime_resume() that are later serviced in khubd when the port is marked active (pm runtime enforces that -runtime_resume() return 0 before the port is marked active). How do you know, when you make the request, that khubd won't start running while the port is still down? If that happened then khubd would know to avoid looking at the port, of course, but the request would be used up and lost. Per above it requires khubd to barrier pm operations to finish bringing the port up. It does this while holding a reference to ensure the port does not go down while checking. I also expect a reference to be taken when the request is queued and dropped when khubd services it. That's not what I meant. Sending a request to khubd means doing something like this: set_bit(port1, hub-resume_child_bits); kick_khubd(hub); khubd may start running even before kick_khubd returns. If it does, it will find that the port has not yet returned to power-on status. Then it will skip the port, leaving the bit set in resume_child_bits, and go on to look at all the other ports in this hub. Thus, when the port does finish powering up, khubd won't be running any more. And there will be nothing to start it up, since kick_khubd has already done its job. So the child won't be resumed in a timely manner. With a lock, this problem doesn't arise. A lock for this scenario would effectively duplicate dev-power.lock, or be wider than necessary if I understand where you are considering holding it. I'm not sure why you say that. We could use the new status_lock. That said, I do believe you are still skeptical of pm runtime synchronization. A little bit. I'd feel better if you solve the problem outlined above. Also, I'm not sure what advantages PM synchronization has over a plain, simple mutex. Earlier you said it was more robust, but that's not clear to me. We could certainly take the lock when setting/clearing -power_is_on, and disable khubd from processing !power_is_on ports. However, that solution needs a way to flush a currently running khubd, or prevent a port from suspending while khubd is active (here comes pm runtime sync again) Easy: khubd will hold the status_lock almost the whole time it is running. (Not while calling pm_runtime_resume, usb_disconnect, or usb_new_device, though.) Since usb_port_runtime_resume will also acquire the lock, there won't be any interference. 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 v3 03/10] usb: find internal hub tier mismatch via acpi
On Wed, 22 Jan 2014, Dan Williams wrote: That's not what I meant. Sending a request to khubd means doing something like this: set_bit(port1, hub-resume_child_bits); kick_khubd(hub); khubd may start running even before kick_khubd returns. If it does, it will find that the port has not yet returned to power-on status. It handles that with pm_runtime_barrier(). When the request bit is set the port is in the RPM_RESUMING state. We hold a reference and khubd forces the port all the way to RPM_ACTIVE before checking the state and dropping references. Ah, yes. I was forgetting about the barrier. Well, we seem to be in agreement on the basic design now. Go ahead and start writing patches. :-) I have a few suggestions. First, begin with a patch that refactors hub_events: Move the guts of the loop over the ports into a separate subroutine. That will make things a little easier to handle. Second, don't bother with the special iterator for the port loop, the one you introduced before. Just put the get and the barrier inline. Third, don't separate out the code that clears the various status-change bits for powered-off ports. Simply handle those bits first, and afterwards skip the rest of the processing if the port isn't powered on Fourth, introduce the runtime PM synchronization and the status_lock in separate patches.. Fifth, don't move the lines that acquire the child device lock. Just drop the status_lock immediately before, and then grab it again after the device is unlocked. 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 v3 03/10] usb: find internal hub tier mismatch via acpi
On Sat, 2014-01-18 at 08:59 -0500, Alan Stern wrote:
On Fri, 17 Jan 2014, Dan Williams wrote:
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.
Yes, patch 9 narrowly addresses a problem that needs a wider solution.
We want to:
1/ prevent khubd from running while reset is in progress
This is the open question mentioned above, if you add in the
requirement that we also want to prevent a reset from starting while
khubd is running (except when khubd performs the reset itself).
2/ prevent khubd from seeing an intermediate device state during
usb_port_resume
khubd doesn't pay all that much attention to device states; mostly it
is concerned with port statuses. We don't want khubd to see an
intermediate port status during usb_port_resume. Basically that means
we don't want khubd to run while usb_port_resume is in progress, and we
don't want usb_port_resume to start while khubd is running unless khubd
performs the resume itself.
Yes, it does not pay all that much attention to device states, but it is
critical that it not read -state while usb_port_resume() is changing
it. Specifically this hole in hub_port_connect_change():
if ((portstatus USB_PORT_STAT_CONNECTION) udev
udev-state != USB_STATE_NOTATTACHED) {
if (portstatus USB_PORT_STAT_ENABLE) {
status = 0; /* Nothing to do */
#ifdef CONFIG_PM_RUNTIME
...usb_port_resume() can change the state at this point and cause a
disconnect.
} else if (udev-state == USB_STATE_SUSPENDED
udev-persist_enabled) {
/* For a suspended device, treat this as a
* remote wakeup event.
*/
status = usb_remote_wakeup(udev);
#endif
} else {
/* Don't resuscitate */;
}
}
3/ prevent khubd from running in the interval between completion of
ubs_port_runtime_resume() completing and usb_port_resume() running.
Hmmm. I don't remember what usb_port_runtime_resume does about a
connected USB-2 child device. It can't simply do a runtime resume of
the child. Maybe it should tell khubd to resume the child? Then khubd
would never see the intermediate state; the next time it looked at the
port, it would issue the runtime resume. (Provided that
usb_port_runtime_resume didn't complete after khubd was already
running.)
Yes, that's what I had in the patch, more straightforward than the work
queue.
All these scenarios to point to problems with -device_state collisions
not necessarily the port state.
I don't think so, for the reason mentioned above.
I'll rephrase, checking USB_STATE_SUSPENDED needs usb_port_resume()
synchronization.
And it is starting
to seem less likely that we can rely on runtime PM synchronization to
do everything we want. For example, if something is suspended then
there is no way to prevent a runtime resume from occurring at any
moment.
I propose we use usb_port pm runtime synchronization to block khubd when
we know the portstatus is not valid (powered off), and we need a new
lock to synchronize with usb_device pm operations before checking the
device state.
How about a new lock that synchronizes
device state changes with state checks? This replaces patch 9, only
compile tested:
When considering overall strategies for solving a problem, I find that
posting patches is almost never helpful. A patch focuses the mind on
low-level coding details, many of which are irrelevant to the problem
at hand, preventing you from concentrating on the overall picture --
which is the most important thing at this stage of planning.
Pseudo-code can be better in this regard, and I sometimes use it when a
degree of precision is necessary in the discussion. However, I don't
think we have reached that point yet.
Getting closer...
Therefore I would greatly prefer if you could describe in prose what
you want to do.
Ok.
I think we agree that khubd needs to not look at the portstatus while
the port is down. pm runtime synchronization takes care of that and
prevents khubd from starting while the port is inactive. With that in
place we can now make requests in usb_port_runtime_resume() that are
later serviced in khubd when the port is marked active (pm runtime
enforces that -runtime_resume() return 0 before the port is marked
active). This mechanism can be used for forcing at least one child
resume for each
Re: [PATCH v3 03/10] usb: find internal hub tier mismatch via acpi
The lock hangs off the usb_device rather than the the usb_port since it
is meant to prevent unintended disconnects. Any portstatus vs khubd
collisions are handled by pm runtime synchronization since there is no
expectation that khubd resumes a usb_port.
Once khubd has made a determination that it wants to resume or reset a
device it will of course need to drop the state lock. Those actions
will implicitly take the state lock.
Here's the patch (to replace patch 9), now tested. There are of course
cleanups that can follow this, but holding off until we have agreement
on closing these races.
[..]
Sarah,
I had meant to send this hack out separately. I believe this is the
infinite loop that Tianyu had seen previously. I had not been able to
reproduce until now (new timings now that khubd flushes device pm
ops). This works around a remote wakeup vs port poweroff request
race. A wakeup request sets the timer, but by the time it expires the
port has been turned off, so we hit:
if ((raw_port_status PORT_RESET) ||
!(raw_port_status PORT_PE))
return 0x;
...in xhci_get_port_status. Cancelling the resume seems the right
choice given that userspace has stated I don't care about remote
wakeups on this port by clearing pm_qos_no_poweroff.
Thoughts?
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 9992fbfec85f..2333ec573594 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1004,9 +1004,16 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq,
u16 wValue,
xhci_disable_port(hcd, xhci, wIndex,
port_array[wIndex], temp);
break;
- case USB_PORT_FEAT_POWER:
- writel(temp ~PORT_POWER, port_array[wIndex]);
+ case USB_PORT_FEAT_POWER: {
+ struct xhci_bus_state *bus_state;
+ bus_state = xhci-bus_state[hcd_index(hcd)];
+ writel(temp ~PORT_POWER, port_array[wIndex]);
+ if (test_and_clear_bit(wIndex,
bus_state-resuming_ports)) {
+ bus_state-resume_done[wIndex] = 0;
+ xhci_dbg(xhci, port%d: resume cancelled\n,
+wIndex);
+ }
spin_unlock_irqrestore(xhci-lock, flags);
temp = usb_acpi_power_manageable(hcd-self.root_hub,
wIndex);
@@ -1015,6 +1022,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq,
u16 wValue,
wIndex, false);
spin_lock_irqsave(xhci-lock, flags);
break;
+ }
default:
goto error;
}
--
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 v3 03/10] usb: find internal hub tier mismatch via acpi
On Tue, 21 Jan 2014, Dan Williams wrote:
We want to:
1/ prevent khubd from running while reset is in progress
This is the open question mentioned above, if you add in the
requirement that we also want to prevent a reset from starting while
khubd is running (except when khubd performs the reset itself).
2/ prevent khubd from seeing an intermediate device state during
usb_port_resume
khubd doesn't pay all that much attention to device states; mostly it
is concerned with port statuses. We don't want khubd to see an
intermediate port status during usb_port_resume. Basically that means
we don't want khubd to run while usb_port_resume is in progress, and we
don't want usb_port_resume to start while khubd is running unless khubd
performs the resume itself.
Yes, it does not pay all that much attention to device states, but it is
critical that it not read -state while usb_port_resume() is changing
it. Specifically this hole in hub_port_connect_change():
It doesn't matter. We don't want khubd and usb_port_resume to run at
the same time, for other reasons, and therefore we don't have to worry
about khubd seeing an intermediate device state caused by
usb_port_resume.
It is true, however, that the region of usb_port_resume currently
protected by hub-busy_bits isn't big enough.
3/ prevent khubd from running in the interval between completion of
ubs_port_runtime_resume() completing and usb_port_resume() running.
Hmmm. I don't remember what usb_port_runtime_resume does about a
connected USB-2 child device. It can't simply do a runtime resume of
the child. Maybe it should tell khubd to resume the child? Then khubd
would never see the intermediate state; the next time it looked at the
port, it would issue the runtime resume. (Provided that
usb_port_runtime_resume didn't complete after khubd was already
running.)
Yes, that's what I had in the patch, more straightforward than the work
queue.
Okay.
All these scenarios to point to problems with -device_state collisions
not necessarily the port state.
I don't think so, for the reason mentioned above.
I'll rephrase, checking USB_STATE_SUSPENDED needs usb_port_resume()
synchronization.
Which we need anyway, so the device_state aspect doesn't matter. (You
may have overlooked the port-status tests in hub_handle_remote_wakeup.)
I propose we use usb_port pm runtime synchronization to block khubd when
we know the portstatus is not valid (powered off),
Suppose, when you power the port back on, you find that the device is
no longer plugged in. How do you prevent the port from being powered
off again before khubd has a chance to finalize the device? I guess
you would have to do a pm_runtime_get on the port whenever the port's
bit in hub-change_bits gets set.
and we need a new
lock to synchronize with usb_device pm operations before checking the
device state.
Forget about the device state. It's a distraction.
I think we agree that khubd needs to not look at the portstatus while
the port is down. pm runtime synchronization takes care of that and
prevents khubd from starting while the port is inactive. With that in
place we can now make requests in usb_port_runtime_resume() that are
later serviced in khubd when the port is marked active (pm runtime
enforces that -runtime_resume() return 0 before the port is marked
active).
How do you know, when you make the request, that khubd won't start
running while the port is still down? If that happened then khubd
would know to avoid looking at the port, of course, but the request
would be used up and lost.
With a lock, this problem doesn't arise.
This mechanism can be used for forcing at least one child
resume for each port resume, and rate-limiting port power requests.
What's the connection with rate-limiting? Under what circumstances
would you need to rate-limit the port power changes?
For the next case, fixing khubd vs device_state changes, we can't use
pm_runtime synchronization because khubd is expected to be able to
resume a suspended usb_device. So we need to prevent khubd from
evaluating the device_state (USB_STATE_SUSPENDED) while usb_device pm
operations are in flight. In the case of dpm_{suspend|resume} khubd is
frozen so there's no collision, in the case of rpm_{suspend|resume} we
need to take a lock.
Agreed.
Let's wrap usb_runtime_{resume|suspend} in a new usb_device-state_lock
and then take that lock in khubd whenever it needs to check -state or
portstatus values associated with suspend. Specifically:
@@ -1765,10 +1774,14 @@ int usb_runtime_resume(struct device *dev)
struct usb_device *udev = to_usb_device(dev);
int status;
- /* Runtime resume for a USB device means resuming both the device
-* and all its interfaces.
+ /* Runtime resume for a USB device means resuming both the
+* device and
Re: [PATCH v3 03/10] usb: find internal hub tier mismatch via acpi
On Tue, Jan 21, 2014 at 2:05 PM, Alan Stern st...@rowland.harvard.edu wrote:
On Tue, 21 Jan 2014, Dan Williams wrote:
We want to:
1/ prevent khubd from running while reset is in progress
This is the open question mentioned above, if you add in the
requirement that we also want to prevent a reset from starting while
khubd is running (except when khubd performs the reset itself).
2/ prevent khubd from seeing an intermediate device state during
usb_port_resume
khubd doesn't pay all that much attention to device states; mostly it
is concerned with port statuses. We don't want khubd to see an
intermediate port status during usb_port_resume. Basically that means
we don't want khubd to run while usb_port_resume is in progress, and we
don't want usb_port_resume to start while khubd is running unless khubd
performs the resume itself.
Yes, it does not pay all that much attention to device states, but it is
critical that it not read -state while usb_port_resume() is changing
it. Specifically this hole in hub_port_connect_change():
It doesn't matter. We don't want khubd and usb_port_resume to run at
the same time, for other reasons, and therefore we don't have to worry
about khubd seeing an intermediate device state caused by
usb_port_resume.
I won't argue this anymore. Protecting khubd against portstatus
changes makes the device state moot.
[..]
All these scenarios to point to problems with -device_state collisions
not necessarily the port state.
I don't think so, for the reason mentioned above.
I'll rephrase, checking USB_STATE_SUSPENDED needs usb_port_resume()
synchronization.
Which we need anyway, so the device_state aspect doesn't matter. (You
may have overlooked the port-status tests in hub_handle_remote_wakeup.)
No, I considered it.
I think we only need protection vs usb_port_{suspend|resume} in three cases
1/ hub_handle_remote_wakeup() where we check if the link/device is suspended
2/ hub_connect_change() where we check for connection
3/ in hub_events() where we check if warm reset is required as
usb_port_resume may also want to perform warm resets
-state and portstatus are intertwined in these checks, but I can see
your point that it really is more about portstatus. Sychronizing
-state comes along for the ride.
I propose we use usb_port pm runtime synchronization to block khubd when
we know the portstatus is not valid (powered off),
Suppose, when you power the port back on, you find that the device is
no longer plugged in. How do you prevent the port from being powered
off again before khubd has a chance to finalize the device? I guess
you would have to do a pm_runtime_get on the port whenever the port's
bit in hub-change_bits gets set.
Simple, take a pm reference in usb_port_runtime_resume and flag the
port as needs child resume. Drop the reference once the child
resume completes. The port is !active until it returns from
usb_port_runtime_resume, and khubd performs a pm barrier
and we need a new
lock to synchronize with usb_device pm operations before checking the
device state.
Forget about the device state. It's a distraction.
Done.
I think we agree that khubd needs to not look at the portstatus while
the port is down. pm runtime synchronization takes care of that and
prevents khubd from starting while the port is inactive. With that in
place we can now make requests in usb_port_runtime_resume() that are
later serviced in khubd when the port is marked active (pm runtime
enforces that -runtime_resume() return 0 before the port is marked
active).
How do you know, when you make the request, that khubd won't start
running while the port is still down? If that happened then khubd
would know to avoid looking at the port, of course, but the request
would be used up and lost.
Per above it requires khubd to barrier pm operations to finish
bringing the port up. It does this while holding a reference to
ensure the port does not go down while checking. I also expect a
reference to be taken when the request is queued and dropped when
khubd services it.
With a lock, this problem doesn't arise.
A lock for this scenario would effectively duplicate dev-power.lock,
or be wider than necessary if I understand where you are considering
holding it.
This mechanism can be used for forcing at least one child
resume for each port resume, and rate-limiting port power requests.
What's the connection with rate-limiting? Under what circumstances
would you need to rate-limit the port power changes?
Only testing scenarios where the port is turning on and off at a
potentially high frequency. Forcing child resume limits that period
to the latency of device recovery.
For the next case, fixing khubd vs device_state changes, we can't use
pm_runtime synchronization because khubd is expected to be able to
resume a suspended usb_device. So we need to prevent khubd from
evaluating the device_state
Re: [PATCH v3 03/10] usb: find internal hub tier mismatch via acpi
On Fri, 17 Jan 2014, Dan Williams wrote: 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. Yes, patch 9 narrowly addresses a problem that needs a wider solution. We want to: 1/ prevent khubd from running while reset is in progress This is the open question mentioned above, if you add in the requirement that we also want to prevent a reset from starting while khubd is running (except when khubd performs the reset itself). 2/ prevent khubd from seeing an intermediate device state during usb_port_resume khubd doesn't pay all that much attention to device states; mostly it is concerned with port statuses. We don't want khubd to see an intermediate port status during usb_port_resume. Basically that means we don't want khubd to run while usb_port_resume is in progress, and we don't want usb_port_resume to start while khubd is running unless khubd performs the resume itself. 3/ prevent khubd from running in the interval between completion of ubs_port_runtime_resume() completing and usb_port_resume() running. Hmmm. I don't remember what usb_port_runtime_resume does about a connected USB-2 child device. It can't simply do a runtime resume of the child. Maybe it should tell khubd to resume the child? Then khubd would never see the intermediate state; the next time it looked at the port, it would issue the runtime resume. (Provided that usb_port_runtime_resume didn't complete after khubd was already running.) All these scenarios to point to problems with -device_state collisions not necessarily the port state. I don't think so, for the reason mentioned above. And it is starting to seem less likely that we can rely on runtime PM synchronization to do everything we want. For example, if something is suspended then there is no way to prevent a runtime resume from occurring at any moment. How about a new lock that synchronizes device state changes with state checks? This replaces patch 9, only compile tested: When considering overall strategies for solving a problem, I find that posting patches is almost never helpful. A patch focuses the mind on low-level coding details, many of which are irrelevant to the problem at hand, preventing you from concentrating on the overall picture -- which is the most important thing at this stage of planning. Pseudo-code can be better in this regard, and I sometimes use it when a degree of precision is necessary in the discussion. However, I don't think we have reached that point yet. Therefore I would greatly prefer if you could describe in prose what you want to do. 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 v3 03/10] usb: find internal hub tier mismatch via acpi
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
Re: [PATCH v3 03/10] usb: find internal hub tier mismatch via acpi
On Fri, 2014-01-17 at 10:53 -0500, Alan Stern wrote:
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
Re: [PATCH v3 03/10] usb: find internal hub tier mismatch via acpi
On Wed, Jan 15, 2014 at 10:39:56AM -0500, Alan Stern wrote: On Tue, 14 Jan 2014, Sarah Sharp wrote: It's always possible for xhci-hcd to prevent the USB-2 root hub from being suspended by calling pm_runtime_get_noresume. The corresponding _put can be done after the USB-3 root hub has been registered. Sure, it's on my todo list to fix that. I just wondered if there were other race conditions present, given that I could find one off the top of my head. 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. Sarah Sharp -- 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 v3 03/10] usb: find internal hub tier mismatch via acpi
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? 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 v3 03/10] usb: find internal hub tier mismatch via acpi
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);
+
Re: [PATCH v3 03/10] usb: find internal hub tier mismatch via acpi
On Tue, 14 Jan 2014, Sarah Sharp wrote: It's always possible for xhci-hcd to prevent the USB-2 root hub from being suspended by calling pm_runtime_get_noresume. The corresponding _put can be done after the USB-3 root hub has been registered. Sure, it's on my todo list to fix that. I just wondered if there were other race conditions present, given that I could find one off the top of my head. 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. 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 v3 03/10] usb: find internal hub tier mismatch via acpi
On Tue, Jan 14, 2014 at 1:47 PM, Dan Williams dan.j.willi...@intel.com wrote: On Tue, Jan 14, 2014 at 1:42 PM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: On Mon, Jan 13, 2014 at 08:57:38PM -0500, Alan Stern wrote: On Mon, 13 Jan 2014, Sarah Sharp wrote: On Mon, Jan 13, 2014 at 02:56:57PM -0800, Sarah Sharp wrote: I haven't looked at this too closely, but what happens if: - the USB 2.0 roothub is registered - userspace immediately sets autosuspend_delay to zero and pm_qos_no_port_poweroff to zero - usb_acpi_find_device changes the connect_type to something other than unknown - before usb_acpi_check_port_peer() is called, the port is suspended Or that call finishes, but no peer port is set yet, because the USB 3.0 roothub isn't registered yet. The USB 2.0 bus can be suspended before the USB 3.0 bus has finished registering, as I've seen from this thread: http://marc.info/?l=linux-usbm=138914518219334w=2 It's always possible for xhci-hcd to prevent the USB-2 root hub from being suspended by calling pm_runtime_get_noresume. The corresponding _put can be done after the USB-3 root hub has been registered. Sure, it's on my todo list to fix that. I just wondered if there were other race conditions present, given that I could find one off the top of my head. At least as far as this patchset is concerned there is no race/requirement to hold off usb-2 root hub power management. The patchset expects usb2 ports to suspend independent of their peer usb3 port state, and forces usb3 ports to always resume before usb2 peers. Let me qualify that, there is no requirement to hold off usb-2 root hub power management *unless* the root hub fails to maintain VBUS like an external hub. The patchset assumes that ACPI port power off needs to be called for both ports in a peer before VBUS goes away. My test platform seems to enforce that, but we may need to quirk platforms that violate this assumption. For that quirk we would need to disable port poweroff until ACPI enumeration completed. -- 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 v3 03/10] usb: find internal hub tier mismatch via acpi
On Mon, Jan 13, 2014 at 08:57:38PM -0500, Alan Stern wrote: On Mon, 13 Jan 2014, Sarah Sharp wrote: On Mon, Jan 13, 2014 at 02:56:57PM -0800, Sarah Sharp wrote: I haven't looked at this too closely, but what happens if: - the USB 2.0 roothub is registered - userspace immediately sets autosuspend_delay to zero and pm_qos_no_port_poweroff to zero - usb_acpi_find_device changes the connect_type to something other than unknown - before usb_acpi_check_port_peer() is called, the port is suspended Or that call finishes, but no peer port is set yet, because the USB 3.0 roothub isn't registered yet. The USB 2.0 bus can be suspended before the USB 3.0 bus has finished registering, as I've seen from this thread: http://marc.info/?l=linux-usbm=138914518219334w=2 It's always possible for xhci-hcd to prevent the USB-2 root hub from being suspended by calling pm_runtime_get_noresume. The corresponding _put can be done after the USB-3 root hub has been registered. Sure, it's on my todo list to fix that. I just wondered if there were other race conditions present, given that I could find one off the top of my head. What do you think about the rest of the patchset? Sarah Sharp -- 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 v3 03/10] usb: find internal hub tier mismatch via acpi
On Tue, Jan 14, 2014 at 1:42 PM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: On Mon, Jan 13, 2014 at 08:57:38PM -0500, Alan Stern wrote: On Mon, 13 Jan 2014, Sarah Sharp wrote: On Mon, Jan 13, 2014 at 02:56:57PM -0800, Sarah Sharp wrote: I haven't looked at this too closely, but what happens if: - the USB 2.0 roothub is registered - userspace immediately sets autosuspend_delay to zero and pm_qos_no_port_poweroff to zero - usb_acpi_find_device changes the connect_type to something other than unknown - before usb_acpi_check_port_peer() is called, the port is suspended Or that call finishes, but no peer port is set yet, because the USB 3.0 roothub isn't registered yet. The USB 2.0 bus can be suspended before the USB 3.0 bus has finished registering, as I've seen from this thread: http://marc.info/?l=linux-usbm=138914518219334w=2 It's always possible for xhci-hcd to prevent the USB-2 root hub from being suspended by calling pm_runtime_get_noresume. The corresponding _put can be done after the USB-3 root hub has been registered. Sure, it's on my todo list to fix that. I just wondered if there were other race conditions present, given that I could find one off the top of my head. At least as far as this patchset is concerned there is no race/requirement to hold off usb-2 root hub power management. The patchset expects usb2 ports to suspend independent of their peer usb3 port state, and forces usb3 ports to always resume before usb2 peers. What do you think about the rest of the patchset? -- 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 v3 03/10] usb: find internal hub tier mismatch via acpi
On Tue, Jan 07, 2014 at 12:29:44PM -0800, Dan Williams wrote:
ACPI identifies peer ports by setting their 'group_token' and 'group_position'
_PLD data to the same value. If a platform has tier mismatch (see Appendix D
figure 131 in the xhci spec), ACPI can override the default peer port
association (for internal hubs).
Location data is cached as an opaque cookie in usb_port_location data.
I haven't looked at this too closely, but what happens if:
- the USB 2.0 roothub is registered
- userspace immediately sets autosuspend_delay to zero and
pm_qos_no_port_poweroff to zero
- usb_acpi_find_device changes the connect_type to something other than
unknown
- before usb_acpi_check_port_peer() is called, the port is suspended
In that case, we'll potentially power off a suspended USB 2.0 port
before we know if it has a USB 3.0 peer port. It seems like you
shouldn't change the connection type until you set the peer port.
Unless you need that connection type for matching the peer port?
Also, why only match on group token and group position? The xHCI spec
in Appendix D says, The _UPC declarations for LS/FS/HS and SS ports
that are paired to form a USB 3.0 compatible connector. A 'pair' is
defined by two ports that declare _PLDs with identical Panel, Vertical
Position, Horizontal Position, Shape, Group Orientation and Group Token
parameter values. (Note, I know nothing about what these fields are
actually filled in with, I'm just quoting the spec here.)
Sarah Sharp
Signed-off-by: Dan Williams dan.j.willi...@intel.com
---
drivers/usb/core/hub.h |6 +++
drivers/usb/core/port.c | 96
+++
drivers/usb/core/usb-acpi.c | 35 +---
drivers/usb/core/usb.h |2 +
4 files changed, 131 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index b5efc713498e..2ba10798c943 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -76,6 +76,10 @@ struct usb_hub {
struct usb_port **ports;
};
+struct usb_port_location {
+ u32 cookie;
+};
+
/**
* struct usb port - kernel's representation of a usb port
* @child: usb device attatched to the port
@@ -83,6 +87,7 @@ struct usb_hub {
* @port_owner: port's owner
* @peer: related usb2 and usb3 ports (share the same connector)
* @connect_type: port's connect type
+ * @location: opaque representation of platform connector location
* @portnum: port index num based one
* @power_is_on: port's power state
* @did_runtime_put: port has done pm_runtime_put().
@@ -93,6 +98,7 @@ struct usb_port {
struct dev_state *port_owner;
struct usb_port *peer;
enum usb_port_connect_type connect_type;
+ struct usb_port_location location;
u8 portnum;
unsigned power_is_on:1;
unsigned did_runtime_put:1;
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 7fd22020d7ee..8fae3cd03305 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -149,11 +149,14 @@ struct device_type usb_port_device_type = {
static struct usb_port *find_peer_port(struct usb_hub *hub, int port1)
{
- struct usb_device *hdev = hub-hdev;
+ struct usb_device *hdev = hub ? hub-hdev : NULL;
struct usb_device *peer_hdev;
struct usb_port *peer = NULL;
struct usb_hub *peer_hub;
+ if (!hub)
+ return NULL;
+
/* set the default peer port for root hubs. Platform firmware
* can later fix this up if tier-mismatch is present. Assumes
* the primary_hcd is usb2.0 and registered first
@@ -193,6 +196,97 @@ static struct usb_port *find_peer_port(struct usb_hub
*hub, int port1)
return peer;
}
+static void reset_peer(struct usb_port *port_dev, struct usb_port *peer)
+{
+ if (!peer)
+ return;
+
+ spin_lock(peer_lock);
+ if (port_dev-peer)
+ put_device(port_dev-peer-dev);
+ if (peer-peer)
+ put_device(peer-peer-dev);
+ port_dev-peer = peer;
+ peer-peer = port_dev;
+ get_device(peer-dev);
+ get_device(port_dev-dev);
+ spin_unlock(peer_lock);
+}
+
+/* assumes that location data is only set for external connectors and that
+ * external hubs never have tier mismatch
+ */
+static int redo_find_peer_port(struct device *dev, void *data)
+{
+ struct usb_port *port_dev = to_usb_port(dev);
+
+ if (is_usb_port(dev)) {
+ struct usb_device *hdev = to_usb_device(dev-parent-parent);
+ struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+ int port1 = port_dev-portnum;
+ struct usb_port *peer;
+
+ peer = find_peer_port(hub, port1);
+ reset_peer(port_dev, peer);
+ }
+
+ return device_for_each_child(dev, NULL, redo_find_peer_port);
+}
+
+static int pair_port(struct device *dev, void *data)
+{
+
Re: [PATCH v3 03/10] usb: find internal hub tier mismatch via acpi
On Mon, Jan 13, 2014 at 02:56:57PM -0800, Sarah Sharp wrote:
On Tue, Jan 07, 2014 at 12:29:44PM -0800, Dan Williams wrote:
ACPI identifies peer ports by setting their 'group_token' and
'group_position'
_PLD data to the same value. If a platform has tier mismatch (see Appendix
D
figure 131 in the xhci spec), ACPI can override the default peer port
association (for internal hubs).
Location data is cached as an opaque cookie in usb_port_location data.
I haven't looked at this too closely, but what happens if:
- the USB 2.0 roothub is registered
- userspace immediately sets autosuspend_delay to zero and
pm_qos_no_port_poweroff to zero
- usb_acpi_find_device changes the connect_type to something other than
unknown
- before usb_acpi_check_port_peer() is called, the port is suspended
Or that call finishes, but no peer port is set yet, because the USB 3.0
roothub isn't registered yet. The USB 2.0 bus can be suspended before
the USB 3.0 bus has finished registering, as I've seen from this thread:
http://marc.info/?l=linux-usbm=138914518219334w=2
In that case, we'll potentially power off a suspended USB 2.0 port
before we know if it has a USB 3.0 peer port. It seems like you
shouldn't change the connection type until you set the peer port.
Unless you need that connection type for matching the peer port?
Also, why only match on group token and group position? The xHCI spec
in Appendix D says, The _UPC declarations for LS/FS/HS and SS ports
that are paired to form a USB 3.0 compatible connector. A 'pair' is
defined by two ports that declare _PLDs with identical Panel, Vertical
Position, Horizontal Position, Shape, Group Orientation and Group Token
parameter values. (Note, I know nothing about what these fields are
actually filled in with, I'm just quoting the spec here.)
Sarah Sharp
Signed-off-by: Dan Williams dan.j.willi...@intel.com
---
drivers/usb/core/hub.h |6 +++
drivers/usb/core/port.c | 96
+++
drivers/usb/core/usb-acpi.c | 35 +---
drivers/usb/core/usb.h |2 +
4 files changed, 131 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index b5efc713498e..2ba10798c943 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -76,6 +76,10 @@ struct usb_hub {
struct usb_port **ports;
};
+struct usb_port_location {
+ u32 cookie;
+};
+
/**
* struct usb port - kernel's representation of a usb port
* @child: usb device attatched to the port
@@ -83,6 +87,7 @@ struct usb_hub {
* @port_owner: port's owner
* @peer: related usb2 and usb3 ports (share the same connector)
* @connect_type: port's connect type
+ * @location: opaque representation of platform connector location
* @portnum: port index num based one
* @power_is_on: port's power state
* @did_runtime_put: port has done pm_runtime_put().
@@ -93,6 +98,7 @@ struct usb_port {
struct dev_state *port_owner;
struct usb_port *peer;
enum usb_port_connect_type connect_type;
+ struct usb_port_location location;
u8 portnum;
unsigned power_is_on:1;
unsigned did_runtime_put:1;
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 7fd22020d7ee..8fae3cd03305 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -149,11 +149,14 @@ struct device_type usb_port_device_type = {
static struct usb_port *find_peer_port(struct usb_hub *hub, int port1)
{
- struct usb_device *hdev = hub-hdev;
+ struct usb_device *hdev = hub ? hub-hdev : NULL;
struct usb_device *peer_hdev;
struct usb_port *peer = NULL;
struct usb_hub *peer_hub;
+ if (!hub)
+ return NULL;
+
/* set the default peer port for root hubs. Platform firmware
* can later fix this up if tier-mismatch is present. Assumes
* the primary_hcd is usb2.0 and registered first
@@ -193,6 +196,97 @@ static struct usb_port *find_peer_port(struct usb_hub
*hub, int port1)
return peer;
}
+static void reset_peer(struct usb_port *port_dev, struct usb_port *peer)
+{
+ if (!peer)
+ return;
+
+ spin_lock(peer_lock);
+ if (port_dev-peer)
+ put_device(port_dev-peer-dev);
+ if (peer-peer)
+ put_device(peer-peer-dev);
+ port_dev-peer = peer;
+ peer-peer = port_dev;
+ get_device(peer-dev);
+ get_device(port_dev-dev);
+ spin_unlock(peer_lock);
+}
+
+/* assumes that location data is only set for external connectors and that
+ * external hubs never have tier mismatch
+ */
+static int redo_find_peer_port(struct device *dev, void *data)
+{
+ struct usb_port *port_dev = to_usb_port(dev);
+
+ if (is_usb_port(dev)) {
+ struct usb_device *hdev = to_usb_device(dev-parent-parent);
+
Re: [PATCH v3 03/10] usb: find internal hub tier mismatch via acpi
On Mon, Jan 13, 2014 at 2:56 PM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: On Tue, Jan 07, 2014 at 12:29:44PM -0800, Dan Williams wrote: ACPI identifies peer ports by setting their 'group_token' and 'group_position' _PLD data to the same value. If a platform has tier mismatch (see Appendix D figure 131 in the xhci spec), ACPI can override the default peer port association (for internal hubs). Location data is cached as an opaque cookie in usb_port_location data. I haven't looked at this too closely, but what happens if: - the USB 2.0 roothub is registered - userspace immediately sets autosuspend_delay to zero and pm_qos_no_port_poweroff to zero - usb_acpi_find_device changes the connect_type to something other than unknown Note, in this revision I've kept the established policy of not checking connect_type. We'll immediately suspend on setting pm_qos_no_poweroff to zero and device going idle. - before usb_acpi_check_port_peer() is called, the port is suspended In that case, we'll potentially power off a suspended USB 2.0 port before we know if it has a USB 3.0 peer port. It seems like you shouldn't change the connection type until you set the peer port. Unless you need that connection type for matching the peer port? The sequencing enforced by usb_port_runtime_suspend() is directional in that a usb2 port powering off before usb3 is always permitted (otherwise we run into cyclical dependencies). The implementation expects that the device will enforce the preference to connect on its usb3 interface, and it should as long as VBUS and the RX terminations are maintained, which they would be in this scenario. In the reverse scenario (detecting usb3 before usb2 peer) we have a check for !peer to block suspending the usb3 port prematurely. However, I wonder if it would be advisable to detect the case where a usb3 device has downgraded to it's usb2 connection. In that case I think we want to disable port power off because the assumption of always enforce usb3 powered down last has been invalidated. I can look into an incremental patch to add that safeguard for flaky devices, but it has not been a problem in testing. Also, why only match on group token and group position? The xHCI spec in Appendix D says, The _UPC declarations for LS/FS/HS and SS ports that are paired to form a USB 3.0 compatible connector. A 'pair' is defined by two ports that declare _PLDs with identical Panel, Vertical Position, Horizontal Position, Shape, Group Orientation and Group Token parameter values. (Note, I know nothing about what these fields are actually filled in with, I'm just quoting the spec here.) True, I was going off the ACPI spec (6.1.8) that says that Group Token is Unique numerical value identifying a group. So my understanding is that the other fields are just positional, but group token and group position are sufficient for uniquely identifying connectors. -- 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 v3 03/10] usb: find internal hub tier mismatch via acpi
On Mon, 13 Jan 2014, Sarah Sharp wrote: On Mon, Jan 13, 2014 at 02:56:57PM -0800, Sarah Sharp wrote: I haven't looked at this too closely, but what happens if: - the USB 2.0 roothub is registered - userspace immediately sets autosuspend_delay to zero and pm_qos_no_port_poweroff to zero - usb_acpi_find_device changes the connect_type to something other than unknown - before usb_acpi_check_port_peer() is called, the port is suspended Or that call finishes, but no peer port is set yet, because the USB 3.0 roothub isn't registered yet. The USB 2.0 bus can be suspended before the USB 3.0 bus has finished registering, as I've seen from this thread: http://marc.info/?l=linux-usbm=138914518219334w=2 It's always possible for xhci-hcd to prevent the USB-2 root hub from being suspended by calling pm_runtime_get_noresume. The corresponding _put can be done after the USB-3 root hub has been registered. 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 v3 03/10] usb: find internal hub tier mismatch via acpi
ACPI identifies peer ports by setting their 'group_token' and 'group_position'
_PLD data to the same value. If a platform has tier mismatch (see Appendix D
figure 131 in the xhci spec), ACPI can override the default peer port
association (for internal hubs).
Location data is cached as an opaque cookie in usb_port_location data.
Signed-off-by: Dan Williams dan.j.willi...@intel.com
---
drivers/usb/core/hub.h |6 +++
drivers/usb/core/port.c | 96 +++
drivers/usb/core/usb-acpi.c | 35 +---
drivers/usb/core/usb.h |2 +
4 files changed, 131 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index b5efc713498e..2ba10798c943 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -76,6 +76,10 @@ struct usb_hub {
struct usb_port **ports;
};
+struct usb_port_location {
+ u32 cookie;
+};
+
/**
* struct usb port - kernel's representation of a usb port
* @child: usb device attatched to the port
@@ -83,6 +87,7 @@ struct usb_hub {
* @port_owner: port's owner
* @peer: related usb2 and usb3 ports (share the same connector)
* @connect_type: port's connect type
+ * @location: opaque representation of platform connector location
* @portnum: port index num based one
* @power_is_on: port's power state
* @did_runtime_put: port has done pm_runtime_put().
@@ -93,6 +98,7 @@ struct usb_port {
struct dev_state *port_owner;
struct usb_port *peer;
enum usb_port_connect_type connect_type;
+ struct usb_port_location location;
u8 portnum;
unsigned power_is_on:1;
unsigned did_runtime_put:1;
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 7fd22020d7ee..8fae3cd03305 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -149,11 +149,14 @@ struct device_type usb_port_device_type = {
static struct usb_port *find_peer_port(struct usb_hub *hub, int port1)
{
- struct usb_device *hdev = hub-hdev;
+ struct usb_device *hdev = hub ? hub-hdev : NULL;
struct usb_device *peer_hdev;
struct usb_port *peer = NULL;
struct usb_hub *peer_hub;
+ if (!hub)
+ return NULL;
+
/* set the default peer port for root hubs. Platform firmware
* can later fix this up if tier-mismatch is present. Assumes
* the primary_hcd is usb2.0 and registered first
@@ -193,6 +196,97 @@ static struct usb_port *find_peer_port(struct usb_hub
*hub, int port1)
return peer;
}
+static void reset_peer(struct usb_port *port_dev, struct usb_port *peer)
+{
+ if (!peer)
+ return;
+
+ spin_lock(peer_lock);
+ if (port_dev-peer)
+ put_device(port_dev-peer-dev);
+ if (peer-peer)
+ put_device(peer-peer-dev);
+ port_dev-peer = peer;
+ peer-peer = port_dev;
+ get_device(peer-dev);
+ get_device(port_dev-dev);
+ spin_unlock(peer_lock);
+}
+
+/* assumes that location data is only set for external connectors and that
+ * external hubs never have tier mismatch
+ */
+static int redo_find_peer_port(struct device *dev, void *data)
+{
+ struct usb_port *port_dev = to_usb_port(dev);
+
+ if (is_usb_port(dev)) {
+ struct usb_device *hdev = to_usb_device(dev-parent-parent);
+ struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+ int port1 = port_dev-portnum;
+ struct usb_port *peer;
+
+ peer = find_peer_port(hub, port1);
+ reset_peer(port_dev, peer);
+ }
+
+ return device_for_each_child(dev, NULL, redo_find_peer_port);
+}
+
+static int pair_port(struct device *dev, void *data)
+{
+ struct usb_port *peer = data;
+ struct usb_port *port_dev = to_usb_port(dev);
+
+ if (!is_usb_port(dev)
+ || port_dev-location.cookie != peer-location.cookie)
+ return device_for_each_child(dev, peer, pair_port);
+
+ dev_dbg(dev-parent-parent, port%d peer = %s port%d (by location)\n,
+ port_dev-portnum, dev_name(peer-dev.parent-parent),
+ peer-portnum);
+ if (port_dev-peer != peer) {
+ /* Sigh, tier mismatch has invalidated our ancestry.
+* This should not be too onerous even in deep hub
+* topologies as we will discover tier mismatch early
+* (after platform internal hubs have been enumerated),
+* before external hubs are probed.
+*/
+ reset_peer(port_dev, peer);
+ device_for_each_child(dev, NULL, redo_find_peer_port);
+ }
+
+ return true;
+}
+
+void usb_set_hub_port_location(struct usb_device *hdev, int port1,
+ u32 cookie)
+{
+ struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+ struct usb_hcd *hcd =
