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