Re: [RFC] usb: Add support for root hub port status CAS

[Andiry Xu <andiry.xu@xxxxxxx>]

On 06/04/2012 04:46 PM, Stanislaw Ledwon wrote:
> The host controller port status register supports CAS (Could Attach
> Status) bit. This bit could be set when USB3.0 device is connectd
> when system is in Sx state. When the system wakes to S0 this port
> status with CAS bit is reported and this port can't be used by any
> device.
> 
> When CAS bit is set the port should be reset by warm reset. This
> was not supported by xhci driver.
> 
> The issue was found when pendrive was connected to suspended
> platfrom. The link state of "Compliance Mode" was reported togather
> with CAS bit. This link state was also not supported by xhci and
> core/hub.c.
> 
> The CAS bit is defined only for xhci root hub port and it is
> not supported on regular hubs. The link status is used to force
> warm reset on port. Make the USB core issue a warm reset when port
> is in ether the 'inactive' or 'compliance mode'. Change the xHCI driver
> to report 'compliance mode' when the CAS is set. This force warm reset
> on the root hub port.
> 

Why fake the port status to compliance mode? All you want to do is issue
a warm reset when finding a CAS bit set. Fake a bogus USB3 wPortStatus
bit is simpler and clearer.

It's like this:

1. define a bogus bit in ch11.h. You can use bit13 since it's reserved.

#define USB_SS_PORT_STAT_CAS_BOGUS 0x2000 /* bit 13 */

2. In GetPortStatus in xhci_hub_control, report the CAS.

	If (temp & PORT_CAS)
		status |= USB_SS_PORT_STAT_CAS_BOGUS;

3. in hub.c, issue a warm reset if CAS is found.

+static bool hub_port_warm_reset_required(struct usb_hub *hub, u16
portstatus)
 {
 	return hub_is_superspeed(hub->hdev) &&
-		(portstatus & USB_PORT_STAT_LINK_STATE) ==
-		USB_SS_PORT_LS_SS_INACTIVE;
+		(((portstatus & USB_PORT_STAT_LINK_STATE) ==
+		  USB_SS_PORT_LS_SS_INACTIVE) ||
+		 (portstatus & USB_PORT_STAT_CAS_BOGUS));
 }

> Signed-off-by: Stanislaw Ledwon <staszek.ledwon@xxxxxxxxxxxxxxx>
> ---
>  drivers/usb/core/hub.c      |   18 +++++++++-------
>  drivers/usb/host/xhci-hub.c |   44 +++++++++++++++++++++++++++++++++++++-----
>  drivers/usb/host/xhci.h     |    6 ++++-
>  3 files changed, 53 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index ec6c97d..572769b 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2102,12 +2102,16 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
>  static int hub_port_reset(struct usb_hub *hub, int port1,
>  			struct usb_device *udev, unsigned int delay, bool warm);
>  
> -/* Is a USB 3.0 port in the Inactive state? */
> -static bool hub_port_inactive(struct usb_hub *hub, u16 portstatus)
> +/* Is a USB 3.0 port in the Inactive or Complinance Mode state?
> + * Port worm reset is required to recover
> + */
> +static bool hub_port_warm_reset_required(struct usb_hub *hub, u16 portstatus)
>  {
>  	return hub_is_superspeed(hub->hdev) &&
> -		(portstatus & USB_PORT_STAT_LINK_STATE) ==
> -		USB_SS_PORT_LS_SS_INACTIVE;
> +		(((portstatus & USB_PORT_STAT_LINK_STATE) ==
> +		  USB_SS_PORT_LS_SS_INACTIVE) ||
> +		 ((portstatus & USB_PORT_STAT_LINK_STATE) ==
> +		  USB_SS_PORT_LS_COMP_MOD)) ;
>  }
>  
>  static int hub_port_wait_reset(struct usb_hub *hub, int port1,
> @@ -2143,7 +2147,7 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
>  			 *
>  			 * See https://bugzilla.kernel.org/show_bug.cgi?id=41752
>  			 */
> -			if (hub_port_inactive(hub, portstatus)) {
> +			if (hub_port_warm_reset_required(hub, portstatus)) {
>  				int ret;
>  
>  				if ((portchange & USB_PORT_STAT_C_CONNECTION))
> @@ -3753,9 +3757,7 @@ static void hub_events(void)
>  			/* Warm reset a USB3 protocol port if it's in
>  			 * SS.Inactive state.
>  			 */
> -			if (hub_is_superspeed(hub->hdev) &&
> -				(portstatus & USB_PORT_STAT_LINK_STATE)
> -					== USB_SS_PORT_LS_SS_INACTIVE) {
> +			if (hub_port_warm_reset_required(hub, portstatus)) {
>  				dev_dbg(hub_dev, "warm reset port %d\n", i);
>  				hub_port_reset(hub, i, NULL,
>  						HUB_BH_RESET_TIME, true);
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 673ad12..58e1e73 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -462,6 +462,42 @@ void xhci_test_and_clear_bit(struct xhci_hcd *xhci, __le32 __iomem **port_array,
>  	}
>  }
>  
> +/* Updates Link Status for super Speed port */
> +void xhci_hub_set_link_status(u32 *status, u32 status_reg)

The function name is ambiguous. Its purpose is to report a bogus port
status, not "set_link_status". There is another function named
xhci_set_link_state() in the same file, which does complete different
things.

Also, this should be a static function.

> +{
> +	u32 pls = status_reg & PORT_PLS_MASK;
> +
> +	/* resume state is a xHCI internal state.
> +	 * Do not report it to usb core.
> +	 */
> +	if (pls == XDEV_RESUME)
> +		return;
> +
> +	/* When the CAS bit is set then warm reset
> +	 * should be performed on port
> +	 */
> +	if (status_reg & PORT_CAS) {
> +		/* The CAS bit can be set while the port is
> +		 * in any link state.
> +		 * Only roothubs have CAS bit, so we
> +		 * pretend to be in compliance mode
> +		 * unless we're already in compliance
> +		 * or the inactive state.
> +		 */
> +		if (pls != USB_SS_PORT_LS_COMP_MOD &&
> +		    pls != USB_SS_PORT_LS_SS_INACTIVE) {
> +			pls = USB_SS_PORT_LS_COMP_MOD;
> +		}
> +		/* Return also connection bit -
> +		 * hub state machine resets port
> +		 * when this bit is set.
> +		 */
> +		pls |= USB_PORT_STAT_CONNECTION;

Why do you need this setting? Does PORTSC return a PORT_CONNECT when CAS
is set?

Thanks,
Andiry

> +	}
> +	/* update status field */
> +	*status |= pls;
> +}
> +
>  int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  		u16 wIndex, char *buf, u16 wLength)
>  {
> @@ -604,13 +640,9 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  			else
>  				status |= USB_PORT_STAT_POWER;
>  		}
> -		/* Port Link State */
> +		/* Update Port Link State for super speed ports*/
>  		if (hcd->speed == HCD_USB3) {
> -			/* resume state is a xHCI internal state.
> -			 * Do not report it to usb core.
> -			 */
> -			if ((temp & PORT_PLS_MASK) != XDEV_RESUME)
> -				status |= (temp & PORT_PLS_MASK);
> +			xhci_hub_set_link_status(&status, temp);
>  		}
>  		if (bus_state->port_c_suspend & (1 << wIndex))
>  			status |= 1 << USB_PORT_FEAT_C_SUSPEND;
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 3d69c4b..7e8280a 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -341,7 +341,11 @@ struct xhci_op_regs {
>  #define PORT_PLC	(1 << 22)
>  /* port configure error change - port failed to configure its link partner */
>  #define PORT_CEC	(1 << 23)
> -/* bit 24 reserved */
> +/* Cold Attach Status - xHC can set this bit to report device attached during
> + * Sx state. Warm port reset should be perfomed to clear this bit and move port
> + * to connected state.
> + */
> +#define PORT_CAS	(1 << 24)
>  /* wake on connect (enable) */
>  #define PORT_WKCONN_E	(1 << 25)
>  /* wake on disconnect (enable) */


--
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