| Message ID | a07922bd-4550-41d8-a7cd-8943baf6f8fb@siemens.com |
|---|---|
| State | New |
| Headers | show |
| Series | USB: serial: pl2303: account for deficits of clones | expand |
On Sun, Sep 01, 2024 at 11:11:29PM +0200, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > There are apparently incomplete clones of the HXD type chip in use. > Those return -EPIPE on GET_LINE_REQUEST and BREAK_REQUEST. Avoid > flooding the kernel log with those errors. Rather use the > line_settings cache for GET_LINE_REQUEST and signal missing support by > returning -ENOTTY from pl2303_set_break. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > drivers/usb/serial/pl2303.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c > index d93f5d584557..04cafa819390 100644 > --- a/drivers/usb/serial/pl2303.c > +++ b/drivers/usb/serial/pl2303.c > @@ -731,12 +731,13 @@ static int pl2303_get_line_request(struct usb_serial_port *port, > GET_LINE_REQUEST, GET_LINE_REQUEST_TYPE, > 0, 0, buf, 7, 100); > if (ret != 7) { > - dev_err(&port->dev, "%s - failed: %d\n", __func__, ret); > + struct pl2303_private *priv = usb_get_serial_port_data(port); > > - if (ret >= 0) > - ret = -EIO; > + dev_dbg(&port->dev, "%s - failed, falling back on cache: %d\n", > + __func__, ret); > + memcpy(buf, priv->line_settings, 7); Ugh, how is this device working in other operating systems? > > - return ret; > + return 0; > } > > dev_dbg(&port->dev, "%s - %7ph\n", __func__, buf); > @@ -1078,8 +1079,8 @@ static int pl2303_set_break(struct usb_serial_port *port, bool enable) > BREAK_REQUEST, BREAK_REQUEST_TYPE, state, > 0, NULL, 0, 100); > if (result) { > - dev_err(&port->dev, "error sending break = %d\n", result); > - return result; > + dev_dbg(&port->dev, "error sending break = %d\n", result); > + return -ENOTTY; Are you sure that ENOTTY is correct here? Why not just send back -EINVAL or something like that telling userspace that this is not allowed for this device? thanks, greg k-h
On 02.09.24 08:06, Greg Kroah-Hartman wrote: > On Sun, Sep 01, 2024 at 11:11:29PM +0200, Jan Kiszka wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> There are apparently incomplete clones of the HXD type chip in use. >> Those return -EPIPE on GET_LINE_REQUEST and BREAK_REQUEST. Avoid >> flooding the kernel log with those errors. Rather use the >> line_settings cache for GET_LINE_REQUEST and signal missing support by >> returning -ENOTTY from pl2303_set_break. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> --- >> drivers/usb/serial/pl2303.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c >> index d93f5d584557..04cafa819390 100644 >> --- a/drivers/usb/serial/pl2303.c >> +++ b/drivers/usb/serial/pl2303.c >> @@ -731,12 +731,13 @@ static int pl2303_get_line_request(struct usb_serial_port *port, >> GET_LINE_REQUEST, GET_LINE_REQUEST_TYPE, >> 0, 0, buf, 7, 100); >> if (ret != 7) { >> - dev_err(&port->dev, "%s - failed: %d\n", __func__, ret); >> + struct pl2303_private *priv = usb_get_serial_port_data(port); >> >> - if (ret >= 0) >> - ret = -EIO; >> + dev_dbg(&port->dev, "%s - failed, falling back on cache: %d\n", >> + __func__, ret); >> + memcpy(buf, priv->line_settings, 7); > > Ugh, how is this device working in other operating systems? > I don't know. Also, the last downstream driver Prolific posted [1] didn't point to this specialty. I opened a few of those adapter cables (some received from [2], others from a different source), and the chips were not labeled (in contrast to the picture from [2]). So I'm suspecting clones to be in play. Reminds me of the quest for sane WIFI USB adapters. >> >> - return ret; >> + return 0; >> } >> >> dev_dbg(&port->dev, "%s - %7ph\n", __func__, buf); >> @@ -1078,8 +1079,8 @@ static int pl2303_set_break(struct usb_serial_port *port, bool enable) >> BREAK_REQUEST, BREAK_REQUEST_TYPE, state, >> 0, NULL, 0, 100); >> if (result) { >> - dev_err(&port->dev, "error sending break = %d\n", result); >> - return result; >> + dev_dbg(&port->dev, "error sending break = %d\n", result); >> + return -ENOTTY; > > Are you sure that ENOTTY is correct here? Why not just send back > -EINVAL or something like that telling userspace that this is not > allowed for this device? I was copying from serial_break() which now returns -ENOTTY if the handler is NULL. If you prefer -EINVAL, I can change. Just looking for a consistent code. Jan [1] https://prolificusa.com/wp-content/uploads/2021/01/PL2303G_Linux_Driver_v1.0.6.zip [2] https://www.berrybase.de/en/usb-ttl/uart/rs232-adapter-mit-pl2303hx-chipsatz
On Mon, Sep 02, 2024 at 08:28:42AM +0200, Jan Kiszka wrote: > On 02.09.24 08:06, Greg Kroah-Hartman wrote: > > On Sun, Sep 01, 2024 at 11:11:29PM +0200, Jan Kiszka wrote: > >> From: Jan Kiszka <jan.kiszka@siemens.com> > >> > >> There are apparently incomplete clones of the HXD type chip in use. > >> Those return -EPIPE on GET_LINE_REQUEST and BREAK_REQUEST. Avoid > >> flooding the kernel log with those errors. Rather use the > >> line_settings cache for GET_LINE_REQUEST and signal missing support by > >> returning -ENOTTY from pl2303_set_break. > >> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >> --- > >> drivers/usb/serial/pl2303.c | 13 +++++++------ > >> 1 file changed, 7 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c > >> index d93f5d584557..04cafa819390 100644 > >> --- a/drivers/usb/serial/pl2303.c > >> +++ b/drivers/usb/serial/pl2303.c > >> @@ -731,12 +731,13 @@ static int pl2303_get_line_request(struct usb_serial_port *port, > >> GET_LINE_REQUEST, GET_LINE_REQUEST_TYPE, > >> 0, 0, buf, 7, 100); > >> if (ret != 7) { > >> - dev_err(&port->dev, "%s - failed: %d\n", __func__, ret); > >> + struct pl2303_private *priv = usb_get_serial_port_data(port); > >> > >> - if (ret >= 0) > >> - ret = -EIO; > >> + dev_dbg(&port->dev, "%s - failed, falling back on cache: %d\n", > >> + __func__, ret); > >> + memcpy(buf, priv->line_settings, 7); > > > > Ugh, how is this device working in other operating systems? > > > > I don't know. Also, the last downstream driver Prolific posted [1] > didn't point to this specialty. Given that prolific's windows driver is known to explicitly attempt to disable clone devices, I would be amazed if it supported it at all :) > I opened a few of those adapter cables (some received from [2], others > from a different source), and the chips were not labeled (in contrast to > the picture from [2]). So I'm suspecting clones to be in play. Reminds > me of the quest for sane WIFI USB adapters. This chip has had lots of different cloned devices over the years, which is pretty odd given that the thing is so cheap to start with. Anyway, we live with it and move on... > >> > >> - return ret; > >> + return 0; > >> } > >> > >> dev_dbg(&port->dev, "%s - %7ph\n", __func__, buf); > >> @@ -1078,8 +1079,8 @@ static int pl2303_set_break(struct usb_serial_port *port, bool enable) > >> BREAK_REQUEST, BREAK_REQUEST_TYPE, state, > >> 0, NULL, 0, 100); > >> if (result) { > >> - dev_err(&port->dev, "error sending break = %d\n", result); > >> - return result; > >> + dev_dbg(&port->dev, "error sending break = %d\n", result); > >> + return -ENOTTY; > > > > Are you sure that ENOTTY is correct here? Why not just send back > > -EINVAL or something like that telling userspace that this is not > > allowed for this device? > > I was copying from serial_break() which now returns -ENOTTY if the > handler is NULL. If you prefer -EINVAL, I can change. Just looking for a > consistent code. Ah, yeah, that's tricky. I'll let Johan pick one :) thanks, greg k-h
On Sun, Sep 01, 2024 at 11:11:29PM +0200, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > There are apparently incomplete clones of the HXD type chip in use. > Those return -EPIPE on GET_LINE_REQUEST and BREAK_REQUEST. Avoid > flooding the kernel log with those errors. Rather use the > line_settings cache for GET_LINE_REQUEST and signal missing support by > returning -ENOTTY from pl2303_set_break. This looks mostly fine to me, but could you please try to make these changes only apply to the clones or, since those probably cannot be detected reliably, HXD? What is the 'lsusb -v' for these devices? > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > drivers/usb/serial/pl2303.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c > index d93f5d584557..04cafa819390 100644 > --- a/drivers/usb/serial/pl2303.c > +++ b/drivers/usb/serial/pl2303.c > @@ -731,12 +731,13 @@ static int pl2303_get_line_request(struct usb_serial_port *port, > GET_LINE_REQUEST, GET_LINE_REQUEST_TYPE, > 0, 0, buf, 7, 100); > if (ret != 7) { > - dev_err(&port->dev, "%s - failed: %d\n", __func__, ret); > + struct pl2303_private *priv = usb_get_serial_port_data(port); > > - if (ret >= 0) > - ret = -EIO; > + dev_dbg(&port->dev, "%s - failed, falling back on cache: %d\n", > + __func__, ret); > + memcpy(buf, priv->line_settings, 7); > > - return ret; > + return 0; > } Looking at the driver, it seems like we could move the only call to this function to probe, and perhaps then an error message for cloned devices is not such a big deal. But even that could be suppressed based on the type. Or we could use this call failing to flag the device as a likely clone and use that flag to also skip break signalling completely. > dev_dbg(&port->dev, "%s - %7ph\n", __func__, buf); > @@ -1078,8 +1079,8 @@ static int pl2303_set_break(struct usb_serial_port *port, bool enable) > BREAK_REQUEST, BREAK_REQUEST_TYPE, state, > 0, NULL, 0, 100); > if (result) { > - dev_err(&port->dev, "error sending break = %d\n", result); > - return result; > + dev_dbg(&port->dev, "error sending break = %d\n", result); > + return -ENOTTY; > } And similar here, the log level could depend on the type, unless the function should just bail out early for clones. > > return 0; Johan
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c index d93f5d584557..04cafa819390 100644 --- a/drivers/usb/serial/pl2303.c +++ b/drivers/usb/serial/pl2303.c @@ -731,12 +731,13 @@ static int pl2303_get_line_request(struct usb_serial_port *port, GET_LINE_REQUEST, GET_LINE_REQUEST_TYPE, 0, 0, buf, 7, 100); if (ret != 7) { - dev_err(&port->dev, "%s - failed: %d\n", __func__, ret); + struct pl2303_private *priv = usb_get_serial_port_data(port); - if (ret >= 0) - ret = -EIO; + dev_dbg(&port->dev, "%s - failed, falling back on cache: %d\n", + __func__, ret); + memcpy(buf, priv->line_settings, 7); - return ret; + return 0; } dev_dbg(&port->dev, "%s - %7ph\n", __func__, buf); @@ -1078,8 +1079,8 @@ static int pl2303_set_break(struct usb_serial_port *port, bool enable) BREAK_REQUEST, BREAK_REQUEST_TYPE, state, 0, NULL, 0, 100); if (result) { - dev_err(&port->dev, "error sending break = %d\n", result); - return result; + dev_dbg(&port->dev, "error sending break = %d\n", result); + return -ENOTTY; } return 0;