Hi Mathias,
On 9/13/2023 7:21 AM, Mathias Nyman wrote:
Hi
On 13.9.2023 0.51, Wesley Cheng wrote:
Hi Mathias,
This is one way, but we can probably avoid re-reading all the usb3
portsc registers
by checking if any bit is set in either:
// bitfield, set if xhci usb3 port neatly set to U3 with a hub
request
xhci->usb3_rhub.bus_state.suspended_ports
// bitfield, set if xhci usb3 port is forced to U3 during xhci suspend.
xhci->usb3_rhub.bus_state.bus_suspended
But haven't checked this works in all corner cases.
Thanks for the suggestion. I think I also looked at seeing if we
could use the suspended_ports param, and it was missing one of the
use cases we had. I haven't thought on combining it with the
bus_suspend param also to see if it could work. Let me give it a
try, and I'll get back to you.
So in one of our normal use cases, which is to use an USB OTG adapter
with our devices, we can have this connected with no device. In this
situation, the XHCI HCD and root hub are enumerated, and is in a state
where nothing is connected to the port. I added a print to the
xhci_resume() path to check the status of "suspended_ports" and
"bus_suspended" and they seem to reflect the same status as when there
is something connected (to a device that supports autosuspend).
Here's some pointers I've found on why these parameters may not work:
1. bus_suspended is only set (for the bus) if we reach the
bus_suspend() callback from USB HCD if the link is still in U0. If
USB autosuspend is enabled, then the suspending of the root hub udev,
would have caused a call to suspend the port (usb_port_suspend()), and
that would set "suspended_ports" and placed the link in U3 already.
2. "suspended_ports" can't differentiate if a device is connected or
not after plugging in a USB3 device that has autosuspend enabled. It
looks like on device disconnection, the suspended_ports isn't cleared
for that port number. It is only cleared during the resume path where
a get port status is queried:
static void xhci_get_usb3_port_status(struct xhci_port *port, u32
*status,
u32 portsc)
{
...
/* USB3 specific wPortStatus bits */
if (portsc & PORT_POWER) {
*status |= USB_SS_PORT_STAT_POWER;
/* link state handling */
if (link_state == XDEV_U0)
bus_state->suspended_ports &= ~(1 << portnum);
}
IMO, this seems kind of weird, because the PLS shows that the port is
in the RxDetect state, so it technically isn't suspended. If you
think we should clear suspended_ports on disconnect, then I think we
can also change the logic to rely on it for avoiding the unnecessary
delay in xhci_resume().
I think you found a bug.
We should clear suspended_ports bit if link state in portsc is anything
other than U3, Resume or Recovery.
Not doing so might cause USB_PORT_STAT_C_SUSPEND bit to be set
incorrectly in a USB2 get port status request.
So we want something like:
if (suspended_ports bit is set) {
if (U3 || Resume || Recovery) {
don't touch anything
} else {
clear suspended_port bit
if ((U2 || U0) && USB2)
set bus_state->port_c_suspend bit
}
I'll look into it
Thanks, Mathias. Will take some time to take a look as well since I
have a reliable set up that observes this issue. If you have any test
code you might want to try, let me know!
Thanks
Wesley Cheng
