Re: [PATCH] usb: core: Fix side effect of clear port feature in hub port reset

[Alan Stern]

On Tue, 1 Sep 2015, hyunho747.kim wrote:

> After successful port reset by set_port_feature, some devices show
> immediate link connection which generates port connect change interrupt.
> But, the next step is an unconditional usb_clear_port_feature
> and this flow always clears USB_PORT_FEAT_C_CONNECTION bit in port status
> though next kick_khubd is reserved by link connection interrupt.
> This flow can make an ambiguous state in the next port_evnet operation,
> port_status is connected state such as 0x203 but
> port_change is zero value caused by the previous clear port feature.
> So, port_event can't call hub_port_connect_change and
> there is no other way to peform connect procedure.

It sounds like you have not read this comment in hub_port_wait_reset():

	/* bomb out completely if the connection bounced.  A USB 3.0
	 * connection may bounce if multiple warm resets were issued,
	 * but the device may have successfully re-connected. Ignore it.
	 */

I agree that clearing the USB_PORT_FEAT_C_CONNECTION bit after reading
the port status can cause a race.  However I don't think your solution
is the correct one.

> Signed-off-by: hyunho747.kim <hyunho747.kim@xxxxxxx>

You need to put a real name here, not an email address.  I suspect your 
real name isn't "hyunho747.kim".

> ---
>  drivers/usb/core/hub.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 73dfa19..4508aff 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2761,18 +2761,21 @@ static int hub_port_reset(struct usb_hub *hub, int port1,
>  
>  		/* Check for disconnect or reset */
>  		if (status == 0 || status == -ENOTCONN || status == -ENODEV) {
> -			usb_clear_port_feature(hub->hdev, port1,
> +			if (status)
> +				usb_clear_port_feature(hub->hdev, port1,
>  					USB_PORT_FEAT_C_RESET);

What is the reason for this change?  Clearing the USB_PORT_FEAT_C_RESET 
bit won't cause any problems.

>  			if (!hub_is_superspeed(hub->hdev))
>  				goto done;
>  
> -			usb_clear_port_feature(hub->hdev, port1,
> +			if (status) {
> +				usb_clear_port_feature(hub->hdev, port1,
>  					USB_PORT_FEAT_C_BH_PORT_RESET);

And what is the reason for this change?

> -			usb_clear_port_feature(hub->hdev, port1,
> +				usb_clear_port_feature(hub->hdev, port1,
>  					USB_PORT_FEAT_C_PORT_LINK_STATE);
> -			usb_clear_port_feature(hub->hdev, port1,
> +				usb_clear_port_feature(hub->hdev, port1,
>  					USB_PORT_FEAT_C_CONNECTION);
> +			}

I'm not so sure about these changes.  The bits definitely do need to be 
cleared at some point, but this may not be the right place to clear 
them.  Or maybe we need to check that the port is still enabled after 
the bits have been cleared.

You need to think more carefully about this patch.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/