[PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications

[PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications

[Bjørn Mork]

The driver enforces a strict one-to-one relationship between the
received RESPONSE_AVAILABLE notifications and messages read from
the device. At the same time, it will cancel the interrupt URB
when there is no client holding the character device open.

Many devices do not cope well with this behaviour.  They maintain
a FIFO queue of messages, and send notifications on a best effort
basis.  Messages are queued regardless of whether the notification
is successful or not. So if the driver loses a single notification,
which can easily happen when the interrupt URB is cancelled, then
the device and driver becomes out-of-sync. New messages end up
at the end of the queue, while the associated notification makes
the driver read only the first message from the queue.

This state is permanent from a user point of view. There is no
no way to flush the device queue without resetting the device or
using another driver.

The problem is easy to hit with current QMI and MBIM command line
tools, which typically close the character device after seeing
the reply they expect. Any pending unsolicited messages from the
device will then trigger the driver bug.

Fix by always reading all queued messages from the device when
the notification URB is first submitted.  This is expected to
end with an -EPIPE status when there are no more pending
messages, so demote the printk associated with -EPIPE to debug
level.

The workaround has been tested on a large number of different MBIM
and QMI devices, as well as the Ericsson F5521gw and H5321gw modems
with real Device Management functions.

Cc: <stable@vger.kernel.org>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
Uhm, well, time flies when you are having fun :)

For those who cannot remember more than a month worth of list contents,
and therefore cannot understand whyt this is "v2", please look at the
thread here: http://www.spinics.net/lists/linux-usb/msg140856.html

FWIW, I have been using this patch actively myself for all that time,
mostly with a QMI modem.  But also occasionally with the same modem in
MBIM mode and with assorted other modems. I have also verified that
this has no effect on any of the "proper" CDC WDM devices I have.

v2 changes are:
 - avoid propagating any EPIPE caused by polling for data without
   notification. This is not a problem for standards complying
   CDC WDM devices, as they are required to support such polling,
   but proved to cause problems for some QMI devices.

I hope this is acceptable for stable too, as it fixes a long standing
usability problems with many MBIM devices.


 drivers/usb/class/cdc-wdm.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 61ea87917433..43e1cfb361f3 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -178,7 +178,7 @@ static void wdm_in_callback(struct urb *urb)
 				"nonzero urb status received: -ESHUTDOWN");
 			goto skip_error;
 		case -EPIPE:
-			dev_err(&desc->intf->dev,
+			dev_dbg(&desc->intf->dev,
 				"nonzero urb status received: -EPIPE\n");
 			break;
 		default:
@@ -200,10 +200,29 @@ static void wdm_in_callback(struct urb *urb)
 			desc->reslength = length;
 		}
 	}
+
+	/*
+	 * If desc->resp_count is unset, then the urb was submitted
+	 * without a prior notification.  If the device returned any
+	 * data, then this implies that it had messages queued without
+	 * notifying us.  Continue reading until that queue is flushed.
+	 */
+	if (!desc->resp_count) {
+		if (!length) {
+			/* do not propagate the expected -EPIPE */
+			desc->rerr = 0;
+			goto unlock;
+		}
+		dev_dbg(&desc->intf->dev, "got %d bytes without notification\n", length);
+		set_bit(WDM_RESPONDING, &desc->flags);
+		usb_submit_urb(desc->response, GFP_ATOMIC);
+	}
+
 skip_error:
 	wake_up(&desc->wait);
 
 	set_bit(WDM_READ, &desc->flags);
+unlock:
 	spin_unlock(&desc->iuspin);
 }
 
@@ -647,6 +666,16 @@ static int wdm_open(struct inode *inode, struct file *file)
 			dev_err(&desc->intf->dev,
 				"Error submitting int urb - %d\n", rv);
 			rv = usb_translate_errors(rv);
+		} else {
+			/*
+			 * Some devices keep pending messages queued
+			 * without resending notifications.  We must
+			 * flush the message queue before we can
+			 * assume a one-to-one relationship between
+			 * notifications and messages in the queue
+			 */
+			set_bit(WDM_RESPONDING, &desc->flags);
+			rv = usb_submit_urb(desc->response, GFP_KERNEL);
 		}
 	} else {
 		rv = 0;
-- 
2.1.4

Re: [PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications

[Oliver Neukum]

On Sun, 2016-07-03 at 21:59 +0200, Bjørn Mork wrote:
> 		default:
> @@ -200,10 +200,29 @@ static void wdm_in_callback(struct urb *urb)
>  			desc->reslength = length;
>  		}
>  	}
> +
> +	/*
> +	 * If desc->resp_count is unset, then the urb was submitted
> +	 * without a prior notification.  If the device returned any
> +	 * data, then this implies that it had messages queued without
> +	 * notifying us.  Continue reading until that queue is flushed.
> +	 */
> +	if (!desc->resp_count) {
> +		if (!length) {
> +			/* do not propagate the expected -EPIPE */
> +			desc->rerr = 0;
> +			goto unlock;
> +		}
> +		dev_dbg(&desc->intf->dev, "got %d bytes without notification\n", length);
> +		set_bit(WDM_RESPONDING, &desc->flags);
> +		usb_submit_urb(desc->response, GFP_ATOMIC);

You must check for being in an overflow condition.

> +	}
> +
>  skip_error:
>  	wake_up(&desc->wait);
>  
>  	set_bit(WDM_READ, &desc->flags);
> +unlock:
>  	spin_unlock(&desc->iuspin);
>  }
>  
> @@ -647,6 +666,16 @@ static int wdm_open(struct inode *inode, struct file *file)

If you go for that approack you also need to do it
in resume()

	Regards
		Oliver

Re: [PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:

> On Sun, 2016-07-03 at 21:59 +0200, Bjørn Mork wrote:
>> 		default:
>> @@ -200,10 +200,29 @@ static void wdm_in_callback(struct urb *urb)
>>  			desc->reslength = length;
>>  		}
>>  	}
>> +
>> +	/*
>> +	 * If desc->resp_count is unset, then the urb was submitted
>> +	 * without a prior notification.  If the device returned any
>> +	 * data, then this implies that it had messages queued without
>> +	 * notifying us.  Continue reading until that queue is flushed.
>> +	 */
>> +	if (!desc->resp_count) {
>> +		if (!length) {
>> +			/* do not propagate the expected -EPIPE */
>> +			desc->rerr = 0;
>> +			goto unlock;
>> +		}
>> +		dev_dbg(&desc->intf->dev, "got %d bytes without notification\n", length);
>> +		set_bit(WDM_RESPONDING, &desc->flags);
>> +		usb_submit_urb(desc->response, GFP_ATOMIC);
>
> You must check for being in an overflow condition.

No, I don't think that will give the wanted/expected result.  The main
point here is to flush whatever the data the device has queued up, but
haven't notified us about (or for which we've lost the notification for
some reason). We want to continue flushing whether or not we can store
the received data.  The goal is to get back into a syncronouos state,
not necessarily to save everything the device has queued.  If we
overflow, then the lines just before this hunk will handle that just
fine by setting the WDM_OVERFLOW bit and dropping the data.  That's what
we want.



>> +	}
>> +
>>  skip_error:
>>  	wake_up(&desc->wait);
>>  
>>  	set_bit(WDM_READ, &desc->flags);
>> +unlock:
>>  	spin_unlock(&desc->iuspin);
>>  }
>>  
>> @@ -647,6 +666,16 @@ static int wdm_open(struct inode *inode, struct file *file)
>
> If you go for that approack you also need to do it
> in resume()

Yes, I wondered about that...  But I didn't add it because I am unable
to provoke the problem there, so I cannot really test if it has any
positive effect.  Do you still want it?


Bjørn

Re: [PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications

[Oliver Neukum]

On Mon, 2016-07-04 at 13:54 +0200, Bjørn Mork wrote:
> Oliver Neukum <oneukum@suse.com> writes:
> 
> > On Sun, 2016-07-03 at 21:59 +0200, Bjørn Mork wrote:
> >> 		default:
> >> @@ -200,10 +200,29 @@ static void wdm_in_callback(struct urb *urb)
> >>  			desc->reslength = length;
> >>  		}
> >>  	}
> >> +
> >> +	/*
> >> +	 * If desc->resp_count is unset, then the urb was submitted
> >> +	 * without a prior notification.  If the device returned any
> >> +	 * data, then this implies that it had messages queued without
> >> +	 * notifying us.  Continue reading until that queue is flushed.
> >> +	 */
> >> +	if (!desc->resp_count) {
> >> +		if (!length) {
> >> +			/* do not propagate the expected -EPIPE */
> >> +			desc->rerr = 0;
> >> +			goto unlock;
> >> +		}
> >> +		dev_dbg(&desc->intf->dev, "got %d bytes without notification\n", length);
> >> +		set_bit(WDM_RESPONDING, &desc->flags);
> >> +		usb_submit_urb(desc->response, GFP_ATOMIC);
> >
> > You must check for being in an overflow condition.
> 
> No, I don't think that will give the wanted/expected result.  The main
> point here is to flush whatever the data the device has queued up, but

That raises the point why we store the data at all.

> haven't notified us about (or for which we've lost the notification for
> some reason). We want to continue flushing whether or not we can store
> the received data.  The goal is to get back into a syncronouos state,
> not necessarily to save everything the device has queued.  If we
> overflow, then the lines just before this hunk will handle that just
> fine by setting the WDM_OVERFLOW bit and dropping the data.  That's what
> we want.

OK, you know the protocol better than I.

> >> +	}
> >> +
> >>  skip_error:
> >>  	wake_up(&desc->wait);
> >>  
> >>  	set_bit(WDM_READ, &desc->flags);
> >> +unlock:
> >>  	spin_unlock(&desc->iuspin);
> >>  }
> >>  
> >> @@ -647,6 +666,16 @@ static int wdm_open(struct inode *inode, struct file *file)
> >
> > If you go for that approack you also need to do it
> > in resume()
> 
> Yes, I wondered about that...  But I didn't add it because I am unable
> to provoke the problem there, so I cannot really test if it has any
> positive effect.  Do you still want it?

Yes. I think the window is small, but we don't want strange
irreproducible flukes.

	Regards
		Oliver

Re: [PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:
> On Mon, 2016-07-04 at 13:54 +0200, Bjørn Mork wrote:
>> Oliver Neukum <oneukum@suse.com> writes:
>> 

>> > If you go for that approack you also need to do it
>> > in resume()
>> 
>> Yes, I wondered about that...  But I didn't add it because I am unable
>> to provoke the problem there, so I cannot really test if it has any
>> positive effect.  Do you still want it?
>
> Yes. I think the window is small, but we don't want strange
> irreproducible flukes.

OK.  Will post a v3 with the same code in resume once I've done some
basic testing.


Bjørn

Re: [PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications

[Bjørn Mork]

Bjørn Mork <bjorn@mork.no> writes:
> Oliver Neukum <oneukum@suse.com> writes:
>> On Mon, 2016-07-04 at 13:54 +0200, Bjørn Mork wrote:
>>> Oliver Neukum <oneukum@suse.com> writes:
>>> 
>
>>> > If you go for that approack you also need to do it
>>> > in resume()
>>> 
>>> Yes, I wondered about that...  But I didn't add it because I am unable
>>> to provoke the problem there, so I cannot really test if it has any
>>> positive effect.  Do you still want it?
>>
>> Yes. I think the window is small, but we don't want strange
>> irreproducible flukes.
>
> OK.  Will post a v3 with the same code in resume once I've done some
> basic testing.

This isn't going very well.

First, this whole thing is a hack for something that should not be an
issue in the first place. Firmware should never queue data without
sending a notification.  The problem does not affect any CDC WDM device,
and I don't think it is a common problem for QMI devices either.  I have
mostly observed it with MBIM.  But there it is very real and extremely
annoying.  I can easily provoke it with two different firmware bases
(from Intel and Qualcomm).

The proposed driver solution is a hack, allowing us to resyncronize
without resetting the firmware. We have no reliable way to reset the
firmware and doing so will have large side effects anyway.  The hack
allows us to continue, with a possibly lost message or two as the only
consequences.  We don't have to break any existing session etc.

But the hack is far from perfect. We race against the firmware every
time it runs.  We risk reading data which the firmware has just prepared
and are sending us a notification for, only to get no data when we
respond to the notification.  This is no problem for any CDC WDM
device.  One of the reads will turn up emtpy, but it doesn't matter if
that's the one before or after the notification.  The spec explicitly
allows empty reads.

But it turns out that it is a real problem for the very same MBIM
firmwares which caused us to need the hack in the first place: They
stall when you try to read and there is no data.  Which means that a
lost race can end up with an EPIPE being returned to _read(), where it
is translated and returned to userspace.

We don't want that.  I consider it an acceptable risk for every open(),
given that I have no better way to do this resyncronization.  But it is
not acceptable that it can happen on every autosuspend sequence, which
is the consequence of adding the hack to resume.  I've now been running
a few hours with it, and the result is occasional unexpected read errors
after resuming.

This is a mess.  I fully agree that it looks like this either should go
into both open() and resume(), or none.  The real answer is 'none'.

But we need the hack for the yucky MBIM firmwares.  Putting it in open()
is defining open() as a syncronization point for something which should
not need syncronization in a perfect world.  I can expand the comment
there a bit to make this clear. Making resume() a syncronization point
as well is only making things worse.


Bjørn

Re: [PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications

[Oliver Neukum]

On Mon, 2016-07-04 at 19:01 +0200, Bjørn Mork wrote:

> The proposed driver solution is a hack, allowing us to resyncronize
> without resetting the firmware. We have no reliable way to reset the
> firmware and doing so will have large side effects anyway.  The hack
> allows us to continue, with a possibly lost message or two as the only
> consequences.  We don't have to break any existing session etc.

OK, then I propose it is time to bring out the sledge hammer.
How about changing usb_cdc_wdm_register() to include a flag
about the necessity to drain the buffers?

> But it turns out that it is a real problem for the very same MBIM
> firmwares which caused us to need the hack in the first place: They
> stall when you try to read and there is no data.  Which means that a
> lost race can end up with an EPIPE being returned to _read(), where it
> is translated and returned to userspace.

This sucks and it tells me even more that CDC MBIM should tell us when
to apply this work around at startup.

> This is a mess.  I fully agree that it looks like this either should go
> into both open() and resume(), or none.  The real answer is 'none'.
> 
> But we need the hack for the yucky MBIM firmwares.  Putting it in open()
> is defining open() as a syncronization point for something which should
> not need syncronization in a perfect world.  I can expand the comment
> there a bit to make this clear. Making resume() a syncronization point
> as well is only making things worse.

I'd say add a flag, let CDC MBIM set it unconditionally (we can switch
to a black list if we absolutely need to) and include a big, fat
commentary. There is no good solution for this problem.

	Regards
		Oliver

Re: [PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications

[Bjørn Mork]

[removed the stable CC since this discussion isn't relevant to stable
 anymore]

Oliver Neukum <oneukum@suse.com> writes:

> On Mon, 2016-07-04 at 19:01 +0200, Bjørn Mork wrote:
>
>> The proposed driver solution is a hack, allowing us to resyncronize
>> without resetting the firmware. We have no reliable way to reset the
>> firmware and doing so will have large side effects anyway.  The hack
>> allows us to continue, with a possibly lost message or two as the only
>> consequences.  We don't have to break any existing session etc.
>
> OK, then I propose it is time to bring out the sledge hammer.
> How about changing usb_cdc_wdm_register() to include a flag
> about the necessity to drain the buffers?

Maybe... But I am hesitating a bit about yet again changing the
usb_cdc_wdm_register() signature.  It is messy, living in the usb tree
while affecting 3 drivers in the netdev tree.  I'm also of the opinion
that those 3 drivers will need this enabled. At least I'm sure both
qmi_wwan and cdc_mbim will.

How about splitting the behaviour locally in cdc-wdm, keeping the
current behaviour for CDC WDM devices and changing to "drain on open"
for the netdev drivers?  Something like this plus the necessary logic
dealing with the "drain_on_open" ("next_desc" is a label in wdm_probe):

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 61ea87917433..fce875bb7bb1 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -753,7 +753,8 @@ static void wdm_rxwork(struct work_struct *work)
 /* --- hotplug --- */
 
 static int wdm_create(struct usb_interface *intf, struct usb_endpoint_descriptor *ep,
-		u16 bufsize, int (*manage_power)(struct usb_interface *, int))
+		u16 bufsize, int (*manage_power)(struct usb_interface *, int),
+		bool drain_on_open)
 {
 	int rv = -ENOMEM;
 	struct wdm_device *desc;
@@ -913,7 +914,7 @@ next_desc:
 		goto err;
 	ep = &iface->endpoint[0].desc;
 
-	rv = wdm_create(intf, ep, maxcom, &wdm_manage_power);
+	rv = wdm_create(intf, ep, maxcom, &wdm_manage_power, false);
 
 err:
 	return rv;
@@ -945,7 +946,7 @@ struct usb_driver *usb_cdc_wdm_register(struct usb_interface *intf,
 {
 	int rv = -EINVAL;
 
-	rv = wdm_create(intf, ep, bufsize, manage_power);
+	rv = wdm_create(intf, ep, bufsize, manage_power, true);
 	if (rv < 0)
 		goto err;
 


>> But it turns out that it is a real problem for the very same MBIM
>> firmwares which caused us to need the hack in the first place: They
>> stall when you try to read and there is no data.  Which means that a
>> lost race can end up with an EPIPE being returned to _read(), where it
>> is translated and returned to userspace.
>
> This sucks and it tells me even more that CDC MBIM should tell us when
> to apply this work around at startup.

Yes.  We could alternatively filter the EPIPE from read(), since it
isn't supposed to happen anyway.  But it's not going to look less ugly :(

>> This is a mess.  I fully agree that it looks like this either should go
>> into both open() and resume(), or none.  The real answer is 'none'.
>> 
>> But we need the hack for the yucky MBIM firmwares.  Putting it in open()
>> is defining open() as a syncronization point for something which should
>> not need syncronization in a perfect world.  I can expand the comment
>> there a bit to make this clear. Making resume() a syncronization point
>> as well is only making things worse.
>
> I'd say add a flag, let CDC MBIM set it unconditionally (we can switch
> to a black list if we absolutely need to) and include a big, fat
> commentary. There is no good solution for this problem.

Execpt for the extreme ugliness, I don't think it will hurt to apply
this unconditionally for all the network drivers using cdc-wdm, as long
as it is limited to open only.

I certainly want to avoid any blacklist.  Device IDs are cheap in this
market. The MBIM modems typically run Android and the device ID is
configured in NVRAM for whatever OEM laptop vendor it is sold to.
Having a catch-all class driver for MBIM is an absolute requirement.
Making it depend on lists of devices is not an option, IMHO.

On the opposite side we have the huawei NCM modems, where device IDs are
so expensive that they have been using the same small set for thousands
of modems based on various hardware and firmware.


Bjørn

Re: [PATCH v2] cdc-wdm: fix "out-of-sync" due to missing notifications

[Oliver Neukum]

On Sat, 2016-07-09 at 20:31 +0200, Bjørn Mork wrote:
> [removed the stable CC since this discussion isn't relevant to stable
>  anymore]
> 
> Oliver Neukum <oneukum@suse.com> writes:

[..]
> How about splitting the behaviour locally in cdc-wdm, keeping the
> current behaviour for CDC WDM devices and changing to "drain on open"
> for the netdev drivers?  Something like this plus the necessary logic
> dealing with the "drain_on_open" ("next_desc" is a label in wdm_probe):

Even better solution.

[..]
> Yes.  We could alternatively filter the EPIPE from read(), since it
> isn't supposed to happen anyway.  But it's not going to look less ugly :(

I'd rather not. Dropping errors really is evil.

> Execpt for the extreme ugliness, I don't think it will hurt to apply
> this unconditionally for all the network drivers using cdc-wdm, as long
> as it is limited to open only.

It is very hard to see how would can avoid it in the long run in resume.

> I certainly want to avoid any blacklist.  Device IDs are cheap in this
> market. The MBIM modems typically run Android and the device ID is
> configured in NVRAM for whatever OEM laptop vendor it is sold to.
> Having a catch-all class driver for MBIM is an absolute requirement.
> Making it depend on lists of devices is not an option, IMHO.

Obviously. I am just not optimistic that we can do without specific
exceptions in the long run.

	Regards
		Oliver