Roger,
With the latest kernels, I experienced some strange corruption, some
'*****' being randomly inserted in the character flow, like this:
ashes:~#
ashes:~#
a*******shes:~#
ashes:~#
ashes:~#
Further investigation shows that the problem was introduced during
Alan's "TTY layer buffering revamp" patch, the amount of data to be
copied being reduced after buffer allocation. Moving the count fixup
around solves the problem.
Patch against v2.6.16-rc5.
Signed-off-by: Marc Zyngier <[email protected]>
diff --git a/drivers/char/sx.c b/drivers/char/sx.c
index 588e75e..a6b4f02 100644
--- a/drivers/char/sx.c
+++ b/drivers/char/sx.c
@@ -1095,17 +1095,17 @@ static inline void sx_receive_chars (str
sx_dprintk (SX_DEBUG_RECEIVE, "rxop=%d, c = %d.\n", rx_op, c);
+ /* Don't copy past the end of the hardware receive buffer */
+ if (rx_op + c > 0x100) c = 0x100 - rx_op;
+
+ sx_dprintk (SX_DEBUG_RECEIVE, "c = %d.\n", c);
+
/* Don't copy more bytes than there is room for in the buffer */
c = tty_prepare_flip_string(tty, &rp, c);
sx_dprintk (SX_DEBUG_RECEIVE, "c = %d.\n", c);
- /* Don't copy past the end of the hardware receive buffer */
- if (rx_op + c > 0x100) c = 0x100 - rx_op;
-
- sx_dprintk (SX_DEBUG_RECEIVE, "c = %d.\n", c);
-
/* If for one reason or another, we can't copy more data, we're done! */
if (c == 0) break;
--
And if you don't know where you're going, any road will take you there...
On Mon, Feb 27, 2006 at 12:08:00PM +0100, Marc Zyngier wrote:
> Roger,
>
> With the latest kernels, I experienced some strange corruption, some
> '*****' being randomly inserted in the character flow, like this:
>
> ashes:~#
> ashes:~#
> a*******shes:~#
> ashes:~#
> ashes:~#
>
> Further investigation shows that the problem was introduced during
> Alan's "TTY layer buffering revamp" patch, the amount of data to be
> copied being reduced after buffer allocation. Moving the count fixup
> around solves the problem.
>
> Patch against v2.6.16-rc5.
>
> Signed-off-by: Marc Zyngier <[email protected]>
>
> diff --git a/drivers/char/sx.c b/drivers/char/sx.c
> index 588e75e..a6b4f02 100644
> --- a/drivers/char/sx.c
> +++ b/drivers/char/sx.c
> @@ -1095,17 +1095,17 @@ static inline void sx_receive_chars (str
>
> sx_dprintk (SX_DEBUG_RECEIVE, "rxop=%d, c = %d.\n", rx_op, c);
>
> + /* Don't copy past the end of the hardware receive buffer */
> + if (rx_op + c > 0x100) c = 0x100 - rx_op;
> +
> + sx_dprintk (SX_DEBUG_RECEIVE, "c = %d.\n", c);
> +
> /* Don't copy more bytes than there is room for in the buffer */
>
> c = tty_prepare_flip_string(tty, &rp, c);
>
> sx_dprintk (SX_DEBUG_RECEIVE, "c = %d.\n", c);
>
> - /* Don't copy past the end of the hardware receive buffer */
> - if (rx_op + c > 0x100) c = 0x100 - rx_op;
> -
> - sx_dprintk (SX_DEBUG_RECEIVE, "c = %d.\n", c);
> -
The idea behind this code is that we start out with the maximum amount of
data we'd want to copy, and then reduce it to prevent copying more data
than we should for any other reason we can think of.
The reason I do it this way is that the hardware for instance may increase
the number of bytes that can be copied, just because another byte came in.
The rest of the system may INCREASE the number of bytes we can put in the buffer
because someone copied bytes out of the buffer or something like that
(probably doesn't happen like that because it's called a flip-buffer, but
whatever).
Both of these "races" are not really races: We'd end up copying less data this
time, and we'd copy the rest the next time.
So, if the modern flip buffer stuff expects us to provide the exact number
of bytes we prepared the flip buffer for we shouldn't adjust the number of chars
copied after preparing the flip buffer.
So indeed the check for the end of the hardware buffer needs to go before the
prepare flip string.
Approved!
Roger.
--
** [email protected] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement.
Does it sit on the couch all day? Is it unemployed? Please be specific!
Define 'it' and what it isn't doing. --------- Adapted from lxrbot FAQ