usb: cdc-acm: add PPS support
Subsystems (2)
PATCH 1/1 usb: cdc-acm: add PPS support
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.
--- drivers/usb/class/cdc-acm.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
drivers/usb/class/cdc-acm.c +11 -1
@@ -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 );
Signature · 1 line
2.41.0
Oliver Neukum
maintainerQuoted 3 lines · 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.
Hi,
do we really want to do this with acm>read_lock held?
Regards
Oliver
On Mon, 14 Aug 2023 14:32:57 +0200 Oliver Neukum oneukum@suse=2Ecom said
Quoted 6 lines · Dan Drown
Quoted 3 lines
This patch adds support for PPS to CDC devices=2E Changes to the DCD pin are monitored and passed to the ldisc system, which is used by pps-ldisc=2E
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 surroundi= ng code=2E Are your concerns around performance or deadlocks?
Oliver Neukum
maintainerQuoted 9 lines · Dan Drown
On Mon, 14 Aug 2023 14:32:57 +0200 Oliver Neukum [email protected] said
Quoted 6 lines · Dan Drown
Quoted 3 lines
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
Quoted 7 lines · Oliver Neukum
Quoted 2 lines · Dan Drown
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.
Quoted 6 lines
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 inwait_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?
Oliver Neukum
maintainerQuoted 6 lines · Dan Drown
Looking at the callers of
tty_register_ldisc, the only one that passes in adcd_changehandler is the pps-ldisc. That's not to say that couldn't change in the future.I could move the call to
dcd_changebefore the read lock is acquired, would that work for you?
Yes, better safe than sorry in that regard.
Thank you
Oliver
PATCH 1/1 usb: cdc-acm: move ldisc dcd notification outside of acm's read lock
dcd_change notification call moved outside of the acm->read_lock to protect any future tty ldisc that calls wait_serial_change()
--- drivers/usb/class/cdc-acm.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
drivers/usb/class/cdc-acm.c +11 -10
@@ -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 );
Signature · 1 line
2.41.0
Re: [PATCH] usb: cdc-acm: move ldisc dcd notification outside of acm's read lock
Quoted 4 lines · Dan Drown
dcd_changenotification call moved outside of theacm->read_lockto protect any future tty ldisc that callswait_serial_change()Signed-off-by Dan Drown <[email protected]>