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