From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:55134 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751874AbdJBQ1q (ORCPT ); Mon, 2 Oct 2017 12:27:46 -0400 Message-ID: <1506961662.9526.1.camel@redhat.com> Subject: Re: [PATCH] USB: cdc-wdm: ignore -EPIPE from GetEncapsulatedResponse From: Dan Williams To: =?ISO-8859-1?Q?Bj=F8rn?= Mork , linux-usb@vger.kernel.org Cc: Oliver Neukum , stable@vger.kernel.org Date: Mon, 02 Oct 2017 11:27:42 -0500 In-Reply-To: <20170922201818.30544-1-bjorn@mork.no> References: <20170922201818.30544-1-bjorn@mork.no> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: 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) {