| Message ID | 20230604123505.4661-1-johan@kernel.org |
|---|---|
| Headers | show |
| Series | USB: serial: return errors from break handling | expand |
On 04.06.23 14:35, Johan Hovold wrote: > @@ -1077,15 +1077,19 @@ static void pl2303_set_break(struct usb_serial_port *port, bool enable) > result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0), > BREAK_REQUEST, BREAK_REQUEST_TYPE, state, > 0, NULL, 0, 100); > - if (result) > + if (result) { > dev_err(&port->dev, "error sending break = %d\n", result); > + return result; > + } > + > + return 0; > } Hi, this code was always fishy, but I am afraid it worked by accident albeit spamming the logs. If I may quote from the kerneldoc of usb_control_msg(): * usb_control_msg - Builds a control urb, sends it off and waits for completion [..] * Return: If successful, the number of bytes transferred. Otherwise, a negative * error number. */ You need to test for < 0, not != 0 Regards Oliver
Hi Oliver, On Tue, Jun 06, 2023 at 01:13:30PM +0200, Oliver Neukum wrote: > On 04.06.23 14:35, Johan Hovold wrote: > > @@ -1077,15 +1077,19 @@ static void pl2303_set_break(struct usb_serial_port *port, bool enable) > > result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0), > > BREAK_REQUEST, BREAK_REQUEST_TYPE, state, > > 0, NULL, 0, 100); > > - if (result) > > + if (result) { > > dev_err(&port->dev, "error sending break = %d\n", result); > > + return result; > > + } > > + > > + return 0; > > } And thanks for taking a look. > this code was always fishy, but I am afraid it worked by accident albeit > spamming the logs. > If I may quote from the kerneldoc of usb_control_msg(): > > * usb_control_msg - Builds a control urb, sends it off and waits for completion > > [..] > > * Return: If successful, the number of bytes transferred. Otherwise, a negative > * error number. > */ > > You need to test for < 0, not != 0 No, the current code is just fine as there is no data stage so usb_control_msg() returns 0 or negative errno. Johan
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(-)