Re: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
Am 15.07.2015 um 13:16 schrieb Johan Hovold: Your changes caused a regression that was discovered mere days before 3.12 was released. At the time the reason had not been fully determined so the patches were consequently reverted. Simply not true, re-read the ML archieves. In that case I would of course have supported the revert. We both know the real reason. Good luck, Frank -- 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: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
Am 14.07.2015 um 22:29 schrieb Greg KH: On Tue, Jul 14, 2015 at 09:29:29PM +0200, Frank Schäfer wrote: If you want to pick this up and improve the divisor calculations that'd be great. Maybe you should just start doing your job as the maintainer and accept one of the patches people are sending to you to get this issue fixed ? That kind of attitude isn't acceptable here, sorry. Since at least 2012 lots of time and work has been invested into this by different people. I have not seen any patches not commented on about this issue that have not been reviewed or accepted in one way or another. But till today, there are still zero reviews and also no alternative patch from you. It's up to the person who has the issue to send in the fix for it, otherwise the code will be left alone as it is working for almost everyone as-is. Please refrain from lashing out if you wish to have patches applied, that is not how this works at all. greg k-h Right, makes no sense, excuse me. Please disregard this message. I will now shut up again as I did in the past. Regards, Frank -- 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: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
On Thu, Jul 16, 2015 at 10:12:55PM +0200, Frank Schäfer wrote: Simply not true, re-read the ML archieves. For those of us who don't understand, please explain. In that case I would of course have supported the revert. We both know the real reason. What is the real reason that you feel this happened? thanks, greg k-h -- 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: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
On Wed, Jul 08, 2015 at 12:51:03PM +0200, Michał Pecio wrote:
This commit fixes the following issues:
1. The 9th bit of buf was believed to be the LSB of divisor's
exponent, but the hardware interprets it as MSB (9th bit) of the
mantissa. The exponent is actually one bit shorter and applies
to base 4, not 2 as previously believed.
2. Loop iterations doubled the exponent instead of incrementing.
3. The exponent wasn't checked for overflow.
4. The function returned requested rate instead of actual rate.
Due to issue #2, the old code deviated from the wrong formula
described in #1 and actually yielded correct rates when divisor
was lower than 4096 by giving exponents of 0, 2 or 4 base-2,
interpreted as 0, 1, 2 base-4 with the 9th mantissa bit clear.
However, at 93.75 kbaud or less the rate turned out too slow
due to #1 and #2 or too fast due to #2 with #3.
I tested this patch by sending and validating 0x00,0x01,..,0xff
to an FTDI dongle at 234, 987, 2401, 9601, 31415, 115199, 250k,
500k, 750k, 1M, 1.5M, 3M+1 baud. All rates passed.
I also used pv to check speed at some rates unsupported by FTDI:
45 (the lowest possible), 2M, 4M, 5M and 6M-1. Looked sane.
Signed-off-by: Michal Pecio michal.pe...@gmail.com
---
drivers/usb/serial/pl2303.c | 32 ++--
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index f5257af..8141ccc 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -362,22 +362,34 @@ static speed_t pl2303_encode_baud_rate_direct(unsigned
char buf[4],
static speed_t pl2303_encode_baud_rate_divisor(unsigned char buf[4],
speed_t baud)
{
- unsigned int tmp;
-
/*
* Apparently the formula is:
- * baudrate = 12M * 32 / (2^buf[1]) / buf[0]
+ * baudrate = 12M * 32 / (mantissa * 4^exponent)
+ * where
+ * mantissa = buf[8:0]
+ * exponent = buf[11:9]
*/
- tmp = 1200 * 32 / baud;
+ u32 baseline = 1200 * 32;
+ u32 mantissa = baseline / baud;
+ u32 exponent = 0;
You'd get a checkpatch warning here about a missing new line after the
declarations (always run checkpatch.pl before submitting).
Please declare the variables at the top of the function and initialise
after the comment.
+ while (mantissa = 512) {
+ if (likely(exponent 7)) {
Don't use likely().
+ mantissa = 2; /* divide by 4 */
+ exponent++;
+ } else {
+ /* Exponent is maxed. Trim mantissa and leave. */
+ mantissa = 511;
+ break;
+ }
+ }
+
buf[3] = 0x80;
buf[2] = 0;
- buf[1] = (tmp = 256);
- while (tmp = 256) {
- tmp = 2;
- buf[1] = 1;
- }
- buf[0] = tmp;
+ buf[1] = (exponent 1) | (mantissa 8);
+ buf[0] = (mantissa 0xff);
No parentheses.
+ /* Calculate and return the exact baud rate. */
+ baud = (baseline / mantissa) (exponent 1);
return baud;
}
Looks nice and clean otherwise.
Were you already going to send a v2 or was this version complete?
Care to fix up the above style issues and resend so I can apply this for
4.2?
Thanks,
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
Re: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
On Tue, Jul 14, 2015 at 09:29:29PM +0200, Frank Schäfer wrote: Am 13.07.2015 um 18:47 schrieb Johan Hovold: On Mon, Jul 13, 2015 at 06:08:50PM +0200, Michał Pecio wrote: Commit 57ce61aad748 might be helpful... ;) Good luck, Frank Pretty much the same thing I have done, except that I didn't notice that 0 = 512 :) Apparently, 57ce61aad748 fell victim of a mass-revert caused by some underdebugged issues. Is it known what they were? Is there any chance of getting this driver fixed again? Yes, that series caused some regressions late in the release cycle and was reverted so that it could get some more review and testing. It was the baud rate divisor fix patch (only) which unveiled a single long standing bug in another part of the driver I offered you a simple and 100% safe solution for this (not reporting the actually set baud rate as before). Your changes caused a regression that was discovered mere days before 3.12 was released. At the time the reason had not been fully determined so the patches were consequently reverted. If you want to pick this up and improve the divisor calculations that'd be great. Maybe you should just start doing your job as the maintainer and accept one of the patches people are sending to you to get this issue fixed ? I see you're still upset over this almost two years later. Patches are always welcome if you change your mind. 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
Re: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
Looks nice and clean otherwise. Were you already going to send a v2 or was this version complete? I didn't intend to change anything. This works fine for me. Maybe proper rounding would be a nice addition, but I'm not sure if it's worth the effort. Now we can at least guarantee that the rate is no lower than requested :) I'll just fix the style and resend in a moment. -- 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: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
[ Please try to avoid top-posting. ]
On Tue, Jul 14, 2015 at 07:22:01PM +0200, Michał Pecio wrote:
I managed to reproduce this old issue, both on vanilla v4.1.1 and with
my patch, IF and ONLY if I reverted commit 623c82633 by changing:
- if (!old_termios || memcmp(buf, priv-line_settings, 7)) {
ret = pl2303_set_line_request(port, buf);
if (!ret)
memcpy(priv-line_settings, buf, 7);
- }
That was expected. Thanks for verifying.
Bottom line: my patch seems safe and fixes custom baud rates below 94k,
which are completely screwed without it.
The only thing I could imagine going wrong is chips which actually
interpret baud rate settings the way described in the old comment.
This definitely isn't my HX (rev A) nor my other HX knockoff.
This also isn't whatever chips Frank Schäfer used during 57ce61aad748
development.
Finally, this probably isn't this comment author's chip either. I bet
he wrote the comment and then randomly tweaked the code until it started
working with actual hardware without realizing that the comment is wrong
and doesn't describe the code anymore.
Sounds plausible. I'll take at look at your patch.
Thanks,
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
Re: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
I managed to reproduce this old issue, both on vanilla v4.1.1 and with
my patch, IF and ONLY if I reverted commit 623c82633 by changing:
- if (!old_termios || memcmp(buf, priv-line_settings, 7)) {
ret = pl2303_set_line_request(port, buf);
if (!ret)
memcpy(priv-line_settings, buf, 7);
- }
I also added 'printk(Hello)' to pl2303_set_line_request.
After that, spawning agetty on the pl2303, logging in through FTDI
and listing a directory with two dozen files indeed outputs mangled
filenames. Hello is printed twice for each ls.
No problems happen with getty on FTDI and minicom on pl2303.
The culprit seems to be bash, calling TCGETS/TCSETSW on its tty before
and after every command to toggle some flags (echo, icanon, ...).
Nothing bad happens with standard baud rates, i.e. bash still calls
these ioctls, but there are no Hellos and no corruption.
I also tried forcing divisor encoding on all baud rates with 623c82633
reverted, like it was done back in the days of 3.12-rc. In this
configuration, Hellos and corruptions happen for stardard baud rates
if and only if the driver reports baud rate different than requested.
In the end, my conlusions are as follows:
1. The corruption is caused by repeated attempts to set baud rate which
can't be exactly realized. It has nothing to do with divisor encoding.
2. 623c82633 protects us no matter what bytes we send to the chip
and what we return to the tty stack (until the userspace really sets a
different rate).
3. Without 623c82633, we can only protect ourselves by lying about the
actual rate. It seems that tty_termios_hw_change saves us in this case.
That's why encode_direct and pre-57ce61aad748 encode_divisor work, but
57ce61aad748 failed. It came before 623c82633.
Bottom line: my patch seems safe and fixes custom baud rates below 94k,
which are completely screwed without it.
The only thing I could imagine going wrong is chips which actually
interpret baud rate settings the way described in the old comment.
This definitely isn't my HX (rev A) nor my other HX knockoff.
This also isn't whatever chips Frank Schäfer used during 57ce61aad748
development.
Finally, this probably isn't this comment author's chip either. I bet
he wrote the comment and then randomly tweaked the code until it started
working with actual hardware without realizing that the comment is wrong
and doesn't describe the code anymore.
On Mon, Jul 13, 2015 at 06:08:50PM +0200, Michał Pecio wrote:
Commit 57ce61aad748 might be helpful... ;)
Good luck,
Frank
Pretty much the same thing I have done, except that I didn't notice
that 0 = 512 :)
Apparently, 57ce61aad748 fell victim of a mass-revert caused by some
underdebugged issues. Is it known what they were? Is there any
chance of getting this driver fixed again?
Yes, that series caused some regressions late in the release cycle and
was reverted so that it could get some more review and testing.
There are some details in this thread:
https://marc.info/?l=linux-usbm=138325299205954
If you want to pick this up and improve the divisor calculations
that'd be great.
Thanks,
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
Re: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
Am 14.07.2015 um 19:22 schrieb Michał Pecio:
I managed to reproduce this old issue, both on vanilla v4.1.1 and with
my patch, IF and ONLY if I reverted commit 623c82633 by changing:
- if (!old_termios || memcmp(buf, priv-line_settings, 7)) {
ret = pl2303_set_line_request(port, buf);
if (!ret)
memcpy(priv-line_settings, buf, 7);
- }
I also added 'printk(Hello)' to pl2303_set_line_request.
After that, spawning agetty on the pl2303, logging in through FTDI
and listing a directory with two dozen files indeed outputs mangled
filenames. Hello is printed twice for each ls.
No problems happen with getty on FTDI and minicom on pl2303.
The culprit seems to be bash, calling TCGETS/TCSETSW on its tty before
and after every command to toggle some flags (echo, icanon, ...).
Nothing bad happens with standard baud rates, i.e. bash still calls
these ioctls, but there are no Hellos and no corruption.
I also tried forcing divisor encoding on all baud rates with 623c82633
reverted, like it was done back in the days of 3.12-rc. In this
configuration, Hellos and corruptions happen for stardard baud rates
if and only if the driver reports baud rate different than requested.
In the end, my conlusions are as follows:
1. The corruption is caused by repeated attempts to set baud rate which
can't be exactly realized. It has nothing to do with divisor encoding.
2. 623c82633 protects us no matter what bytes we send to the chip
and what we return to the tty stack (until the userspace really sets a
different rate).
3. Without 623c82633, we can only protect ourselves by lying about the
actual rate. It seems that tty_termios_hw_change saves us in this case.
That's why encode_direct and pre-57ce61aad748 encode_divisor work, but
57ce61aad748 failed. It came before 623c82633.
Bottom line: my patch seems safe and fixes custom baud rates below 94k,
which are completely screwed without it.
The only thing I could imagine going wrong is chips which actually
interpret baud rate settings the way described in the old comment.
This definitely isn't my HX (rev A) nor my other HX knockoff.
This also isn't whatever chips Frank Schäfer used during 57ce61aad748
development.
100% correct.
Finally, this probably isn't this comment author's chip either. I bet
he wrote the comment and then randomly tweaked the code until it started
working with actual hardware without realizing that the comment is wrong
and doesn't describe the code anymore.
I assume he did some (incomplete) reverse-engineering.
The Windows driver (at that time) didn't support custom baud rates
115k. ;)
--
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: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
On Tue, Jul 14, 2015 at 09:29:29PM +0200, Frank Schäfer wrote: If you want to pick this up and improve the divisor calculations that'd be great. Maybe you should just start doing your job as the maintainer and accept one of the patches people are sending to you to get this issue fixed ? That kind of attitude isn't acceptable here, sorry. Since at least 2012 lots of time and work has been invested into this by different people. I have not seen any patches not commented on about this issue that have not been reviewed or accepted in one way or another. But till today, there are still zero reviews and also no alternative patch from you. It's up to the person who has the issue to send in the fix for it, otherwise the code will be left alone as it is working for almost everyone as-is. Please refrain from lashing out if you wish to have patches applied, that is not how this works at all. greg k-h -- 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: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
Am 13.07.2015 um 18:47 schrieb Johan Hovold: On Mon, Jul 13, 2015 at 06:08:50PM +0200, Michał Pecio wrote: Commit 57ce61aad748 might be helpful... ;) Good luck, Frank Pretty much the same thing I have done, except that I didn't notice that 0 = 512 :) Apparently, 57ce61aad748 fell victim of a mass-revert caused by some underdebugged issues. Is it known what they were? Is there any chance of getting this driver fixed again? Yes, that series caused some regressions late in the release cycle and was reverted so that it could get some more review and testing. It was the baud rate divisor fix patch (only) which unveiled a single long standing bug in another part of the driver I offered you a simple and 100% safe solution for this (not reporting the actually set baud rate as before). Instead (after suddenly taking over maintainership) you decided to needlessly revert the whole series to rediscuss things, retest and clean up things. These patches already had successfully completed the review and testing process, were applied by Greg (the maintainer at that time) and pulled by Linus. And during all these months of hard work, there had been _zero_ comments from you. If you want to pick this up and improve the divisor calculations that'd be great. Maybe you should just start doing your job as the maintainer and accept one of the patches people are sending to you to get this issue fixed ? Since at least 2012 lots of time and work has been invested into this by different people. But till today, there are still zero reviews and also no alternative patch from you. Your chance. -- 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: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
Am 13.07.2015 um 18:08 schrieb Michał Pecio: Commit 57ce61aad748 might be helpful... ;) Good luck, Frank Pretty much the same thing I have done, except that I didn't notice that 0 = 512 :) :) Apparently, 57ce61aad748 fell victim of a mass-revert caused by some underdebugged issues. Is it known what they were? Is there any chance of getting this driver fixed again? It had been debugged 100% at that time. The line that fixed reporting of the actually set baud rate unveiled a long standing bug in the driver (see comment in pl2303_set_termios()). That has been fixed in the meantime with commit 623c82633. Just reapply 57ce61aad748 or your patch an everything should be fine. -- 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: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
Am 08.07.2015 um 12:51 schrieb Michał Pecio:
This commit fixes the following issues:
1. The 9th bit of buf was believed to be the LSB of divisor's
exponent, but the hardware interprets it as MSB (9th bit) of the
mantissa. The exponent is actually one bit shorter and applies
to base 4, not 2 as previously believed.
2. Loop iterations doubled the exponent instead of incrementing.
3. The exponent wasn't checked for overflow.
4. The function returned requested rate instead of actual rate.
Due to issue #2, the old code deviated from the wrong formula
described in #1 and actually yielded correct rates when divisor
was lower than 4096 by giving exponents of 0, 2 or 4 base-2,
interpreted as 0, 1, 2 base-4 with the 9th mantissa bit clear.
However, at 93.75 kbaud or less the rate turned out too slow
due to #1 and #2 or too fast due to #2 with #3.
I tested this patch by sending and validating 0x00,0x01,..,0xff
to an FTDI dongle at 234, 987, 2401, 9601, 31415, 115199, 250k,
500k, 750k, 1M, 1.5M, 3M+1 baud. All rates passed.
I also used pv to check speed at some rates unsupported by FTDI:
45 (the lowest possible), 2M, 4M, 5M and 6M-1. Looked sane.
Signed-off-by: Michal Pecio michal.pe...@gmail.com
---
drivers/usb/serial/pl2303.c | 32 ++--
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index f5257af..8141ccc 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -362,22 +362,34 @@ static speed_t pl2303_encode_baud_rate_direct(unsigned
char buf[4],
static speed_t pl2303_encode_baud_rate_divisor(unsigned char buf[4],
speed_t baud)
{
- unsigned int tmp;
-
/*
* Apparently the formula is:
- * baudrate = 12M * 32 / (2^buf[1]) / buf[0]
+ * baudrate = 12M * 32 / (mantissa * 4^exponent)
+ * where
+ * mantissa = buf[8:0]
+ * exponent = buf[11:9]
*/
- tmp = 1200 * 32 / baud;
+ u32 baseline = 1200 * 32;
+ u32 mantissa = baseline / baud;
+ u32 exponent = 0;
+ while (mantissa = 512) {
+ if (likely(exponent 7)) {
+ mantissa = 2; /* divide by 4 */
+ exponent++;
+ } else {
+ /* Exponent is maxed. Trim mantissa and leave. */
+ mantissa = 511;
+ break;
+ }
+ }
+
buf[3] = 0x80;
buf[2] = 0;
- buf[1] = (tmp = 256);
- while (tmp = 256) {
- tmp = 2;
- buf[1] = 1;
- }
- buf[0] = tmp;
+ buf[1] = (exponent 1) | (mantissa 8);
+ buf[0] = (mantissa 0xff);
+ /* Calculate and return the exact baud rate. */
+ baud = (baseline / mantissa) (exponent 1);
return baud;
}
Commit 57ce61aad748 might be helpful... ;)
Good luck,
Frank
--
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: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
On Mon, Jul 13, 2015 at 06:08:50PM +0200, Michał Pecio wrote: Commit 57ce61aad748 might be helpful... ;) Good luck, Frank Pretty much the same thing I have done, except that I didn't notice that 0 = 512 :) Apparently, 57ce61aad748 fell victim of a mass-revert caused by some underdebugged issues. Is it known what they were? Is there any chance of getting this driver fixed again? Yes, that series caused some regressions late in the release cycle and was reverted so that it could get some more review and testing. There are some details in this thread: https://marc.info/?l=linux-usbm=138325299205954 If you want to pick this up and improve the divisor calculations that'd be great. Thanks, 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
Re: [PATCH] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
Commit 57ce61aad748 might be helpful... ;) Good luck, Frank Pretty much the same thing I have done, except that I didn't notice that 0 = 512 :) Apparently, 57ce61aad748 fell victim of a mass-revert caused by some underdebugged issues. Is it known what they were? Is there any chance of getting this driver fixed again? -- 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
