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
