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.
I've had a chance to look closer at this, and I think your general
approach in this patch is the correct one, although there are some
things that need to be fixed in the patch first.
For the record, I've verified that it's only the SET_LINE_REQUEST call
that appears to be causing the problems.
> 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.
This isn't the primary issue. The biggest problem is that requesting the
*same* baud rate could result in data being dropped. Perhaps you could
update the description.
> The solution is to save and compare the encoded settings instead.
Indeed. I think this is preferred over comparing to settings read back
from the chip. I've verified that that would work as well, but for all
we know there could be chips that doesn't return correct data.
> Signed-off-by: Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxx
You used the wrong address here.
> ---
> 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 */
Just call it "line_settings", and you can drop the comment (or let it
read "encoded line settings" and make sure to use a tab before it).
> };
>
> 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;
> -
You should move the comment, but leave the no-changes check in place. It
is still useful in the most common case where the requested baud rate
will match the actually used one.
> 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.
> + */
So update the comment to something like:
/*
* Some PL2303 are known 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.
*
* Note that the tty_termios_hw_change check above is not sufficient
* as a previously requested baud rate may differ from the one
* actually used (and stored in old_termios).
*
* NOTE: No additional locking needed for line_settings as it is
* only used in set_termios, which is already protected by
* a semaphore.
*/
Please note the locking comment.
> + if (!memcmp(priv->line_settings_enc, buf, 7))
> + return;
As I said in my previous comment on the patch, this is not right as you
would fail to update the line control and flow control settings that
follow.
Instead you should make the set-line request below only if the buffers
do no not match. You also want to test whether set_termios is called
from open and do the call in that case as well. In summary, you need:
if (!old_termios || memcmp(buf, priv->line_settings, 7)) {
> +
> 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);
You got the memcpy-parameters switched here so the line_settings are
never updated.
>
> /* change control lines if we are switching to or from B0 */
> spin_lock_irqsave(&priv->lock, flags);
> --
> 1.7.10.4
Care to send an updated v2?
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
