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

[Johan Hovold <johan@kernel.org>]

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?

What is the 'lsusb -v' for these devices?
 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  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
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.

>  	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