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

[Dan Williams]

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: 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100938
> Reported-and-tested-by: Christian Ehrig  turn-bt.com>
> Reported-and-tested-by: Patrick Chilton 
> Reported-and-tested-by: Andreas Böhler 
> Signed-off-by: Bjørn Mork 
> ---
> 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) {
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

Am Freitag, den 22.09.2017, 22:18 +0200 schrieb Bjørn Mork:
> 
> 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: 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100938
> Reported-and-tested-by: Christian Ehrig 
> 
> Reported-and-tested-by: Patrick Chilton 
> Reported-and-tested-by: Andreas Böhler 
> Signed-off-by: Bjørn Mork 
Acked-by: Oliver Neukum 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bjørn Mork]

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: 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100938
Reported-and-tested-by: Christian Ehrig 

Reported-and-tested-by: Patrick Chilton 
Reported-and-tested-by: Andreas Böhler 
Signed-off-by: Bjørn Mork 
---
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...


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) {
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html