| Message ID | ZM8ExV6bAvJtIA1d@vps3.drown.org |
|---|---|
| State | New |
| Headers | show |
| Series | usb: cdc-acm: add PPS support | expand |
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
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?
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
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?
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
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);
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(-)