| Message ID | 20230604123505.4661-1-johan@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | USB: serial: return errors from break handling | expand |
On Sun, Jun 04, 2023 at 02:35:02PM +0200, Johan Hovold wrote: > This series starts returning errors from break handling and also uses > that mechanism to report to user space when break signalling is not > supported (e.g. when device or driver support is missing). > > Note that the tty layer currently returns early but without reporting > errors when a tty driver does not support break signalling. The intent > expressed in commit 9e98966c7bb9 ("tty: rework break handling") from > 2008 appears to be to allow missing support to be reported to user > space however. This worked as expected for me. Tested-by: Corey Minyard <cminyard@mvista.com> > > Johan > > > Changes in v2 > - fix return of potentially uninitialised status variable in > io_edgeport as reported by kernel test robot <lkp@intel.com> and Dan > Carpenter: > > https://lore.kernel.org/all/202306031014.qzAY3uQ6-lkp@intel.com/ > > > Johan Hovold (3): > USB: serial: return errors from break handling > USB: serial: cp210x: disable break signalling on CP2105 SCI > USB: serial: report unsupported break signalling > > drivers/usb/serial/ark3116.c | 7 +++-- > drivers/usb/serial/belkin_sa.c | 12 ++++++--- > drivers/usb/serial/ch341.c | 37 +++++++++++++++++---------- > drivers/usb/serial/cp210x.c | 14 +++++++--- > drivers/usb/serial/digi_acceleport.c | 7 ++--- > drivers/usb/serial/f81232.c | 4 ++- > drivers/usb/serial/f81534.c | 4 ++- > drivers/usb/serial/ftdi_sio.c | 10 +++++--- > drivers/usb/serial/io_edgeport.c | 6 +++-- > drivers/usb/serial/io_ti.c | 9 +++++-- > drivers/usb/serial/keyspan.c | 5 +++- > drivers/usb/serial/keyspan_pda.c | 8 ++++-- > drivers/usb/serial/mct_u232.c | 6 ++--- > drivers/usb/serial/mos7720.c | 9 ++++--- > drivers/usb/serial/mos7840.c | 7 ++--- > drivers/usb/serial/mxuport.c | 6 ++--- > drivers/usb/serial/pl2303.c | 14 ++++++---- > drivers/usb/serial/quatech2.c | 8 ++++-- > drivers/usb/serial/ti_usb_3410_5052.c | 10 +++++--- > drivers/usb/serial/upd78f0730.c | 7 +++-- > drivers/usb/serial/usb-serial.c | 4 +-- > drivers/usb/serial/usb_debug.c | 13 +++++++--- > drivers/usb/serial/whiteheat.c | 7 ++--- > drivers/usb/serial/xr_serial.c | 4 +-- > include/linux/usb/serial.h | 2 +- > 25 files changed, 147 insertions(+), 73 deletions(-) > > -- > 2.39.3 >
On 04.06.23 14:35, Johan Hovold wrote: > This series starts returning errors from break handling and also uses > that mechanism to report to user space when break signalling is not > supported (e.g. when device or driver support is missing). Hi, do you eventually want this to be done for all serial devices? That is does cdc-acm need something like this patch? Regards Oliver
On Tue, Jun 06, 2023 at 01:18:19PM +0200, Oliver Neukum wrote: > On 04.06.23 14:35, Johan Hovold wrote: > > This series starts returning errors from break handling and also uses > > that mechanism to report to user space when break signalling is not > > supported (e.g. when device or driver support is missing). > do you eventually want this to be done for all serial devices? > That is does cdc-acm need something like this patch? Looks good to me. If this turns out to confuse userspace we may have to turn that -ENOTTY into 0 in the tty layer, but we can still use it to avoid the unnecessary wait to "disable" the break state. > From 16430d9f109f904b2bfbac6e43a939209b6c4bc7 Mon Sep 17 00:00:00 2001 > From: Oliver Neukum <oneukum@suse.com> > Date: Tue, 6 Jun 2023 12:57:00 +0200 > Subject: [PATCH] usb: cdc-acm: return correct error code on unsupported break > > Return -ENOTTY if the device says that it doesn't support break > so that the upper layers get error reporting right. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> Acked-by: Johan Hovold <johan@kernel.org> > --- > drivers/usb/class/cdc-acm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c > index 11da5fb284d0..7751f5728716 100644 > --- a/drivers/usb/class/cdc-acm.c > +++ b/drivers/usb/class/cdc-acm.c > @@ -892,6 +892,9 @@ static int acm_tty_break_ctl(struct tty_struct *tty, int state) > struct acm *acm = tty->driver_data; > int retval; > > + if (!(acm->ctrl_caps & USB_CDC_CAP_BRK)) > + return -ENOTTY; > + > retval = acm_send_break(acm, state ? 0xffff : 0); > if (retval < 0) > dev_dbg(&acm->control->dev,
On Tue, Jun 06, 2023 at 01:39:03PM +0200, Johan Hovold wrote: > On Tue, Jun 06, 2023 at 01:18:19PM +0200, Oliver Neukum wrote: > > On 04.06.23 14:35, Johan Hovold wrote: > > > This series starts returning errors from break handling and also uses > > > that mechanism to report to user space when break signalling is not > > > supported (e.g. when device or driver support is missing). > > > do you eventually want this to be done for all serial devices? > > That is does cdc-acm need something like this patch? > > Looks good to me. If this turns out to confuse userspace we may have to > turn that -ENOTTY into 0 in the tty layer, but we can still use it to > avoid the unnecessary wait to "disable" the break state. > > > From 16430d9f109f904b2bfbac6e43a939209b6c4bc7 Mon Sep 17 00:00:00 2001 > > From: Oliver Neukum <oneukum@suse.com> > > Date: Tue, 6 Jun 2023 12:57:00 +0200 > > Subject: [PATCH] usb: cdc-acm: return correct error code on unsupported break > > > > Return -ENOTTY if the device says that it doesn't support break > > so that the upper layers get error reporting right. Yeah, when I asked for this, I didn't realize that no devices did this, I thought it was a one-off with this device. There is a distinct possibility that this will break userland. This sounds like a good plan. -corey > > > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > > Acked-by: Johan Hovold <johan@kernel.org> > > > --- > > drivers/usb/class/cdc-acm.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c > > index 11da5fb284d0..7751f5728716 100644 > > --- a/drivers/usb/class/cdc-acm.c > > +++ b/drivers/usb/class/cdc-acm.c > > @@ -892,6 +892,9 @@ static int acm_tty_break_ctl(struct tty_struct *tty, int state) > > struct acm *acm = tty->driver_data; > > int retval; > > > > + if (!(acm->ctrl_caps & USB_CDC_CAP_BRK)) > > + return -ENOTTY; > > + > > retval = acm_send_break(acm, state ? 0xffff : 0); > > if (retval < 0) > > dev_dbg(&acm->control->dev,
This series starts returning errors from break handling and also uses that mechanism to report to user space when break signalling is not supported (e.g. when device or driver support is missing). Note that the tty layer currently returns early but without reporting errors when a tty driver does not support break signalling. The intent expressed in commit 9e98966c7bb9 ("tty: rework break handling") from 2008 appears to be to allow missing support to be reported to user space however. Johan Changes in v2 - fix return of potentially uninitialised status variable in io_edgeport as reported by kernel test robot <lkp@intel.com> and Dan Carpenter: https://lore.kernel.org/all/202306031014.qzAY3uQ6-lkp@intel.com/ Johan Hovold (3): USB: serial: return errors from break handling USB: serial: cp210x: disable break signalling on CP2105 SCI USB: serial: report unsupported break signalling drivers/usb/serial/ark3116.c | 7 +++-- drivers/usb/serial/belkin_sa.c | 12 ++++++--- drivers/usb/serial/ch341.c | 37 +++++++++++++++++---------- drivers/usb/serial/cp210x.c | 14 +++++++--- drivers/usb/serial/digi_acceleport.c | 7 ++--- drivers/usb/serial/f81232.c | 4 ++- drivers/usb/serial/f81534.c | 4 ++- drivers/usb/serial/ftdi_sio.c | 10 +++++--- drivers/usb/serial/io_edgeport.c | 6 +++-- drivers/usb/serial/io_ti.c | 9 +++++-- drivers/usb/serial/keyspan.c | 5 +++- drivers/usb/serial/keyspan_pda.c | 8 ++++-- drivers/usb/serial/mct_u232.c | 6 ++--- drivers/usb/serial/mos7720.c | 9 ++++--- drivers/usb/serial/mos7840.c | 7 ++--- drivers/usb/serial/mxuport.c | 6 ++--- drivers/usb/serial/pl2303.c | 14 ++++++---- drivers/usb/serial/quatech2.c | 8 ++++-- drivers/usb/serial/ti_usb_3410_5052.c | 10 +++++--- drivers/usb/serial/upd78f0730.c | 7 +++-- drivers/usb/serial/usb-serial.c | 4 +-- drivers/usb/serial/usb_debug.c | 13 +++++++--- drivers/usb/serial/whiteheat.c | 7 ++--- drivers/usb/serial/xr_serial.c | 4 +-- include/linux/usb/serial.h | 2 +- 25 files changed, 147 insertions(+), 73 deletions(-)