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
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 <[email protected]>
---
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);
--
2.1.3
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 <[email protected]>
---
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);
--
2.1.3
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
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
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
Greg,
On Mon, Dec 15, 2014 at 08:38:01AM -0800, Greg Kroah-Hartman wrote:
> 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
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
On Mon, Dec 15, 2014 at 08:38:01AM -0800, Greg Kroah-Hartman wrote:
> 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.
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
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
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 <[email protected]>
I'll apply this one once v3.19-rc1 is out.
Thanks,
Johan
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(-)
--
2.1.3
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 <[email protected]>
---
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
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 <[email protected]>
---
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);
--
2.1.3
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 <[email protected]>
[...]
WBR, Sergei
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 <[email protected]>
>
> [...]
>
> WBR, Sergei
>
--
- 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 <[email protected]>
---
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