[PATCH 0/2] usb: serial: handle -ENODEV and -EPROTO quietly

[Jeremiah Mahler]

If a USB serial device (e.g. /dev/ttyUSB0) with an active program is
unplugged, a bunch of -ENODEV and -EPROTO errors will be produced in the
logs. This patch set quiets these messages without changing the
original behavior.

This change is beneficial when using daemons such as slcand, which is
similar to pppd or slip, that cannot determine whether they should exit
until after the USB serial device is unplugged. Producing these error
messages for a normal use case is not helpful.

Jeremiah Mahler (2):
usb: serial: handle -EPROTO quietly in generic_read_bulk
usb: serial: handle -ENODEV quietly in generic_submit_read_urb

drivers/usb/serial/generic.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

--
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Jeremiah Mahler]

If a USB serial device (e.g. /dev/ttyUSB0) with an active program is

unplugged, a bunch of -EPROTO (71) error messages will be produced by
usb_serial_generic_read_bulk_callback() as it tries to resubmit the
request.

usb_serial_generic_read_bulk_callback - nonzero urb status: -71

Keep the same functionality, resubmit after an -EPROTO error, but change
message log level to debug instead of error so they are handled quietly
by default.

Signed-off-by: Jeremiah Mahler <jmma...@gmail.com>
---
drivers/usb/serial/generic.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 1bd1922..98fe718 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -372,6 +372,10 @@ void usb_serial_generic_read_bulk_callback(struct urb *urb)
dev_err(&port->dev, "%s - urb stopped: %d\n",
__func__, urb->status);
return;
+ case -EPROTO:
+ dev_dbg(&port->dev, "%s - urb resubmit: %d\n",
+ __func__, urb->status);
+ goto resubmit;
default:
dev_err(&port->dev, "%s - nonzero urb status: %d\n",
__func__, urb->status);

[Jeremiah Mahler]

If a USB serial device (e.g. /dev/ttyUSB0) with an active program is

unplugged, an -ENODEV (19) error will be produced after it gives up
trying to resubmit a read.

usb_serial_generic_submit_read_urb - usb_submit_urb failed: -19

Add -ENODEV as one of the permanent errors along with -EPERM that
usb_serial_generic_submit_read_urb() handles quietly without an error.

Signed-off-by: Jeremiah Mahler <jmma...@gmail.com>
---

drivers/usb/serial/generic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 98fe718..cca81c4 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -286,7 +286,7 @@ static int usb_serial_generic_submit_read_urb(struct usb_serial_port *port,

res = usb_submit_urb(port->read_urbs[index], mem_flags);
if (res) {
- if (res != -EPERM) {
+ if (res != -EPERM && res != -ENODEV) {
dev_err(&port->dev,
"%s - usb_submit_urb failed: %d\n",
__func__, res);

[Johan Hovold]

On Thu, Dec 11, 2014 at 03:29:52PM -0800, Jeremiah Mahler wrote:
> If a USB serial device (e.g. /dev/ttyUSB0) with an active program is
> unplugged, a bunch of -ENODEV and -EPROTO errors will be produced in the
> logs. This patch set quiets these messages without changing the
> original behavior.

Don't unplug devices that are in use then. ;)

> This change is beneficial when using daemons such as slcand, which is
> similar to pppd or slip, that cannot determine whether they should exit
> until after the USB serial device is unplugged. Producing these error
> messages for a normal use case is not helpful.

Your patches would hide these errors when they occur during normal use
(e.g. EPROTO).

Receiving an error message when unplugging an active device should not
surprise anyone. And at least you know where it came from (and it's
right there in the logs as well).

Johan

[Jeremiah Mahler]

Johan,

On Mon, Dec 15, 2014 at 11:23:21AM +0100, Johan Hovold wrote:
> On Thu, Dec 11, 2014 at 03:29:52PM -0800, Jeremiah Mahler wrote:
> > If a USB serial device (e.g. /dev/ttyUSB0) with an active program is
> > unplugged, a bunch of -ENODEV and -EPROTO errors will be produced in the
> > logs. This patch set quiets these messages without changing the
> > original behavior.
>
> Don't unplug devices that are in use then. ;)
>

I knew someone was going to say that :-)

> > This change is beneficial when using daemons such as slcand, which is
> > similar to pppd or slip, that cannot determine whether they should exit
> > until after the USB serial device is unplugged. Producing these error
> > messages for a normal use case is not helpful.
>
> Your patches would hide these errors when they occur during normal use
> (e.g. EPROTO).
>
> Receiving an error message when unplugging an active device should not
> surprise anyone. And at least you know where it came from (and it's
> right there in the logs as well).
>
> Johan

Hmm. Yes, I can see why quieting -EPROTO would be bad because it would
hide protocol errors which we want to know about.

I need to re-think this patch.
Nack.

Thanks for the review,
--
- Jeremiah Mahler

[Greg Kroah-Hartman]

On Mon, Dec 15, 2014 at 04:53:05AM -0800, Jeremiah Mahler wrote:
> Johan,
>
> On Mon, Dec 15, 2014 at 11:23:21AM +0100, Johan Hovold wrote:
> > On Thu, Dec 11, 2014 at 03:29:52PM -0800, Jeremiah Mahler wrote:
> > > If a USB serial device (e.g. /dev/ttyUSB0) with an active program is
> > > unplugged, a bunch of -ENODEV and -EPROTO errors will be produced in the
> > > logs. This patch set quiets these messages without changing the
> > > original behavior.
> >
> > Don't unplug devices that are in use then. ;)
> >
> I knew someone was going to say that :-)
>
> > > This change is beneficial when using daemons such as slcand, which is
> > > similar to pppd or slip, that cannot determine whether they should exit
> > > until after the USB serial device is unplugged. Producing these error
> > > messages for a normal use case is not helpful.
> >
> > Your patches would hide these errors when they occur during normal use
> > (e.g. EPROTO).
> >
> > Receiving an error message when unplugging an active device should not
> > surprise anyone. And at least you know where it came from (and it's
> > right there in the logs as well).
> >
> > Johan
>
> Hmm. Yes, I can see why quieting -EPROTO would be bad because it would
> hide protocol errors which we want to know about.

Do you really want to "know about" them? What can a user do with them?
Nothing, so just resubmit and you should be fine.

> I need to re-think this patch.
> Nack.

I like this patch, putting crud in the kernel log that no one can do
anything with for a "normal" operation like yanking a USB device out
while it is open should not happen.

thanks,

greg k-h

[Jeremiah Mahler]

Greg,

Perhaps something at a lower level could return a more apt error number
such as -ENODEV. Then there would be no conflict with -EPROTO.

I will look in to it again and re-submit the patches.

--
- Jeremiah Mahler

[Johan Hovold]

On Mon, Dec 15, 2014 at 08:38:01AM -0800, Greg Kroah-Hartman wrote:

Knowing that a device is flakey (and should be replaced) might be of
some worth?

And wouldn't silencing such errors mean that we could be quietly
dropping data?

> > I need to re-think this patch.
> > Nack.
>
> I like this patch, putting crud in the kernel log that no one can do
> anything with for a "normal" operation like yanking a USB device out
> while it is open should not happen.

The problem is that several errors may be returned from the
host-controller driver as a consequence of disconnect (before the hub
driver can process the disconnect). At least -EPROTO, -EILSEQ, -ETIME
are -EPIPE explicitly listed in Documentation/usb/error-codes.txt for
this, and of those, -EPROTO, -EILSEQ could also indicate hardware
problems.

I don't see how we can get around the trade-off between having a few
error messages in the log in the short window prior to a processed (and
also logged) disconnect, and not reporting potential hardware issues.

Thanks,
Johan

[Johan Hovold]

On Mon, Dec 15, 2014 at 11:10:37PM -0800, Jeremiah Mahler wrote:

> Perhaps something at a lower level could return a more apt error number
> such as -ENODEV. Then there would be no conflict with -EPROTO.

I'm afraid it's not possible to differentiate between a disconnected and
malfunctioning device in that short window before the disconnect is
reported and processes by the hub driver.

> I will look in to it again and re-submit the patches.

Let's see what comes out of this discussion first.

Thanks,
Johan

[Johan Hovold]

On Thu, Dec 11, 2014 at 03:29:54PM -0800, Jeremiah Mahler wrote:
> If a USB serial device (e.g. /dev/ttyUSB0) with an active program is
> unplugged, an -ENODEV (19) error will be produced after it gives up
> trying to resubmit a read.
>
> usb_serial_generic_submit_read_urb - usb_submit_urb failed: -19
>
> Add -ENODEV as one of the permanent errors along with -EPERM that
> usb_serial_generic_submit_read_urb() handles quietly without an error.
>
> Signed-off-by: Jeremiah Mahler <jmma...@gmail.com>

I'll apply this one once v3.19-rc1 is out.

Thanks,
Johan

[Jeremiah Mahler]

NOTE: These patches can wait until after the merge window. I just
wanted this slightly improved version to be available when the window
does close.

If a USB serial device (e.g. /dev/ttyUSB0) with an active program is
unplugged, a bunch of -ENODEV and -EPROTO errors will be produced in the
logs. This patch set quiets these messages without changing the
original behavior.

This change is beneficial when using daemons such as slcand, which is
similar to pppd or slip, that cannot determine whether they should exit
until after the USB serial device is unplugged. Producing these error
messages for a normal use case is not helpful.

Changes in v2:
- Instead of handling -EPROTO specially, use dev_dbg instead of
dev_err like other drivers do.

Jeremiah Mahler (2):
usb: serial: handle -EPROTO quietly in generic_read_bulk
usb: serial: handle -ENODEV quietly in generic_submit_read_urb

drivers/usb/serial/generic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

[Jeremiah Mahler]

If a USB serial device driver, which is built using the generic serial
driver, is unplugged while there is an active program using the device,
it will spam the logs with -EPROTO (71) messages as it attempts to
retry.

Most serial usb drivers (metro-usb, pl2303, mos7840, ...) only output
these messages for debugging. The generic driver treats these as
errors.

Change the default output for the generic serial driver from error to
debug.

Signed-off-by: Jeremiah Mahler <jmma...@gmail.com>
---
drivers/usb/serial/generic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c

index 1bd1922..2d7207b 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -373,7 +373,7 @@ void usb_serial_generic_read_bulk_callback(struct urb *urb)
__func__, urb->status);
return;
default:
- dev_err(&port->dev, "%s - nonzero urb status: %d\n",
+ dev_dbg(&port->dev, "%s - nonzero urb status: %d\n",
__func__, urb->status);
goto resubmit;

[Jeremiah Mahler]

If a USB serial device (e.g. /dev/ttyUSB0) with an active program is

unplugged, an -ENODEV (19) error will be produced after it gives up
trying to resubmit a read.

usb_serial_generic_submit_read_urb - usb_submit_urb failed: -19

Add -ENODEV as one of the permanent errors along with -EPERM that
usb_serial_generic_submit_read_urb() handles quietly without an error.

Signed-off-by: Jeremiah Mahler <jmma...@gmail.com>
---
drivers/usb/serial/generic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c

index 2d7207b..ccf1df7 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c

@@ -286,7 +286,7 @@ static int usb_serial_generic_submit_read_urb(struct usb_serial_port *port,

res = usb_submit_urb(port->read_urbs[index], mem_flags);
if (res) {
- if (res != -EPERM) {
+ if (res != -EPERM && res != -ENODEV) {
dev_err(&port->dev,
"%s - usb_submit_urb failed: %d\n",
__func__, res);

[Sergei Shtylyov]

Hello.

On 12/20/2014 12:11 PM, Jeremiah Mahler wrote:

> If a USB serial device driver, which is built using the generic serial
> driver, is unplugged while there is an active program using the device,

Driver is unplugged? :-)

> it will spam the logs with -EPROTO (71) messages as it attempts to
> retry.

> Most serial usb drivers (metro-usb, pl2303, mos7840, ...) only output
> these messages for debugging. The generic driver treats these as
> errors.

> Change the default output for the generic serial driver from error to
> debug.

> Signed-off-by: Jeremiah Mahler <jmma...@gmail.com>

[...]

WBR, Sergei

[Jeremiah Mahler]

Sergei,

On Sat, Dec 20, 2014 at 03:32:42PM +0300, Sergei Shtylyov wrote:
> Hello.
>
> On 12/20/2014 12:11 PM, Jeremiah Mahler wrote:
>
> >If a USB serial device driver, which is built using the generic serial
> >driver, is unplugged while there is an active program using the device,
>
> Driver is unplugged? :-)
>

Yes, that doesn't make sense. Good catch, thanks :-)

> >it will spam the logs with -EPROTO (71) messages as it attempts to
> >retry.
>
> >Most serial usb drivers (metro-usb, pl2303, mos7840, ...) only output
> >these messages for debugging. The generic driver treats these as
> >errors.
>
> >Change the default output for the generic serial driver from error to
> >debug.
>
> >Signed-off-by: Jeremiah Mahler <jmma...@gmail.com>
>
> [...]
>
> WBR, Sergei
>

--

- Jeremiah Mahler

[Jeremiah Mahler]

If a USB serial device is unplugged while there is an active program
using the device it will spam the logs with -EPROTO (71) messages as it

attempts to retry.

Most serial usb drivers (metro-usb, pl2303, mos7840, ...) only output
these messages for debugging. The generic driver treats these as
errors.

Change the default output for the generic serial driver from error to
debug.

Signed-off-by: Jeremiah Mahler <jmma...@gmail.com>

---

Notes:
Minor change just fixes the wording in the log message.

drivers/usb/serial/generic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c

index 1bd1922..2d7207b 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c

@@ -373,7 +373,7 @@ void usb_serial_generic_read_bulk_callback(struct urb *urb)
__func__, urb->status);
return;
default:
- dev_err(&port->dev, "%s - nonzero urb status: %d\n",
+ dev_dbg(&port->dev, "%s - nonzero urb status: %d\n",
__func__, urb->status);
goto resubmit;
}
--
2.1.3

[Jeremiah Mahler]

(Jan 15) Just a resend of v2 [1].

[1]: https://lkml.org/lkml/2014/12/20/16

If a USB serial device (e.g. /dev/ttyUSB0) with an active program is
unplugged, a bunch of -ENODEV and -EPROTO errors will be produced in the
logs. This patch set quiets these messages without changing the
original behavior.

This change is beneficial when using daemons such as slcand, which is
similar to pppd or slip, that cannot determine whether they should exit
until after the USB serial device is unplugged. Producing these error
messages for a normal use case is not helpful.

Jeremiah Mahler (2):
usb: serial: handle -EPROTO quietly in generic_read_bulk
usb: serial: handle -ENODEV quietly in generic_submit_read_urb

drivers/usb/serial/generic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

--

2.1.4

[Johan Hovold]

On Sat, Jan 10, 2015 at 04:44:32PM -0800, Jeremiah Mahler wrote:
> If a USB serial device is unplugged while there is an active program
> using the device it will spam the logs with -EPROTO (71) messages as it
> attempts to retry.

Can you change this to "might spam", as which error message, and if it is
at all printed, depends on host controller and what (hub) devices are
used.

> Most serial usb drivers (metro-usb, pl2303, mos7840, ...) only output
> these messages for debugging. The generic driver treats these as
> errors.
>
> Change the default output for the generic serial driver from error to
> debug.

Please also correct the commit summary (Subject) so that it matches what
this revised patch now does (you silence all non-critical error
messages).

Fix this up and I'll take both patches.

Thanks,
Johan

[Jeremiah Mahler]

Johan,

On Sun, Jan 11, 2015 at 12:36:18PM +0100, Johan Hovold wrote:
> On Sat, Jan 10, 2015 at 04:44:32PM -0800, Jeremiah Mahler wrote:
> > If a USB serial device is unplugged while there is an active program
> > using the device it will spam the logs with -EPROTO (71) messages as it
> > attempts to retry.
>
> Can you change this to "might spam", as which error message, and if it is
> at all printed, depends on host controller and what (hub) devices are
> used.
>

Ah, yes, good point.

> > Most serial usb drivers (metro-usb, pl2303, mos7840, ...) only output
> > these messages for debugging. The generic driver treats these as
> > errors.
> >
> > Change the default output for the generic serial driver from error to
> > debug.
>
> Please also correct the commit summary (Subject) so that it matches what
> this revised patch now does (you silence all non-critical error
> messages).
>

True, it is more than just -EPROTO now.

> Fix this up and I'll take both patches.
>
> Thanks,
> Johan

Thanks for the suggestions.

v3 coming up.

--
- Jeremiah Mahler

[Jeremiah Mahler]

If a USB serial device (e.g. /dev/ttyUSB0) with an active program is

unplugged, a bunch of -ENODEV and -EPROTO errors may be produced in the

logs. This patch set quiets these messages without changing the
original behavior.

This change is beneficial when using daemons such as slcand, which is
similar to pppd or slip, that cannot determine whether they should exit
until after the USB serial device is unplugged. Producing these error
messages for a normal use case is not helpful.

Changes in v3:

- change "will spam" to "might spam" since it is dependent
upon what host controller and devices are used.

- fix subject: it is silencing all errors, not just one as in
previous versions.

Changes in v2:

- Instead of handling -EPROTO specially, use dev_dbg instead of
dev_err like other drivers do.

Jeremiah Mahler (2):

usb: serial: silence all non-critical read errors

usb: serial: handle -ENODEV quietly in generic_submit_read_urb

drivers/usb/serial/generic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

--

2.1.4

[Jeremiah Mahler]

If a USB serial device is unplugged while there is an active program

using the device it may spam the logs with -EPROTO (71) messages as it
attempts to retry.

Most serial usb drivers (metro-usb, pl2303, mos7840, ...) only output
these messages for debugging. The generic driver treats these as
errors.

Change the default output for the generic serial driver from error to

debug to silence these non-critical errors.

Signed-off-by: Jeremiah Mahler <jmma...@gmail.com>
---

drivers/usb/serial/generic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 1bd1922..2d7207b 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -373,7 +373,7 @@ void usb_serial_generic_read_bulk_callback(struct urb *urb)
__func__, urb->status);
return;
default:
- dev_err(&port->dev, "%s - nonzero urb status: %d\n",
+ dev_dbg(&port->dev, "%s - nonzero urb status: %d\n",
__func__, urb->status);
goto resubmit;
}

[Jeremiah Mahler]

If a USB serial device (e.g. /dev/ttyUSB0) with an active program is

unplugged, an -ENODEV (19) error will be produced after it gives up
trying to resubmit a read.

usb_serial_generic_submit_read_urb - usb_submit_urb failed: -19

Add -ENODEV as one of the permanent errors along with -EPERM that
usb_serial_generic_submit_read_urb() handles quietly without an error.

Signed-off-by: Jeremiah Mahler <jmma...@gmail.com>
---
drivers/usb/serial/generic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c

index 2d7207b..ccf1df7 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c

@@ -286,7 +286,7 @@ static int usb_serial_generic_submit_read_urb(struct usb_serial_port *port,

res = usb_submit_urb(port->read_urbs[index], mem_flags);
if (res) {
- if (res != -EPERM) {
+ if (res != -EPERM && res != -ENODEV) {
dev_err(&port->dev,
"%s - usb_submit_urb failed: %d\n",
__func__, res);

[Johan Hovold]

On Sun, Jan 11, 2015 at 05:42:05AM -0800, Jeremiah Mahler wrote:
> If a USB serial device (e.g. /dev/ttyUSB0) with an active program is
> unplugged, a bunch of -ENODEV and -EPROTO errors may be produced in the
> logs. This patch set quiets these messages without changing the
> original behavior.

Both patches applied, thanks.

Johan