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

[Jan Kiszka <jan.kiszka@siemens.com>]

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