[Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Jonathan Woithe]

Hi all

For many years I have been using a CH341 USB-serial converter (VID:PID
4348:5523) to drive a microcontroller programming dongle.  This was under
kernel 4.4.14.  When I recently upgraded the machine to a 5.15.27 kernel the
programmer stopped working, reporting timeouts.  Using a loopback cable and
minicom I discovered that under 5.15.27 data was only delivered back to
minicom in blocks of 32 characters.  This explains why the programming
software reported timeouts.  It seems that either outgoing data is blocked
until 32 bytes are ready for transmission, or receive data is only reported
in blocks of 32 bytes.

Under 4.4.14 (and all kernels prior to 4.10.0), individual characters are
echoed back by minicom as soon as they hare pressed on the keyboard.

As far as I can tell, the regression affects all kernels since 4.10.0.

I have done a git bisect which identified the following commit as the source
of the problem.

commit 55fa15b5987db22b4f35d3f0798928c126be5f1c
Author: Johan Hovold <johan@kernel.org>
Date:   Fri Jan 6 19:15:16 2017 +0100

    USB: serial: ch341: fix baud rate and line-control handling

    Revert to using direct register writes to set the divisor and
    line-control registers.

    A recent change switched to using the init vendor command to update
    these registers, something which also enabled support for CH341A
    devices. It turns out that simply setting bit 7 in the divisor register
    is sufficient to support CH341A and specifically prevent data from being
    buffered until a full endpoint-size packet (32 bytes) has been received.

    Using the init command also had the side-effect of temporarily
    deasserting the DTR/RTS signals on every termios change (including
    initialisation on open) something which for example could cause problems
    in setups where DTR is used to trigger a reset.

    Fixes: 4e46c410e050 ("USB: serial: ch341: reinitialize chip on
    reconfiguration")
    Signed-off-by: Johan Hovold <johan@kernel.org>


It would be great if this regression could be addressed.  At present I must
boot a pre-4.10 kernel whenever I need to use the programming dongle with
this converter.

Please let me know if there is anything I can do to help resolve the
problem.

Regards
  jonathan

Re: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Greg KH]

On Tue, Jul 12, 2022 at 09:29:57PM +0930, Jonathan Woithe wrote:
> Hi all
> 
> For many years I have been using a CH341 USB-serial converter (VID:PID
> 4348:5523) to drive a microcontroller programming dongle.  This was under
> kernel 4.4.14.  When I recently upgraded the machine to a 5.15.27 kernel the
> programmer stopped working, reporting timeouts.  Using a loopback cable and
> minicom I discovered that under 5.15.27 data was only delivered back to
> minicom in blocks of 32 characters.  This explains why the programming
> software reported timeouts.  It seems that either outgoing data is blocked
> until 32 bytes are ready for transmission, or receive data is only reported
> in blocks of 32 bytes.
> 
> Under 4.4.14 (and all kernels prior to 4.10.0), individual characters are
> echoed back by minicom as soon as they hare pressed on the keyboard.
> 
> As far as I can tell, the regression affects all kernels since 4.10.0.
> 
> I have done a git bisect which identified the following commit as the source
> of the problem.
> 
> commit 55fa15b5987db22b4f35d3f0798928c126be5f1c
> Author: Johan Hovold <johan@kernel.org>
> Date:   Fri Jan 6 19:15:16 2017 +0100

Please always cc: the developer who wrote a commit that you have
questions about, so that they are sure to see it, otherwise it's just
random luck :)

>     USB: serial: ch341: fix baud rate and line-control handling
> 
>     Revert to using direct register writes to set the divisor and
>     line-control registers.
> 
>     A recent change switched to using the init vendor command to update
>     these registers, something which also enabled support for CH341A
>     devices. It turns out that simply setting bit 7 in the divisor register
>     is sufficient to support CH341A and specifically prevent data from being
>     buffered until a full endpoint-size packet (32 bytes) has been received.
> 
>     Using the init command also had the side-effect of temporarily
>     deasserting the DTR/RTS signals on every termios change (including
>     initialisation on open) something which for example could cause problems
>     in setups where DTR is used to trigger a reset.
> 
>     Fixes: 4e46c410e050 ("USB: serial: ch341: reinitialize chip on
>     reconfiguration")
>     Signed-off-by: Johan Hovold <johan@kernel.org>

It seems like this change does the opposite of what it says, we don't
want the device to wait for a full endpoint of packets to send stuff
out, right?

> It would be great if this regression could be addressed.  At present I must
> boot a pre-4.10 kernel whenever I need to use the programming dongle with
> this converter.
> 
> Please let me know if there is anything I can do to help resolve the
> problem.

If you revert this commit on top of the latest kernel release, does it
solve the problem for you?

thanks,

greg k-h

Re: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Johan Hovold]

On Tue, Jul 12, 2022 at 02:43:41PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jul 12, 2022 at 09:29:57PM +0930, Jonathan Woithe wrote:
> > Hi all
> > 
> > For many years I have been using a CH341 USB-serial converter (VID:PID
> > 4348:5523) to drive a microcontroller programming dongle.  This was under
> > kernel 4.4.14.  When I recently upgraded the machine to a 5.15.27 kernel the
> > programmer stopped working, reporting timeouts.  Using a loopback cable and
> > minicom I discovered that under 5.15.27 data was only delivered back to
> > minicom in blocks of 32 characters.  This explains why the programming
> > software reported timeouts.  It seems that either outgoing data is blocked
> > until 32 bytes are ready for transmission, or receive data is only reported
> > in blocks of 32 bytes.
> > 
> > Under 4.4.14 (and all kernels prior to 4.10.0), individual characters are
> > echoed back by minicom as soon as they hare pressed on the keyboard.
> > 
> > As far as I can tell, the regression affects all kernels since 4.10.0.
> > 
> > I have done a git bisect which identified the following commit as the source
> > of the problem.
> > 
> > commit 55fa15b5987db22b4f35d3f0798928c126be5f1c
> > Author: Johan Hovold <johan@kernel.org>
> > Date:   Fri Jan 6 19:15:16 2017 +0100
> 
> Please always cc: the developer who wrote a commit that you have
> questions about, so that they are sure to see it, otherwise it's just
> random luck :)

Thanks for the report, and for forwarding it.
 
> >     USB: serial: ch341: fix baud rate and line-control handling
> > 
> >     Revert to using direct register writes to set the divisor and
> >     line-control registers.
> > 
> >     A recent change switched to using the init vendor command to update
> >     these registers, something which also enabled support for CH341A
> >     devices. It turns out that simply setting bit 7 in the divisor register
> >     is sufficient to support CH341A and specifically prevent data from being
> >     buffered until a full endpoint-size packet (32 bytes) has been received.
> > 
> >     Using the init command also had the side-effect of temporarily
> >     deasserting the DTR/RTS signals on every termios change (including
> >     initialisation on open) something which for example could cause problems
> >     in setups where DTR is used to trigger a reset.
> > 
> >     Fixes: 4e46c410e050 ("USB: serial: ch341: reinitialize chip on
> >     reconfiguration")
> >     Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> It seems like this change does the opposite of what it says, we don't
> want the device to wait for a full endpoint of packets to send stuff
> out, right?

No, the commit does what it says, namely to fix a regression just like
the one reported here (i.e. that data was buffered in 32 byte chunks).

The offending commit was merged in 4.10-rc1 and the above fixed
corrected it 4.10-rc3.

> > It would be great if this regression could be addressed.  At present I must
> > boot a pre-4.10 kernel whenever I need to use the programming dongle with
> > this converter.
> > 
> > Please let me know if there is anything I can do to help resolve the
> > problem.
> 
> If you revert this commit on top of the latest kernel release, does it
> solve the problem for you?

Simply reverting the commit blamed by the bisection should only makes
things worse, at least for some device types.

Perhaps we need to set that bit 7 based on the type, even if the bit
meaning having been inverted seems a bit far-fetched.

Jonathan, could you try simply commenting out the
	
	val |= BIT(7);

statement in ch341_set_baudrate_lcr()?

Also, what chip version do you have (see debug statement in
ch341_configure())?

Johan

Re: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Jonathan Woithe]

On Tue, Jul 12, 2022 at 06:53:38PM +0200, Johan Hovold wrote:
> On Tue, Jul 12, 2022 at 02:43:41PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jul 12, 2022 at 09:29:57PM +0930, Jonathan Woithe wrote:
> > > I have done a git bisect which identified the following commit as the source
> > > of the problem.
> > > 
> > > commit 55fa15b5987db22b4f35d3f0798928c126be5f1c
> > > Author: Johan Hovold <johan@kernel.org>
> > > Date:   Fri Jan 6 19:15:16 2017 +0100
> > 
> > Please always cc: the developer who wrote a commit that you have
> > questions about, so that they are sure to see it, otherwise it's just
> > random luck :)
> 
> Thanks for the report, and for forwarding it.

Apologies for overlooking the CC in this instance.

> > > It would be great if this regression could be addressed.  At present I must
> > > boot a pre-4.10 kernel whenever I need to use the programming dongle with
> > > this converter.
> > > 
> > > Please let me know if there is anything I can do to help resolve the
> > > problem.
> > 
> > If you revert this commit on top of the latest kernel release, does it
> > solve the problem for you?
> 
> Simply reverting the commit blamed by the bisection should only makes
> things worse, at least for some device types.
> 
> Perhaps we need to set that bit 7 based on the type, even if the bit
> meaning having been inverted seems a bit far-fetched.
> 
> Jonathan, could you try simply commenting out the
> 	
> 	val |= BIT(7);
> 
> statement in ch341_set_baudrate_lcr()?

Commenting out the above line brought some improvement.  In minicom with a
loopback connector in place, the first byte sent does not get echoed
back at all.  However, all other bytes are echoed as soon as they are sent.

The kernel used for the above test was 672c0c5 (5.18-rc5), which is the most
recent I can conveniently get onto the test machine at present.  I tested
the unmodified kernel before commenting out the line and confirmed that it
exhibited the full fault condition (bytes come back in blocks of 32).

> Also, what chip version do you have (see debug statement in
> ch341_configure())?

Chip revision is 0x27.

Regards
  jonathan

Re: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Jonathan Woithe]

On Wed, Jul 13, 2022 at 09:52:20AM +0930, Jonathan Woithe wrote:
> On Tue, Jul 12, 2022 at 06:53:38PM +0200, Johan Hovold wrote:
> > On Tue, Jul 12, 2022 at 02:43:41PM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Jul 12, 2022 at 09:29:57PM +0930, Jonathan Woithe wrote:
> > > > It would be great if this regression could be addressed.  At present I must
> > > > boot a pre-4.10 kernel whenever I need to use the programming dongle with
> > > > this converter.
> > > > 
> > > > Please let me know if there is anything I can do to help resolve the
> > > > problem.
> > > 
> > > If you revert this commit on top of the latest kernel release, does it
> > > solve the problem for you?
> > 
> > Simply reverting the commit blamed by the bisection should only makes
> > things worse, at least for some device types.
> > 
> > Perhaps we need to set that bit 7 based on the type, even if the bit
> > meaning having been inverted seems a bit far-fetched.
> > 
> > Jonathan, could you try simply commenting out the
> > 	
> > 	val |= BIT(7);
> > 
> > statement in ch341_set_baudrate_lcr()?
> 
> Commenting out the above line brought some improvement.  In minicom with a
> loopback connector in place, the first byte sent does not get echoed
> back at all.  However, all other bytes are echoed as soon as they are sent.
> 
> The kernel used for the above test was 672c0c5 (5.18-rc5), which is the most
> recent I can conveniently get onto the test machine at present.  I tested
> the unmodified kernel before commenting out the line and confirmed that it
> exhibited the full fault condition (bytes come back in blocks of 32).
> 
> > Also, what chip version do you have (see debug statement in
> > ch341_configure())?
> 
> Chip revision is 0x27.

Is there anything further I can do or provide to help identify a solution to
the regression?

Regards
  jonathan

Re: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Johan Hovold]

[ +CC: Aidan, Grigori and Michael ]

On Wed, Jul 13, 2022 at 09:52:18AM +0930, Jonathan Woithe wrote:
> On Tue, Jul 12, 2022 at 06:53:38PM +0200, Johan Hovold wrote:

> > Simply reverting the commit blamed by the bisection should only makes
> > things worse, at least for some device types.
> > 
> > Perhaps we need to set that bit 7 based on the type, even if the bit
> > meaning having been inverted seems a bit far-fetched.
> > 
> > Jonathan, could you try simply commenting out the
> > 	
> > 	val |= BIT(7);
> > 
> > statement in ch341_set_baudrate_lcr()?
> 
> Commenting out the above line brought some improvement.  In minicom with a
> loopback connector in place, the first byte sent does not get echoed
> back at all.  However, all other bytes are echoed as soon as they are sent.

Ok, so at least that addresses the disabled TX timer.

What happens if you change the line speed? Does the first character
after changing speed also get dropped?

There were a few more changes done to the initialisation sequence at
that time and more changes in this area has followed since.

Could you try the patch below which addresses the disabled tx timer and
restores one of the init commands that were removed in 4.10.

If that doesn't help, we'll keep digging. One more thing to try then
could be to comment out the above line on a 4.10 kernel to rule the
impact of any later changes on the first lost character.

> The kernel used for the above test was 672c0c5 (5.18-rc5), which is the most
> recent I can conveniently get onto the test machine at present.  I tested
> the unmodified kernel before commenting out the line and confirmed that it
> exhibited the full fault condition (bytes come back in blocks of 32).

> > Also, what chip version do you have (see debug statement in
> > ch341_configure())?
> 
> Chip revision is 0x27.

Interesting. The devices I have here both have version 0x30. While the
tx-timer bit has no effect on the CH340G it must be set on the CH341A in
order for the FIFO to empty before it is full.

Michael Hanselmann posted a patch mentioning that devices before 0x30
has never been supported by the mainline driver:

	2c509d1cc86d ("USB: serial: ch341: name prescaler, divisor registers")

but your programmer seems to suggest otherwise, even if there may be
other differences here left to account for.

The joys of reverse-engineering...

Johan


From 82faf260d13c9f01e4af664f31231e5d88d7e4f1 Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Mon, 18 Jul 2022 10:21:41 +0200
Subject: [PATCH] USB: serial: ch341: fix disabled tx timer on older devices

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

Only set the TX timer enable bit for devices with version newer than
0x27.

Also try restoring an older init command before updating the line
settings to see if it has any effect on a lost first byte after
initialisation.

Reported-by: Jonathan Woithe <jwoithe@just42.net>
Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@marvin.atrad.com.au
Not-Signed-off-yet-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ch341.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 2798fca71261..748724ab6b04 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;
 };
 
@@ -250,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,
@@ -308,12 +315,18 @@ 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)
 		return r;
 
+	r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT, 0x501f, 0xd90a);
+	if (r < 0)
+               return r;
+
 	r = ch341_set_baudrate_lcr(dev, priv, priv->baud_rate, priv->lcr);
 	if (r < 0)
 		return r;
-- 
2.35.1

Re: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Johan Hovold]

On Mon, Jul 18, 2022 at 10:53:06AM +0200, Johan Hovold wrote:

> From 82faf260d13c9f01e4af664f31231e5d88d7e4f1 Mon Sep 17 00:00:00 2001
> From: Johan Hovold <johan@kernel.org>
> Date: Mon, 18 Jul 2022 10:21:41 +0200
> Subject: [PATCH] USB: serial: ch341: fix disabled tx timer on older devices
> 
> At least one older CH341 appears to have the TX timer enable bit
> inverted so that setting it disables the TX timer and prevents the FIFO
> from emptying until it is full.
> 
> Only set the TX timer enable bit for devices with version newer than
> 0x27.
> 
> Also try restoring an older init command before updating the line
> settings to see if it has any effect on a lost first byte after
> initialisation.
> 
> Reported-by: Jonathan Woithe <jwoithe@just42.net>
> Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@marvin.atrad.com.au
> Not-Signed-off-yet-by: Johan Hovold <johan@kernel.org>

s/TX/RX/ ...

Johan

Re: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Jonathan Woithe]

On Mon, Jul 18, 2022 at 10:53:05AM +0200, Johan Hovold wrote:
> [ +CC: Aidan, Grigori and Michael ]
> 
> On Wed, Jul 13, 2022 at 09:52:18AM +0930, Jonathan Woithe wrote:
> > On Tue, Jul 12, 2022 at 06:53:38PM +0200, Johan Hovold wrote:
> 
> > > Simply reverting the commit blamed by the bisection should only makes
> > > things worse, at least for some device types.
> > > 
> > > Perhaps we need to set that bit 7 based on the type, even if the bit
> > > meaning having been inverted seems a bit far-fetched.
> > > 
> > > Jonathan, could you try simply commenting out the
> > > 	
> > > 	val |= BIT(7);
> > > 
> > > statement in ch341_set_baudrate_lcr()?
> > 
> > Commenting out the above line brought some improvement.  In minicom with a
> > loopback connector in place, the first byte sent does not get echoed
> > back at all.  However, all other bytes are echoed as soon as they are sent.
> 
> Ok, so at least that addresses the disabled TX timer.
> 
> What happens if you change the line speed? Does the first character
> after changing speed also get dropped?

Yes it does.  Starting out at 115200 in minicom I confirmed the loss of the
first character.  I changed the line speed to 38400 in minicom and the next
character to be pressed was dropped.  All others after it were fine.  I then
returned to 115200 in minicom and the pattern repeated: the first character
after the speed change was lost, but all others came through.  All these
tests were done in the same minicom session under kernel 672c0c5 with the

  val |= BIT(7);

line still commented out.

> There were a few more changes done to the initialisation sequence at
> that time and more changes in this area has followed since.
> 
> Could you try the patch below which addresses the disabled tx timer and
> restores one of the init commands that were removed in 4.10.

Unfortunately the patch didn't bring back the first character.  The first
character is still missed (immediately after starting minicom, and also
after changing the line speed within minicom), but all others are delivered
as they should be.

> If that doesn't help, we'll keep digging. One more thing to try then
> could be to comment out the above line on a 4.10 kernel to rule the
> impact of any later changes on the first lost character.

I assume the "above line" is the

  val |= BIT(7);

statement in ch341_set_baudrate_lcr().  With this line commented out in the
"v4.10" tag (c470abd), the first character is lost but all others are
send/received as expected.  As you suggested, this appears to rule out
later changes as the source of the lost first character.

> > The kernel used for the above test was 672c0c5 (5.18-rc5), which is the most
> > recent I can conveniently get onto the test machine at present.  I tested
> > the unmodified kernel before commenting out the line and confirmed that it
> > exhibited the full fault condition (bytes come back in blocks of 32).
> 
> > > Also, what chip version do you have (see debug statement in
> > > ch341_configure())?
> > 
> > Chip revision is 0x27.
> 
> Interesting. The devices I have here both have version 0x30. While the
> tx-timer bit has no effect on the CH340G it must be set on the CH341A in
> order for the FIFO to empty before it is full.
> 
> Michael Hanselmann posted a patch mentioning that devices before 0x30
> has never been supported by the mainline driver:
> 
> 	2c509d1cc86d ("USB: serial: ch341: name prescaler, divisor registers")
> 
> but your programmer seems to suggest otherwise, even if there may be
> other differences here left to account for.

I can confirm that this USB-serial converter has apparently been working
fine for me with kernel 4.4.14 (and continued to do so up until the comment
identified by the bisect).  This wasn't just with the programmer hardware
though.  I was also using a second identical CH341 USB-serial converter with
minicom for debugging of external hardware, and this also worked fine under
4.4.14.  Given the comment in the commit you referenced above, it may be
that we were just lucky.  Whatever the reason, these two CH341 version 0x27
converters have definitely been working in practice for me.

> The joys of reverse-engineering...

Yes, having been there myself I totally agree.

Regards
  jonathan

Re: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Johan Hovold]

On Tue, Jul 19, 2022 at 01:29:17PM +0930, Jonathan Woithe wrote:
> On Mon, Jul 18, 2022 at 10:53:05AM +0200, Johan Hovold wrote:
> > [ +CC: Aidan, Grigori and Michael ]
> > 
> > On Wed, Jul 13, 2022 at 09:52:18AM +0930, Jonathan Woithe wrote:
> > > On Tue, Jul 12, 2022 at 06:53:38PM +0200, Johan Hovold wrote:
> > 
> > > > Simply reverting the commit blamed by the bisection should only makes
> > > > things worse, at least for some device types.
> > > > 
> > > > Perhaps we need to set that bit 7 based on the type, even if the bit
> > > > meaning having been inverted seems a bit far-fetched.
> > > > 
> > > > Jonathan, could you try simply commenting out the
> > > > 	
> > > > 	val |= BIT(7);
> > > > 
> > > > statement in ch341_set_baudrate_lcr()?
> > > 
> > > Commenting out the above line brought some improvement.  In minicom with a
> > > loopback connector in place, the first byte sent does not get echoed
> > > back at all.  However, all other bytes are echoed as soon as they are sent.
> > 
> > Ok, so at least that addresses the disabled TX timer.
> > 
> > What happens if you change the line speed? Does the first character
> > after changing speed also get dropped?
> 
> Yes it does.  Starting out at 115200 in minicom I confirmed the loss of the
> first character.  I changed the line speed to 38400 in minicom and the next
> character to be pressed was dropped.  All others after it were fine.  I then
> returned to 115200 in minicom and the pattern repeated: the first character
> after the speed change was lost, but all others came through.  All these
> tests were done in the same minicom session under kernel 672c0c5 with the
> 
>   val |= BIT(7);
> 
> line still commented out.

Thanks for confirming.

I noticed one register write which was removed during 4.10 and we also
added support for setting the LCR register which wasn't there before.

Can you try the below patch which adds a register write that should
match what the old driver did at 115200 baud?

If that doesn't help, can you try just ifdeffing out the LCR update?

> > If that doesn't help, we'll keep digging. One more thing to try then
> > could be to comment out the above line on a 4.10 kernel to rule the
> > impact of any later changes on the first lost character.
> 
> I assume the "above line" is the
> 
>   val |= BIT(7);
> 
> statement in ch341_set_baudrate_lcr().  With this line commented out in the
> "v4.10" tag (c470abd), the first character is lost but all others are
> send/received as expected.  As you suggested, this appears to rule out
> later changes as the source of the lost first character.

Ok, good.

Johan


From d384f68dfda4eaf5b0065bdab236800d65ffb71d Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Mon, 18 Jul 2022 10:21:41 +0200
Subject: [PATCH] USB: serial: ch341: fix disabled rx timer on older devices

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.

Also try restoring a pre-4.10 register write when updating the line
settings to see if it has any effect on a lost first byte after
changing speed.

Another thing to try is to disable the LCR update, which wasn't
supported pre-4.10.

Reported-by: Jonathan Woithe <jwoithe@just42.net>
Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@marvin.atrad.com.au
Not-Signed-off-yet-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ch341.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 2798fca71261..bd27847ece2c 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;
 };
 
@@ -250,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,
@@ -259,6 +266,12 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	if (r)
 		return r;
 
+#if 1
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x0f2c, 0x08);
+	if (r)
+		return r;
+#endif
+#if 1
 	/*
 	 * Chip versions before version 0x30 as read using
 	 * CH341_REQ_READ_VERSION used separate registers for line control
@@ -269,7 +282,7 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 			      CH341_REG_LCR2 << 8 | CH341_REG_LCR, lcr);
 	if (r)
 		return r;
-
+#endif
 	return r;
 }
 
@@ -308,7 +321,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: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Jonathan Woithe]

On Tue, Jul 19, 2022 at 07:53:42AM +0200, Johan Hovold wrote:
> On Tue, Jul 19, 2022 at 01:29:17PM +0930, Jonathan Woithe wrote:
> > On Mon, Jul 18, 2022 at 10:53:05AM +0200, Johan Hovold wrote:
> > > [ +CC: Aidan, Grigori and Michael ]
> > > 
> > > On Wed, Jul 13, 2022 at 09:52:18AM +0930, Jonathan Woithe wrote:
> > > > On Tue, Jul 12, 2022 at 06:53:38PM +0200, Johan Hovold wrote:
> > > 
> > > > > Simply reverting the commit blamed by the bisection should only makes
> > > > > things worse, at least for some device types.
> > > > > 
> > > > > Perhaps we need to set that bit 7 based on the type, even if the bit
> > > > > meaning having been inverted seems a bit far-fetched.
> > > > > 
> > > > > Jonathan, could you try simply commenting out the
> > > > > 	
> > > > > 	val |= BIT(7);
> > > > > 
> > > > > statement in ch341_set_baudrate_lcr()?
> > > > 
> > > > Commenting out the above line brought some improvement.  In minicom with a
> > > > loopback connector in place, the first byte sent does not get echoed
> > > > back at all.  However, all other bytes are echoed as soon as they are sent.
> > > 
> > > Ok, so at least that addresses the disabled TX timer.
> > > 
> > > What happens if you change the line speed? Does the first character
> > > after changing speed also get dropped?
> > 
> > Yes it does.  Starting out at 115200 in minicom I confirmed the loss of the
> > first character.  I changed the line speed to 38400 in minicom and the next
> > character to be pressed was dropped.  All others after it were fine.  I then
> > returned to 115200 in minicom and the pattern repeated: the first character
> > after the speed change was lost, but all others came through.  All these
> > tests were done in the same minicom session under kernel 672c0c5 with the
> > 
> >   val |= BIT(7);
> > 
> > line still commented out.
> 
> Thanks for confirming.
> 
> I noticed one register write which was removed during 4.10 and we also
> added support for setting the LCR register which wasn't there before.
> 
> Can you try the below patch which adds a register write that should
> match what the old driver did at 115200 baud?

I applied this patch to 672c0c5.  With the patch as supplied there was no
change: the first character was lost but all others were delivered as
expected.

> If that doesn't help, can you try just ifdeffing out the LCR update?

I changed the second of the added "#if -0" directives to "#if 1" (thus
removing the LCR update).  This made things work: all characters were echoed
back to minicom as they were pressed.

As a final test I changed the first of the added "#if 1" directives to
"#if 0", effectively removing the extra register write you added earlier.
The resulting kernel still worked.  It appears that this extra register
write doesn't have any effect as far as the missing first character is
concerned.

On the basis of this, it seems the LCR update causes the loss of the first
character on version 0x27 hardware.

Regards
  jonathan

> >From d384f68dfda4eaf5b0065bdab236800d65ffb71d Mon Sep 17 00:00:00 2001
> From: Johan Hovold <johan@kernel.org>
> Date: Mon, 18 Jul 2022 10:21:41 +0200
> Subject: [PATCH] USB: serial: ch341: fix disabled rx timer on older devices
> 
> 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.
> 
> Also try restoring a pre-4.10 register write when updating the line
> settings to see if it has any effect on a lost first byte after
> changing speed.
> 
> Another thing to try is to disable the LCR update, which wasn't
> supported pre-4.10.
> 
> Reported-by: Jonathan Woithe <jwoithe@just42.net>
> Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@marvin.atrad.com.au
> Not-Signed-off-yet-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/serial/ch341.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 2798fca71261..bd27847ece2c 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;
>  };
>  
> @@ -250,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,
> @@ -259,6 +266,12 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
>  	if (r)
>  		return r;
>  
> +#if 1
> +	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x0f2c, 0x08);
> +	if (r)
> +		return r;
> +#endif
> +#if 1
>  	/*
>  	 * Chip versions before version 0x30 as read using
>  	 * CH341_REQ_READ_VERSION used separate registers for line control
> @@ -269,7 +282,7 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
>  			      CH341_REG_LCR2 << 8 | CH341_REG_LCR, lcr);
>  	if (r)
>  		return r;
> -
> +#endif
>  	return r;
>  }
>  
> @@ -308,7 +321,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: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Johan Hovold]

On Tue, Jul 19, 2022 at 04:04:53PM +0930, Jonathan Woithe wrote:
> On Tue, Jul 19, 2022 at 07:53:42AM +0200, Johan Hovold wrote:

> > I noticed one register write which was removed during 4.10 and we also
> > added support for setting the LCR register which wasn't there before.
> > 
> > Can you try the below patch which adds a register write that should
> > match what the old driver did at 115200 baud?
> 
> I applied this patch to 672c0c5.  With the patch as supplied there was no
> change: the first character was lost but all others were delivered as
> expected.
> 
> > If that doesn't help, can you try just ifdeffing out the LCR update?
> 
> I changed the second of the added "#if -0" directives to "#if 1" (thus
> removing the LCR update).  This made things work: all characters were echoed
> back to minicom as they were pressed.

Perfect, thanks for confirming.

> As a final test I changed the first of the added "#if 1" directives to
> "#if 0", effectively removing the extra register write you added earlier.
> The resulting kernel still worked.  It appears that this extra register
> write doesn't have any effect as far as the missing first character is
> concerned.

Thanks for checking.

> On the basis of this, it seems the LCR update causes the loss of the first
> character on version 0x27 hardware.

Right, so it seems we have two issues here:

First, the disabled rx timer, which we can fix.

And second, the lost character after updating LCR, which we may be able
to work around, for example, by disabling LCR control for older devices
(i.e. pre-4.10 behaviour) or by not updating LCR unless the settings
have changed (but that would still likely cause a lost character when we
do). A third option may be to experiment with using the INIT command to
set the line-speed and LCR, but that also has some side-effects like
toggling the modem-control lines on termios changes, which I wanted to
avoid.

And we still need to set LCR on probe, which may trigger the lost
character unless we do this using the INIT command and that turns out
to work.

Can you try the below patch and see if that still triggers the lost
character after probe and line settings changes? Please also try to
confirm that LCR can be updated this way at all.

Note that using INIT like this seems to trigger a similar issue with my
CH341A.

Johan


From 1c937d58f83df00a7879ff82a2b980d11b3faf7b Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Mon, 18 Jul 2022 10:21:41 +0200
Subject: [PATCH] USB: serial: ch341: fix disabled rx timer on older devices

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.

Also try using the INIT command to update LCR to work around a separate
issue with lost characters after LCR updates on older devices. Note that
using INIT like this triggers a similar problem on newer CH341A.

Reported-by: Jonathan Woithe <jwoithe@just42.net>
Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@marvin.atrad.com.au
Not-Signed-off-yet-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ch341.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 2798fca71261..d6a4b2572c9c 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;
 };
 
@@ -250,8 +253,24 @@ 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.
+	 */
+	if (priv->version > 0x27)
+		val |= BIT(7);
+
+	/*
+	 * At least one device with version 0x27 loses characters after
+	 * updating LCR directly, try using INIT for that.
+	 *
+	 * Note that this appears to cause lost characters on newer CH341A.
 	 */
-	val |= BIT(7);
+	if (priv->version <= 0x27) {
+		r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT,
+				lcr << 8 | 0x9c, val);
+		return r;
+	}
 
 	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
 			      CH341_REG_DIVISOR << 8 | CH341_REG_PRESCALER,
@@ -308,7 +327,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: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Jonathan Woithe]

On Tue, Jul 19, 2022 at 09:20:58AM +0200, Johan Hovold wrote:
> On Tue, Jul 19, 2022 at 04:04:53PM +0930, Jonathan Woithe wrote:
> > On Tue, Jul 19, 2022 at 07:53:42AM +0200, Johan Hovold wrote:
> 
> > > I noticed one register write which was removed during 4.10 and we also
> > > added support for setting the LCR register which wasn't there before.
> > > 
> > > Can you try the below patch which adds a register write that should
> > > match what the old driver did at 115200 baud?
> > 
> > I applied this patch to 672c0c5.  With the patch as supplied there was no
> > change: the first character was lost but all others were delivered as
> > expected.
> > 
> > > If that doesn't help, can you try just ifdeffing out the LCR update?
> > 
> > I changed the second of the added "#if -0" directives to "#if 1" (thus
> > removing the LCR update).  This made things work: all characters were echoed
> > back to minicom as they were pressed.
> 
> Perfect, thanks for confirming.
> 
> > As a final test I changed the first of the added "#if 1" directives to
> > "#if 0", effectively removing the extra register write you added earlier.
> > The resulting kernel still worked.  It appears that this extra register
> > write doesn't have any effect as far as the missing first character is
> > concerned.
> 
> Thanks for checking.
> 
> > On the basis of this, it seems the LCR update causes the loss of the first
> > character on version 0x27 hardware.
> 
> Right, so it seems we have two issues here:
> 
> First, the disabled rx timer, which we can fix.
> 
> And second, the lost character after updating LCR, which we may be able
> to work around, for example, by disabling LCR control for older devices
> (i.e. pre-4.10 behaviour) or by not updating LCR unless the settings
> have changed (but that would still likely cause a lost character when we
> do). A third option may be to experiment with using the INIT command to
> set the line-speed and LCR, but that also has some side-effects like
> toggling the modem-control lines on termios changes, which I wanted to
> avoid.
> 
> And we still need to set LCR on probe, which may trigger the lost
> character unless we do this using the INIT command and that turns out
> to work.
> 
> Can you try the below patch and see if that still triggers the lost
> character after probe and line settings changes? Please also try to
> confirm that LCR can be updated this way at all.

Using the patch below, the device appears to function correctly: characters
are echoed back to minicom as soon as they are sent and the first characater
is not lost.  Changing line speed in minicom does not result in a lost
character either.

Of course all testing to this point has been with a loopback adapter which
means there's no indication whether line speed (or any other LCR parameters)
have really changed.  For complicated reasons I can't easily test using the
programmer hardware right now.  I will see if I can work out a way to verify
speed and LCR changes with the hardware I have available.  Hacking up a
null-modem cable and connecting to a PC which happens to have a real
hardware serial port may be the easiest.

Regards
  jonathan

> >From 1c937d58f83df00a7879ff82a2b980d11b3faf7b Mon Sep 17 00:00:00 2001
> From: Johan Hovold <johan@kernel.org>
> Date: Mon, 18 Jul 2022 10:21:41 +0200
> Subject: [PATCH] USB: serial: ch341: fix disabled rx timer on older devices
> 
> 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.
> 
> Also try using the INIT command to update LCR to work around a separate
> issue with lost characters after LCR updates on older devices. Note that
> using INIT like this triggers a similar problem on newer CH341A.
> 
> Reported-by: Jonathan Woithe <jwoithe@just42.net>
> Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@marvin.atrad.com.au
> Not-Signed-off-yet-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/serial/ch341.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 2798fca71261..d6a4b2572c9c 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;
>  };
>  
> @@ -250,8 +253,24 @@ 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.
> +	 */
> +	if (priv->version > 0x27)
> +		val |= BIT(7);
> +
> +	/*
> +	 * At least one device with version 0x27 loses characters after
> +	 * updating LCR directly, try using INIT for that.
> +	 *
> +	 * Note that this appears to cause lost characters on newer CH341A.
>  	 */
> -	val |= BIT(7);
> +	if (priv->version <= 0x27) {
> +		r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT,
> +				lcr << 8 | 0x9c, val);
> +		return r;
> +	}
>  
>  	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
>  			      CH341_REG_DIVISOR << 8 | CH341_REG_PRESCALER,
> @@ -308,7 +327,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: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Jonathan Woithe]

On Tue, Jul 19, 2022 at 05:35:22PM +0930, Jonathan Woithe wrote:
> > Can you try the below patch and see if that still triggers the lost
> > character after probe and line settings changes? Please also try to
> > confirm that LCR can be updated this way at all.
> 
> :
> Of course all testing to this point has been with a loopback adapter which
> means there's no indication whether line speed (or any other LCR parameters)
> have really changed.  For complicated reasons I can't easily test using the
> programmer hardware right now.  I will see if I can work out a way to verify
> speed and LCR changes with the hardware I have available.  Hacking up a
> null-modem cable and connecting to a PC which happens to have a real
> hardware serial port may be the easiest.

I have done further testing with the patch below in place.  Unfortunately
the PC with the real serial port decided not to boot, so instead I'm testing
with a 3-wire null modem cable into a PL2303-based USB-serial converter on
another box running a 5.15.38 kernel.  Flow control was turned off since the
cable wasn't wired for that.

Tests done in minicom:

 * Speed: change from 115200 to 38400 and back again.

   The speed setting of the CH341 appears to work correctly.

 * Data length: change from 8 to 7 bits in minicom on both adapters

   CH341 = 8 bits, PL2303 = 8 bits
    - Data delivered correctly in both directions.

   CH341 = 8 bits, PL2303 = 7 bits
    - Data sent by the CH341 received correctly by the PL2303
    - Data sent by the PL2303 not received correctly by the CH341

   CH341 = 7 bits, PL2303 = 7 bits
    - Data sent by the CH341 received correctly by the PL2303
    - Data sent by the PL2303 not received correctly by the CH341

   CH341 = 7 bits, PL2303 = 8 bits
    - Data delivered correctly in both directions.

   There are too many variables in play to know what to make of this given
   that the response of the PL2303 driver to these changes is unknown.  At
   first glance the results appear to be rather inconsistent.  Maybe the
   data length setting is not always affecting tx and rx on one or both of
   the adapters.

 * Parity: change from None to Even.

   Changing only one adapter to Even parity did not prevent data being
   received correctly.  Data continued to be received correctly when both
   were set to even parity.

 * Stop bits: change from 1 to 2.

   All combinations of stop bit settings resulted in correct delivery of
   data in both directions.

I'm not sure what this proves: most of these settings don't appear to be
having any effect on either adapter.  In the end, 8N1 works and this is,
after all, what most people would be using.  The situation with the patch is
clearly better than it was without the patch because the CH341 is now
delivering data in a timely manner - just like it did under 4.4.14.

The behaviour of the LCR setting on version 0x27 may not be a new thing: all
my applications use 8N1, so if any of the other combinations were faulty
under 4.4.14 I wouldn't know.  I may look into testing the LCR settings
under 4.4.14 if you think there's value in it.  I expect it will show that
the patch below restores the 4.4.14 behaviour for hardware version 0x27, and
that the LCR issues were present in 4.4.14 but masked due to the almost
universal use of 8N1).  I can also try to work out an alternative way to
get a real hardware serial port if that would help: I'm thinking it would
remove any doubt about the LCR setting at one end of the link.

Regards
  jonathan

> > >From 1c937d58f83df00a7879ff82a2b980d11b3faf7b Mon Sep 17 00:00:00 2001
> > From: Johan Hovold <johan@kernel.org>
> > Date: Mon, 18 Jul 2022 10:21:41 +0200
> > Subject: [PATCH] USB: serial: ch341: fix disabled rx timer on older devices
> > 
> > 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.
> > 
> > Also try using the INIT command to update LCR to work around a separate
> > issue with lost characters after LCR updates on older devices. Note that
> > using INIT like this triggers a similar problem on newer CH341A.
> > 
> > Reported-by: Jonathan Woithe <jwoithe@just42.net>
> > Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@marvin.atrad.com.au
> > Not-Signed-off-yet-by: Johan Hovold <johan@kernel.org>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/usb/serial/ch341.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> > index 2798fca71261..d6a4b2572c9c 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;
> >  };
> >  
> > @@ -250,8 +253,24 @@ 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.
> > +	 */
> > +	if (priv->version > 0x27)
> > +		val |= BIT(7);
> > +
> > +	/*
> > +	 * At least one device with version 0x27 loses characters after
> > +	 * updating LCR directly, try using INIT for that.
> > +	 *
> > +	 * Note that this appears to cause lost characters on newer CH341A.
> >  	 */
> > -	val |= BIT(7);
> > +	if (priv->version <= 0x27) {
> > +		r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT,
> > +				lcr << 8 | 0x9c, val);
> > +		return r;
> > +	}
> >  
> >  	r = ch341_control_out(dev, CH341_REQ_WRITE_REG,
> >  			      CH341_REG_DIVISOR << 8 | CH341_REG_PRESCALER,
> > @@ -308,7 +327,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)
> > -- 

Re: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Johan Hovold]

On Tue, Jul 19, 2022 at 09:48:59PM +0930, Jonathan Woithe wrote:
> On Tue, Jul 19, 2022 at 05:35:22PM +0930, Jonathan Woithe wrote:
> > > Can you try the below patch and see if that still triggers the lost
> > > character after probe and line settings changes? Please also try to
> > > confirm that LCR can be updated this way at all.
> > 
> > :
> > Of course all testing to this point has been with a loopback adapter which
> > means there's no indication whether line speed (or any other LCR parameters)
> > have really changed.  For complicated reasons I can't easily test using the
> > programmer hardware right now.  I will see if I can work out a way to verify
> > speed and LCR changes with the hardware I have available.  Hacking up a
> > null-modem cable and connecting to a PC which happens to have a real
> > hardware serial port may be the easiest.
> 
> I have done further testing with the patch below in place.  Unfortunately
> the PC with the real serial port decided not to boot, so instead I'm testing
> with a 3-wire null modem cable into a PL2303-based USB-serial converter on
> another box running a 5.15.38 kernel.  Flow control was turned off since the
> cable wasn't wired for that.
> 
> Tests done in minicom:
> 
>  * Speed: change from 115200 to 38400 and back again.
> 
>    The speed setting of the CH341 appears to work correctly.
> 
>  * Data length: change from 8 to 7 bits in minicom on both adapters
> 
>    CH341 = 8 bits, PL2303 = 8 bits
>     - Data delivered correctly in both directions.
> 
>    CH341 = 8 bits, PL2303 = 7 bits
>     - Data sent by the CH341 received correctly by the PL2303
>     - Data sent by the PL2303 not received correctly by the CH341
> 
>    CH341 = 7 bits, PL2303 = 7 bits
>     - Data sent by the CH341 received correctly by the PL2303
>     - Data sent by the PL2303 not received correctly by the CH341
> 
>    CH341 = 7 bits, PL2303 = 8 bits
>     - Data delivered correctly in both directions.
>
>    There are too many variables in play to know what to make of this given
>    that the response of the PL2303 driver to these changes is unknown.  At
>    first glance the results appear to be rather inconsistent.  Maybe the
>    data length setting is not always affecting tx and rx on one or both of
>    the adapters.

Thanks for testing this. The above observations appear consistent with
LCR updates for your CH341 not taking effect (e.g. stuck at 8N1 unlike
the PL2303).

An easy way to test this is to send the letter 'q' (0x71) from CH341 to
PL2303 (in 8N1 mode). This should be received as 0xf1 if the CH341 is in
7N1 mode as the stop bit is interpreted as MSB, or as 0x71 if still in
8N1 mode.

> I'm not sure what this proves: most of these settings don't appear to be
> having any effect on either adapter.  In the end, 8N1 works and this is,
> after all, what most people would be using.  The situation with the patch is
> clearly better than it was without the patch because the CH341 is now
> delivering data in a timely manner - just like it did under 4.4.14.
> 
> The behaviour of the LCR setting on version 0x27 may not be a new thing: all
> my applications use 8N1, so if any of the other combinations were faulty
> under 4.4.14 I wouldn't know.  I may look into testing the LCR settings
> under 4.4.14 if you think there's value in it.  I expect it will show that
> the patch below restores the 4.4.14 behaviour for hardware version 0x27, and
> that the LCR issues were present in 4.4.14 but masked due to the almost
> universal use of 8N1).  I can also try to work out an alternative way to
> get a real hardware serial port if that would help: I'm thinking it would
> remove any doubt about the LCR setting at one end of the link.

Actually, LCR configuration wasn't supported before 4.10 either so the
only question would be if LCR control works at all with your chip
(except for the lost character).

I found this thread where Micahel provides some details after apparently
having disassembled the vendor driver:

	https://lore.kernel.org/all/2e80916d-1be8-dc0f-abf9-adc0feea1803@msgid.hansmi.ch/

Based on that (and the comment that made it into the driver), chips
before version 0x30 uses a different protocol for updating LCR so that
anything but 8N1 has indeed likely never worked for these chips. 

Could you try the patch below, which simply disables LCR updates for
older chips and which I believe you already confirmed works.

And then as a follow up, see if enabling the LCR update again has any
effect on the word size (e.g. rerun the test you did above, or the "q"
test I mentioned).

This may give an indication of how far we are from being able to support
LCR updates on older chips even this is not something we need to
implement now.

Johan


From 4d12bb8ba4fe760cd5893b7c5f981524e16536fc Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Mon, 18 Jul 2022 10:21:41 +0200
Subject: [PATCH] USB: serial: ch341: fix disabled rx timer on older devices

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

Also disable LCR updates for pre-0x30 devices which use a different
protocol and where the current register write causes the next received
character to be lost.

FIXME: split in two patches

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")
Not-Signed-off-yet-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ch341.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 2798fca71261..af01a462cc43 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;
 };
 
@@ -250,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,
@@ -265,6 +272,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 +318,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: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Jonathan Woithe]

On Sat, Jul 23, 2022 at 12:57:03PM +0200, Johan Hovold wrote:
> > I have done further testing with the patch below in place.  Unfortunately
> > the PC with the real serial port decided not to boot, so instead I'm testing
> > with a 3-wire null modem cable into a PL2303-based USB-serial converter on
> > another box running a 5.15.38 kernel.  Flow control was turned off since the
> > cable wasn't wired for that.
> > 
> > Tests done in minicom:
> > :
> 
> Thanks for testing this. The above observations appear consistent with
> LCR updates for your CH341 not taking effect (e.g. stuck at 8N1 unlike
> the PL2303).
> 
> An easy way to test this is to send the letter 'q' (0x71) from CH341 to
> PL2303 (in 8N1 mode). This should be received as 0xf1 if the CH341 is in
> 7N1 mode as the stop bit is interpreted as MSB, or as 0x71 if still in
> 8N1 mode.

Apologies for the delayed response to your requests.

The result of the above test:

 1. With both the PL2303 and CH341 in 8N1, "q" is received correctly by
    both ends.

 2. With the CH341 configured for 7N1 in minicom and the PL2303 left in
    8N1, a "q" sent by the CH341 system is received as "q" by the PL2303.

Thus your suspicion seems to be correct: the LCR changes are not taking
effect on the CH341.

> > The behaviour of the LCR setting on version 0x27 may not be a new thing: all
> > my applications use 8N1, so if any of the other combinations were faulty
> > under 4.4.14 I wouldn't know.  I may look into testing the LCR settings
> > under 4.4.14 if you think there's value in it.  I expect it will show that
> > the patch below restores the 4.4.14 behaviour for hardware version 0x27, and
> > that the LCR issues were present in 4.4.14 but masked due to the almost
> > universal use of 8N1).  I can also try to work out an alternative way to
> > get a real hardware serial port if that would help: I'm thinking it would
> > remove any doubt about the LCR setting at one end of the link.
> 
> Actually, LCR configuration wasn't supported before 4.10 either so the
> only question would be if LCR control works at all with your chip
> (except for the lost character).
> 
> I found this thread where Micahel provides some details after apparently
> having disassembled the vendor driver:
> 
> 	https://lore.kernel.org/all/2e80916d-1be8-dc0f-abf9-adc0feea1803@msgid.hansmi.ch/
> 
> Based on that (and the comment that made it into the driver), chips
> before version 0x30 uses a different protocol for updating LCR so that
> anything but 8N1 has indeed likely never worked for these chips. 
> 
> Could you try the patch below, which simply disables LCR updates for
> older chips and which I believe you already confirmed works.

A you expected, I can confirm that with the most recent patch in place as
sent:

 1. There are no delays sending or receiving characters on the CH341.

 2. There is no loss of the first character sent or received by the CH341.

 3. With both the PL2303 and CH341 in 8N1, "q" is received correctly by
    both ends.

 4. With the CH341 configured for 7N1 in minicom and the PL2303 left in
    8N1, a "q" sent by the CH341 system is received as "q" by the PL2303.

That is, it seems to work (notwithstanding the LCR issue).

> And then as a follow up, see if enabling the LCR update again has any
> effect on the word size (e.g. rerun the test you did above, or the "q"
> test I mentioned).

I commented out the "priv->version < 0x30" conditional to re-enable the LCR
update and repeated the above test.  The result was the same: with the
PL2303 in 8N1 and the CH341 supposedly in 7N1, both ends received a "q" as
"q".

> This may give an indication of how far we are from being able to support
> LCR updates on older chips even this is not something we need to
> implement now.

It seems there are still some mysteries left to solve surrounding the
earlier chips.  At least they seem to work in 8N1 through, which is what
most things would be using these days.

Regards
  jonathan

> >From 4d12bb8ba4fe760cd5893b7c5f981524e16536fc Mon Sep 17 00:00:00 2001
> From: Johan Hovold <johan@kernel.org>
> Date: Mon, 18 Jul 2022 10:21:41 +0200
> Subject: [PATCH] USB: serial: ch341: fix disabled rx timer on older devices
> 
> 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).
> 
> Also disable LCR updates for pre-0x30 devices which use a different
> protocol and where the current register write causes the next received
> character to be lost.
> 
> FIXME: split in two patches
> 
> 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")
> Not-Signed-off-yet-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/serial/ch341.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 2798fca71261..af01a462cc43 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;
>  };
>  
> @@ -250,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,
> @@ -265,6 +272,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 +318,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: [Regression] CH341 USB-serial converter passes data in 32 byte chunks

[Johan Hovold]

On Tue, Jul 26, 2022 at 09:15:41AM +0930, Jonathan Woithe wrote:
> On Sat, Jul 23, 2022 at 12:57:03PM +0200, Johan Hovold wrote:

> > Thanks for testing this. The above observations appear consistent with
> > LCR updates for your CH341 not taking effect (e.g. stuck at 8N1 unlike
> > the PL2303).
> > 
> > An easy way to test this is to send the letter 'q' (0x71) from CH341 to
> > PL2303 (in 8N1 mode). This should be received as 0xf1 if the CH341 is in
> > 7N1 mode as the stop bit is interpreted as MSB, or as 0x71 if still in
> > 8N1 mode.
> 
> Apologies for the delayed response to your requests.
> 
> The result of the above test:
> 
>  1. With both the PL2303 and CH341 in 8N1, "q" is received correctly by
>     both ends.
> 
>  2. With the CH341 configured for 7N1 in minicom and the PL2303 left in
>     8N1, a "q" sent by the CH341 system is received as "q" by the PL2303.
> 
> Thus your suspicion seems to be correct: the LCR changes are not taking
> effect on the CH341.

Thanks for confirming.

> > Actually, LCR configuration wasn't supported before 4.10 either so the
> > only question would be if LCR control works at all with your chip
> > (except for the lost character).
> > 
> > I found this thread where Micahel provides some details after apparently
> > having disassembled the vendor driver:
> > 
> > 	https://lore.kernel.org/all/2e80916d-1be8-dc0f-abf9-adc0feea1803@msgid.hansmi.ch/
> > 
> > Based on that (and the comment that made it into the driver), chips
> > before version 0x30 uses a different protocol for updating LCR so that
> > anything but 8N1 has indeed likely never worked for these chips. 
> > 
> > Could you try the patch below, which simply disables LCR updates for
> > older chips and which I believe you already confirmed works.
> 
> A you expected, I can confirm that with the most recent patch in place as
> sent:
> 
>  1. There are no delays sending or receiving characters on the CH341.
> 
>  2. There is no loss of the first character sent or received by the CH341.
> 
>  3. With both the PL2303 and CH341 in 8N1, "q" is received correctly by
>     both ends.
> 
>  4. With the CH341 configured for 7N1 in minicom and the PL2303 left in
>     8N1, a "q" sent by the CH341 system is received as "q" by the PL2303.
> 
> That is, it seems to work (notwithstanding the LCR issue).
> 
> > And then as a follow up, see if enabling the LCR update again has any
> > effect on the word size (e.g. rerun the test you did above, or the "q"
> > test I mentioned).
> 
> I commented out the "priv->version < 0x30" conditional to re-enable the LCR
> update and repeated the above test.  The result was the same: with the
> PL2303 in 8N1 and the CH341 supposedly in 7N1, both ends received a "q" as
> "q".
> 
> > This may give an indication of how far we are from being able to support
> > LCR updates on older chips even this is not something we need to
> > implement now.
> 
> It seems there are still some mysteries left to solve surrounding the
> earlier chips.  At least they seem to work in 8N1 through, which is what
> most things would be using these days.

Thanks for confirming, and for all your careful testing so far.

I'll wrap this this up in the next few weeks (merge window) and get this
fixed in 5.20-rc and backported to stable. I'll keep you on CC.

Johan