[RFC/RFT PATCH] pl2303: avoid data corruption with some chip types
Some PL2303 chips are reported to lose bytes if the serial settings are set to
the same values as before.
At least HXD chips are affected, HX chips seem to be ok.
The current code tries to avoid this by calling tty_termios_hw_change(), but
this check isn't sufficient because different requested baud rates can result
in identic encoded baud rates.
The solution is to save and compare the encoded settings instead.
Signed-off-by: Frank Schäfer fschaefer@googlemail.com
Cc: sta...@kernel.org
---
drivers/usb/serial/pl2303.c | 22 ++
1 Datei geändert, 14 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index bedf8e4..27a2691 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -155,6 +155,7 @@ struct pl2303_private {
spinlock_t lock;
u8 line_control;
u8 line_status;
+ u8 line_settings_enc[7]; /* baud rate, data bits, stop bits, parity */
};
static int pl2303_vendor_read(__u16 value, __u16 index,
@@ -500,14 +501,6 @@ static void pl2303_set_termios(struct tty_struct *tty,
int i;
u8 control;
- /*
-* The PL2303 is reported to lose bytes if you change serial settings
-* even to the same values as before. Thus we actually need to filter
-* in this specific case.
-*/
- if (old_termios !tty_termios_hw_change(tty-termios, old_termios))
- return;
-
buf = kzalloc(7, GFP_KERNEL);
if (!buf) {
dev_err(port-dev, %s - out of memory.\n, __func__);
@@ -591,10 +584,23 @@ static void pl2303_set_termios(struct tty_struct *tty,
dev_dbg(port-dev, parity = none\n);
}
+ /*
+* Some PL2303 chips are reported to lose bytes if you change the
+* serial settings even to the same values as before.
+* At least HXD chips are affected, HX chips seem to be ok.
+* Thus we actually need to filter in this specific case.
+* NOTE: a tty_termios_hw_change() check isn't sufficient, because
+* different baud rates can result in identic encoded values.
+*/
+ if (!memcmp(priv-line_settings_enc, buf, 7))
+ return;
+
i = usb_control_msg(serial-dev, usb_sndctrlpipe(serial-dev, 0),
SET_LINE_REQUEST, SET_LINE_REQUEST_TYPE,
0, 0, buf, 7, 100);
dev_dbg(port-dev, 0x21:0x20:0:0 %d\n, i);
+ if (i == 7)
+ memcpy(buf, priv-line_settings_enc, 7);
/* change control lines if we are switching to or from B0 */
spin_lock_irqsave(priv-lock, flags);
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/RFT PATCH] pl2303: avoid data corruption with some chip types
On Fri, Nov 01, 2013 at 12:49:22PM +0100, Frank Schäfer wrote: Some PL2303 chips are reported to lose bytes if the serial settings are set to the same values as before. At least HXD chips are affected, HX chips seem to be ok. The current code tries to avoid this by calling tty_termios_hw_change(), but this check isn't sufficient because different requested baud rates can result in identic encoded baud rates. The solution is to save and compare the encoded settings instead. I tried this on top of rc7 and I can still see data get corrupted (both 115200 and 460800 bps). -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/RFT PATCH] pl2303: avoid data corruption with some chip types
Am 01.11.2013 13:19, schrieb Mika Westerberg:
On Fri, Nov 01, 2013 at 12:49:22PM +0100, Frank Schäfer wrote:
Some PL2303 chips are reported to lose bytes if the serial settings are set
to
the same values as before.
At least HXD chips are affected, HX chips seem to be ok.
The current code tries to avoid this by calling tty_termios_hw_change(), but
this check isn't sufficient because different requested baud rates can result
in identic encoded baud rates.
The solution is to save and compare the encoded settings instead.
I tried this on top of rc7 and I can still see data get corrupted (both
115200 and 460800 bps).
Ok... Ready for the next round ? ;)
Attached are two further experimental patches.
Regards,
Frank
From 95a3f0e0751dad4ae39fdd7a915a2fa22997a540 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Frank=20Sch=C3=A4fer?= fschaefer@googlemail.com
Date: Fri, 1 Nov 2013 13:56:53 +0100
Subject: [PATCH] EXPERIMENTAL PATCH 1
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Frank Schäfer fschaefer@googlemail.com
---
drivers/usb/serial/pl2303.c | 30 ++
1 Datei geändert, 18 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index bedf8e4..8496eef 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -496,20 +496,13 @@ static void pl2303_set_termios(struct tty_struct *tty,
struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
struct pl2303_private *priv = usb_get_serial_port_data(port);
unsigned long flags;
- unsigned char *buf;
+ unsigned char *buf, *buf_old;
int i;
u8 control;
- /*
- * The PL2303 is reported to lose bytes if you change serial settings
- * even to the same values as before. Thus we actually need to filter
- * in this specific case.
- */
- if (old_termios !tty_termios_hw_change(tty-termios, old_termios))
- return;
-
buf = kzalloc(7, GFP_KERNEL);
- if (!buf) {
+ buf_old = kzalloc(7, GFP_KERNEL);
+ if (!buf || buf_old) {
dev_err(port-dev, %s - out of memory.\n, __func__);
/* Report back no change occurred */
if (old_termios)
@@ -519,8 +512,8 @@ static void pl2303_set_termios(struct tty_struct *tty,
i = usb_control_msg(serial-dev, usb_rcvctrlpipe(serial-dev, 0),
GET_LINE_REQUEST, GET_LINE_REQUEST_TYPE,
- 0, 0, buf, 7, 100);
- dev_dbg(port-dev, 0xa1:0x21:0:0 %d - %7ph\n, i, buf);
+ 0, 0, buf_old, 7, 100);
+ dev_dbg(port-dev, 0xa1:0x21:0:0 %d - %7ph\n, i, buf_old);
if (C_CSIZE(tty)) {
switch (C_CSIZE(tty)) {
@@ -591,6 +584,17 @@ static void pl2303_set_termios(struct tty_struct *tty,
dev_dbg(port-dev, parity = none\n);
}
+ /*
+ * Some PL2303 chips are reported to lose bytes if you change the
+ * serial settings even to the same values as before.
+ * At least HXD chips are affected, HX chips seem to be ok.
+ * Thus we actually need to filter in this specific case.
+ * NOTE: a tty_termios_hw_change() check isn't sufficient, because
+ * different baud rates can result in identic encoded values.
+ */
+ if (!memcmp(buf, buf_old, 7))
+ goto out;
+
i = usb_control_msg(serial-dev, usb_sndctrlpipe(serial-dev, 0),
SET_LINE_REQUEST, SET_LINE_REQUEST_TYPE,
0, 0, buf, 7, 100);
@@ -626,7 +630,9 @@ static void pl2303_set_termios(struct tty_struct *tty,
pl2303_vendor_write(0x0, 0x0, serial);
}
+out:
kfree(buf);
+ kfree(buf_old);
}
static void pl2303_dtr_rts(struct usb_serial_port *port, int on)
--
1.7.10.4
From d303ff67ab1b817bf75b32eaa276369039ee4af2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Frank=20Sch=C3=A4fer?= fschaefer@googlemail.com
Date: Fri, 1 Nov 2013 13:59:58 +0100
Subject: [PATCH] EXPERIMENTAL PATCH 2
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Frank Schäfer fschaefer@googlemail.com
---
drivers/usb/serial/pl2303.c | 29 -
1 Datei geändert, 16 Zeilen hinzugefügt(+), 13 Zeilen entfernt(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index bedf8e4..b0d3d66 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -155,6 +155,7 @@ struct pl2303_private {
spinlock_t lock;
u8 line_control;
u8 line_status;
+ u8 line_settings_enc[7]; /* baud rate, data bits, stop bits, parity */
};
static int pl2303_vendor_read(__u16 value, __u16 index,
@@ -500,14 +501,6 @@ static void pl2303_set_termios(struct tty_struct *tty,
int i;
u8 control;
- /*
- * The PL2303 is reported to lose bytes if you change serial settings
- * even to the same values as before. Thus we actually need to filter
- * in this specific case.
- */
- if (old_termios !tty_termios_hw_change(tty-termios, old_termios))
- return;
-
buf = kzalloc(7, GFP_KERNEL);
if (!buf) {
dev_err(port-dev, %s - out of memory.\n, __func__);
@@ -517,11 +510,6 @@ static void
Re: [RFC/RFT PATCH] pl2303: avoid data corruption with some chip types
On Fri, Nov 01, 2013 at 12:49:22PM +0100, Frank Schäfer wrote:
Some PL2303 chips are reported to lose bytes if the serial settings are set to
the same values as before.
At least HXD chips are affected, HX chips seem to be ok.
The current code tries to avoid this by calling tty_termios_hw_change(), but
this check isn't sufficient because different requested baud rates can result
in identic encoded baud rates.
The solution is to save and compare the encoded settings instead.
We're gonna need something along these lines, but this is just a hack
that not even correct.
Signed-off-by: Frank Schäfer fschaefer@googlemail.com
Cc: sta...@kernel.org
---
drivers/usb/serial/pl2303.c | 22 ++
1 Datei geändert, 14 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index bedf8e4..27a2691 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -155,6 +155,7 @@ struct pl2303_private {
spinlock_t lock;
u8 line_control;
u8 line_status;
+ u8 line_settings_enc[7]; /* baud rate, data bits, stop bits, parity */
};
static int pl2303_vendor_read(__u16 value, __u16 index,
@@ -500,14 +501,6 @@ static void pl2303_set_termios(struct tty_struct *tty,
int i;
u8 control;
- /*
- * The PL2303 is reported to lose bytes if you change serial settings
- * even to the same values as before. Thus we actually need to filter
- * in this specific case.
- */
- if (old_termios !tty_termios_hw_change(tty-termios, old_termios))
- return;
-
buf = kzalloc(7, GFP_KERNEL);
if (!buf) {
dev_err(port-dev, %s - out of memory.\n, __func__);
Here another call to get the line settings is made. Have you verified
that that isn't related to the lost bytes?
@@ -591,10 +584,23 @@ static void pl2303_set_termios(struct tty_struct *tty,
dev_dbg(port-dev, parity = none\n);
}
+ /*
+ * Some PL2303 chips are reported to lose bytes if you change the
+ * serial settings even to the same values as before.
+ * At least HXD chips are affected, HX chips seem to be ok.
+ * Thus we actually need to filter in this specific case.
+ * NOTE: a tty_termios_hw_change() check isn't sufficient, because
+ * different baud rates can result in identic encoded values.
+ */
+ if (!memcmp(priv-line_settings_enc, buf, 7))
+ return;
+
Here you're returning if there no baud rate change, which means that
flow control is never handled a bit further down in set_termios.
i = usb_control_msg(serial-dev, usb_sndctrlpipe(serial-dev, 0),
SET_LINE_REQUEST, SET_LINE_REQUEST_TYPE,
0, 0, buf, 7, 100);
dev_dbg(port-dev, 0x21:0x20:0:0 %d\n, i);
+ if (i == 7)
+ memcpy(buf, priv-line_settings_enc, 7);
/* change control lines if we are switching to or from B0 */
spin_lock_irqsave(priv-lock, flags);
--
1.7.10.4
Again, this too much of a hack. There is a long-standing bug that needs
to be addressed. Let's think some more of how best to handle that (and
get to the bottom with exactly what requests are causing it, etc).
The important thing is to fix the regression. We can fix this bug during
the 3.13-cycle if the fix is not too invasive.
Sorry,
Johan
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
