Re: [PATCH] USB: cdc-wdm: ignore -EPIPE from GetEncapsulatedResponse

[Dan Williams <dcbw@redhat.com>]

On Fri, 2017-09-22 at 22:18 +0200, Bjørn Mork wrote:
> The driver will forward errors to userspace after turning most of
> them
> into -EIO. But all status codes are not equal. The -EPIPE (stall) in
> particular can be seen more as a result of normal USB signaling than
> an actual error. The state is automatically cleared by the USB core
> without intervention from either driver or userspace.
> 
> And most devices and firmwares will never trigger a stall as a result
> of GetEncapsulatedResponse. This is in fact a requirement for CDC WDM
> devices. Quoting from section 7.1 of the CDC WMC spec revision 1.1:
> 
>   The function shall not return STALL in response to
>   GetEncapsulatedResponse.
> 
> But this driver is also handling GetEncapsulatedResponse on behalf of
> the qmi_wwan and cdc_mbim drivers. Unfortunately the relevant specs
> are not as clear wrt stall. So some QMI and MBIM devices *will*
> occasionally stall, causing the GetEncapsulatedResponse to return an
> -EPIPE status. Translating this into -EIO for userspace has proven to
> be harmful. Treating it as an empty read is safer, making the driver
> behave as if the device was conforming to the CDC WDM spec.
> 
> There have been numerous reports of issues related to -EPIPE errors
> from some newer CDC MBIM devices in particular, like for example the
> Fibocom L831-EAU.  Testing on this device has shown that the issues
> go away if we simply ignore the -EPIPE status.  Similar handling of
> -EPIPE is already known from e.g. usb_get_string()
> 
> The -EPIPE log message is still kept to let us track devices with
> this
> unexpected behaviour, hoping that it attracts attention from firmware
> developers.
> 
> Cc: <stable@vger.kernel.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100938
> Reported-and-tested-by: Christian Ehrig <christian.ehrig@mediamarktsa
> turn-bt.com>
> Reported-and-tested-by: Patrick Chilton <chpatrick@gmail.com>
> Reported-and-tested-by: Andreas Böhler <news@aboehler.at>
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
> The fallout of this problem has been reported by a number of people
> for a long time.  Many more than those being listed above. Apologies
> to all who didn't get the appropriate credit.  Sorry, I am lousy with
> paper work and lost track of you all.
> 
> Hoping the fix will make up for it...

FWIW, tested on the oldest MBIM devices I have (Ericsson F5321 and
Huawei EM820W for while with light traffic, browsing and git pull/push.
  Neither of these devices exhibit the original problem of course. 
Might find time to test on the newer ones this week or next, but it
seems OK.

Dan

> 
> Bjørn
> 
>  drivers/usb/class/cdc-wdm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-
> wdm.c
> index 0b845e550fbd..9f001659807a 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -194,8 +194,10 @@ static void wdm_in_callback(struct urb *urb)
>  	/*
>  	 * only set a new error if there is no previous error.
>  	 * Errors are only cleared during read/open
> +	 * Avoid propagating -EPIPE (stall) to userspace since it is
> +	 * better handled as an empty read
>  	 */
> -	if (desc->rerr  == 0)
> +	if (desc->rerr == 0 && status != -EPIPE)
>  		desc->rerr = status;
>  
>  	if (length + desc->length > desc->wMaxCommand) {