[PATCH 1/2] cdc-acm: do not reset notification buffer index upon urb unlinking

[PATCH 1/2] cdc-acm: do not reset notification buffer index upon urb unlinking

[Tobias Herzog]

Resetting the write index of the notification buffer on urb unlink (e.g.
closing a cdc-acm device from userspace) may lead to wrong interpretation
of further received notifications, in case the index is not 0 when urb
unlink happens (i.e. when parts of a notification already have been
transferred). On the device side there is no "reset" of the notification
transimission and thus we would get out of sync with the device.

Signed-off-by: Tobias Herzog <t-herzog@gmx.de>
---
 drivers/usb/class/cdc-acm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 27346d6..50339ca 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -355,7 +355,6 @@ static void acm_ctrl_irq(struct urb *urb)
 	case -ENOENT:
 	case -ESHUTDOWN:
 		/* this urb is terminated, clean up */
-		acm->nb_index = 0;
 		dev_dbg(&acm->control->dev,
 			"%s - urb shutting down with status: %d\n",
 			__func__, status);
-- 
2.1.4

[PATCH 2/2] cdc-acm: correct counting of UART states in serial state notification

[Tobias Herzog]

The usb standard ("Universal Serial Bus Class Definitions for Communication
Devices") distiguishes between "consistent signals" (DSR, DCD), and
"irregular signals" (break, ring, parity error, framing error, overrun).
The bits of "irregular signals" are set, if this error/event occurred on
the device side and are immeadeatly unset, if the serial state notification
was sent.
Like other drivers of real serial ports do, just the occurence of those
events should be counted in serial_icounter_struct (but no 1->0
transitions).

Signed-off-by: Tobias Herzog <t-herzog@gmx.de>
---
 drivers/usb/class/cdc-acm.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 50339ca..187f386 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -310,17 +310,17 @@ static void acm_process_notification(struct acm *acm, unsigned char *buf)
 
 		if (difference & ACM_CTRL_DSR)
 			acm->iocount.dsr++;
-		if (difference & ACM_CTRL_BRK)
-			acm->iocount.brk++;
-		if (difference & ACM_CTRL_RI)
-			acm->iocount.rng++;
 		if (difference & ACM_CTRL_DCD)
 			acm->iocount.dcd++;
-		if (difference & ACM_CTRL_FRAMING)
+		if (newctrl & ACM_CTRL_BRK)
+			acm->iocount.brk++;
+		if (newctrl & ACM_CTRL_RI)
+			acm->iocount.rng++;
+		if (newctrl & ACM_CTRL_FRAMING)
 			acm->iocount.frame++;
-		if (difference & ACM_CTRL_PARITY)
+		if (newctrl & ACM_CTRL_PARITY)
 			acm->iocount.parity++;
-		if (difference & ACM_CTRL_OVERRUN)
+		if (newctrl & ACM_CTRL_OVERRUN)
 			acm->iocount.overrun++;
 		spin_unlock_irqrestore(&acm->read_lock, flags);
 
-- 
2.1.4

Re: [PATCH 2/2] cdc-acm: correct counting of UART states in serial state notification

[Greg KH]

On Sat, Sep 22, 2018 at 10:11:11PM +0200, Tobias Herzog wrote:
> The usb standard ("Universal Serial Bus Class Definitions for Communication
> Devices") distiguishes between "consistent signals" (DSR, DCD), and
> "irregular signals" (break, ring, parity error, framing error, overrun).
> The bits of "irregular signals" are set, if this error/event occurred on
> the device side and are immeadeatly unset, if the serial state notification
> was sent.
> Like other drivers of real serial ports do, just the occurence of those
> events should be counted in serial_icounter_struct (but no 1->0
> transitions).
> 
> Signed-off-by: Tobias Herzog <t-herzog@gmx.de>
> ---
>  drivers/usb/class/cdc-acm.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

Oliver, any thoughts about this patch series?

thanks,

greg k-h

Re: [PATCH 1/2] cdc-acm: do not reset notification buffer index upon urb unlinking

[Oliver Neukum]

On Sa, 2018-09-22 at 22:11 +0200, Tobias Herzog wrote:
> Resetting the write index of the notification buffer on urb unlink (e.g.
> closing a cdc-acm device from userspace) may lead to wrong interpretation
> of further received notifications, in case the index is not 0 when urb
> unlink happens (i.e. when parts of a notification already have been
> transferred). On the device side there is no "reset" of the notification
> transimission and thus we would get out of sync with the device.

Hi,

you are right, when you are describing the problem. The fix seems to me
to fix a part, albeit a large part of the issue. As far as I can tell,
the transfer could be partially succesful, meaning that the driver
loses a part in the middle of the notification.

	Regards
		Oliver

Re: [PATCH 1/2] cdc-acm: do not reset notification buffer index upon urb unlinking

[Oliver Neukum]

On Sa, 2018-09-22 at 22:11 +0200, Tobias Herzog wrote:
> Resetting the write index of the notification buffer on urb unlink (e.g.
> closing a cdc-acm device from userspace) may lead to wrong interpretation
> of further received notifications, in case the index is not 0 when urb
> unlink happens (i.e. when parts of a notification already have been
> transferred). On the device side there is no "reset" of the notification
> transimission and thus we would get out of sync with the device.
> 
> Signed-off-by: Tobias Herzog <t-herzog@gmx.de>
Acked-by: Oliver Neukum <oneukum@suse.com>

Re: [PATCH 2/2] cdc-acm: correct counting of UART states in serial state notification

[Oliver Neukum]

On Sa, 2018-09-22 at 22:11 +0200, Tobias Herzog wrote:
> The usb standard ("Universal Serial Bus Class Definitions for Communication
> Devices") distiguishes between "consistent signals" (DSR, DCD), and
> "irregular signals" (break, ring, parity error, framing error, overrun).
> The bits of "irregular signals" are set, if this error/event occurred on
> the device side and are immeadeatly unset, if the serial state notification
> was sent.
> Like other drivers of real serial ports do, just the occurence of those
> events should be counted in serial_icounter_struct (but no 1->0
> transitions).
> 
> Signed-off-by: Tobias Herzog <t-herzog@gmx.de>
Acked-by: Oliver Neukum <oneukum@suse.com>