| Message ID | b4cd2557-9a61-5ccd-32ad-48b0c68bef6b@jrr.cz (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] usbserial: cp210x - icount support for parity error checking | expand |
On Sat, Jun 20, 2020 at 09:58:40PM +0200, Jerry wrote: > usbserial: add cp210x support for icount to detect parity error in received data Why is this here? > Motivation - current version of cp210x driver doesn't provide any way to detect > a parity error in received data from userspace. Some serial protocols like STM32 > bootloader protect data only by even parity so application needs to detect > whether parity error happened to read again peripheral data. > > I created a simple patch which adds support for icount (ioctl TIOCGICOUNT) which > sends GET_COMM_STATUS command to CP210X and according received flags increments > fields for parity error, frame error, break and overrun. > So application can detect an error condition after reading data from ttyUSB > and repeat operation. There is no impact for applications which don't > call ioctl TIOCGICOUNT. > This patch is also placed at http://yyy.jrr.cz/cp210x.patch (my first patch) Please read the section entitled "The canonical patch format" in the kernel file, Documentation/SubmittingPatches for what is needed in order to properly describe the change. > Signed-off-by: Jaromir Skorpil <Jerry@jrr.cz> This does not match your From: line :( thanks, greg k-h
Greg Kroah-Hartman wrote on 6/21/20 10:58 AM: > On Sat, Jun 20, 2020 at 09:58:40PM +0200, Jerry wrote: >> usbserial: add cp210x support for icount to detect parity error in received data > Why is this here? > Because it seems be mandatory? https://www.kernel.org/doc/html/latest/process/5.Posting.html#patch-formatting-and-changelogs "A one-line description of what the patch does. This message should be enough for a reader who sees it with no other context to figure out the scope of the patch; it is the line that will show up in the “short form” changelogs. This message is usually formatted with the relevant subsystem name first, followed by the purpose of the patch. For example: gpio: fix build on CONFIG_GPIO_SYSFS=n" Did I misunderstand your rule or used wrong name of subsystem? Should I type? USB serial: add cp210x support for icount to detect parity error in received data >> Motivation - current version of cp210x driver doesn't provide any way to detect >> a parity error in received data from userspace. Some serial protocols like STM32 >> bootloader protect data only by even parity so application needs to detect >> whether parity error happened to read again peripheral data. >> >> I created a simple patch which adds support for icount (ioctl TIOCGICOUNT) which >> sends GET_COMM_STATUS command to CP210X and according received flags increments >> fields for parity error, frame error, break and overrun. >> So application can detect an error condition after reading data from ttyUSB >> and repeat operation. There is no impact for applications which don't >> call ioctl TIOCGICOUNT. >> This patch is also placed at http://yyy.jrr.cz/cp210x.patch (my first patch) > Please read the section entitled "The canonical patch format" in the > kernel file, Documentation/SubmittingPatches for what is needed in order > to properly describe the change. I read it, but still not sure what exactly was wrong? Yes, I wrapped lines of description to 80 colums and now I noticed that only 75 columns is allowed but I doubt that it is all? Sorry for my bad English, it is hard to guess whether you see a problem in function of patch, patch formatting, tab/spaces, description content, spelling, line wrapping, mail client identification, something else or all of them? >> Signed-off-by: Jaromir Skorpil <Jerry@jrr.cz> > This does not match your From: line :( I supposed that only mail address in From line matter? I understand that real name is mandatory only for Signed-off-by field? > > thanks, > > greg k-h Jerry
On Sun, Jun 21, 2020 at 11:45:11AM +0200, Jerry wrote: > Greg Kroah-Hartman wrote on 6/21/20 10:58 AM: > > On Sat, Jun 20, 2020 at 09:58:40PM +0200, Jerry wrote: > > > usbserial: add cp210x support for icount to detect parity error in received data > > Why is this here? > > > Because it seems be mandatory? > https://www.kernel.org/doc/html/latest/process/5.Posting.html#patch-formatting-and-changelogs > > "A one-line description of what the patch does. This message should be > enough for a reader who sees it with no other context to figure out the > scope of the patch; it is the line that will show up in the “short form” > changelogs. This message is usually formatted with the relevant subsystem > name first, followed by the purpose of the patch. For example: > gpio: fix build on CONFIG_GPIO_SYSFS=n" Yes, that should have been the first line of the git commit, which ends up being the subject line for your email. > Did I misunderstand your rule or used wrong name of subsystem? Should I > type? > USB serial: add cp210x support for icount to detect parity error in received > data That would have been fine too, you can't do it twice, once as a subject and once as the first line in the email, otherwise that would look really odd, right? > > > Motivation - current version of cp210x driver doesn't provide any way to detect > > > a parity error in received data from userspace. Some serial protocols like STM32 > > > bootloader protect data only by even parity so application needs to detect > > > whether parity error happened to read again peripheral data. > > > > > > I created a simple patch which adds support for icount (ioctl TIOCGICOUNT) which > > > sends GET_COMM_STATUS command to CP210X and according received flags increments > > > fields for parity error, frame error, break and overrun. > > > So application can detect an error condition after reading data from ttyUSB > > > and repeat operation. There is no impact for applications which don't > > > call ioctl TIOCGICOUNT. > > > This patch is also placed at http://yyy.jrr.cz/cp210x.patch (my first patch) > > Please read the section entitled "The canonical patch format" in the > > kernel file, Documentation/SubmittingPatches for what is needed in order > > to properly describe the change. > I read it, but still not sure what exactly was wrong? Yes, I wrapped lines > of description to 80 colums and now I noticed that only 75 columns is > allowed but I doubt that it is all? That is one thing, but also the "This patch..." should not be in a changelog, right? Look at the other changes sent to the list for examples of how to do this. > > > Signed-off-by: Jaromir Skorpil <Jerry@jrr.cz> > > This does not match your From: line :( > I supposed that only mail address in From line matter? > I understand that real name is mandatory only for Signed-off-by field? It has to match the From: line of your email to ensure that this really is the same person. thanks, greg k-h
Greg Kroah-Hartman wrote on 6/21/20 11:55 AM: >> I read it, but still not sure what exactly was wrong? Yes, I wrapped lines >> of description to 80 colums and now I noticed that only 75 columns is >> allowed but I doubt that it is all? > That is one thing, but also the "This patch..." should not be in a > changelog, right? Look at the other changes sent to the list for > examples of how to do this. Yes, I looked at another messages here and there are a lot of things which I don't understand. For example two dash -- marker at the end (bellow patch) with some strange number (2.7.4). I didn't find anything about that in documentation. And documentation request diff -up https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#diff-up but patches here use another settings because diff -up never give me line like index 86638c1..f1b46b5 100644 before file names but put me file date and time next to filename. So what version of diff should I use? I have diff (GNU diffutils) 3.7 > >>>> Signed-off-by: Jaromir Skorpil <Jerry@jrr.cz> >>> This does not match your From: line :( >> I supposed that only mail address in From line matter? >> I understand that real name is mandatory only for Signed-off-by field? > It has to match the From: line of your email to ensure that this really > is the same person. Really? I looked at another message as you advised and it seems that even YOUR name often changes? https://marc.info/?l=linux-usb&m=159257306831535 https://marc.info/?l=linux-usb&m=159256948030250 Why is a name so important when you can't verify it? Typing the same text twice doesn't prove anything. In fact my real name can't be written in ascii because of diacritics marks and I doubt that it will work here correctly if I use unicode... > thanks, > > greg k-h Jerry
On Sun, Jun 21, 2020 at 12:34:52PM +0200, Jerry wrote: > Greg Kroah-Hartman wrote on 6/21/20 11:55 AM: > > > I read it, but still not sure what exactly was wrong? Yes, I wrapped lines > > > of description to 80 colums and now I noticed that only 75 columns is > > > allowed but I doubt that it is all? > > That is one thing, but also the "This patch..." should not be in a > > changelog, right? Look at the other changes sent to the list for > > examples of how to do this. > Yes, I looked at another messages here and there are a lot of things which I > don't understand. For example two dash -- marker at the end (bellow patch) > with some strange number (2.7.4). I didn't find anything about that in > documentation. That is the git version number, and you can ignore it. > And documentation request diff -up > https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#diff-up > but patches here use another settings because diff -up never give me line > like > index 86638c1..f1b46b5 100644 > before file names but put me file date and time next to filename. So what > version of diff should I use? I have diff (GNU diffutils) 3.7 git creates that, if you use it for creating a diff. If you want to use diff "by hand", you can do that too, I was not objecting to that at all. The only problems I found was in your changelog text, and your email header. > > > > > Signed-off-by: Jaromir Skorpil <Jerry@jrr.cz> > > > > This does not match your From: line :( > > > I supposed that only mail address in From line matter? > > > I understand that real name is mandatory only for Signed-off-by field? > > It has to match the From: line of your email to ensure that this really > > is the same person. > Really? > I looked at another message as you advised and it seems that even YOUR name > often changes? > https://marc.info/?l=linux-usb&m=159257306831535 > https://marc.info/?l=linux-usb&m=159256948030250 Short name vs. "real name" doesn't matter much when sending email text responses, but it does when sending patches, as you are making a legal agreement about that signed-off-by text. So it has to be the same, otherwise I can not take your patch. > Why is a name so important when you can't verify it? Typing the same text > twice doesn't prove anything. In fact my real name can't be written in ascii > because of diacritics marks and I doubt that it will work here correctly if > I use unicode... Please use unicode (well utf-8 if possible), I'm all for that, there is no requirement to use ascii only for your name in patches. I wish more people would do that when needed, as it's only fair to them to be able to properly represent their names and not have to change them somehow. thanks, greg k-h
Greg Kroah-Hartman wrote on 6/21/20 3:58 PM: > Please use unicode (well utf-8 if possible), I'm all for that, there is > no requirement to use ascii only for your name in patches. I wish more > people would do that when needed, as it's only fair to them to be able > to properly represent their names and not have to change them somehow. I tried it and it seems that www.spinics.net understand UTF-8 but at marc.info it doesn't work correctly. https://marc.info/?l=linux-usb&m=159279589104617 It seems that this page don't send correct encoding to web browser so Firefox uses windows-1252 and insted of capital S with caron I can see A with ring. Similarily insted of I with acute accent the browser shows A with tilda... it looks horible. And even if I force UTF8 encoding for view, it corrects mail From but my name at Signed-off-by line stays damaged. So UTF-8 seems be a bad idea for kernel mailling list. Best regards Jerry / Jaromir Skorpil
On Mon, Jun 22, 2020 at 06:38:13AM +0200, Jerry wrote: > Greg Kroah-Hartman wrote on 6/21/20 3:58 PM: > > Please use unicode (well utf-8 if possible), I'm all for that, there is > > no requirement to use ascii only for your name in patches. I wish more > > people would do that when needed, as it's only fair to them to be able > > to properly represent their names and not have to change them somehow. > I tried it and it seems that www.spinics.net understand UTF-8 but at > marc.info it doesn't work correctly. > https://marc.info/?l=linux-usb&m=159279589104617 Why care about marc.info? We don't control that, nor do we use it. If lore.kernel.org does not work, please let us know, we can fix that. > It seems that this page don't send correct encoding to web browser so > Firefox uses windows-1252 and insted of capital S with caron I can see A > with ring. Similarily insted of I with acute accent the browser shows A with > tilda... it looks horible. And even if I force UTF8 encoding for view, it > corrects mail From but my name at Signed-off-by line stays damaged. > > So UTF-8 seems be a bad idea for kernel mailling list. It should not be, again, if lore.kernel.org does not work, please let us know. thanks, greg k-h
Greg Kroah-Hartman wrote on 6/22/20 7:30 AM: >> I tried it and it seems that www.spinics.net understand UTF-8 but at >> marc.info it doesn't work correctly. >> https://marc.info/?l=linux-usb&m=159279589104617 > Why care about marc.info? We don't control that, nor do we use it. > > If lore.kernel.org does not work, please let us know, we can fix that. When I subscribed to linux-usb mailing list, I received an e-mail from Majordomo@vger.kernel.org. And in this mail, there were only two links for archives: http://marc.info/?l=linux-usb http://www.spinics.net/lists/linux-usb/ So I supposed that these servers are officially connected with Kernel.org. I didn't know about an existence of lore.kernel.org until now. If I post a message to somewhere I want to be correctly displayed to all users... >> It seems that this page don't send correct encoding to web browser so >> Firefox uses windows-1252 and insted of capital S with caron I can see A >> with ring. Similarily insted of I with acute accent the browser shows A with >> tilda... it looks horible. And even if I force UTF8 encoding for view, it >> corrects mail From but my name at Signed-off-by line stays damaged. >> >> So UTF-8 seems be a bad idea for kernel mailling list. > It should not be, again, if lore.kernel.org does not work, please let > us know. > > thanks, > > greg k-h Anyway, thanks a lot for your patience. Jerry
diff -up linux-5.8-rc1/drivers/usb/serial/cp210x.c j/drivers/usb/serial/cp210x.c --- linux-5.8-rc1/drivers/usb/serial/cp210x.c 2020-06-14 21:45:04.000000000 +0200 +++ j/drivers/usb/serial/cp210x.c 2020-06-20 18:50:17.135843606 +0200 @@ -43,6 +43,8 @@ static int cp210x_tiocmget(struct tty_st static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int); static int cp210x_tiocmset_port(struct usb_serial_port *port, unsigned int, unsigned int); +static int cp210x_get_icount(struct tty_struct *tty, + struct serial_icounter_struct *icount); static void cp210x_break_ctl(struct tty_struct *, int); static int cp210x_attach(struct usb_serial *); static void cp210x_disconnect(struct usb_serial *); @@ -274,6 +276,7 @@ static struct usb_serial_driver cp210x_d .tx_empty = cp210x_tx_empty, .tiocmget = cp210x_tiocmget, .tiocmset = cp210x_tiocmset, + .get_icount = cp210x_get_icount, .attach = cp210x_attach, .disconnect = cp210x_disconnect, .release = cp210x_release, @@ -393,6 +396,13 @@ struct cp210x_comm_status { u8 bReserved; } __packed; +/* cp210x_comm_status::ulErrors */ +#define CP210X_SERIAL_ERR_BREAK BIT(0) +#define CP210X_SERIAL_ERR_FRAME BIT(1) +#define CP210X_SERIAL_ERR_HW_OVERRUN BIT(2) +#define CP210X_SERIAL_ERR_QUEUE_OVERRUN BIT(3) +#define CP210X_SERIAL_ERR_PARITY BIT(4) + /* * CP210X_PURGE - 16 bits passed in wValue of USB request. * SiLabs app note AN571 gives a strange description of the 4 bits: @@ -836,10 +846,10 @@ static void cp210x_close(struct usb_seri } /* - * Read how many bytes are waiting in the TX queue. + * Read how many bytes are waiting in the TX queue and update error counters. */ -static int cp210x_get_tx_queue_byte_count(struct usb_serial_port *port, - u32 *count) +static int cp210x_get_comm_status(struct usb_serial_port *port, + u32 *tx_count) { struct usb_serial *serial = port->serial; struct cp210x_port_private *port_priv = usb_get_serial_port_data(port); @@ -855,7 +865,18 @@ static int cp210x_get_tx_queue_byte_coun 0, port_priv->bInterfaceNumber, sts, sizeof(*sts), USB_CTRL_GET_TIMEOUT); if (result == sizeof(*sts)) { - *count = le32_to_cpu(sts->ulAmountInOutQueue); + if (tx_count) + *tx_count = le32_to_cpu(sts->ulAmountInOutQueue); + if (sts->ulErrors & CP210X_SERIAL_ERR_BREAK) + port->icount.brk++; + if (sts->ulErrors & CP210X_SERIAL_ERR_FRAME) + port->icount.frame++; + if (sts->ulErrors & CP210X_SERIAL_ERR_HW_OVERRUN) + port->icount.overrun++; + if (sts->ulErrors & CP210X_SERIAL_ERR_QUEUE_OVERRUN) + port->icount.buf_overrun++; + if (sts->ulErrors & CP210X_SERIAL_ERR_PARITY) + port->icount.parity++; result = 0; } else { dev_err(&port->dev, "failed to get comm status: %d\n", result); @@ -873,13 +894,26 @@ static bool cp210x_tx_empty(struct usb_s int err; u32 count; - err = cp210x_get_tx_queue_byte_count(port, &count); + err = cp210x_get_comm_status(port, &count); if (err) return true; return !count; } +static int cp210x_get_icount(struct tty_struct *tty, + struct serial_icounter_struct *icount) +{ + struct usb_serial_port *port = tty->driver_data; + int result; + + result = cp210x_get_comm_status(port, NULL); + if (result) + return result; + + return usb_serial_generic_get_icount(tty, icount); +} + /* * cp210x_get_termios * Reads the baud rate, data bits, parity, stop bits and flow control mode
usbserial: add cp210x support for icount to detect parity error in received data Motivation - current version of cp210x driver doesn't provide any way to detect a parity error in received data from userspace. Some serial protocols like STM32 bootloader protect data only by even parity so application needs to detect whether parity error happened to read again peripheral data. I created a simple patch which adds support for icount (ioctl TIOCGICOUNT) which sends GET_COMM_STATUS command to CP210X and according received flags increments fields for parity error, frame error, break and overrun. So application can detect an error condition after reading data from ttyUSB and repeat operation. There is no impact for applications which don't call ioctl TIOCGICOUNT. This patch is also placed at http://yyy.jrr.cz/cp210x.patch (my first patch) Signed-off-by: Jaromir Skorpil <Jerry@jrr.cz> ---