[PATCH] usb: cdc-acm: add PPS support

[PATCH] usb: cdc-acm: add PPS support

[Dan Drown]

This patch adds support for PPS to CDC devices. Changes to the DCD pin
are monitored and passed to the ldisc system, which is used by
pps-ldisc.

Signed-off-by: Dan Drown <dan-netdev@drown.org>
---
 drivers/usb/class/cdc-acm.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 11da5fb284d0..9b34199474c4 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -28,6 +28,7 @@
 #include <linux/serial.h>
 #include <linux/tty_driver.h>
 #include <linux/tty_flip.h>
+#include <linux/tty_ldisc.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/uaccess.h>
@@ -324,8 +325,17 @@ static void acm_process_notification(struct acm *acm, unsigned char *buf)
 
 		if (difference & USB_CDC_SERIAL_STATE_DSR)
 			acm->iocount.dsr++;
-		if (difference & USB_CDC_SERIAL_STATE_DCD)
+		if (difference & USB_CDC_SERIAL_STATE_DCD) {
+			if (acm->port.tty) {
+				struct tty_ldisc *ld = tty_ldisc_ref(acm->port.tty);
+				if (ld) {
+					if (ld->ops->dcd_change)
+						ld->ops->dcd_change(acm->port.tty, newctrl & USB_CDC_SERIAL_STATE_DCD);
+					tty_ldisc_deref(ld);
+				}
+			}
 			acm->iocount.dcd++;
+		}
 		if (newctrl & USB_CDC_SERIAL_STATE_BREAK) {
 			acm->iocount.brk++;
 			tty_insert_flip_char(&acm->port, 0, TTY_BREAK);
-- 
2.41.0

Re: [PATCH] usb: cdc-acm: add PPS support

[Oliver Neukum]

On 06.08.23 04:26, Dan Drown wrote:
> This patch adds support for PPS to CDC devices. Changes to the DCD pin
> are monitored and passed to the ldisc system, which is used by
> pps-ldisc.

Hi,

do we really want to do this with acm>read_lock held?

	Regards
		Oliver

Re: [PATCH] usb: cdc-acm: add PPS support

[Dan Drown]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

On Mon, 14 Aug 2023 14:32:57 +0200 Oliver Neukum oneukum@suse.com said
> On 06.08.23 04:26, Dan Drown wrote:
> > This patch adds support for PPS to CDC devices. Changes to the DCD pin
> > are monitored and passed to the ldisc system, which is used by
> > pps-ldisc.
>
> do we really want to do this with acm>read_lock held?

Looks like it was put there to protect the iocount changes in the surrounding code. Are your concerns around performance or deadlocks?

Re: [PATCH] usb: cdc-acm: add PPS support

[Oliver Neukum]

On 15.08.23 03:02, Dan Drown wrote:
> On Mon, 14 Aug 2023 14:32:57 +0200 Oliver Neukum oneukum@suse.com said
>> On 06.08.23 04:26, Dan Drown wrote:
>> > This patch adds support for PPS to CDC devices. Changes to the DCD pin
>> > are monitored and passed to the ldisc system, which is used by
>> > pps-ldisc.
>>
>> do we really want to do this with acm>read_lock held?
> 
> Looks like it was put there to protect the iocount changes in the surrounding code. Are your concerns around performance or deadlocks?

Hi,

the lock is there for that and so that wait_serial_change()
will read consistent counts.

The latter concerns me. We are calling potentially arbitrary code. That
you intend it for PPS doesn't change that we'll call it for
every line discipline that supports that callback.
Line disciplines are supposed to do something with tty devices,
aren't they? So what methods could they call in turn?
Something that can end in wait_serial_change()?

	Regards
		Oliver

Re: [PATCH] usb: cdc-acm: add PPS support

[Dan Drown]

On Wed, Aug 16, 2023 at 11:50:15AM +0200, Oliver Neukum wrote:
> On 15.08.23 03:02, Dan Drown wrote:
> > Looks like it was put there to protect the iocount changes in the
> > surrounding code. Are your concerns around performance or deadlocks?
> 
> the lock is there for that and so that wait_serial_change()
> will read consistent counts.
> 
> The latter concerns me. We are calling potentially arbitrary code. That
> you intend it for PPS doesn't change that we'll call it for
> every line discipline that supports that callback.
> Line disciplines are supposed to do something with tty devices,
> aren't they? So what methods could they call in turn?
> Something that can end in wait_serial_change()?

Looking at the callers of tty_register_ldisc, the only one that passes
in a dcd_change handler is the pps-ldisc. That's not to say that
couldn't change in the future.

I could move the call to dcd_change before the read lock is acquired,
would that work for you?

Re: [PATCH] usb: cdc-acm: add PPS support

[Oliver Neukum]

On 16.08.23 16:17, Dan Drown wrote:

> Looking at the callers of tty_register_ldisc, the only one that passes
> in a dcd_change handler is the pps-ldisc. That's not to say that
> couldn't change in the future.
> 
> I could move the call to dcd_change before the read lock is acquired,
> would that work for you?

Yes, better safe than sorry in that regard.

	Thank you
		Oliver

[PATCH] usb: cdc-acm: move ldisc dcd notification outside of acm's read lock

[Dan Drown]

dcd_change notification call moved outside of the acm->read_lock
to protect any future tty ldisc that calls wait_serial_change()

Signed-off-by: Dan Drown <dan-netdev@drown.org>
---
 drivers/usb/class/cdc-acm.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 9b34199474c4..dfb28c7c3069 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -319,23 +319,24 @@ static void acm_process_notification(struct acm *acm, unsigned char *buf)
 		}
 
 		difference = acm->ctrlin ^ newctrl;
+
+		if ((difference & USB_CDC_SERIAL_STATE_DCD) && acm->port.tty) {
+			struct tty_ldisc *ld = tty_ldisc_ref(acm->port.tty);
+			if (ld) {
+				if (ld->ops->dcd_change)
+					ld->ops->dcd_change(acm->port.tty, newctrl & USB_CDC_SERIAL_STATE_DCD);
+				tty_ldisc_deref(ld);
+			}
+		}
+
 		spin_lock_irqsave(&acm->read_lock, flags);
 		acm->ctrlin = newctrl;
 		acm->oldcount = acm->iocount;
 
 		if (difference & USB_CDC_SERIAL_STATE_DSR)
 			acm->iocount.dsr++;
-		if (difference & USB_CDC_SERIAL_STATE_DCD) {
-			if (acm->port.tty) {
-				struct tty_ldisc *ld = tty_ldisc_ref(acm->port.tty);
-				if (ld) {
-					if (ld->ops->dcd_change)
-						ld->ops->dcd_change(acm->port.tty, newctrl & USB_CDC_SERIAL_STATE_DCD);
-					tty_ldisc_deref(ld);
-				}
-			}
+		if (difference & USB_CDC_SERIAL_STATE_DCD)
 			acm->iocount.dcd++;
-		}
 		if (newctrl & USB_CDC_SERIAL_STATE_BREAK) {
 			acm->iocount.brk++;
 			tty_insert_flip_char(&acm->port, 0, TTY_BREAK);
-- 
2.41.0

Re: [PATCH] usb: cdc-acm: move ldisc dcd notification outside of acm's read lock

[Oliver Neukum]

On 17.08.23 03:09, Dan Drown wrote:
> dcd_change notification call moved outside of the acm->read_lock
> to protect any future tty ldisc that calls wait_serial_change()
> 
> Signed-off-by: Dan Drown <dan-netdev@drown.org>
Acked-by: Oliver Neukum <oneukum@suse.com>