[PATCH] USB: serial: pl2303: account for deficits of clones

[PATCH] USB: serial: pl2303: account for deficits of clones

[Jan Kiszka]

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);
 
-		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;
-- 
2.43.0

Re: [PATCH] USB: serial: pl2303: account for deficits of clones

[Greg Kroah-Hartman]

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

Re: [PATCH] USB: serial: pl2303: account for deficits of clones

[Jan Kiszka]

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

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH] USB: serial: pl2303: account for deficits of clones

[Greg Kroah-Hartman]

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

Re: [PATCH] USB: serial: pl2303: account for deficits of clones

[Johan Hovold]

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

Re: [PATCH] USB: serial: pl2303: account for deficits of clones

[Jan Kiszka]

On 09.09.24 14:32, Johan Hovold 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.
> 
> 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?
> 

OK. Is there a way to switch between dev_err and dev_dbg without
duplicating the line?

> What is the 'lsusb -v' for these devices?

Bus 001 Device 019: ID 067b:2303 Prolific Technology, Inc. PL2303 Serial
Port / Mobile Action MA-8910P
Couldn't open device, some information will be missing
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               1.10
  bDeviceClass            0
  bDeviceSubClass         0
  bDeviceProtocol         0
  bMaxPacketSize0        64
  idVendor           0x067b Prolific Technology, Inc.
  idProduct          0x2303 PL2303 Serial Port / Mobile Action MA-8910P
  bcdDevice            4.00
  iManufacturer           1 Prolific Technology Inc.
  iProduct                2 USB-Serial Controller D
  iSerial                 0
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength       0x0027
    bNumInterfaces          1
    bConfigurationValue     1
    iConfiguration          0
    bmAttributes         0x80
      (Bus Powered)
    MaxPower              100mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           3
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass      0
      bInterfaceProtocol      0
      iInterface              0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x000a  1x 10 bytes
        bInterval               1
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x02  EP 2 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               0


Unfortunately, I don't have a HXD device around that behaves and is
likely no clone, thus have no reference.

>  
>> 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

Nope, this is called on every pl2303_set_termios, thus is even under
user control.

> 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.

Oh, you meant the function below? But that is also in user hands (as well).

Flagging after the first call is theoretically possible - but is it
worth the related effort? I doubt that a bit.

Jan

> 
>>  	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

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH] USB: serial: pl2303: account for deficits of clones

[Johan Hovold]

On Mon, Sep 09, 2024 at 02:52:25PM +0200, Jan Kiszka wrote:
> On 09.09.24 14:32, Johan Hovold 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.
> > 
> > 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?
> > 
> 
> OK. Is there a way to switch between dev_err and dev_dbg without
> duplicating the line?

Perhaps, did you check? But I think we should go with adding a flag and
suppressing the known broken calls completely post probe.
 
> > What is the 'lsusb -v' for these devices?
> 
> Bus 001 Device 019: ID 067b:2303 Prolific Technology, Inc. PL2303 Serial
> Port / Mobile Action MA-8910P
> Couldn't open device, some information will be missing
> Device Descriptor:
>   bLength                18
>   bDescriptorType         1
>   bcdUSB               1.10
>   bDeviceClass            0
>   bDeviceSubClass         0
>   bDeviceProtocol         0
>   bMaxPacketSize0        64
>   idVendor           0x067b Prolific Technology, Inc.
>   idProduct          0x2303 PL2303 Serial Port / Mobile Action MA-8910P
>   bcdDevice            4.00

So this would be detected as an HXD by the current driver. Not sure what
else could be used except possibly the product string and the fact that
these requests fail to determine if it's a legit HXD.

> >> @@ -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
> 
> Nope, this is called on every pl2303_set_termios, thus is even under
> user control.

What do you mean by "nope"? I'm saying that it looks like it may be
possible to move this call to probe() given how it is used currently.

Or you can just add an additional call to probe() without the dev_err()
and use that for clone detection.

> > 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.
> 
> Oh, you meant the function below? But that is also in user hands (as well).
> 
> Flagging after the first call is theoretically possible - but is it
> worth the related effort? I doubt that a bit.

I'm saying that we can issue the get_line_settings request during
probe() (for HXD) and if it fails, we treat the device as a clone and
skip the requests that are not supported completely.

Johan

Re: [PATCH] USB: serial: pl2303: account for deficits of clones

[Jan Kiszka]

On 09.09.24 15:43, Johan Hovold wrote:
> On Mon, Sep 09, 2024 at 02:52:25PM +0200, Jan Kiszka wrote:
>> On 09.09.24 14:32, Johan Hovold 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.
>>>
>>> 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?
>>>
>>
>> OK. Is there a way to switch between dev_err and dev_dbg without
>> duplicating the line?
> 
> Perhaps, did you check? But I think we should go with adding a flag and
> suppressing the known broken calls completely post probe.
>  
>>> What is the 'lsusb -v' for these devices?
>>
>> Bus 001 Device 019: ID 067b:2303 Prolific Technology, Inc. PL2303 Serial
>> Port / Mobile Action MA-8910P
>> Couldn't open device, some information will be missing
>> Device Descriptor:
>>   bLength                18
>>   bDescriptorType         1
>>   bcdUSB               1.10
>>   bDeviceClass            0
>>   bDeviceSubClass         0
>>   bDeviceProtocol         0
>>   bMaxPacketSize0        64
>>   idVendor           0x067b Prolific Technology, Inc.
>>   idProduct          0x2303 PL2303 Serial Port / Mobile Action MA-8910P
>>   bcdDevice            4.00
> 
> So this would be detected as an HXD by the current driver. Not sure what
> else could be used except possibly the product string and the fact that
> these requests fail to determine if it's a legit HXD.
> 
>>>> @@ -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
>>
>> Nope, this is called on every pl2303_set_termios, thus is even under
>> user control.
> 
> What do you mean by "nope"? I'm saying that it looks like it may be
> possible to move this call to probe() given how it is used currently.
> 
> Or you can just add an additional call to probe() without the dev_err()
> and use that for clone detection.
> 
>>> 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.
>>
>> Oh, you meant the function below? But that is also in user hands (as well).
>>
>> Flagging after the first call is theoretically possible - but is it
>> worth the related effort? I doubt that a bit.
> 
> I'm saying that we can issue the get_line_settings request during
> probe() (for HXD) and if it fails, we treat the device as a clone and
> skip the requests that are not supported completely.

OK, now I get the plan. Let me see...

Jan

-- 
Siemens AG, Technology
Linux Expert Center