[PATCH 0/2] USB: serial: ch341: fixes for older devices

[PATCH 0/2] USB: serial: ch341: fixes for older devices

[Johan Hovold]

Jonathan Woithe reported that there was a regression for some older
ch341 devices in 4.10. Turns out there were two of them.

Jonathan, feel free to reply to this mail with a Tested-by tag. You've
already tested these changes during development even if I've since split
the patch in two. And thanks again for your careful testing while
tracking this down.

Johan


Johan Hovold (2):
  USB: serial: ch341: fix lost character on LCR updates
  USB: serial: ch341: fix disabled rx timer on older devices

 drivers/usb/serial/ch341.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

-- 
2.35.1

[PATCH 1/2] USB: serial: ch341: fix lost character on LCR updates

[Johan Hovold]

Disable LCR updates for pre-0x30 devices which use a different (unknown)
protocol for line control and where the current register write causes
the next received character to be lost.

Note that updating LCR using the INIT command has no effect on these
devices either.

Reported-by: Jonathan Woithe <jwoithe@just42.net>
Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@marvin.atrad.com.au
Fixes: 4e46c410e050 ("USB: serial: ch341: reinitialize chip on reconfiguration")
Fixes: 55fa15b5987d ("USB: serial: ch341: fix baud rate and line-control handling")
Cc: stable@vger.kernel.org      # 4.10
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ch341.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 2798fca71261..2bcce172355b 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -97,7 +97,10 @@ struct ch341_private {
 	u8 mcr;
 	u8 msr;
 	u8 lcr;
+
 	unsigned long quirks;
+	u8 version;
+
 	unsigned long break_end;
 };
 
@@ -265,6 +268,9 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	 * (stop bits, parity and word length). Version 0x30 and above use
 	 * CH341_REG_LCR only and CH341_REG_LCR2 is always set to zero.
 	 */
+	if (priv->version < 0x30)
+		return 0;
+
 	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
 			      CH341_REG_LCR2 << 8 | CH341_REG_LCR, lcr);
 	if (r)
@@ -308,7 +314,9 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
 	r = ch341_control_in(dev, CH341_REQ_READ_VERSION, 0, 0, buffer, size);
 	if (r)
 		return r;
-	dev_dbg(&dev->dev, "Chip version: 0x%02x\n", buffer[0]);
+
+	priv->version = buffer[0];
+	dev_dbg(&dev->dev, "Chip version: 0x%02x\n", priv->version);
 
 	r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT, 0, 0);
 	if (r < 0)
-- 
2.35.1

Re: [PATCH 1/2] USB: serial: ch341: fix lost character on LCR updates

[Jiri Slaby]

On 31. 08. 22, 10:15, Johan Hovold wrote:
> Disable LCR updates for pre-0x30 devices which use a different (unknown)
> protocol for line control and where the current register write causes
> the next received character to be lost.
> 
> Note that updating LCR using the INIT command has no effect on these
> devices either.
> 
> Reported-by: Jonathan Woithe <jwoithe@just42.net>
> Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@marvin.atrad.com.au
> Fixes: 4e46c410e050 ("USB: serial: ch341: reinitialize chip on reconfiguration")
> Fixes: 55fa15b5987d ("USB: serial: ch341: fix baud rate and line-control handling")
> Cc: stable@vger.kernel.org      # 4.10
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/usb/serial/ch341.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 2798fca71261..2bcce172355b 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -97,7 +97,10 @@ struct ch341_private {
>   	u8 mcr;
>   	u8 msr;
>   	u8 lcr;
> +
>   	unsigned long quirks;
> +	u8 version;

Could you move version above quirks? That would not create another 
7-byte padding in here. Actually it would not make ch341_private larger 
on 64bit at all, if I am looking correctly.

Other than that, looks good.

thanks,
-- 
js
suse labs

Re: [PATCH 1/2] USB: serial: ch341: fix lost character on LCR updates

[Johan Hovold]

On Wed, Aug 31, 2022 at 10:36:18AM +0200, Jiri Slaby wrote:
> On 31. 08. 22, 10:15, Johan Hovold wrote:
> > Disable LCR updates for pre-0x30 devices which use a different (unknown)
> > protocol for line control and where the current register write causes
> > the next received character to be lost.
> > 
> > Note that updating LCR using the INIT command has no effect on these
> > devices either.
> > 
> > Reported-by: Jonathan Woithe <jwoithe@just42.net>
> > Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@marvin.atrad.com.au
> > Fixes: 4e46c410e050 ("USB: serial: ch341: reinitialize chip on reconfiguration")
> > Fixes: 55fa15b5987d ("USB: serial: ch341: fix baud rate and line-control handling")
> > Cc: stable@vger.kernel.org      # 4.10
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >   drivers/usb/serial/ch341.c | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> > index 2798fca71261..2bcce172355b 100644
> > --- a/drivers/usb/serial/ch341.c
> > +++ b/drivers/usb/serial/ch341.c
> > @@ -97,7 +97,10 @@ struct ch341_private {
> >   	u8 mcr;
> >   	u8 msr;
> >   	u8 lcr;
> > +
> >   	unsigned long quirks;
> > +	u8 version;
> 
> Could you move version above quirks? That would not create another 
> 7-byte padding in here. Actually it would not make ch341_private larger 
> on 64bit at all, if I am looking correctly.

No, I added it after quirks on purpose as it isn't protected by the
spinlock and doesn't change during runtime like the shadow registers.

And I really don't care about saving 8 bytes on 64-bit. :)

Johan

[PATCH 2/2] USB: serial: ch341: fix disabled rx timer on older devices

[Johan Hovold]

At least one older CH341 appears to have the RX timer enable bit
inverted so that setting it disables the RX timer and prevents the FIFO
from emptying until it is full.

Only set the RX timer enable bit for devices with version newer than
0x27 (even though this probably affects all pre-0x30 devices).

Reported-by: Jonathan Woithe <jwoithe@just42.net>
Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@marvin.atrad.com.au
Fixes: 4e46c410e050 ("USB: serial: ch341: reinitialize chip on reconfiguration")
Cc: stable@vger.kernel.org      # 4.10
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ch341.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 2bcce172355b..af01a462cc43 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -253,8 +253,12 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	/*
 	 * CH341A buffers data until a full endpoint-size packet (32 bytes)
 	 * has been received unless bit 7 is set.
+	 *
+	 * At least one device with version 0x27 appears to have this bit
+	 * inverted.
 	 */
-	val |= BIT(7);
+	if (priv->version > 0x27)
+		val |= BIT(7);
 
 	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
 			      CH341_REG_DIVISOR << 8 | CH341_REG_PRESCALER,
-- 
2.35.1

Re: [PATCH 0/2] USB: serial: ch341: fixes for older devices

[Jonathan Woithe]

On Wed, Aug 31, 2022 at 10:15:23AM +0200, Johan Hovold wrote:
> Jonathan Woithe reported that there was a regression for some older
> ch341 devices in 4.10. Turns out there were two of them.
> 
> Jonathan, feel free to reply to this mail with a Tested-by tag. You've
> already tested these changes during development even if I've since split
> the patch in two. And thanks again for your careful testing while
> tracking this down.

Tested-by: Jonathan Woithe <jwoithe@just42.net>

> Johan Hovold (2):
>   USB: serial: ch341: fix lost character on LCR updates
>   USB: serial: ch341: fix disabled rx timer on older devices
> 
>  drivers/usb/serial/ch341.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> -- 
> 2.35.1