New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 682469 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

chromeos-kernel: cherry-pick cdc-wdm bug fixes / patches from upstream

Project Member Reported by benchan@chromium.org, Jan 18 2017

Issue description

Our last round of cherry-picks ( issue 326559 ) was done in Feb 2014. There are a few notable bug fixes on the cdc-wdm driver have landed in upstream kernel:

  (1) 76cb03e7d5d7ba49175784dce961696da66c44cc: cdc-wdm: return correct error codes
  (2) 28965e17ee7a9591c241b831fee050d2391688c6: cdc-wdm: unify error handling in write
  (3) 323ece54e0761198946ecd0c2091f1d2bfdfcb64: cdc-wdm: fix endianness bug in debug statements
  (4) 85e8a0b9a3565c8185068b6b340cc8c6dd4411f4: cdc-wdm: error returns need to be translated
  (5) 833415a3e781a26fe480a34d45086bdb4fe1e4c0: cdc-wdm: fix "out-of-sync" due to missing notifications
  (6) c1da59dad0ebd3f9bd238f3fff82b1f7ffda7829: cdc-wdm: Clear read pipeline in case of error

  * In particular, (5) and (6) are worth fixing.

ChromeOS uses the cdc-wdm driver in conjunction with the cdc-mbim driver to support MBIM modems on Kip and Blaze, which run 3.10 kernel.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 19 2017

Labels: merge-merged-chromeos-3.10
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/18fcefb67864cab6fa6302a137bd29e2789ae01b

commit 18fcefb67864cab6fa6302a137bd29e2789ae01b
Author: Ben Chan <benchan@chromium.org>
Date: Wed Jan 18 21:08:48 2017

Revert "CHROMIUM: usb: cdc-wdm: Clear read pipeline in case of error."

This reverts commit 914f664e4b421f72470047255a748739550e3014.

This local patch has landed in the upstream Linux kernel as commit
c1da59dad0ebd3f9bd238f3fff82b1f7ffda7829:

    cdc-wdm: Clear read pipeline in case of error

    Implemented queued response handling. This queue is processed every time the
    WDM_READ flag is cleared.

    In case of a read error, userspace may not actually read the data, since the
    driver returns an error through wdm_poll. After this, the underlying device may
    attempt to send us more data, but the queue is not processed. While userspace is
    also blocked, because the read error is never cleared.

    After this patch, we proactively process the queue on a read error. If there was
    an outstanding response to handle, that will clear the error (or go through the
    same logic again, if another read error occurs). If there was no outstanding
    response, this will bring the queue size back to 0, unblocking a future response
    from the underlying device.

    Signed-off-by: Robert Foss <robert.foss@collabora.com>
    Tested-by: Robert Foss <robert.foss@collabora.com>
    Acked-by: Oliver Neukum <oneukum@suse.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

We revert this local patch and then apply the upstream version instead.

BUG= chromium:682469 
TEST=Run network3g tests on Kip and Blaze with MBIM modem.

Change-Id: I574dbc862ab2a006e443d59f023b1830e76a2db5
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430352
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/18fcefb67864cab6fa6302a137bd29e2789ae01b/drivers/usb/class/cdc-wdm.c

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 19 2017

Labels: merge-merged-chromeos-3.10
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/02cc35cf4bcbe7acaa47c71fde74718f3a6dfaf5

commit 02cc35cf4bcbe7acaa47c71fde74718f3a6dfaf5
Author: Oliver Neukum <oneukum@suse.de>
Date: Fri Mar 20 13:29:18 2015

UPSTREAM: cdc-wdm: unify error handling in write

This makes sure the error handling path is the same for
all error conditions, thus reducing code duplication.

Signed-off-by: Oliver Neukum <oneukum@suse.de>0
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 28965e17ee7a9591c241b831fee050d2391688c6)

BUG= chromium:682469 
TEST=Run network3g tests on Kip and Blaze with MBIM modem.

Change-Id: I3bbf4f2dd34b67a95965c736a2837a6eabe79b8d
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430354
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/02cc35cf4bcbe7acaa47c71fde74718f3a6dfaf5/drivers/usb/class/cdc-wdm.c

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/66d6a0a2b9b4c7e6d6295979b587fb42dceb63a1

commit 66d6a0a2b9b4c7e6d6295979b587fb42dceb63a1
Author: Oliver Neukum <oneukum@suse.de>
Date: Fri Mar 20 13:28:56 2015

UPSTREAM: cdc-wdm: return correct error codes

Lieing to user space is wrong. The real reason for a failure
to write should be returned to user space.

Signed-off-by: Oliver Neukum <oneukum@suse.de>0
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 76cb03e7d5d7ba49175784dce961696da66c44cc)

BUG= chromium:682469 
TEST=Run network3g tests on Kip and Blaze with MBIM modem.

Change-Id: Ib4c27d0611a7bffc29bedc44e26ffdcb0fa11b9e
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430353
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/66d6a0a2b9b4c7e6d6295979b587fb42dceb63a1/drivers/usb/class/cdc-wdm.c

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/fa43baf578d088e74646081494d282926ad7ce3a

commit fa43baf578d088e74646081494d282926ad7ce3a
Author: Oliver Neukum <oneukum@suse.de>
Date: Fri Mar 20 13:29:34 2015

UPSTREAM: cdc-wdm: fix endianness bug in debug statements

Values directly from descriptors given in debug statements
must be converted to native endianness.

Signed-off-by: Oliver Neukum <oneukum@suse.de>
CC: stable@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 323ece54e0761198946ecd0c2091f1d2bfdfcb64)

BUG= chromium:682469 
TEST=Run network3g tests on Kip and Blaze with MBIM modem.

Change-Id: Ibcf52dc5e4751c5e2af17ff3ea2921002ea7065c
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430355
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/fa43baf578d088e74646081494d282926ad7ce3a/drivers/usb/class/cdc-wdm.c

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/8058eeb77d49ccb9803dcd23e563dd1ad1c5550d

commit 8058eeb77d49ccb9803dcd23e563dd1ad1c5550d
Author: Oliver Neukum <oneukum@suse.de>
Date: Mon Mar 23 13:34:43 2015

UPSTREAM: cdc-wdm: error returns need to be translated

One more case of error codes not correctly being
correctly returned to user space.

Signed-off-by: Olive Neukum <oneukum@suse.com>0
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 85e8a0b9a3565c8185068b6b340cc8c6dd4411f4)

BUG= chromium:682469 
TEST=Run network3g tests on Kip and Blaze with MBIM modem.

Change-Id: I6a54e7ff363851dff8fd49be1ac12420c51d2b88
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430356
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/8058eeb77d49ccb9803dcd23e563dd1ad1c5550d/drivers/usb/class/cdc-wdm.c

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e75e848b5eea0212c4e2851087fa2e6360192d19

commit e75e848b5eea0212c4e2851087fa2e6360192d19
Author: Bjørn Mork <bjorn@mork.no>
Date: Sun Jul 10 15:45:14 2016

UPSTREAM: cdc-wdm: fix "out-of-sync" due to missing notifications

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.

Signed-off-by: Bjrn Mork <bjorn@mork.no>
Acked-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 833415a3e781a26fe480a34d45086bdb4fe1e4c0)

BUG= chromium:682469 
TEST=Run network3g tests on Kip and Blaze with MBIM modem.

Change-Id: I2ae9bf6b04721950ea24c9c058a773a718845b08
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430357
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/e75e848b5eea0212c4e2851087fa2e6360192d19/drivers/usb/class/cdc-wdm.c

Project Member

Comment 7 by bugdroid1@chromium.org, Jan 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/bdb31a615e39cacd7624d618b536edad36236470

commit bdb31a615e39cacd7624d618b536edad36236470
Author: Robert Foss <robert.foss@collabora.com>
Date: Tue Aug 09 14:54:52 2016

UPSTREAM: cdc-wdm: Clear read pipeline in case of error

Implemented queued response handling. This queue is processed every time the
WDM_READ flag is cleared.

In case of a read error, userspace may not actually read the data, since the
driver returns an error through wdm_poll. After this, the underlying device may
attempt to send us more data, but the queue is not processed. While userspace is
also blocked, because the read error is never cleared.

After this patch, we proactively process the queue on a read error. If there was
an outstanding response to handle, that will clear the error (or go through the
same logic again, if another read error occurs). If there was no outstanding
response, this will bring the queue size back to 0, unblocking a future response
from the underlying device.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
Tested-by: Robert Foss <robert.foss@collabora.com>
Acked-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit c1da59dad0ebd3f9bd238f3fff82b1f7ffda7829)

BUG= chromium:682469 
TEST=Run network3g tests on Kip and Blaze with MBIM modem.

Change-Id: Id5f32b7e7439579656dd1f69a8ef17126a789ad5
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430358
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/bdb31a615e39cacd7624d618b536edad36236470/drivers/usb/class/cdc-wdm.c

Status: Started (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 20 2017

Labels: merge-merged-chromeos-3.18
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ab22221756c9cea9de5efd927779e63a3dacfbe4

commit ab22221756c9cea9de5efd927779e63a3dacfbe4
Author: Oliver Neukum <oneukum@suse.de>
Date: Mon Mar 23 13:34:43 2015

UPSTREAM: cdc-wdm: error returns need to be translated

One more case of error codes not correctly being
correctly returned to user space.

Signed-off-by: Olive Neukum <oneukum@suse.com>0
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 85e8a0b9a3565c8185068b6b340cc8c6dd4411f4)

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: I6a54e7ff363851dff8fd49be1ac12420c51d2b88
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430338
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/ab22221756c9cea9de5efd927779e63a3dacfbe4/drivers/usb/class/cdc-wdm.c

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/3e0630dfa5c8588aa4f49fb5eac88fcfc942ebbc

commit 3e0630dfa5c8588aa4f49fb5eac88fcfc942ebbc
Author: Bjørn Mork <bjorn@mork.no>
Date: Sun Jul 10 15:45:14 2016

UPSTREAM: cdc-wdm: fix "out-of-sync" due to missing notifications

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.

Signed-off-by: Bjrn Mork <bjorn@mork.no>
Acked-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 833415a3e781a26fe480a34d45086bdb4fe1e4c0)

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: I2ae9bf6b04721950ea24c9c058a773a718845b08
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430339
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/3e0630dfa5c8588aa4f49fb5eac88fcfc942ebbc/drivers/usb/class/cdc-wdm.c

Project Member

Comment 11 by bugdroid1@chromium.org, Jan 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e9b2a871c33b64162f1d7cc6c19045b5a4a7e9b5

commit e9b2a871c33b64162f1d7cc6c19045b5a4a7e9b5
Author: Robert Foss <robert.foss@collabora.com>
Date: Tue Aug 09 14:54:52 2016

UPSTREAM: cdc-wdm: Clear read pipeline in case of error

Implemented queued response handling. This queue is processed every time the
WDM_READ flag is cleared.

In case of a read error, userspace may not actually read the data, since the
driver returns an error through wdm_poll. After this, the underlying device may
attempt to send us more data, but the queue is not processed. While userspace is
also blocked, because the read error is never cleared.

After this patch, we proactively process the queue on a read error. If there was
an outstanding response to handle, that will clear the error (or go through the
same logic again, if another read error occurs). If there was no outstanding
response, this will bring the queue size back to 0, unblocking a future response
from the underlying device.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
Tested-by: Robert Foss <robert.foss@collabora.com>
Acked-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit c1da59dad0ebd3f9bd238f3fff82b1f7ffda7829)

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: Id5f32b7e7439579656dd1f69a8ef17126a789ad5
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430340
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/e9b2a871c33b64162f1d7cc6c19045b5a4a7e9b5/drivers/usb/class/cdc-wdm.c

Project Member

Comment 12 by bugdroid1@chromium.org, Jan 20 2017

Labels: merge-merged-chromeos-4.4
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/f292d50304dde0345e09ecd99d3bd594c316e060

commit f292d50304dde0345e09ecd99d3bd594c316e060
Author: Ben Chan <benchan@chromium.org>
Date: Thu Jan 19 19:56:40 2017

Revert "CHROMIUM: usb: cdc-wdm: Clear read pipeline in case of error."

This reverts commit d0161e5f21d41470658f09d295e81bffc3b6903c.

This local patch has landed in the upstream Linux kernel as commit
c1da59dad0ebd3f9bd238f3fff82b1f7ffda7829:

    cdc-wdm: Clear read pipeline in case of error

    Implemented queued response handling. This queue is processed every time the
    WDM_READ flag is cleared.

    In case of a read error, userspace may not actually read the data, since the
    driver returns an error through wdm_poll. After this, the underlying device may
    attempt to send us more data, but the queue is not processed. While userspace is
    also blocked, because the read error is never cleared.

    After this patch, we proactively process the queue on a read error. If there was
    an outstanding response to handle, that will clear the error (or go through the
    same logic again, if another read error occurs). If there was no outstanding
    response, this will bring the queue size back to 0, unblocking a future response
    from the underlying device.

    Signed-off-by: Robert Foss <robert.foss@collabora.com>
    Tested-by: Robert Foss <robert.foss@collabora.com>
    Acked-by: Oliver Neukum <oneukum@suse.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

We revert this local patch and then apply the upstream version instead.

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: I574dbc862ab2a006e443d59f023b1830e76a2db5
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430366
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/f292d50304dde0345e09ecd99d3bd594c316e060/drivers/usb/class/cdc-wdm.c

Project Member

Comment 13 by bugdroid1@chromium.org, Jan 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/5ced708314698f18dbe7e17e59a66d61e2f20a90

commit 5ced708314698f18dbe7e17e59a66d61e2f20a90
Author: Bjørn Mork <bjorn@mork.no>
Date: Sun Jul 10 15:45:14 2016

UPSTREAM: cdc-wdm: fix "out-of-sync" due to missing notifications

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.

Signed-off-by: Bjrn Mork <bjorn@mork.no>
Acked-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 833415a3e781a26fe480a34d45086bdb4fe1e4c0)

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: I2ae9bf6b04721950ea24c9c058a773a718845b08
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430367
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/5ced708314698f18dbe7e17e59a66d61e2f20a90/drivers/usb/class/cdc-wdm.c

Project Member

Comment 14 by bugdroid1@chromium.org, Jan 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/eb8fa00274066c5e624d9c5c26fbc63e0fc2e7f6

commit eb8fa00274066c5e624d9c5c26fbc63e0fc2e7f6
Author: Robert Foss <robert.foss@collabora.com>
Date: Tue Aug 09 14:54:52 2016

UPSTREAM: cdc-wdm: Clear read pipeline in case of error

Implemented queued response handling. This queue is processed every time the
WDM_READ flag is cleared.

In case of a read error, userspace may not actually read the data, since the
driver returns an error through wdm_poll. After this, the underlying device may
attempt to send us more data, but the queue is not processed. While userspace is
also blocked, because the read error is never cleared.

After this patch, we proactively process the queue on a read error. If there was
an outstanding response to handle, that will clear the error (or go through the
same logic again, if another read error occurs). If there was no outstanding
response, this will bring the queue size back to 0, unblocking a future response
from the underlying device.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
Tested-by: Robert Foss <robert.foss@collabora.com>
Acked-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit c1da59dad0ebd3f9bd238f3fff82b1f7ffda7829)

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: Id5f32b7e7439579656dd1f69a8ef17126a789ad5
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430368
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/eb8fa00274066c5e624d9c5c26fbc63e0fc2e7f6/drivers/usb/class/cdc-wdm.c

Project Member

Comment 15 by bugdroid1@chromium.org, Jan 20 2017

Labels: merge-merged-chromeos-3.14
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/06f191174974aa5264a5fae1595551a1f61224b6

commit 06f191174974aa5264a5fae1595551a1f61224b6
Author: Ben Chan <benchan@chromium.org>
Date: Thu Jan 19 20:15:15 2017

Revert "CHROMIUM: usb: cdc-wdm: Clear read pipeline in case of error."

This reverts commit a33f028543bfd9480ba58cf6a56e97ec2b304382.

This local patch has landed in the upstream Linux kernel as commit
c1da59dad0ebd3f9bd238f3fff82b1f7ffda7829:

    cdc-wdm: Clear read pipeline in case of error

    Implemented queued response handling. This queue is processed every time the
    WDM_READ flag is cleared.

    In case of a read error, userspace may not actually read the data, since the
    driver returns an error through wdm_poll. After this, the underlying device may
    attempt to send us more data, but the queue is not processed. While userspace is
    also blocked, because the read error is never cleared.

    After this patch, we proactively process the queue on a read error. If there was
    an outstanding response to handle, that will clear the error (or go through the
    same logic again, if another read error occurs). If there was no outstanding
    response, this will bring the queue size back to 0, unblocking a future response
    from the underlying device.

    Signed-off-by: Robert Foss <robert.foss@collabora.com>
    Tested-by: Robert Foss <robert.foss@collabora.com>
    Acked-by: Oliver Neukum <oneukum@suse.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

We revert this local patch and then apply the upstream version instead.

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: I574dbc862ab2a006e443d59f023b1830e76a2db5
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430369
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/06f191174974aa5264a5fae1595551a1f61224b6/drivers/usb/class/cdc-wdm.c

Project Member

Comment 16 by bugdroid1@chromium.org, Jan 20 2017

Labels: merge-merged-chromeos-3.14
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4125a53b8c691bd34e927e7467fe3ce88d5f433e

commit 4125a53b8c691bd34e927e7467fe3ce88d5f433e
Author: Oliver Neukum <oneukum@suse.de>
Date: Fri Mar 20 13:28:56 2015

UPSTREAM: cdc-wdm: return correct error codes

Lieing to user space is wrong. The real reason for a failure
to write should be returned to user space.

Signed-off-by: Oliver Neukum <oneukum@suse.de>0
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 76cb03e7d5d7ba49175784dce961696da66c44cc)

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: Ib4c27d0611a7bffc29bedc44e26ffdcb0fa11b9e
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430870
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/4125a53b8c691bd34e927e7467fe3ce88d5f433e/drivers/usb/class/cdc-wdm.c

Project Member

Comment 17 by bugdroid1@chromium.org, Jan 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/6dd1c721fde0a431b0dba78f2ebd48636aad4249

commit 6dd1c721fde0a431b0dba78f2ebd48636aad4249
Author: Oliver Neukum <oneukum@suse.de>
Date: Fri Mar 20 13:29:18 2015

UPSTREAM: cdc-wdm: unify error handling in write

This makes sure the error handling path is the same for
all error conditions, thus reducing code duplication.

Signed-off-by: Oliver Neukum <oneukum@suse.de>0
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 28965e17ee7a9591c241b831fee050d2391688c6)

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: I3bbf4f2dd34b67a95965c736a2837a6eabe79b8d
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430871
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/6dd1c721fde0a431b0dba78f2ebd48636aad4249/drivers/usb/class/cdc-wdm.c

Project Member

Comment 18 by bugdroid1@chromium.org, Jan 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/618ec581b0a8b860a2ca2b4c98fa146da1784fa5

commit 618ec581b0a8b860a2ca2b4c98fa146da1784fa5
Author: Oliver Neukum <oneukum@suse.de>
Date: Fri Mar 20 13:29:34 2015

UPSTREAM: cdc-wdm: fix endianness bug in debug statements

Values directly from descriptors given in debug statements
must be converted to native endianness.

Signed-off-by: Oliver Neukum <oneukum@suse.de>
CC: stable@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 323ece54e0761198946ecd0c2091f1d2bfdfcb64)

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: Ibcf52dc5e4751c5e2af17ff3ea2921002ea7065c
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430872
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/618ec581b0a8b860a2ca2b4c98fa146da1784fa5/drivers/usb/class/cdc-wdm.c

Project Member

Comment 19 by bugdroid1@chromium.org, Jan 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/5142e7b3b9771576e3da92c5aff531209deff760

commit 5142e7b3b9771576e3da92c5aff531209deff760
Author: Oliver Neukum <oneukum@suse.de>
Date: Mon Mar 23 13:34:43 2015

UPSTREAM: cdc-wdm: error returns need to be translated

One more case of error codes not correctly being
correctly returned to user space.

Signed-off-by: Olive Neukum <oneukum@suse.com>0
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 85e8a0b9a3565c8185068b6b340cc8c6dd4411f4)

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: I6a54e7ff363851dff8fd49be1ac12420c51d2b88
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430873
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/5142e7b3b9771576e3da92c5aff531209deff760/drivers/usb/class/cdc-wdm.c

Project Member

Comment 20 by bugdroid1@chromium.org, Jan 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/b07c74703e51a10e567f8728fd4390301825cc0a

commit b07c74703e51a10e567f8728fd4390301825cc0a
Author: Bjørn Mork <bjorn@mork.no>
Date: Sun Jul 10 15:45:14 2016

UPSTREAM: cdc-wdm: fix "out-of-sync" due to missing notifications

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.

Signed-off-by: Bjrn Mork <bjorn@mork.no>
Acked-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 833415a3e781a26fe480a34d45086bdb4fe1e4c0)

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: I2ae9bf6b04721950ea24c9c058a773a718845b08
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430874
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/b07c74703e51a10e567f8728fd4390301825cc0a/drivers/usb/class/cdc-wdm.c

Project Member

Comment 21 by bugdroid1@chromium.org, Jan 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/b093583dda7e0be315bbf9aea4a697d1e20cd58e

commit b093583dda7e0be315bbf9aea4a697d1e20cd58e
Author: Robert Foss <robert.foss@collabora.com>
Date: Tue Aug 09 14:54:52 2016

UPSTREAM: cdc-wdm: Clear read pipeline in case of error

Implemented queued response handling. This queue is processed every time the
WDM_READ flag is cleared.

In case of a read error, userspace may not actually read the data, since the
driver returns an error through wdm_poll. After this, the underlying device may
attempt to send us more data, but the queue is not processed. While userspace is
also blocked, because the read error is never cleared.

After this patch, we proactively process the queue on a read error. If there was
an outstanding response to handle, that will clear the error (or go through the
same logic again, if another read error occurs). If there was no outstanding
response, this will bring the queue size back to 0, unblocking a future response
from the underlying device.

Signed-off-by: Robert Foss <robert.foss@collabora.com>
Tested-by: Robert Foss <robert.foss@collabora.com>
Acked-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit c1da59dad0ebd3f9bd238f3fff82b1f7ffda7829)

BUG= chromium:682469 
TEST=Run network3g tests with an external MBIM modem.

Change-Id: Id5f32b7e7439579656dd1f69a8ef17126a789ad5
Signed-off-by: Ben Chan <benchan@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/430875
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/b093583dda7e0be315bbf9aea4a697d1e20cd58e/drivers/usb/class/cdc-wdm.c

Status: Fixed (was: Started)

Comment 23 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 24 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 26 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)

Sign in to add a comment