Re: [PATCH] xhci: Fix the lack of support for the Handle Port Configure Error

[Mathias Nyman <mathias.nyman@intel.com>]

On 06.02.2015 17:20, Gregory CLEMENT wrote:
> From: Shimmer Huang <shimmerh@marvell.com>
> 
> Linux xHCI driver does not check the CEC bit in register PORTSC when
> handling port status events. If Port Configure Error for root hub port
> occurs, CEC bit in PORTSC would be set by xHC and remains 1. This
> happends when the root port fails to configure its link partner,
> e.g. the port fails to exchange port capabilities information using
> Port Capability LMPs.
> 
> Then the Port Status Change Events will be blocked until all status
> change bits(CEC is one of the change bits) are cleared('0') (refer to
> xHCI spec 4.19.2). Otherwise, the port status change event for this
> root port will not be generated anymore, then root port would look
> like “dead” for user and can’t be recovered until a Host Controller
> Reset(HCRST)
> 
> This patch is to check CEC bit and clear the CEC bit if it's set to 1
> in function handle_port_status().
> 
> [gregory.clement@free-electrons.com: ported from 3.10 and added more
> explanations(from Shimmer) in the commit log]
> 
> Signed-off-by: Guang Shen <gshen@marvell.com>
> Signed-off-by: Shimmer Huang <shimmerh@marvell.com>
> Signed-off-by: Nadav Haklai <nadavh@marvell.com>
> Reviewed-by: Yehuda Yitschak <yehuday@marvell.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Cc: <stable@vger.kernel.org>
> ---
> Hi,
> 
> usually I tried to add a kernel version for the stable team, but for
> this patch I don't know since when it makes sens to apply it.
> 
> Gregory
> 
> 
>  drivers/usb/host/xhci-ring.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index e692e769c50c..45d8dd7e07f3 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1541,6 +1541,13 @@ static void handle_port_status(struct xhci_hcd *xhci,
>  			port_id);
>  
>  	temp = readl(port_array[faked_port_index]);
> +
> +	if (temp & PORT_CEC) {
> +		xhci_dbg(xhci, "port failed to configure its link partner.\n");
> +		xhci_test_and_clear_bit(xhci, port_array,
> +					faked_port_index, PORT_CEC);
> +	}
> +
>  	if (hcd->state == HC_STATE_SUSPENDED) {
>  		xhci_dbg(xhci, "resume root hub\n");
>  		usb_hcd_resume_root_hub(hcd);
> 

Nice catch,

Looks like we're not handling the Config Error Change (CEC) at all in xhci.

I think we instead need to set a Config Error in the return status of the GetPortStatus
requests by setting USB_PORT_FEAT_C_PORT_CONFIG_ERROR in xhci_get_port_status().

This should cause a ClearPortFeature request, where we then can clear the bit in
xhci_clear_port_change_bit()

So going the long way round informing the usb core about the error instead of
immediately clearing the bit.

-Mathias