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@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.
>>
>> 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);
> 
> 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