Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
Another candidate for removal is drivers/char/pcmcia/synclink_cs.c Everything I said about synclink.c/synclinkmp.c is true of that as well: the hardware stopped being manufactured decades ago and is not available for testing. The possibility of these cards still being around/functional for use with the latest kernel is about zero. If Lee Jones does wants to add that to his patch, great. If not then I can do so. (I resent this message in plain text after it was rejected for HTML, sorry if anyone got a duplicate.) > On Nov 5, 2020, at 3:27 AM, Lee Jones wrote: > > Will post the removal patches when my tests finish.
Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
> On Nov 5, 2020, at 1:10 AM, Jiri Slaby wrote: > > OK, let the loop alone. I would bet a half a pig that noone is able to test > this driver. But one has to write this for someone to raise and admit they > are still using it. In fact, there are _4_ google replies to "Microgate > Corporation" "SyncLink Multiport Adapter" "lspci". The hardware used with synclink.c and synclinkmp.c has not been manufactured for 15 years and was low volume. The chances of either driver still being in use is very low. Not even Microgate (me) has the ability to test either anymore (no hardware). I don’t know the policy about driver removal, but I think both could be removed without upsetting anyone. synclink_gt.c is still in production and the driver still actively used. If there are no objections to removing the the old drivers (synclink.c/synclink_mp.c) I could make a patch to do so tomorrow (it is 1:30am here now). Nothing eliminates niggling warnings like removing dead code.
Re: [PATCH] tty/n_hdlc: fix sleep in !TASK_RUNNING state warning
> On Jan 4, 2019, at 2:23 AM, Tetsuo Handa > wrote: > > But why not to clarify what are appropriate sanity checks? > ... > want a cleanup for scripts/checkpatch.pl . These are good goals. I avoid purely cosmetic patches. I do not object to cosmetic patches from others that do not change behavior. The checks that concern you deal with changing tty line disciplines. Dealing with line discipline changes has been an ongoing issue since n_hdlc was derived from other line disciplines 20 years ago, with major overhauls along the way. It is complex: driver layers shifting during operation while dealing properly with opens, closes, hangups, and sleeping operations. Patches have been added to the latest unreleased kernel to address line discipline changes, it is still evolving. Why are the existing line discipline checks in n_hdlc where they are? Becasue that’s how they evolved from where they started to accomodate these changes. There are not many and their function is known: has the line discipline changed at that point? I know that is not satisfying but coming up with a definitive comment saying a check is absolutely required in one place and not in another requires more insight into the long history of a moving target than I have. Without that insight I would not alter existing checks in code that is not causing problems.
Re: [PATCH] tty/n_hdlc: fix sleep in !TASK_RUNNING state warning
> On Jan 3, 2019, at 3:32 AM, Tetsuo Handa > wrote: > > On 2019/01/03 18:09, Jiri Slaby wrote: >> On 02. 01. 19, 16:04, Tetsuo Handa wrote: >>> + if (wait_event_interruptible(tty->read_wait, >>> +(ret = -EIO, test_bit(TTY_OTHER_CLOSED, >flags)) || >>> +(ret = 0, tty_hung_up_p(file)) || >>> +(rbuf = n_hdlc_buf_get(_hdlc->rx_buf_list)) != NULL || >>> +(ret = -EAGAIN, tty_io_nonblock(tty, file >>> + return -EINTR; >> >> Oh, that looks really ugly. Could you move all this to a function >> returning a bool and taking and as parameters? >> > > OK. Something like this? I agree with Jiri that placing all the conditional logic in a single expression is difficult to read. But exchanging that many locals with a separate function defeats the original purpose of simplifying code and this implementation changes the logic (write no longer checks for line discipline changing under it during wait). Converting to wait_event_interruptible where possible is a good goal but this instance may be better left alone. The current structure mirrors the existing n_tty line discipline.
Re: [PATCH] tty/n_hdlc: fix sleep in !TASK_RUNNING state warning
> On Jan 2, 2019, at 7:04 AM, Tetsuo Handa > wrote: > > On 2019/01/01 12:11, Paul Fulghum wrote: >> NAK to this patch. It causes lost wakeups in both read and write paths. >> >> The write path does not need changing. >> >> The read path can be fixed by setting current to TASK_RUNNING at the top of >> the if (rbuf) block >> so the warning is not triggered by copy_to_user(). If this block runs the >> condition is satisfied >> and it breaks out of the polling loop where it is already being set to >> TASK_RUNNING and removed >> from the wait queue. This particular path just needs to account for the >> copy_to_user which occurs >> before breaking out. >> >> I’ll make a patch to do this when I have the ability to test it in a day or >> two. >> > > OK. Then, any chance it is rewritten using wait_event_interruptible() in > order to reduce lines? > ( wait_event_interruptible() automatically calls might_sleep(), but is it > acceptable for you? ) > This looks good to me. I applied it and tested blocking (sleep/no sleep) and non-blocking (success/EAGAIN) paths for both read and write.
[PATCH] tty/n_hdlc: fix __might_sleep warning
Fix __might_sleep warning in tty/n_hdlc.c read due to copy_to_user call while
current is TASK_INTERRUPTIBLE.
This is a false positive since the code path does not depend on current state
remaining TASK_INTERRUPTIBLE.
The loop breaks out and sets TASK_RUNNING after calling copy_to_user.
This patch supresses the warning by setting TASK_RUNNING before calling
copy_to_user.
[1]
https://syzkaller.appspot.com/bug?id=17d5de7f1fcab794cb8c40032f893f52de899324
Signed-off-by: Paul Fulghum
Reported-by: syzbot
Cc: Greg Kroah-Hartman
Cc: Tetsuo Handa
Cc: Arnd Bergmann
Cc: Alan Cox
—
--- a/drivers/tty/n_hdlc.c 2018-12-23 15:55:59.0 -0800
+++ b/drivers/tty/n_hdlc.c 2019-01-01 11:44:47.148153954 -0800
@@ -597,6 +597,7 @@ static ssize_t n_hdlc_tty_read(struct tt
/* too large for caller's buffer */
ret = -EOVERFLOW;
} else {
+ __set_current_state(TASK_RUNNING);
if (copy_to_user(buf, rbuf->buf, rbuf->count))
ret = -EFAULT;
else
Re: [PATCH] tty/n_hdlc: fix sleep in !TASK_RUNNING state warning
On Dec 31, 2018, at 7:11 PM, Paul Fulghum wrote:
NAK to this patch. It causes lost wakeups in both read and write paths.
The write path does not need changing.
The read path can be fixed by setting current to TASK_RUNNING at the top of the
if (rbuf) block so the warning is not triggered by copy_to_user(). If this
block runs the condition is satisfied and it breaks out of the polling loop
where it is already being set to TASK_RUNNING and removed from the wait queue.
This particular path just needs to account for the copy_to_user which occurs
before breaking out.
I’ll make a patch to do this when I have the ability to test it in a day or two.
> On Dec 29, 2018, at 3:48 AM, Tetsuo Handa
> wrote:
>
> syzbot is hitting __might_sleep() warning [1], for commit 1035b63d3c6fc34a
> ("n_hdlc: fix read and write locking") changed to set TASK_INTERRUPTIBLE
> state before calling copy_to_user(). Let's set TASK_INTERRUPTIBLE state
> immediately before calling schedule().
>
> [1]
> https://syzkaller.appspot.com/bug?id=17d5de7f1fcab794cb8c40032f893f52de899324
>
> Signed-off-by: Tetsuo Handa
> Reported-by: syzbot
> Cc: Paul Fulghum
> Cc: Arnd Bergmann
> Cc: Alan Cox
> ---
> drivers/tty/n_hdlc.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
> index dabb391..7835489 100644
> --- a/drivers/tty/n_hdlc.c
> +++ b/drivers/tty/n_hdlc.c
> @@ -589,8 +589,6 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty,
> struct file *file,
> if (tty_hung_up_p(file))
> break;
>
> - set_current_state(TASK_INTERRUPTIBLE);
> -
> rbuf = n_hdlc_buf_get(_hdlc->rx_buf_list);
> if (rbuf) {
> if (rbuf->count > nr) {
> @@ -617,6 +615,7 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty,
> struct file *file,
> break;
> }
>
> + set_current_state(TASK_INTERRUPTIBLE);
> schedule();
>
> if (signal_pending(current)) {
> @@ -673,8 +672,6 @@ static ssize_t n_hdlc_tty_write(struct tty_struct *tty,
> struct file *file,
> add_wait_queue(>write_wait, );
>
> for (;;) {
> - set_current_state(TASK_INTERRUPTIBLE);
> -
> tbuf = n_hdlc_buf_get(_hdlc->tx_free_buf_list);
> if (tbuf)
> break;
> @@ -683,6 +680,8 @@ static ssize_t n_hdlc_tty_write(struct tty_struct *tty,
> struct file *file,
> error = -EAGAIN;
> break;
> }
> +
> + set_current_state(TASK_INTERRUPTIBLE);
> schedule();
>
> n_hdlc = tty2n_hdlc (tty);
> --
> 1.8.3.1
>
>
--
Paul Fulghum
MicroGate Systems, Ltd.
=Customer Driven, by Design=
(512) 345-7791 x102 (Voice)
(512) 343-9046 (Fax)
Central Time Zone (GMT -5h)
www.microgate.com
[PATCH] synclink fix ldisc buffer argument
Fix call to line discipline receive_buf by synclink drivers.
Dummy flag buffer argument is ignored by N_HDLC line discipline but might
be of insufficient size if accessed by a different line discipline
selected by mistake. flag buffer allocation now matches max size of data
buffer. Unused char_buf buffers are removed.
Signed-off-by: Paul Fulghum
--- a/drivers/char/pcmcia/synclink_cs.c 2012-11-26 14:15:45.0 -0600
+++ b/drivers/char/pcmcia/synclink_cs.c 2012-12-03 10:51:40.0 -0600
@@ -210,7 +210,7 @@ typedef struct _mgslpc_info {
char testing_irq;
unsigned int init_error;/* startup error (DIAGS)*/
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
+ char *flag_buf;
bool drop_rts_on_tx_done;
struct _input_signal_eventsinput_signal_events;
@@ -2666,6 +2666,14 @@ static int rx_alloc_buffers(MGSLPC_INFO
if (info->rx_buf == NULL)
return -ENOMEM;
+ /* unused flag buffer to satisfy receive_buf calling interface */
+ info->flag_buf = kzalloc(info->max_frame_size, GFP_KERNEL);
+ if (!info->flag_buf) {
+ kfree(info->rx_buf);
+ info->rx_buf = NULL;
+ return -ENOMEM;
+ }
+
rx_reset_buffers(info);
return 0;
}
@@ -2674,6 +2682,8 @@ static void rx_free_buffers(MGSLPC_INFO
{
kfree(info->rx_buf);
info->rx_buf = NULL;
+ kfree(info->flag_buf);
+ info->flag_buf = NULL;
}
static int claim_resources(MGSLPC_INFO *info)
--- a/drivers/tty/synclink.c2012-11-26 14:15:45.0 -0600
+++ b/drivers/tty/synclink.c2012-12-03 10:51:56.0 -0600
@@ -291,8 +291,7 @@ struct mgsl_struct {
bool lcr_mem_requested;
u32 misc_ctrl_value;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
+ char *flag_buf;
bool drop_rts_on_tx_done;
bool loopmode_insert_requested;
@@ -3891,7 +3890,13 @@ static int mgsl_alloc_intermediate_rxbuf
info->intermediate_rxbuffer = kmalloc(info->max_frame_size, GFP_KERNEL
| GFP_DMA);
if ( info->intermediate_rxbuffer == NULL )
return -ENOMEM;
-
+ /* unused flag buffer to satisfy receive_buf calling interface */
+ info->flag_buf = kzalloc(info->max_frame_size, GFP_KERNEL);
+ if (!info->flag_buf) {
+ kfree(info->intermediate_rxbuffer);
+ info->intermediate_rxbuffer = NULL;
+ return -ENOMEM;
+ }
return 0;
} /* end of mgsl_alloc_intermediate_rxbuffer_memory() */
@@ -3910,6 +3915,8 @@ static void mgsl_free_intermediate_rxbuf
{
kfree(info->intermediate_rxbuffer);
info->intermediate_rxbuffer = NULL;
+ kfree(info->flag_buf);
+ info->flag_buf = NULL;
} /* end of mgsl_free_intermediate_rxbuffer_memory() */
--- a/drivers/tty/synclinkmp.c 2012-11-26 14:15:45.0 -0600
+++ b/drivers/tty/synclinkmp.c 2012-12-03 10:52:09.0 -0600
@@ -262,8 +262,7 @@ typedef struct _synclinkmp_info {
bool sca_statctrl_requested;
u32 misc_ctrl_value;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
+ char *flag_buf;
bool drop_rts_on_tx_done;
struct _input_signal_eventsinput_signal_events;
@@ -3544,6 +3543,13 @@ static int alloc_tmp_rx_buf(SLMP_INFO *i
info->tmp_rx_buf = kmalloc(info->max_frame_size, GFP_KERNEL);
if (info->tmp_rx_buf == NULL)
return -ENOMEM;
+ /* unused flag buffer to satisfy receive_buf calling interface */
+ info->flag_buf = kzalloc(info->max_frame_size, GFP_KERNEL);
+ if (!info->flag_buf) {
+ kfree(info->tmp_rx_buf);
+ info->tmp_rx_buf = NULL;
+ return -ENOMEM;
+ }
return 0;
}
@@ -3551,6 +3557,8 @@ static void free_tmp_rx_buf(SLMP_INFO *i
{
kfree(info->tmp_rx_buf);
info->tmp_rx_buf = NULL;
+ kfree(info->flag_buf);
+ info->flag_buf = NULL;
}
static int claim_resources(SLMP_INFO *info)
--- a/drivers/tty/synclink_gt.c 2012-11-26 14:15:45.0 -0600
+++ b/drivers/tty/synclink_gt.c 2012-12-03 11:00:15.0 -0600
@@ -317,8 +317,7 @@ struct slgt_info {
unsigned char *tx_buf;
int tx_count;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
+ char *flag_buf;
bool drop_rts_on_tx_done;
struct _input_signal_eventsinput_signal_events;
@@ -3355,11 +3354,24 @@ static int block_til_ready(struct tty_st
return retval;
}
+/*
+ * allocate buffers used for calling line discipline receive_buf
+ * directly in synchronous mode
+ * note: add 5 bytes to max frame size to allow appending
+ * 32-bit CRC and status byte when configured to
Re: [PATCH] synclink fix ldisc buffer argument
On 12/2/2012 8:20 PM, Chen Gang wrote: > pardon (I am just learning) > does 65535 mean HDLC_MAX_FRAME_SIZE ? > why do we need info->max_frame_size >= 4096 ? > in drivers/tty/synclink_gt.c: > 3550 if (info->max_frame_size < 4096) > 3551 info->max_frame_size = 4096; > 3552 else if (info->max_frame_size > 65535) > 3553 info->max_frame_size = 65535; > 3554 > ... > 3603 info->max_frame_size = 4096; The hardware can send and receive HDLC frames up to 64K in size. The driver defaults to 4K max frame size to save buffer space for the common case (line 3603 in alloc_dev()). The module parameter max_frame_size can override the default in add_device() (lines 3550-3554 are from add_device() range checking the module parameter) > if possible: > can we move the relative comments (which are inside function) to the > location just above ldisc_receive_buf ? The added comment from my first patch described the reuse of the data buffer as the flag buffer. Alan prefers to use a zero initialized dummy buffer for the flag buffer argument. Doing it that way, the comment is not needed. -- Paul Fulghum MicroGate Systems, Ltd. =Customer Driven, by Design= (800)444-1982 (US Sales) (512)345-7791 x102 (Direct) (512)343-9046 (Fax) Central Time Zone (GMT -6h) www.microgate.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink fix ldisc buffer argument
On 12/2/2012 8:20 PM, Chen Gang wrote: pardon (I am just learning) does 65535 mean HDLC_MAX_FRAME_SIZE ? why do we need info-max_frame_size = 4096 ? in drivers/tty/synclink_gt.c: 3550 if (info-max_frame_size 4096) 3551 info-max_frame_size = 4096; 3552 else if (info-max_frame_size 65535) 3553 info-max_frame_size = 65535; 3554 ... 3603 info-max_frame_size = 4096; The hardware can send and receive HDLC frames up to 64K in size. The driver defaults to 4K max frame size to save buffer space for the common case (line 3603 in alloc_dev()). The module parameter max_frame_size can override the default in add_device() (lines 3550-3554 are from add_device() range checking the module parameter) if possible: can we move the relative comments (which are inside function) to the location just above ldisc_receive_buf ? The added comment from my first patch described the reuse of the data buffer as the flag buffer. Alan prefers to use a zero initialized dummy buffer for the flag buffer argument. Doing it that way, the comment is not needed. -- Paul Fulghum MicroGate Systems, Ltd. =Customer Driven, by Design= (800)444-1982 (US Sales) (512)345-7791 x102 (Direct) (512)343-9046 (Fax) Central Time Zone (GMT -6h) www.microgate.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] synclink fix ldisc buffer argument
Fix call to line discipline receive_buf by synclink drivers.
Dummy flag buffer argument is ignored by N_HDLC line discipline but might
be of insufficient size if accessed by a different line discipline
selected by mistake. flag buffer allocation now matches max size of data
buffer. Unused char_buf buffers are removed.
Signed-off-by: Paul Fulghum pau...@microgate.com
--- a/drivers/char/pcmcia/synclink_cs.c 2012-11-26 14:15:45.0 -0600
+++ b/drivers/char/pcmcia/synclink_cs.c 2012-12-03 10:51:40.0 -0600
@@ -210,7 +210,7 @@ typedef struct _mgslpc_info {
char testing_irq;
unsigned int init_error;/* startup error (DIAGS)*/
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
+ char *flag_buf;
bool drop_rts_on_tx_done;
struct _input_signal_eventsinput_signal_events;
@@ -2666,6 +2666,14 @@ static int rx_alloc_buffers(MGSLPC_INFO
if (info-rx_buf == NULL)
return -ENOMEM;
+ /* unused flag buffer to satisfy receive_buf calling interface */
+ info-flag_buf = kzalloc(info-max_frame_size, GFP_KERNEL);
+ if (!info-flag_buf) {
+ kfree(info-rx_buf);
+ info-rx_buf = NULL;
+ return -ENOMEM;
+ }
+
rx_reset_buffers(info);
return 0;
}
@@ -2674,6 +2682,8 @@ static void rx_free_buffers(MGSLPC_INFO
{
kfree(info-rx_buf);
info-rx_buf = NULL;
+ kfree(info-flag_buf);
+ info-flag_buf = NULL;
}
static int claim_resources(MGSLPC_INFO *info)
--- a/drivers/tty/synclink.c2012-11-26 14:15:45.0 -0600
+++ b/drivers/tty/synclink.c2012-12-03 10:51:56.0 -0600
@@ -291,8 +291,7 @@ struct mgsl_struct {
bool lcr_mem_requested;
u32 misc_ctrl_value;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
+ char *flag_buf;
bool drop_rts_on_tx_done;
bool loopmode_insert_requested;
@@ -3891,7 +3890,13 @@ static int mgsl_alloc_intermediate_rxbuf
info-intermediate_rxbuffer = kmalloc(info-max_frame_size, GFP_KERNEL
| GFP_DMA);
if ( info-intermediate_rxbuffer == NULL )
return -ENOMEM;
-
+ /* unused flag buffer to satisfy receive_buf calling interface */
+ info-flag_buf = kzalloc(info-max_frame_size, GFP_KERNEL);
+ if (!info-flag_buf) {
+ kfree(info-intermediate_rxbuffer);
+ info-intermediate_rxbuffer = NULL;
+ return -ENOMEM;
+ }
return 0;
} /* end of mgsl_alloc_intermediate_rxbuffer_memory() */
@@ -3910,6 +3915,8 @@ static void mgsl_free_intermediate_rxbuf
{
kfree(info-intermediate_rxbuffer);
info-intermediate_rxbuffer = NULL;
+ kfree(info-flag_buf);
+ info-flag_buf = NULL;
} /* end of mgsl_free_intermediate_rxbuffer_memory() */
--- a/drivers/tty/synclinkmp.c 2012-11-26 14:15:45.0 -0600
+++ b/drivers/tty/synclinkmp.c 2012-12-03 10:52:09.0 -0600
@@ -262,8 +262,7 @@ typedef struct _synclinkmp_info {
bool sca_statctrl_requested;
u32 misc_ctrl_value;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
+ char *flag_buf;
bool drop_rts_on_tx_done;
struct _input_signal_eventsinput_signal_events;
@@ -3544,6 +3543,13 @@ static int alloc_tmp_rx_buf(SLMP_INFO *i
info-tmp_rx_buf = kmalloc(info-max_frame_size, GFP_KERNEL);
if (info-tmp_rx_buf == NULL)
return -ENOMEM;
+ /* unused flag buffer to satisfy receive_buf calling interface */
+ info-flag_buf = kzalloc(info-max_frame_size, GFP_KERNEL);
+ if (!info-flag_buf) {
+ kfree(info-tmp_rx_buf);
+ info-tmp_rx_buf = NULL;
+ return -ENOMEM;
+ }
return 0;
}
@@ -3551,6 +3557,8 @@ static void free_tmp_rx_buf(SLMP_INFO *i
{
kfree(info-tmp_rx_buf);
info-tmp_rx_buf = NULL;
+ kfree(info-flag_buf);
+ info-flag_buf = NULL;
}
static int claim_resources(SLMP_INFO *info)
--- a/drivers/tty/synclink_gt.c 2012-11-26 14:15:45.0 -0600
+++ b/drivers/tty/synclink_gt.c 2012-12-03 11:00:15.0 -0600
@@ -317,8 +317,7 @@ struct slgt_info {
unsigned char *tx_buf;
int tx_count;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
+ char *flag_buf;
bool drop_rts_on_tx_done;
struct _input_signal_eventsinput_signal_events;
@@ -3355,11 +3354,24 @@ static int block_til_ready(struct tty_st
return retval;
}
+/*
+ * allocate buffers used for calling line discipline receive_buf
+ * directly in synchronous mode
+ * note: add 5 bytes to max frame size to allow appending
+ * 32-bit CRC and status byte when configured to do so
+ */
static int alloc_tmp_rbuf(struct slgt_info *info)
{
info-tmp_rbuf = kmalloc(info-max_frame_size + 5
[PATCH] synclink fix ldisc buffer argument
Fix call to line discipline receive_buf by synclink drivers.
Dummy flag buffer argument is ignored by N_HDLC line discipline but might
be of insufficient size if accessed by a different line discipline
selected by mistake. Calls are changed to use data buffer argument for
both data and flag buffer so valid memory is provided if the wrong
line discipline is used. Unused char_buf and flag_buf are removed.
Signed-off-by: Paul Fulghum
--- a/drivers/char/pcmcia/synclink_cs.c 2012-11-26 14:15:45.0 -0600
+++ b/drivers/char/pcmcia/synclink_cs.c 2012-11-30 12:50:23.0 -0600
@@ -210,7 +210,6 @@ typedef struct _mgslpc_info {
char testing_irq;
unsigned int init_error;/* startup error (DIAGS)*/
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
bool drop_rts_on_tx_done;
struct _input_signal_eventsinput_signal_events;
@@ -3707,7 +3706,16 @@ static bool rx_get_frame(MGSLPC_INFO *in
hdlcdev_rx(info, buf->data, framesize);
else
#endif
- ldisc_receive_buf(tty, buf->data,
info->flag_buf, framesize);
+ {
+ /*
+* Call N_HDLC line discipline directly to
maintain
+* frame boundaries. Reuse the data buffer
argument for the
+* flag buffer argument. The flag buffer is
ignored by N_HDLC.
+* If a different line discipline is selected
by mistake it
+* will have valid memory for both arguments.
+*/
+ ldisc_receive_buf(tty, buf->data, buf->data,
framesize);
+ }
}
}
--- a/drivers/tty/synclink.c2012-11-26 14:15:45.0 -0600
+++ b/drivers/tty/synclink.c2012-11-30 12:59:29.0 -0600
@@ -291,8 +291,6 @@ struct mgsl_struct {
bool lcr_mem_requested;
u32 misc_ctrl_value;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
bool drop_rts_on_tx_done;
bool loopmode_insert_requested;
@@ -6661,7 +6659,17 @@ static bool mgsl_get_rx_frame(struct mgs
hdlcdev_rx(info,info->intermediate_rxbuffer,framesize);
else
#endif
- ldisc_receive_buf(tty,
info->intermediate_rxbuffer, info->flag_buf, framesize);
+ {
+ /*
+* Call N_HDLC line discipline directly to
maintain
+* frame boundaries. Reuse the data buffer
argument for the
+* flag buffer argument. The flag buffer is
ignored by N_HDLC.
+* If a different line discipline is selected
by mistake it
+* will have valid memory for both arguments.
+*/
+ ldisc_receive_buf(tty,
info->intermediate_rxbuffer,
+ info->intermediate_rxbuffer,
framesize);
+ }
}
}
/* Free the buffers used by this frame. */
@@ -6833,7 +6841,15 @@ static bool mgsl_get_raw_rx_frame(struct
memcpy( info->intermediate_rxbuffer,
pBufEntry->virt_addr, framesize);
info->icount.rxok++;
- ldisc_receive_buf(tty, info->intermediate_rxbuffer,
info->flag_buf, framesize);
+ /*
+* Call N_HDLC line discipline directly to maintain
+* block boundaries. Reuse the data buffer argument for
the
+* flag buffer argument. The flag buffer is ignored by
N_HDLC.
+* If a different line discipline is selected by
mistake it
+* will have valid memory for both arguments.
+*/
+ ldisc_receive_buf(tty, info->intermediate_rxbuffer,
+ info->intermediate_rxbuffer,
framesize);
}
/* Free the buffers used by this frame. */
--- a/drivers/tty/synclinkmp.c 2012-11-26 14:15:45.0 -0600
+++ b/drivers/tty/synclinkmp.c 2012-11-30 13:01:36.0 -0600
@@ -262,8 +262,6 @@ typedef struct _synclinkmp_info {
bool sca_statctrl_requested;
u32 misc_ctrl_value;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
bool drop_rts_on_tx_done;
struct _input_signal_eventsinput_signal_events;
@@ -4979,8 +4977,17 @@ CheckAgain:
hdlcdev_rx(info,info->tmp_r
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
On 11/29/2012 8:52 PM, Chen Gang wrote: > 于 2012年11月30日 02:32, Greg KH 写道: >> On Thu, Nov 29, 2012 at 01:57:59PM +0800, Chen Gang wrote: >>>> And, I really don't understand here, why do you want to change this? >>>> What is it going to change? And why? >>> >>> Why: >>> for the context MGSLPC_INFO *info in drivers/char/pcmcia/synclink_cs.c >>> info->max_frame_size can be the value between 4096 .. 65535 (can be >>> set by its module input parameter) >>> info->flag_buf length is 4096 (MAX_ASYNC_BUFFER_SIZE) >>> in function rx_get_frame >>> the framesize is limit by info->max_frame_size, but may still be >>> larger that 4096. >>> when call function ldisc_receive_buf, info->flag_buf is equal to >>> 4096, but framesize can be more than 4096. it will cause memory over flow. The confusion centers on calling the line discipline receive_buf function with a data buffer larger than the flag buffer. The synclink drivers support asynchronous and synchronous (HDLC) serial communications. In asynchronous mode, the tty flip buffer is used to feed data to the line discipline. In this mode, the above argument does not apply. The receive_buf function is not called directly. In synchronous mode, the driver calls the line discipline receive_buf function directly to feed one HDLC frame of data per call. Maintaining frame boundaries is needed in this mode. This is done only with the N_HDLC line discipline which expects this format and ignores the flag buffer. The flag buffer passed is just a place holder to meet the calling conventions of the line discipline receive_buf function. The only danger is if: 1. driver is configured for synchronous mode 2. driver is configured for frames > 4K 3. line discipline other than N_HDLC is selected In this case the line discipline might try to access beyond the end of the flag buffer. This is a non-functional configuration that would not occur on purpose. Increasing the flag buffer size would prevent a problem in this degenerate case of purposeful misconfiguration. This would be at the expense of larger allocations that are not used. I think the correct fix is for me to change the direct calls to pass the same buffer for both data and flag and add a comment describing the fact the flag buffer is ignored when using N_HDLC. That way a misconfigured setup won't cause problems and no unneeded allocations are made. My suggestion is to leave it as is for now until I can make those changes. I admit the current code is ugly enough to cause confusion (sorry Chen Gang), but I don't see any immediate danger. -- Paul Fulghum MicroGate Systems, Ltd. =Customer Driven, by Design= (800)444-1982 (US Sales) (512)345-7791 x102 (Direct) (512)343-9046 (Fax) Central Time Zone (GMT -6h) www.microgate.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
On 11/29/2012 8:52 PM, Chen Gang wrote: 于 2012年11月30日 02:32, Greg KH 写道: On Thu, Nov 29, 2012 at 01:57:59PM +0800, Chen Gang wrote: And, I really don't understand here, why do you want to change this? What is it going to change? And why? Why: for the context MGSLPC_INFO *info in drivers/char/pcmcia/synclink_cs.c info-max_frame_size can be the value between 4096 .. 65535 (can be set by its module input parameter) info-flag_buf length is 4096 (MAX_ASYNC_BUFFER_SIZE) in function rx_get_frame the framesize is limit by info-max_frame_size, but may still be larger that 4096. when call function ldisc_receive_buf, info-flag_buf is equal to 4096, but framesize can be more than 4096. it will cause memory over flow. The confusion centers on calling the line discipline receive_buf function with a data buffer larger than the flag buffer. The synclink drivers support asynchronous and synchronous (HDLC) serial communications. In asynchronous mode, the tty flip buffer is used to feed data to the line discipline. In this mode, the above argument does not apply. The receive_buf function is not called directly. In synchronous mode, the driver calls the line discipline receive_buf function directly to feed one HDLC frame of data per call. Maintaining frame boundaries is needed in this mode. This is done only with the N_HDLC line discipline which expects this format and ignores the flag buffer. The flag buffer passed is just a place holder to meet the calling conventions of the line discipline receive_buf function. The only danger is if: 1. driver is configured for synchronous mode 2. driver is configured for frames 4K 3. line discipline other than N_HDLC is selected In this case the line discipline might try to access beyond the end of the flag buffer. This is a non-functional configuration that would not occur on purpose. Increasing the flag buffer size would prevent a problem in this degenerate case of purposeful misconfiguration. This would be at the expense of larger allocations that are not used. I think the correct fix is for me to change the direct calls to pass the same buffer for both data and flag and add a comment describing the fact the flag buffer is ignored when using N_HDLC. That way a misconfigured setup won't cause problems and no unneeded allocations are made. My suggestion is to leave it as is for now until I can make those changes. I admit the current code is ugly enough to cause confusion (sorry Chen Gang), but I don't see any immediate danger. -- Paul Fulghum MicroGate Systems, Ltd. =Customer Driven, by Design= (800)444-1982 (US Sales) (512)345-7791 x102 (Direct) (512)343-9046 (Fax) Central Time Zone (GMT -6h) www.microgate.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] synclink fix ldisc buffer argument
Fix call to line discipline receive_buf by synclink drivers.
Dummy flag buffer argument is ignored by N_HDLC line discipline but might
be of insufficient size if accessed by a different line discipline
selected by mistake. Calls are changed to use data buffer argument for
both data and flag buffer so valid memory is provided if the wrong
line discipline is used. Unused char_buf and flag_buf are removed.
Signed-off-by: Paul Fulghum pau...@microgate.com
--- a/drivers/char/pcmcia/synclink_cs.c 2012-11-26 14:15:45.0 -0600
+++ b/drivers/char/pcmcia/synclink_cs.c 2012-11-30 12:50:23.0 -0600
@@ -210,7 +210,6 @@ typedef struct _mgslpc_info {
char testing_irq;
unsigned int init_error;/* startup error (DIAGS)*/
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
bool drop_rts_on_tx_done;
struct _input_signal_eventsinput_signal_events;
@@ -3707,7 +3706,16 @@ static bool rx_get_frame(MGSLPC_INFO *in
hdlcdev_rx(info, buf-data, framesize);
else
#endif
- ldisc_receive_buf(tty, buf-data,
info-flag_buf, framesize);
+ {
+ /*
+* Call N_HDLC line discipline directly to
maintain
+* frame boundaries. Reuse the data buffer
argument for the
+* flag buffer argument. The flag buffer is
ignored by N_HDLC.
+* If a different line discipline is selected
by mistake it
+* will have valid memory for both arguments.
+*/
+ ldisc_receive_buf(tty, buf-data, buf-data,
framesize);
+ }
}
}
--- a/drivers/tty/synclink.c2012-11-26 14:15:45.0 -0600
+++ b/drivers/tty/synclink.c2012-11-30 12:59:29.0 -0600
@@ -291,8 +291,6 @@ struct mgsl_struct {
bool lcr_mem_requested;
u32 misc_ctrl_value;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
bool drop_rts_on_tx_done;
bool loopmode_insert_requested;
@@ -6661,7 +6659,17 @@ static bool mgsl_get_rx_frame(struct mgs
hdlcdev_rx(info,info-intermediate_rxbuffer,framesize);
else
#endif
- ldisc_receive_buf(tty,
info-intermediate_rxbuffer, info-flag_buf, framesize);
+ {
+ /*
+* Call N_HDLC line discipline directly to
maintain
+* frame boundaries. Reuse the data buffer
argument for the
+* flag buffer argument. The flag buffer is
ignored by N_HDLC.
+* If a different line discipline is selected
by mistake it
+* will have valid memory for both arguments.
+*/
+ ldisc_receive_buf(tty,
info-intermediate_rxbuffer,
+ info-intermediate_rxbuffer,
framesize);
+ }
}
}
/* Free the buffers used by this frame. */
@@ -6833,7 +6841,15 @@ static bool mgsl_get_raw_rx_frame(struct
memcpy( info-intermediate_rxbuffer,
pBufEntry-virt_addr, framesize);
info-icount.rxok++;
- ldisc_receive_buf(tty, info-intermediate_rxbuffer,
info-flag_buf, framesize);
+ /*
+* Call N_HDLC line discipline directly to maintain
+* block boundaries. Reuse the data buffer argument for
the
+* flag buffer argument. The flag buffer is ignored by
N_HDLC.
+* If a different line discipline is selected by
mistake it
+* will have valid memory for both arguments.
+*/
+ ldisc_receive_buf(tty, info-intermediate_rxbuffer,
+ info-intermediate_rxbuffer,
framesize);
}
/* Free the buffers used by this frame. */
--- a/drivers/tty/synclinkmp.c 2012-11-26 14:15:45.0 -0600
+++ b/drivers/tty/synclinkmp.c 2012-11-30 13:01:36.0 -0600
@@ -262,8 +262,6 @@ typedef struct _synclinkmp_info {
bool sca_statctrl_requested;
u32 misc_ctrl_value;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
bool drop_rts_on_tx_done;
struct _input_signal_eventsinput_signal_events;
@@ -4979,8 +4977,17 @@ CheckAgain:
hdlcdev_rx(info,info-tmp_rx_buf,framesize);
else
#endif
Re: serial port regression caused by "Char: tty_ioctl, use wait_event_interruptible_timeout" patch
Jiri Slaby wrote: It should be fixed already as of git-describe db99247a v2.6.24-rc6-95-gdb99247 So since 2.6.24-rc7. Maybe we should consider the fix for 2.6.23 too. Whoops, I missed that. Problem solved :-) Thanks, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial port regression caused by "Char: tty_ioctl, use wait_event_interruptible_timeout" patch
Rick Warner wrote: This modification solved my problem. Will this change go into mainline, or will we need to maintain our own branch of the kernel to keep this working? It should be accepted into mainline as it restores the original behavior. I'll put together a patch submission tomorrow unless Jiri beats me to it. Thanks, Paul -- Paul Fulghum Microgate Systems, Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial port regression caused by "Char: tty_ioctl, use wait_event_interruptible_timeout" patch
Paul Fulghum wrote: Instead of reverting the patch can you try modifying this part of the patch: + if (wait_event_interruptible_timeout(tty->write_wait, + !tty->driver->chars_in_buffer(tty), timeout)) + return; by changing it to: + if (wait_event_interruptible_timeout(tty->write_wait, + !tty->driver->chars_in_buffer(tty), timeout) < 0) + return; It looks like the patch changed the behavior of tty_wait_until_sent by not calling the driver specific wait_until_sent if a timeout occurs. I mispoke, the patch changed the behavior by not calling the driver specific wait_until_sent if the condition is true. Original behavior: call driver->wait_until_sent() on timeout or true condition (skip for signal) Patch behavior: call driver->wait_until_sent() only on timeout (rc == 0) (skip for signal or true) By modifying the patch as described above, the original behavior is restored. -- Paul Fulghum Microgate Systems, Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial port regression caused by "Char: tty_ioctl, use wait_event_interruptible_timeout" patch
Rick Warner wrote: I narrowed down the problem doing a binary search on git snapshots between .22 and .23, and found the breakage between git6 and git7. Further isolating it found the patch mentioned in the subject to be the cause. I reversed the patch in the .23 source and it now works properly. Should the code be reverted back as I did, or is there something I should change in our userspace code that reads from the serial port to correct it instead? Instead of reverting the patch can you try modifying this part of the patch: + if (wait_event_interruptible_timeout(tty->write_wait, + !tty->driver->chars_in_buffer(tty), timeout)) + return; by changing it to: + if (wait_event_interruptible_timeout(tty->write_wait, + !tty->driver->chars_in_buffer(tty), timeout) < 0) + return; It looks like the patch changed the behavior of tty_wait_until_sent by not calling the driver specific wait_until_sent if a timeout occurs. -- Paul Fulghum Microgate Systems, Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial port regression caused by Char: tty_ioctl, use wait_event_interruptible_timeout patch
Rick Warner wrote: I narrowed down the problem doing a binary search on git snapshots between .22 and .23, and found the breakage between git6 and git7. Further isolating it found the patch mentioned in the subject to be the cause. I reversed the patch in the .23 source and it now works properly. Should the code be reverted back as I did, or is there something I should change in our userspace code that reads from the serial port to correct it instead? Instead of reverting the patch can you try modifying this part of the patch: + if (wait_event_interruptible_timeout(tty-write_wait, + !tty-driver-chars_in_buffer(tty), timeout)) + return; by changing it to: + if (wait_event_interruptible_timeout(tty-write_wait, + !tty-driver-chars_in_buffer(tty), timeout) 0) + return; It looks like the patch changed the behavior of tty_wait_until_sent by not calling the driver specific wait_until_sent if a timeout occurs. -- Paul Fulghum Microgate Systems, Ltd. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial port regression caused by Char: tty_ioctl, use wait_event_interruptible_timeout patch
Paul Fulghum wrote: Instead of reverting the patch can you try modifying this part of the patch: + if (wait_event_interruptible_timeout(tty-write_wait, + !tty-driver-chars_in_buffer(tty), timeout)) + return; by changing it to: + if (wait_event_interruptible_timeout(tty-write_wait, + !tty-driver-chars_in_buffer(tty), timeout) 0) + return; It looks like the patch changed the behavior of tty_wait_until_sent by not calling the driver specific wait_until_sent if a timeout occurs. I mispoke, the patch changed the behavior by not calling the driver specific wait_until_sent if the condition is true. Original behavior: call driver-wait_until_sent() on timeout or true condition (skip for signal) Patch behavior: call driver-wait_until_sent() only on timeout (rc == 0) (skip for signal or true) By modifying the patch as described above, the original behavior is restored. -- Paul Fulghum Microgate Systems, Ltd. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial port regression caused by Char: tty_ioctl, use wait_event_interruptible_timeout patch
Rick Warner wrote: This modification solved my problem. Will this change go into mainline, or will we need to maintain our own branch of the kernel to keep this working? It should be accepted into mainline as it restores the original behavior. I'll put together a patch submission tomorrow unless Jiri beats me to it. Thanks, Paul -- Paul Fulghum Microgate Systems, Ltd. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial port regression caused by Char: tty_ioctl, use wait_event_interruptible_timeout patch
Jiri Slaby wrote: It should be fixed already as of git-describe db99247a v2.6.24-rc6-95-gdb99247 So since 2.6.24-rc7. Maybe we should consider the fix for 2.6.23 too. Whoops, I missed that. Problem solved :-) Thanks, Paul -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] synclink_gt fix missed serial input signal changes
Fix missed serial input signal changes caused by rereading the
serial status register during interrupt processing. Now
processing is performed on original status register value.
Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]>
--- a/drivers/char/synclink_gt.c2008-01-24 16:58:37.0 -0600
+++ b/drivers/char/synclink_gt.c2008-01-28 12:22:22.0 -0600
@@ -2040,37 +2040,41 @@ static void bh_transmit(struct slgt_info
tty_wakeup(tty);
}
-static void dsr_change(struct slgt_info *info)
+static void dsr_change(struct slgt_info *info, unsigned short status)
{
- get_signals(info);
+ if (status & BIT3) {
+ info->signals |= SerialSignal_DSR;
+ info->input_signal_events.dsr_up++;
+ } else {
+ info->signals &= ~SerialSignal_DSR;
+ info->input_signal_events.dsr_down++;
+ }
DBGISR(("dsr_change %s signals=%04X\n", info->device_name,
info->signals));
if ((info->dsr_chkcount)++ == IO_PIN_SHUTDOWN_LIMIT) {
slgt_irq_off(info, IRQ_DSR);
return;
}
info->icount.dsr++;
- if (info->signals & SerialSignal_DSR)
- info->input_signal_events.dsr_up++;
- else
- info->input_signal_events.dsr_down++;
wake_up_interruptible(>status_event_wait_q);
wake_up_interruptible(>event_wait_q);
info->pending_bh |= BH_STATUS;
}
-static void cts_change(struct slgt_info *info)
+static void cts_change(struct slgt_info *info, unsigned short status)
{
- get_signals(info);
+ if (status & BIT2) {
+ info->signals |= SerialSignal_CTS;
+ info->input_signal_events.cts_up++;
+ } else {
+ info->signals &= ~SerialSignal_CTS;
+ info->input_signal_events.cts_down++;
+ }
DBGISR(("cts_change %s signals=%04X\n", info->device_name,
info->signals));
if ((info->cts_chkcount)++ == IO_PIN_SHUTDOWN_LIMIT) {
slgt_irq_off(info, IRQ_CTS);
return;
}
info->icount.cts++;
- if (info->signals & SerialSignal_CTS)
- info->input_signal_events.cts_up++;
- else
- info->input_signal_events.cts_down++;
wake_up_interruptible(>status_event_wait_q);
wake_up_interruptible(>event_wait_q);
info->pending_bh |= BH_STATUS;
@@ -2091,20 +2095,21 @@ static void cts_change(struct slgt_info
}
}
-static void dcd_change(struct slgt_info *info)
+static void dcd_change(struct slgt_info *info, unsigned short status)
{
- get_signals(info);
+ if (status & BIT1) {
+ info->signals |= SerialSignal_DCD;
+ info->input_signal_events.dcd_up++;
+ } else {
+ info->signals &= ~SerialSignal_DCD;
+ info->input_signal_events.dcd_down++;
+ }
DBGISR(("dcd_change %s signals=%04X\n", info->device_name,
info->signals));
if ((info->dcd_chkcount)++ == IO_PIN_SHUTDOWN_LIMIT) {
slgt_irq_off(info, IRQ_DCD);
return;
}
info->icount.dcd++;
- if (info->signals & SerialSignal_DCD) {
- info->input_signal_events.dcd_up++;
- } else {
- info->input_signal_events.dcd_down++;
- }
#if SYNCLINK_GENERIC_HDLC
if (info->netcount) {
if (info->signals & SerialSignal_DCD)
@@ -2127,20 +2132,21 @@ static void dcd_change(struct slgt_info
}
}
-static void ri_change(struct slgt_info *info)
+static void ri_change(struct slgt_info *info, unsigned short status)
{
- get_signals(info);
+ if (status & BIT0) {
+ info->signals |= SerialSignal_RI;
+ info->input_signal_events.ri_up++;
+ } else {
+ info->signals &= ~SerialSignal_RI;
+ info->input_signal_events.ri_down++;
+ }
DBGISR(("ri_change %s signals=%04X\n", info->device_name,
info->signals));
if ((info->ri_chkcount)++ == IO_PIN_SHUTDOWN_LIMIT) {
slgt_irq_off(info, IRQ_RI);
return;
}
- info->icount.dcd++;
- if (info->signals & SerialSignal_RI) {
- info->input_signal_events.ri_up++;
- } else {
- info->input_signal_events.ri_down++;
- }
+ info->icount.rng++;
wake_up_interruptible(>status_event_wait_q);
wake_up_interruptible(>event_wait_q);
info->pending_bh |= BH_STATUS;
@@ -2191,13 +2197,13 @@ static void isr_serial(struct slgt_info
}
if (status & IRQ_DSR)
- dsr_change(info);
+
[PATCH] synclink_gt fix missed serial input signal changes
Fix missed serial input signal changes caused by rereading the
serial status register during interrupt processing. Now
processing is performed on original status register value.
Signed-off-by: Paul Fulghum [EMAIL PROTECTED]
--- a/drivers/char/synclink_gt.c2008-01-24 16:58:37.0 -0600
+++ b/drivers/char/synclink_gt.c2008-01-28 12:22:22.0 -0600
@@ -2040,37 +2040,41 @@ static void bh_transmit(struct slgt_info
tty_wakeup(tty);
}
-static void dsr_change(struct slgt_info *info)
+static void dsr_change(struct slgt_info *info, unsigned short status)
{
- get_signals(info);
+ if (status BIT3) {
+ info-signals |= SerialSignal_DSR;
+ info-input_signal_events.dsr_up++;
+ } else {
+ info-signals = ~SerialSignal_DSR;
+ info-input_signal_events.dsr_down++;
+ }
DBGISR((dsr_change %s signals=%04X\n, info-device_name,
info-signals));
if ((info-dsr_chkcount)++ == IO_PIN_SHUTDOWN_LIMIT) {
slgt_irq_off(info, IRQ_DSR);
return;
}
info-icount.dsr++;
- if (info-signals SerialSignal_DSR)
- info-input_signal_events.dsr_up++;
- else
- info-input_signal_events.dsr_down++;
wake_up_interruptible(info-status_event_wait_q);
wake_up_interruptible(info-event_wait_q);
info-pending_bh |= BH_STATUS;
}
-static void cts_change(struct slgt_info *info)
+static void cts_change(struct slgt_info *info, unsigned short status)
{
- get_signals(info);
+ if (status BIT2) {
+ info-signals |= SerialSignal_CTS;
+ info-input_signal_events.cts_up++;
+ } else {
+ info-signals = ~SerialSignal_CTS;
+ info-input_signal_events.cts_down++;
+ }
DBGISR((cts_change %s signals=%04X\n, info-device_name,
info-signals));
if ((info-cts_chkcount)++ == IO_PIN_SHUTDOWN_LIMIT) {
slgt_irq_off(info, IRQ_CTS);
return;
}
info-icount.cts++;
- if (info-signals SerialSignal_CTS)
- info-input_signal_events.cts_up++;
- else
- info-input_signal_events.cts_down++;
wake_up_interruptible(info-status_event_wait_q);
wake_up_interruptible(info-event_wait_q);
info-pending_bh |= BH_STATUS;
@@ -2091,20 +2095,21 @@ static void cts_change(struct slgt_info
}
}
-static void dcd_change(struct slgt_info *info)
+static void dcd_change(struct slgt_info *info, unsigned short status)
{
- get_signals(info);
+ if (status BIT1) {
+ info-signals |= SerialSignal_DCD;
+ info-input_signal_events.dcd_up++;
+ } else {
+ info-signals = ~SerialSignal_DCD;
+ info-input_signal_events.dcd_down++;
+ }
DBGISR((dcd_change %s signals=%04X\n, info-device_name,
info-signals));
if ((info-dcd_chkcount)++ == IO_PIN_SHUTDOWN_LIMIT) {
slgt_irq_off(info, IRQ_DCD);
return;
}
info-icount.dcd++;
- if (info-signals SerialSignal_DCD) {
- info-input_signal_events.dcd_up++;
- } else {
- info-input_signal_events.dcd_down++;
- }
#if SYNCLINK_GENERIC_HDLC
if (info-netcount) {
if (info-signals SerialSignal_DCD)
@@ -2127,20 +2132,21 @@ static void dcd_change(struct slgt_info
}
}
-static void ri_change(struct slgt_info *info)
+static void ri_change(struct slgt_info *info, unsigned short status)
{
- get_signals(info);
+ if (status BIT0) {
+ info-signals |= SerialSignal_RI;
+ info-input_signal_events.ri_up++;
+ } else {
+ info-signals = ~SerialSignal_RI;
+ info-input_signal_events.ri_down++;
+ }
DBGISR((ri_change %s signals=%04X\n, info-device_name,
info-signals));
if ((info-ri_chkcount)++ == IO_PIN_SHUTDOWN_LIMIT) {
slgt_irq_off(info, IRQ_RI);
return;
}
- info-icount.dcd++;
- if (info-signals SerialSignal_RI) {
- info-input_signal_events.ri_up++;
- } else {
- info-input_signal_events.ri_down++;
- }
+ info-icount.rng++;
wake_up_interruptible(info-status_event_wait_q);
wake_up_interruptible(info-event_wait_q);
info-pending_bh |= BH_STATUS;
@@ -2191,13 +2197,13 @@ static void isr_serial(struct slgt_info
}
if (status IRQ_DSR)
- dsr_change(info);
+ dsr_change(info, status);
if (status IRQ_CTS)
- cts_change(info);
+ cts_change(info, status);
if (status IRQ_DCD)
- dcd_change(info);
+ dcd_change(info, status);
if (status IRQ_RI)
- ri_change(info);
+ ri_change(info
Re: [PATCH 2/2] Char: tty, add tty_schedule_wakeup
Jiri Slaby wrote: + * Functionally the same as tty_wakeup, but it can be used in hot + * paths. since the wakeup is scheduled and done in the future. I'm not familiar with the terminology 'hot paths', what do you mean by that? Do you have an example of where you intend to use this new facility? The patch does not include such an example so it is difficult for me to see why you are adding this function. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Char: tty, add tty_schedule_wakeup
Jiri Slaby wrote: + * Functionally the same as tty_wakeup, but it can be used in hot + * paths. since the wakeup is scheduled and done in the future. I'm not familiar with the terminology 'hot paths', what do you mean by that? Do you have an example of where you intend to use this new facility? The patch does not include such an example so it is difficult for me to see why you are adding this function. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] synclink_gt fix module reference
Get module reference on open() by generic HDLC
to prevent module from unloading while interface
is active.
Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]>
--- a/drivers/char/synclink_gt.c2007-07-08 18:32:17.0 -0500
+++ b/drivers/char/synclink_gt.c2007-08-14 12:27:50.0 -0500
@@ -1570,6 +1570,9 @@ static int hdlcdev_open(struct net_devic
int rc;
unsigned long flags;
+ if (!try_module_get(THIS_MODULE))
+ return -EBUSY;
+
DBGINFO(("%s hdlcdev_open\n", dev->name));
/* generic HDLC layer open processing */
@@ -1639,6 +1642,7 @@ static int hdlcdev_close(struct net_devi
info->netcount=0;
spin_unlock_irqrestore(>netlock, flags);
+ module_put(THIS_MODULE);
return 0;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] synclink_gt fix module reference
Get module reference on open() by generic HDLC to prevent module from unloading while interface is active. Signed-off-by: Paul Fulghum [EMAIL PROTECTED] --- a/drivers/char/synclink_gt.c2007-07-08 18:32:17.0 -0500 +++ b/drivers/char/synclink_gt.c2007-08-14 12:27:50.0 -0500 @@ -1570,6 +1570,9 @@ static int hdlcdev_open(struct net_devic int rc; unsigned long flags; + if (!try_module_get(THIS_MODULE)) + return -EBUSY; + DBGINFO((%s hdlcdev_open\n, dev-name)); /* generic HDLC layer open processing */ @@ -1639,6 +1642,7 @@ static int hdlcdev_close(struct net_devi info-netcount=0; spin_unlock_irqrestore(info-netlock, flags); + module_put(THIS_MODULE); return 0; } - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 55] drivers/char/n_hdlc.c: kmalloc + memset conversion to kzalloc
Krzysztof Halasa wrote: Mariusz Kozlowski <[EMAIL PROTECTED]> writes: Signed-off-by: Mariusz Kozlowski <[EMAIL PROTECTED]> drivers/char/n_hdlc.c | 27451 -> 27413 (-38 bytes) drivers/char/n_hdlc.o | 107068 -> 107088 (+20 bytes) BTW: drivers/char/n_hdlc is a very different thing than these in drivers/net/wan/ and I have no connection with it. Not sure who is responsible, if anyone. I am, my email is at the top of n_hdlc.c but there is no maintainers entry. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Serial buffer memory leak
On Wed, 2007-08-08 at 16:32 +0100, Alan Cox wrote: > Ok try this for size folks. Use tty->flags bits for the flush status. > Wait for the flag to clear again before returning > Fix the doc error noted > Fix flush of empty queue leaving stale flushpending > > > Signed-off-by: Alan Cox <[EMAIL PROTECTED]> It looks good and clean. I compiled and tested the patch with interleaved tcflush() and read() calls, allowing random amounts of receive data to accumulate between each call. Acked-by: Paul Fulghum <[EMAIL PROTECTED]> Thanks, Paul -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Serial buffer memory leak
On Wed, 2007-08-08 at 16:28 +0200, Laurent Pinchart wrote: > The patch fixes the problem (at least under the test conditions that lead me > to discover it in the first place). Thanks Alan. It works here also, but I see a problem. By deferring the flush, ioctl(TCFLSH) returns immediately even though there is possibly still receive data being fed to the ldisc. If this is followed immediately by a read() then the application gets unexpected (stale) data defeating the purpose of the TCFLSH. tty_buffer_flush() needs to wait for buf.flushpending to clear. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Serial buffer memory leak
On Wed, 2007-08-08 at 14:45 +0100, Alan Cox wrote: > > I'm not familiar enough with the tty code to decide what the proper fix > > should > > be. I'll try to write a patch if someone could point me in the right > > direction. > > Something like this perhaps ? That looks good, a little better than the solution I was first considering. I'm compiling now. This was a nasty bug for me to introduce :-( Good work in finding this Laurent. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Serial buffer memory leak
Laurent Pinchart wrote: Patch c5c34d4862e18ef07c1276d233507f540fb5a532 (tty: flush flip buffer on ldisc input queue flush) introduces a race condition which can lead to memory leaks. The problem can be triggered when tcflush() is called when data are being pushed to the line discipline driver by flush_to_ldisc(). flush_to_ldisc() releases tty->buf.lock when calling the line discipline receive_buf function. At that poing tty_buffer_flush() kicks in and sets both tty->buf.head and tty->buf.tail to NULL. When flush_to_ldisc() finishes, it restores tty->buf.head but doesn't touch tty->buf.tail. This corrups the buffer queue, and the next call to tty_buffer_request_room() will allocate a new buffer and overwrite tty->buf.head. The previous buffer is then lost forever without being released. Your description is clear enough, I'll make the patch. Thanks, Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Serial buffer memory leak
Laurent Pinchart wrote: Patch c5c34d4862e18ef07c1276d233507f540fb5a532 (tty: flush flip buffer on ldisc input queue flush) introduces a race condition which can lead to memory leaks. The problem can be triggered when tcflush() is called when data are being pushed to the line discipline driver by flush_to_ldisc(). flush_to_ldisc() releases tty-buf.lock when calling the line discipline receive_buf function. At that poing tty_buffer_flush() kicks in and sets both tty-buf.head and tty-buf.tail to NULL. When flush_to_ldisc() finishes, it restores tty-buf.head but doesn't touch tty-buf.tail. This corrups the buffer queue, and the next call to tty_buffer_request_room() will allocate a new buffer and overwrite tty-buf.head. The previous buffer is then lost forever without being released. Your description is clear enough, I'll make the patch. Thanks, Paul - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Serial buffer memory leak
On Wed, 2007-08-08 at 14:45 +0100, Alan Cox wrote: I'm not familiar enough with the tty code to decide what the proper fix should be. I'll try to write a patch if someone could point me in the right direction. Something like this perhaps ? That looks good, a little better than the solution I was first considering. I'm compiling now. This was a nasty bug for me to introduce :-( Good work in finding this Laurent. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Serial buffer memory leak
On Wed, 2007-08-08 at 16:28 +0200, Laurent Pinchart wrote: The patch fixes the problem (at least under the test conditions that lead me to discover it in the first place). Thanks Alan. It works here also, but I see a problem. By deferring the flush, ioctl(TCFLSH) returns immediately even though there is possibly still receive data being fed to the ldisc. If this is followed immediately by a read() then the application gets unexpected (stale) data defeating the purpose of the TCFLSH. tty_buffer_flush() needs to wait for buf.flushpending to clear. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Serial buffer memory leak
On Wed, 2007-08-08 at 16:32 +0100, Alan Cox wrote: Ok try this for size folks. Use tty-flags bits for the flush status. Wait for the flag to clear again before returning Fix the doc error noted Fix flush of empty queue leaving stale flushpending Signed-off-by: Alan Cox [EMAIL PROTECTED] It looks good and clean. I compiled and tested the patch with interleaved tcflush() and read() calls, allowing random amounts of receive data to accumulate between each call. Acked-by: Paul Fulghum [EMAIL PROTECTED] Thanks, Paul -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 55] drivers/char/n_hdlc.c: kmalloc + memset conversion to kzalloc
Krzysztof Halasa wrote: Mariusz Kozlowski [EMAIL PROTECTED] writes: Signed-off-by: Mariusz Kozlowski [EMAIL PROTECTED] drivers/char/n_hdlc.c | 27451 - 27413 (-38 bytes) drivers/char/n_hdlc.o | 107068 - 107088 (+20 bytes) BTW: drivers/char/n_hdlc is a very different thing than these in drivers/net/wan/ and I have no connection with it. Not sure who is responsible, if anyone. I am, my email is at the top of n_hdlc.c but there is no maintainers entry. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
2.2.5 is using the same UART setup (trigger level of 8) as the current code. There is no obvious difference in the interrupt setup (same devices on the same interrupts). So I have no helpful suggestions :-( -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
2.2.5 is using the same UART setup (trigger level of 8) as the current code. There is no obvious difference in the interrupt setup (same devices on the same interrupts). So I have no helpful suggestions :-( -- Paul - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
Lee Howard wrote: And in repeat tests it is quite evident that IDE disk activity is, indeed, at least part of the problem. As IDE disk activity increases an increased amount of data coming in on the serial port goes missing. Lee, you mentioned 2.2.x kernels did not exhibit this problem. Was this on the same hardware you are currently testing? Which 2.2.x version were you using? Was the 2.2.x serial driver also identifying the UART as a 16550A? Can you get /proc/interrupts output from both the current setup and the 2.2.x setup? It would be interesting to compare the interrupt assignment and UART setup between the versions. -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
Lee Howard wrote: And in repeat tests it is quite evident that IDE disk activity is, indeed, at least part of the problem. As IDE disk activity increases an increased amount of data coming in on the serial port goes missing. Lee, you mentioned 2.2.x kernels did not exhibit this problem. Was this on the same hardware you are currently testing? Which 2.2.x version were you using? Was the 2.2.x serial driver also identifying the UART as a 16550A? Can you get /proc/interrupts output from both the current setup and the 2.2.x setup? It would be interesting to compare the interrupt assignment and UART setup between the versions. -- Paul - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
On Fri, 2007-07-27 at 13:48 -0700, Lee Howard wrote: > Here's the output: > > type: 4 > line: 1 > line: 760 > irq: 3 >flags: 1358954688 > xmit_fifo_size: 16 > custom_divisor: 0 >baud_base: 115200 OK, the FIFO should be enabled. What is known: * The error is a hardware FIFO overrun. - observed message is in n_tty due to driver setting TTY_OVERRUN * The RTS/CTS flow control is not involved - this is done only by the ldisc in response to buffer levels - you verified crtscts is set - you did not observed RTS change when 'overflow error' logged - you did observe RTS change when application stopped reading So this seems to be a latency issue reading the receive FIFO in the ISR. The current rx FIFO trigger level should be 8 bytes (UART_FCR_R_TRIG_10) which gives the ISR 694usec to get the data at 115200bps. IIRC, in 2.2.X kernels this defaulted to 4 bytes (TRIG_01) which gave a little more time to service the interrupt. How does the data rate affect the frequency of the overrun errors? Does 57600bps make them go away? -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
Tilman Schmidt wrote: Could this be related? http://lkml.org/lkml/2007/7/18/245 Quote: "I've recently found (using 2.6.21.4) that configuring a serial ports (ST16654) which use the 8250 driver using setserial results in the UART's FIFOs being disabled (unless you specify autoconfig)." That would make sense. Lee's error is a hardware FIFO overrun which could occur if the FIFO is being disabled as described in your link (by trying to set the uart type with setserial). Since the tty flow control is only triggered by the line discipline in response to ldisc buffer levels and not hardware FIFO overruns, you would never see any flow control action as reported by Lee. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
OK, I see where TTY_OVERRUN is set: include/linux/serial_core.h:uart_insert_char() -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
On Fri, 2007-07-27 at 12:22 -0600, Robert Hancock wrote: > In this situation, though, it appears it's not the TTY buffers that are > filling but the UART's own buffer. I would think this must be caused by > some kind of interrupt latency that results in not draining the FIFO in > time. You are right, this error is output when the character flag TTY_OVERRUN is encountered by n_tty.c which should be set by the driver in response to a hardware FIFO overrun (not an ldisc buffer overrun). I can't see anyplace in serial_core.c or 8250.c that sets TTY_OVERRUN. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
On Fri, 2007-07-27 at 12:22 -0600, Robert Hancock wrote: > Maciej W. Rozycki wrote: > > The TTY line discipline driver could do that based on the amount of > > received data present in its buffer. And it should if asked to (a brief > > look at drivers/char/n_tty.c reveals it does; obviously there may be a bug > > Really, where? In my look through the code I haven't found any mechanism > that would result in RTS being lowered based on TTY buffers filling up, > at least not in the 8250 case. serial_core.c:uart_throttle() serial_core.c:uart_unthrottle() These are called by N_TTY in response to buffer levels. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Fwd: PROBLEM: serial port receive buffers not being flushed properly
From: [EMAIL PROTECTED] > [1.] PROBLEM: serial port receive buffers not being flushed properly > [2.] After calling tcflush() or ioctl() with the TCFLSH argument, > an ioctl() with the FIONREAD argument reports 0 bytes available > for reading. However, a subsequent select() and read() of the port > returns data. This problem is seen in 2.6.21.1 and 2.6.20, > but is not seen in version 2.6.17.14. Other versions have > not been tested. There is receive buffering between the serial driver and line discipline. The line discipline processes FIONREAD and TCFLSH with regard to the line discipline receive buffers but not the intermediate buffering. In 2.6.17 the intermediate buffering pushed receive data to the line discipline without regard for tty->receive_room, so if the ldisc buffers were full the data was lost. In 2.6.18 the intermediate buffering was changed to hold receive data until the line discipline could accept it. So in 2.6.17 all the extra data you sent was simply discarded and flushing the ldisc buffers cleared all of the receive data. Starting with 2.6.18, flushing the ldisc buffers just allowed the intermediate buffering to start feeding more data to the ldisc. This results in the receive data you see after a TCFLSH. Starting with 2.6.22 TCFLSH is processed by the intermediate buffering as well as the ldisc to eliminate any remaining receive data in the intermediate buffering. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Fwd: PROBLEM: serial port receive buffers not being flushed properly
From: [EMAIL PROTECTED] [1.] PROBLEM: serial port receive buffers not being flushed properly [2.] After calling tcflush() or ioctl() with the TCFLSH argument, an ioctl() with the FIONREAD argument reports 0 bytes available for reading. However, a subsequent select() and read() of the port returns data. This problem is seen in 2.6.21.1 and 2.6.20, but is not seen in version 2.6.17.14. Other versions have not been tested. There is receive buffering between the serial driver and line discipline. The line discipline processes FIONREAD and TCFLSH with regard to the line discipline receive buffers but not the intermediate buffering. In 2.6.17 the intermediate buffering pushed receive data to the line discipline without regard for tty-receive_room, so if the ldisc buffers were full the data was lost. In 2.6.18 the intermediate buffering was changed to hold receive data until the line discipline could accept it. So in 2.6.17 all the extra data you sent was simply discarded and flushing the ldisc buffers cleared all of the receive data. Starting with 2.6.18, flushing the ldisc buffers just allowed the intermediate buffering to start feeding more data to the ldisc. This results in the receive data you see after a TCFLSH. Starting with 2.6.22 TCFLSH is processed by the intermediate buffering as well as the ldisc to eliminate any remaining receive data in the intermediate buffering. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
On Fri, 2007-07-27 at 12:22 -0600, Robert Hancock wrote: Maciej W. Rozycki wrote: The TTY line discipline driver could do that based on the amount of received data present in its buffer. And it should if asked to (a brief look at drivers/char/n_tty.c reveals it does; obviously there may be a bug Really, where? In my look through the code I haven't found any mechanism that would result in RTS being lowered based on TTY buffers filling up, at least not in the 8250 case. serial_core.c:uart_throttle() serial_core.c:uart_unthrottle() These are called by N_TTY in response to buffer levels. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
On Fri, 2007-07-27 at 12:22 -0600, Robert Hancock wrote: In this situation, though, it appears it's not the TTY buffers that are filling but the UART's own buffer. I would think this must be caused by some kind of interrupt latency that results in not draining the FIFO in time. You are right, this error is output when the character flag TTY_OVERRUN is encountered by n_tty.c which should be set by the driver in response to a hardware FIFO overrun (not an ldisc buffer overrun). I can't see anyplace in serial_core.c or 8250.c that sets TTY_OVERRUN. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
Tilman Schmidt wrote: Could this be related? http://lkml.org/lkml/2007/7/18/245 Quote: I've recently found (using 2.6.21.4) that configuring a serial ports (ST16654) which use the 8250 driver using setserial results in the UART's FIFOs being disabled (unless you specify autoconfig). That would make sense. Lee's error is a hardware FIFO overrun which could occur if the FIFO is being disabled as described in your link (by trying to set the uart type with setserial). Since the tty flow control is only triggered by the line discipline in response to ldisc buffer levels and not hardware FIFO overruns, you would never see any flow control action as reported by Lee. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
OK, I see where TTY_OVERRUN is set: include/linux/serial_core.h:uart_insert_char() -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: serial flow control appears broken
On Fri, 2007-07-27 at 13:48 -0700, Lee Howard wrote: Here's the output: type: 4 line: 1 line: 760 irq: 3 flags: 1358954688 xmit_fifo_size: 16 custom_divisor: 0 baud_base: 115200 OK, the FIFO should be enabled. What is known: * The error is a hardware FIFO overrun. - observed message is in n_tty due to driver setting TTY_OVERRUN * The RTS/CTS flow control is not involved - this is done only by the ldisc in response to buffer levels - you verified crtscts is set - you did not observed RTS change when 'overflow error' logged - you did observe RTS change when application stopped reading So this seems to be a latency issue reading the receive FIFO in the ISR. The current rx FIFO trigger level should be 8 bytes (UART_FCR_R_TRIG_10) which gives the ISR 694usec to get the data at 115200bps. IIRC, in 2.2.X kernels this defaulted to 4 bytes (TRIG_01) which gave a little more time to service the interrupt. How does the data rate affect the frequency of the overrun errors? Does 57600bps make them go away? -- Paul - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] synclink_gt fix transmit DMA stall
Fix transmit DMA stall when write() called in window after
previous transmit DMA completes but before previous serial
transmission completes.
Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]>
--- a/drivers/char/synclink_gt.c2007-07-08 18:32:17.0 -0500
+++ b/drivers/char/synclink_gt.c2007-07-26 10:39:56.0 -0500
@@ -1,5 +1,5 @@
/*
- * $Id: synclink_gt.c,v 4.36 2006/08/28 20:47:14 paulkf Exp $
+ * $Id: synclink_gt.c,v 4.50 2007/07/25 19:29:25 paulkf Exp $
*
* Device driver for Microgate SyncLink GT serial adapters.
*
@@ -93,7 +93,7 @@
* module identification
*/
static char *driver_name = "SyncLink GT";
-static char *driver_version = "$Revision: 4.36 $";
+static char *driver_version = "$Revision: 4.50 $";
static char *tty_driver_name = "synclink_gt";
static char *tty_dev_prefix = "ttySLG";
MODULE_LICENSE("GPL");
@@ -479,6 +479,7 @@ static void tx_set_idle(struct slgt_info
static unsigned int free_tbuf_count(struct slgt_info *info);
static void reset_tbufs(struct slgt_info *info);
static void tdma_reset(struct slgt_info *info);
+static void tdma_start(struct slgt_info *info);
static void tx_load(struct slgt_info *info, const char *buf, unsigned int
count);
static void get_signals(struct slgt_info *info);
@@ -912,6 +913,8 @@ start:
spin_lock_irqsave(>lock,flags);
if (!info->tx_active)
tx_start(info);
+ else
+ tdma_start(info);
spin_unlock_irqrestore(>lock,flags);
}
@@ -3880,44 +3883,58 @@ static void tx_start(struct slgt_info *i
slgt_irq_on(info, IRQ_TXUNDER + IRQ_TXIDLE);
/* clear tx idle and underrun status bits */
wr_reg16(info, SSR, (unsigned short)(IRQ_TXIDLE +
IRQ_TXUNDER));
-
- if (!(rd_reg32(info, TDCSR) & BIT0)) {
- /* tx DMA stopped, restart tx DMA */
- tdma_reset(info);
- /* set 1st descriptor address */
- wr_reg32(info, TDDAR,
info->tbufs[info->tbuf_start].pdesc);
- switch(info->params.mode) {
- case MGSL_MODE_RAW:
- case MGSL_MODE_MONOSYNC:
- case MGSL_MODE_BISYNC:
- wr_reg32(info, TDCSR, BIT2 + BIT0); /*
IRQ + DMA enable */
- break;
- default:
- wr_reg32(info, TDCSR, BIT0); /* DMA
enable */
- }
- }
-
if (info->params.mode == MGSL_MODE_HDLC)
mod_timer(>tx_timer, jiffies +
msecs_to_jiffies(5000));
} else {
- tdma_reset(info);
- /* set 1st descriptor address */
- wr_reg32(info, TDDAR,
info->tbufs[info->tbuf_start].pdesc);
-
slgt_irq_off(info, IRQ_TXDATA);
slgt_irq_on(info, IRQ_TXIDLE);
/* clear tx idle status bit */
wr_reg16(info, SSR, IRQ_TXIDLE);
-
- /* enable tx DMA */
- wr_reg32(info, TDCSR, BIT0);
}
-
+ tdma_start(info);
info->tx_active = 1;
}
}
+/*
+ * start transmit DMA if inactive and there are unsent buffers
+ */
+static void tdma_start(struct slgt_info *info)
+{
+ unsigned int i;
+
+ if (rd_reg32(info, TDCSR) & BIT0)
+ return;
+
+ /* transmit DMA inactive, check for unsent buffers */
+ i = info->tbuf_start;
+ while (!desc_count(info->tbufs[i])) {
+ if (++i == info->tbuf_count)
+ i = 0;
+ if (i == info->tbuf_current)
+ return;
+ }
+ info->tbuf_start = i;
+
+ /* there are unsent buffers, start transmit DMA */
+
+ /* reset needed if previous error condition */
+ tdma_reset(info);
+
+ /* set 1st descriptor address */
+ wr_reg32(info, TDDAR, info->tbufs[info->tbuf_start].pdesc);
+ switch(info->params.mode) {
+ case MGSL_MODE_RAW:
+ case MGSL_MODE_MONOSYNC:
+ case MGSL_MODE_BISYNC:
+ wr_reg32(info, TDCSR, BIT2 + BIT0); /* IRQ + DMA enable */
+ break;
+ default:
+ wr_reg32(info, TDCSR, BIT0); /* DMA enable */
+ }
+}
+
static void tx_stop(struct slgt_info *info)
{
unsigned short val;
@@ -4651,8 +4668,8 @@ static unsigned int free_tbuf_count(stru
[PATCH] synclink_gt fix transmit DMA stall
Fix transmit DMA stall when write() called in window after
previous transmit DMA completes but before previous serial
transmission completes.
Signed-off-by: Paul Fulghum [EMAIL PROTECTED]
--- a/drivers/char/synclink_gt.c2007-07-08 18:32:17.0 -0500
+++ b/drivers/char/synclink_gt.c2007-07-26 10:39:56.0 -0500
@@ -1,5 +1,5 @@
/*
- * $Id: synclink_gt.c,v 4.36 2006/08/28 20:47:14 paulkf Exp $
+ * $Id: synclink_gt.c,v 4.50 2007/07/25 19:29:25 paulkf Exp $
*
* Device driver for Microgate SyncLink GT serial adapters.
*
@@ -93,7 +93,7 @@
* module identification
*/
static char *driver_name = SyncLink GT;
-static char *driver_version = $Revision: 4.36 $;
+static char *driver_version = $Revision: 4.50 $;
static char *tty_driver_name = synclink_gt;
static char *tty_dev_prefix = ttySLG;
MODULE_LICENSE(GPL);
@@ -479,6 +479,7 @@ static void tx_set_idle(struct slgt_info
static unsigned int free_tbuf_count(struct slgt_info *info);
static void reset_tbufs(struct slgt_info *info);
static void tdma_reset(struct slgt_info *info);
+static void tdma_start(struct slgt_info *info);
static void tx_load(struct slgt_info *info, const char *buf, unsigned int
count);
static void get_signals(struct slgt_info *info);
@@ -912,6 +913,8 @@ start:
spin_lock_irqsave(info-lock,flags);
if (!info-tx_active)
tx_start(info);
+ else
+ tdma_start(info);
spin_unlock_irqrestore(info-lock,flags);
}
@@ -3880,44 +3883,58 @@ static void tx_start(struct slgt_info *i
slgt_irq_on(info, IRQ_TXUNDER + IRQ_TXIDLE);
/* clear tx idle and underrun status bits */
wr_reg16(info, SSR, (unsigned short)(IRQ_TXIDLE +
IRQ_TXUNDER));
-
- if (!(rd_reg32(info, TDCSR) BIT0)) {
- /* tx DMA stopped, restart tx DMA */
- tdma_reset(info);
- /* set 1st descriptor address */
- wr_reg32(info, TDDAR,
info-tbufs[info-tbuf_start].pdesc);
- switch(info-params.mode) {
- case MGSL_MODE_RAW:
- case MGSL_MODE_MONOSYNC:
- case MGSL_MODE_BISYNC:
- wr_reg32(info, TDCSR, BIT2 + BIT0); /*
IRQ + DMA enable */
- break;
- default:
- wr_reg32(info, TDCSR, BIT0); /* DMA
enable */
- }
- }
-
if (info-params.mode == MGSL_MODE_HDLC)
mod_timer(info-tx_timer, jiffies +
msecs_to_jiffies(5000));
} else {
- tdma_reset(info);
- /* set 1st descriptor address */
- wr_reg32(info, TDDAR,
info-tbufs[info-tbuf_start].pdesc);
-
slgt_irq_off(info, IRQ_TXDATA);
slgt_irq_on(info, IRQ_TXIDLE);
/* clear tx idle status bit */
wr_reg16(info, SSR, IRQ_TXIDLE);
-
- /* enable tx DMA */
- wr_reg32(info, TDCSR, BIT0);
}
-
+ tdma_start(info);
info-tx_active = 1;
}
}
+/*
+ * start transmit DMA if inactive and there are unsent buffers
+ */
+static void tdma_start(struct slgt_info *info)
+{
+ unsigned int i;
+
+ if (rd_reg32(info, TDCSR) BIT0)
+ return;
+
+ /* transmit DMA inactive, check for unsent buffers */
+ i = info-tbuf_start;
+ while (!desc_count(info-tbufs[i])) {
+ if (++i == info-tbuf_count)
+ i = 0;
+ if (i == info-tbuf_current)
+ return;
+ }
+ info-tbuf_start = i;
+
+ /* there are unsent buffers, start transmit DMA */
+
+ /* reset needed if previous error condition */
+ tdma_reset(info);
+
+ /* set 1st descriptor address */
+ wr_reg32(info, TDDAR, info-tbufs[info-tbuf_start].pdesc);
+ switch(info-params.mode) {
+ case MGSL_MODE_RAW:
+ case MGSL_MODE_MONOSYNC:
+ case MGSL_MODE_BISYNC:
+ wr_reg32(info, TDCSR, BIT2 + BIT0); /* IRQ + DMA enable */
+ break;
+ default:
+ wr_reg32(info, TDCSR, BIT0); /* DMA enable */
+ }
+}
+
static void tx_stop(struct slgt_info *info)
{
unsigned short val;
@@ -4651,8 +4668,8 @@ static unsigned int free_tbuf_count(stru
i=0;
} while (i != info-tbuf_current);
- /* last buffer with zero count may be in use, assume
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: Done. I still smell a dead lock issue tho. Yes, but it is an existing problem that was kicked about with no real resolution. No one can blame you for that! :-) -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
On Wed, 2007-07-18 at 19:17 +0100, James Simmons wrote: > I have no problem leaving at one. Here is the new patch. I did address the > problem with tty_flip_buffer_push in this patch. It is possible for a > driver to call tty_flip_buffer_push within a interrupt context if they > set the low_latency flag. > --- a/drivers/char/tty_io.c > +++ b/drivers/char/tty_io.c > @@ -3647,11 +3647,7 @@ void tty_flip_buffer_push(struct tty_struct *tty) > if (tty->buf.tail != NULL) > tty->buf.tail->commit = tty->buf.tail->used; > spin_unlock_irqrestore(>buf.lock, flags); > - > - if (tty->low_latency) > - flush_to_ldisc(>buf.work.work); > - else > - schedule_delayed_work(>buf.work, 1); > + schedule_delayed_work(>buf.work, 1); > } While I have no problem with this, it would be a significant behavior change (more so than changing the initial delay to 0). IIRC, when the serial_core dead lock was being debugged (by Russel King with some Dell guy who reported it 1-2 years ago) this change was suggested and rejected because some callers are not running at IRQ context and want to use low_latency. In the end, I think they decided to leave the correct use of low_latency WRT running in IRQ context to the caller. It might be safest to drop this portion so you can get the obvious part of the patch accepted (consolidating the redundant xxx_schedule_flip functions). -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
On Wed, 2007-07-18 at 19:17 +0100, James Simmons wrote: I have no problem leaving at one. Here is the new patch. I did address the problem with tty_flip_buffer_push in this patch. It is possible for a driver to call tty_flip_buffer_push within a interrupt context if they set the low_latency flag. --- a/drivers/char/tty_io.c +++ b/drivers/char/tty_io.c @@ -3647,11 +3647,7 @@ void tty_flip_buffer_push(struct tty_struct *tty) if (tty-buf.tail != NULL) tty-buf.tail-commit = tty-buf.tail-used; spin_unlock_irqrestore(tty-buf.lock, flags); - - if (tty-low_latency) - flush_to_ldisc(tty-buf.work.work); - else - schedule_delayed_work(tty-buf.work, 1); + schedule_delayed_work(tty-buf.work, 1); } While I have no problem with this, it would be a significant behavior change (more so than changing the initial delay to 0). IIRC, when the serial_core dead lock was being debugged (by Russel King with some Dell guy who reported it 1-2 years ago) this change was suggested and rejected because some callers are not running at IRQ context and want to use low_latency. In the end, I think they decided to leave the correct use of low_latency WRT running in IRQ context to the caller. It might be safest to drop this portion so you can get the obvious part of the patch accepted (consolidating the redundant xxx_schedule_flip functions). -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: Done. I still smell a dead lock issue tho. Yes, but it is an existing problem that was kicked about with no real resolution. No one can blame you for that! :-) -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: James Simmons, le Tue 17 Jul 2007 19:37:57 +0100, a écrit : - schedule_delayed_work(>buf.work, 0); It was schedule_delayed_work(>buf.work, 1); in con_schedule_flip() ; could that matter? I did not detect any regressions. The console behavior stays exactly the same as the patch changes tty_schedule_flip to use the 0 delay. The change to tty_schedule_flip() to use 0 delay also is OK. I had looked at this when James originally posted this patch and found: I looked further back and in the 2.4 kernels this scheduling was done with the timer task queue (process receive data on next timer tick). I guess the schedule_delayed_work() with a time delay of 1 was the best approximation of the previous behavior. There is no logical reason to delay the first attempt at processing receive data so schedule_delayed_work() in tty_schedule_flip() should be changed to 0 (as was the case for con_schedule_flip). The schedule_delayed_work in flush_to_ldisc() will continue to use a delay of 1 if the ldisc can't accept more data. This allows the user app and ldisc to catch up. Subsequent calls to tty_schedule_flip won't affect this 'back off' delay because once the work is scheduled (with a delay of 1) new scheduling calls are ignored for the same work structure. I've been testing the change to 0 in tty_schedule_flip() under various loads and data rates with no ill effects. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: The low_latency is used by the drivers in the case where its not in a interrupt context. Well we are trusting the drivers. Now if it is true what you said then tty_flip_buffer_push has a bug. Looking at several drivers including serial devices they set the low_latency flag. The generic serial driver (8250) is the one that was dead locking when that code originally existed. It was setting low_latency and calling from interrupt context. And the initial schedule has no reason to add the extra delay. So do you support a non delay work queue as well? No, the delay work must be used for flush_to_ldisc() so it makes no sense to define two different work queues (one delayed and one not) for the same work. I support your patch. The current stuff works and your patch works. With your patch, you actually reduce initial latency for processing receive data. Whichever way everyone else wants to go. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
Linus Torvalds wrote: - schedule_delayed_work(>buf.work, 1); + schedule_delayed_work(>buf.work, 0); Is there any real reason for this? I think that patch is bogus. Either it should stay at 1, or the whole work should be a non-scheduled one instead. Do we really need to handle it asap for the console, or is it ok to wait for the next tick, like the regular tty case used to? And if we need to handle it asap, why the "delayed"? The scheduling is to move the processing out of interrupt context. The receive data is often extracted from the hardware at interrupt time and then queued for processing. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: Because sometimes you do want the delay. In other parts of the tty code we do delay. What should be done is Correct, so we must stick with the delayed work structure which requires calling the delayed work function. if (tty->low_latency) flush_to_ldisc(>buf.work.work); else schedule_delayed_work(>buf.work, 1); Is this acceptable to you? That does not make sense to me. If you are calling from interrupt context, you do not want to call flush_to_ldisc() directly regardless of low_latency. This used to be the way it was done and it ended up causing deadlocks in just that situation. And the initial schedule has no reason to add the extra delay. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: Because sometimes you do want the delay. In other parts of the tty code we do delay. What should be done is Correct, so we must stick with the delayed work structure which requires calling the delayed work function. if (tty-low_latency) flush_to_ldisc(tty-buf.work.work); else schedule_delayed_work(tty-buf.work, 1); Is this acceptable to you? That does not make sense to me. If you are calling from interrupt context, you do not want to call flush_to_ldisc() directly regardless of low_latency. This used to be the way it was done and it ended up causing deadlocks in just that situation. And the initial schedule has no reason to add the extra delay. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: The low_latency is used by the drivers in the case where its not in a interrupt context. Well we are trusting the drivers. Now if it is true what you said then tty_flip_buffer_push has a bug. Looking at several drivers including serial devices they set the low_latency flag. The generic serial driver (8250) is the one that was dead locking when that code originally existed. It was setting low_latency and calling from interrupt context. And the initial schedule has no reason to add the extra delay. So do you support a non delay work queue as well? No, the delay work must be used for flush_to_ldisc() so it makes no sense to define two different work queues (one delayed and one not) for the same work. I support your patch. The current stuff works and your patch works. With your patch, you actually reduce initial latency for processing receive data. Whichever way everyone else wants to go. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
Linus Torvalds wrote: - schedule_delayed_work(tty-buf.work, 1); + schedule_delayed_work(tty-buf.work, 0); Is there any real reason for this? I think that patch is bogus. Either it should stay at 1, or the whole work should be a non-scheduled one instead. Do we really need to handle it asap for the console, or is it ok to wait for the next tick, like the regular tty case used to? And if we need to handle it asap, why the delayed? The scheduling is to move the processing out of interrupt context. The receive data is often extracted from the hardware at interrupt time and then queued for processing. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: James Simmons, le Tue 17 Jul 2007 19:37:57 +0100, a écrit : - schedule_delayed_work(t-buf.work, 0); It was schedule_delayed_work(t-buf.work, 1); in con_schedule_flip() ; could that matter? I did not detect any regressions. The console behavior stays exactly the same as the patch changes tty_schedule_flip to use the 0 delay. The change to tty_schedule_flip() to use 0 delay also is OK. I had looked at this when James originally posted this patch and found: I looked further back and in the 2.4 kernels this scheduling was done with the timer task queue (process receive data on next timer tick). I guess the schedule_delayed_work() with a time delay of 1 was the best approximation of the previous behavior. There is no logical reason to delay the first attempt at processing receive data so schedule_delayed_work() in tty_schedule_flip() should be changed to 0 (as was the case for con_schedule_flip). The schedule_delayed_work in flush_to_ldisc() will continue to use a delay of 1 if the ldisc can't accept more data. This allows the user app and ldisc to catch up. Subsequent calls to tty_schedule_flip won't affect this 'back off' delay because once the work is scheduled (with a delay of 1) new scheduling calls are ignored for the same work structure. I've been testing the change to 0 in tty_schedule_flip() under various loads and data rates with no ill effects. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty restore locked ioctl file op
Björn Steinbrink wrote: Sorry for the delay, your mails didn't make it into my inbox, and I usually just mark threads on which I'm Cc'ed as read in my lkml mailbox, thus I didn't notice it earlier. Any traces of the lost mails on your side? No clues on this end. The patch works as expected, no Oops in sight. Regarding the reproducability, it might be that it was easier to trigger on rc1. When I retried today with rc4, I only got 2 Oopses in a minute, while the first test had spitten out about 20 Oopses in 10 seconds (not sure if I really had rc1 running back then, though). I'm sure it's very timing dependent. I'm confident with this fix. Thanks for your help. -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty restore locked ioctl file op
Björn Steinbrink wrote: Sorry for the delay, your mails didn't make it into my inbox, and I usually just mark threads on which I'm Cc'ed as read in my lkml mailbox, thus I didn't notice it earlier. Any traces of the lost mails on your side? No clues on this end. The patch works as expected, no Oops in sight. Regarding the reproducability, it might be that it was easier to trigger on rc1. When I retried today with rc4, I only got 2 Oopses in a minute, while the first test had spitten out about 20 Oopses in 10 seconds (not sure if I really had rc1 running back then, though). I'm sure it's very timing dependent. I'm confident with this fix. Thanks for your help. -- Paul - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty restore locked ioctl file op
Restore tty locked ioctl handler which was replaced with
an unlocked ioctl handler in hung_up_tty_fops by the patch:
commit e10cc1df1d2014f68a4bdcf73f6dd122c4561f94
Author: Paul Fulghum <[EMAIL PROTECTED]>
Date: Thu May 10 22:22:50 2007 -0700
tty: add compat_ioctl
This was reported in:
[Bug 8473] New: Oops: 0010 [1] SMP
The bug is caused by switching to hung_up_tty_fops in do_tty_hangup.
An ioctl call can be waiting on BLK after testing for existence of
the locked ioctl handler in the normal tty fops, but before calling
the locked ioctl handler. If a hangup occurs at that point, the
locked ioctl fop is NULL and an oops occurs.
Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]>
--- a/drivers/char/tty_io.c 2007-06-08 14:26:10.0 -0500
+++ b/drivers/char/tty_io.c 2007-06-08 14:28:58.0 -0500
@@ -1173,8 +1173,14 @@ static unsigned int hung_up_tty_poll(str
return POLLIN | POLLOUT | POLLERR | POLLHUP | POLLRDNORM | POLLWRNORM;
}
-static long hung_up_tty_ioctl(struct file * file,
- unsigned int cmd, unsigned long arg)
+static int hung_up_tty_ioctl(struct inode * inode, struct file * file,
+unsigned int cmd, unsigned long arg)
+{
+ return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
+}
+
+static long hung_up_tty_compat_ioctl(struct file * file,
+unsigned int cmd, unsigned long arg)
{
return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
}
@@ -1222,8 +1228,8 @@ static const struct file_operations hung
.read = hung_up_tty_read,
.write = hung_up_tty_write,
.poll = hung_up_tty_poll,
- .unlocked_ioctl = hung_up_tty_ioctl,
- .compat_ioctl = hung_up_tty_ioctl,
+ .ioctl = hung_up_tty_ioctl,
+ .compat_ioctl = hung_up_tty_compat_ioctl,
.release= tty_release,
};
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Bug 8473] New: Oops: 0010 [1] SMP
On Fri, 2007-06-08 at 10:16 -0500, Paul Fulghum wrote: > On Fri, 2007-06-08 at 05:06 +0200, Björn Steinbrink wrote: > > This is do_tty_hangup() exchanging the fops while we're waiting for the > > lock. The new fops (hung_up_tty_fops) only have the unlocked variant and > > thus we Oops our way. ... > Here is a patch that restores the locked ioctl for hung_up_tty_ioctl. > Can you try it and see if it removes your oops? Unfortunately I can't get the timing right to trigger this, but it is very clear the locked ioctl fop must not be allowed to disappear like my original patch allows. Andrew: Would you prefer I resend the entire compat ioctl patch or submit an incremental patch like in my message I'm quoting above? -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Bug 8473] New: Oops: 0010 [1] SMP
On Fri, 2007-06-08 at 05:06 +0200, Björn Steinbrink wrote:
> This is do_tty_hangup() exchanging the fops while we're waiting for the
> lock. The new fops (hung_up_tty_fops) only have the unlocked variant and
> thus we Oops our way.
>
> The following program reproduces it quite easily on a SMP box. I'm
> running it from X as root like this:
> while true; do xterm /path/to/program; done
I am unable to reproduce the oops on either i386 SMP or x86_64 SMP
using your test program. This is against plain 2.6.21 with only
my compat ioctl patch applied.
Here is a patch that restores the locked ioctl for hung_up_tty_ioctl.
Can you try it and see if it removes your oops?
--- a/drivers/char/tty_io.c 2007-06-08 10:07:39.0 -0500
+++ b/drivers/char/tty_io.c 2007-06-08 10:09:59.0 -0500
@@ -1150,8 +1150,14 @@ static unsigned int hung_up_tty_poll(str
return POLLIN | POLLOUT | POLLERR | POLLHUP | POLLRDNORM | POLLWRNORM;
}
-static long hung_up_tty_ioctl(struct file * file,
- unsigned int cmd, unsigned long arg)
+static int hung_up_tty_ioctl(struct inode * inode, struct file * file,
+unsigned int cmd, unsigned long arg)
+{
+ return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
+}
+
+static long hung_up_tty_compat_ioctl(struct file * file,
+unsigned int cmd, unsigned long arg)
{
return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
}
@@ -1199,8 +1205,8 @@ static const struct file_operations hung
.read = hung_up_tty_read,
.write = hung_up_tty_write,
.poll = hung_up_tty_poll,
- .unlocked_ioctl = hung_up_tty_ioctl,
- .compat_ioctl = hung_up_tty_ioctl,
+ .ioctl = hung_up_tty_ioctl,
+ .compat_ioctl = hung_up_tty_compat_ioctl,
.release= tty_release,
};
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Bug 8473] New: Oops: 0010 [1] SMP
On Thu, 2007-06-07 at 20:16 -0700, Andrew Morton wrote: > On Fri, 8 Jun 2007 05:06:29 +0200 Björn Steinbrink <[EMAIL PROTECTED]> wrote: > > This is do_tty_hangup() exchanging the fops while we're waiting for the > > lock. The new fops (hung_up_tty_fops) only have the unlocked variant and > > thus we Oops our way. > > ah, yes, that explains it, thanks. Culprit: > > commit e10cc1df1d2014f68a4bdcf73f6dd122c4561f94 > Author: Paul Fulghum <[EMAIL PROTECTED]> > Date: Thu May 10 22:22:50 2007 -0700 > > tty: add compat_ioctl > > Add compat_ioctl method for tty code to allow processing of 32 bit ioctl > calls on 64 bit systems by tty core, tty drivers, and line disciplines. OK, the change of hung_up_tty_ioctl() from locked to unlocked is not necessary for this patch. On the surface it seemed a clever way of not needing two different functions to do the same simple: return cmd == TIOCSPGRP ? -ENOTTY : -EIO; which does not need any locking for its own sake. But clearly the hangup behavior requires the locked version. I'll redo the patch with hung_up_tty_ioctl remaining locked. That will separate the compat ioctl stuff from this issue. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Bug 8473] New: Oops: 0010 [1] SMP
On Thu, 2007-06-07 at 20:16 -0700, Andrew Morton wrote: On Fri, 8 Jun 2007 05:06:29 +0200 Björn Steinbrink [EMAIL PROTECTED] wrote: This is do_tty_hangup() exchanging the fops while we're waiting for the lock. The new fops (hung_up_tty_fops) only have the unlocked variant and thus we Oops our way. ah, yes, that explains it, thanks. Culprit: commit e10cc1df1d2014f68a4bdcf73f6dd122c4561f94 Author: Paul Fulghum [EMAIL PROTECTED] Date: Thu May 10 22:22:50 2007 -0700 tty: add compat_ioctl Add compat_ioctl method for tty code to allow processing of 32 bit ioctl calls on 64 bit systems by tty core, tty drivers, and line disciplines. OK, the change of hung_up_tty_ioctl() from locked to unlocked is not necessary for this patch. On the surface it seemed a clever way of not needing two different functions to do the same simple: return cmd == TIOCSPGRP ? -ENOTTY : -EIO; which does not need any locking for its own sake. But clearly the hangup behavior requires the locked version. I'll redo the patch with hung_up_tty_ioctl remaining locked. That will separate the compat ioctl stuff from this issue. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Bug 8473] New: Oops: 0010 [1] SMP
On Fri, 2007-06-08 at 05:06 +0200, Björn Steinbrink wrote:
This is do_tty_hangup() exchanging the fops while we're waiting for the
lock. The new fops (hung_up_tty_fops) only have the unlocked variant and
thus we Oops our way.
The following program reproduces it quite easily on a SMP box. I'm
running it from X as root like this:
while true; do xterm /path/to/program; done
I am unable to reproduce the oops on either i386 SMP or x86_64 SMP
using your test program. This is against plain 2.6.21 with only
my compat ioctl patch applied.
Here is a patch that restores the locked ioctl for hung_up_tty_ioctl.
Can you try it and see if it removes your oops?
--- a/drivers/char/tty_io.c 2007-06-08 10:07:39.0 -0500
+++ b/drivers/char/tty_io.c 2007-06-08 10:09:59.0 -0500
@@ -1150,8 +1150,14 @@ static unsigned int hung_up_tty_poll(str
return POLLIN | POLLOUT | POLLERR | POLLHUP | POLLRDNORM | POLLWRNORM;
}
-static long hung_up_tty_ioctl(struct file * file,
- unsigned int cmd, unsigned long arg)
+static int hung_up_tty_ioctl(struct inode * inode, struct file * file,
+unsigned int cmd, unsigned long arg)
+{
+ return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
+}
+
+static long hung_up_tty_compat_ioctl(struct file * file,
+unsigned int cmd, unsigned long arg)
{
return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
}
@@ -1199,8 +1205,8 @@ static const struct file_operations hung
.read = hung_up_tty_read,
.write = hung_up_tty_write,
.poll = hung_up_tty_poll,
- .unlocked_ioctl = hung_up_tty_ioctl,
- .compat_ioctl = hung_up_tty_ioctl,
+ .ioctl = hung_up_tty_ioctl,
+ .compat_ioctl = hung_up_tty_compat_ioctl,
.release= tty_release,
};
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Bug 8473] New: Oops: 0010 [1] SMP
On Fri, 2007-06-08 at 10:16 -0500, Paul Fulghum wrote: On Fri, 2007-06-08 at 05:06 +0200, Björn Steinbrink wrote: This is do_tty_hangup() exchanging the fops while we're waiting for the lock. The new fops (hung_up_tty_fops) only have the unlocked variant and thus we Oops our way. ... Here is a patch that restores the locked ioctl for hung_up_tty_ioctl. Can you try it and see if it removes your oops? Unfortunately I can't get the timing right to trigger this, but it is very clear the locked ioctl fop must not be allowed to disappear like my original patch allows. Andrew: Would you prefer I resend the entire compat ioctl patch or submit an incremental patch like in my message I'm quoting above? -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty restore locked ioctl file op
Restore tty locked ioctl handler which was replaced with
an unlocked ioctl handler in hung_up_tty_fops by the patch:
commit e10cc1df1d2014f68a4bdcf73f6dd122c4561f94
Author: Paul Fulghum [EMAIL PROTECTED]
Date: Thu May 10 22:22:50 2007 -0700
tty: add compat_ioctl
This was reported in:
[Bug 8473] New: Oops: 0010 [1] SMP
The bug is caused by switching to hung_up_tty_fops in do_tty_hangup.
An ioctl call can be waiting on BLK after testing for existence of
the locked ioctl handler in the normal tty fops, but before calling
the locked ioctl handler. If a hangup occurs at that point, the
locked ioctl fop is NULL and an oops occurs.
Signed-off-by: Paul Fulghum [EMAIL PROTECTED]
--- a/drivers/char/tty_io.c 2007-06-08 14:26:10.0 -0500
+++ b/drivers/char/tty_io.c 2007-06-08 14:28:58.0 -0500
@@ -1173,8 +1173,14 @@ static unsigned int hung_up_tty_poll(str
return POLLIN | POLLOUT | POLLERR | POLLHUP | POLLRDNORM | POLLWRNORM;
}
-static long hung_up_tty_ioctl(struct file * file,
- unsigned int cmd, unsigned long arg)
+static int hung_up_tty_ioctl(struct inode * inode, struct file * file,
+unsigned int cmd, unsigned long arg)
+{
+ return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
+}
+
+static long hung_up_tty_compat_ioctl(struct file * file,
+unsigned int cmd, unsigned long arg)
{
return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
}
@@ -1222,8 +1228,8 @@ static const struct file_operations hung
.read = hung_up_tty_read,
.write = hung_up_tty_write,
.poll = hung_up_tty_poll,
- .unlocked_ioctl = hung_up_tty_ioctl,
- .compat_ioctl = hung_up_tty_ioctl,
+ .ioctl = hung_up_tty_ioctl,
+ .compat_ioctl = hung_up_tty_compat_ioctl,
.release= tty_release,
};
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] Char: n_hdlc, allow RESTARTSYS retval of tty write
On Thu, 2007-05-24 at 14:28 +0200, Jiri Slaby wrote: > n_hdlc, allow RESTARTSYS retval of tty write > > Cc: Paul Fulghum <[EMAIL PROTECTED]> > Signed-off-by: Jiri Slaby <[EMAIL PROTECTED]> > Acked-by: Paul Fulghum <[EMAIL PROTECTED]> -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] Char: n_hdlc, allow RESTARTSYS retval of tty write
On Thu, 2007-05-24 at 14:28 +0200, Jiri Slaby wrote: n_hdlc, allow RESTARTSYS retval of tty write Cc: Paul Fulghum [EMAIL PROTECTED] Signed-off-by: Jiri Slaby [EMAIL PROTECTED] Acked-by: Paul Fulghum [EMAIL PROTECTED] -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: Great Here is the patch updated with the delay value set to zero. Acked-by: Paul Fulghum <[EMAIL PROTECTED]> You should submit this to Andrew Morton <[EMAIL PROTECTED]> so it can get into the mm tree. -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
Paul Fulghum wrote: As the tty flip buffer code has evolved, that delay value of 1 was carried along. It may have had some historical purpose, but I can't figure it out and it appears to have no use currently. I looked further back and in the 2.4 kernels this scheduling was done with the timer task queue (process receive data on next timer tick). I guess the schedule_delayed_work() with a time delay of 1 was the best approximation of the previous behavior. There is no logical reason to delay the first attempt at processing receive data so schedule_delayed_work() in tty_schedule_flip() should be changed to 0 (as was the case for con_schedule_flip). The schedule_delayed_work in flush_to_ldisc() will continue to use a delay of 1 if the ldisc can't accept more data. This allows the user app and ldisc to catch up. Subsequent calls to tty_schedule_flip won't affect this 'back off' delay because once the work is scheduled (with a delay of 1) new scheduling calls are ignored for the same work structure. I've been testing the change to 0 in tty_schedule_flip() under various loads and data rates with no ill effects. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] synclink_gt add compat_ioctl
Add support for 32 bit ioctl on 64 bit systems for synclink_gt
Cc: Arnd Bergmann <[EMAIL PROTECTED]>
Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]>
--- a/include/linux/Kbuild 2007-04-25 22:08:32.0 -0500
+++ b/include/linux/Kbuild 2007-05-09 10:11:22.0 -0500
@@ -140,7 +140,6 @@ header-y += snmp.h
header-y += sockios.h
header-y += som.h
header-y += sound.h
-header-y += synclink.h
header-y += telephony.h
header-y += termios.h
header-y += ticable.h
@@ -315,6 +314,7 @@ unifdef-y += sonypi.h
unifdef-y += soundcard.h
unifdef-y += stat.h
unifdef-y += stddef.h
+unifdef-y += synclink.h
unifdef-y += sysctl.h
unifdef-y += tcp.h
unifdef-y += time.h
--- a/include/linux/synclink.h 2007-04-25 22:08:32.0 -0500
+++ b/include/linux/synclink.h 2007-05-09 10:37:20.0 -0500
@@ -291,4 +291,29 @@ struct gpio_desc {
#define MGSL_IOCGGPIO _IOR(MGSL_MAGIC_IOC,17,struct gpio_desc)
#define MGSL_IOCWAITGPIO _IOWR(MGSL_MAGIC_IOC,18,struct gpio_desc)
+#ifdef __KERNEL__
+/* provide 32 bit ioctl compatibility on 64 bit systems */
+#ifdef CONFIG_COMPAT
+#include
+struct MGSL_PARAMS32
+{
+ compat_ulong_t mode;
+ unsigned char loopback;
+ unsigned short flags;
+ unsigned char encoding;
+ compat_ulong_t clock_speed;
+ unsigned char addr_filter;
+ unsigned short crc_type;
+ unsigned char preamble_length;
+ unsigned char preamble;
+ compat_ulong_t data_rate;
+ unsigned char data_bits;
+ unsigned char stop_bits;
+ unsigned char parity;
+};
+#define MGSL_IOCSPARAMS32 _IOW(MGSL_MAGIC_IOC,0,struct MGSL_PARAMS32)
+#define MGSL_IOCGPARAMS32 _IOR(MGSL_MAGIC_IOC,1,struct MGSL_PARAMS32)
+#endif
+#endif
+
#endif /* _SYNCLINK_H_ */
--- a/drivers/char/synclink_gt.c2007-04-25 22:08:32.0 -0500
+++ b/drivers/char/synclink_gt.c2007-05-09 10:12:29.0 -0500
@@ -1171,6 +1171,112 @@ static int ioctl(struct tty_struct *tty,
}
/*
+ * support for 32 bit ioctl calls on 64 bit systems
+ */
+#ifdef CONFIG_COMPAT
+static long get_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user
*user_params)
+{
+ struct MGSL_PARAMS32 tmp_params;
+
+ DBGINFO(("%s get_params32\n", info->device_name));
+ tmp_params.mode= (compat_ulong_t)info->params.mode;
+ tmp_params.loopback= info->params.loopback;
+ tmp_params.flags = info->params.flags;
+ tmp_params.encoding= info->params.encoding;
+ tmp_params.clock_speed = (compat_ulong_t)info->params.clock_speed;
+ tmp_params.addr_filter = info->params.addr_filter;
+ tmp_params.crc_type= info->params.crc_type;
+ tmp_params.preamble_length = info->params.preamble_length;
+ tmp_params.preamble= info->params.preamble;
+ tmp_params.data_rate = (compat_ulong_t)info->params.data_rate;
+ tmp_params.data_bits = info->params.data_bits;
+ tmp_params.stop_bits = info->params.stop_bits;
+ tmp_params.parity = info->params.parity;
+ if (copy_to_user(user_params, _params, sizeof(struct
MGSL_PARAMS32)))
+ return -EFAULT;
+ return 0;
+}
+
+static long set_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user
*new_params)
+{
+ struct MGSL_PARAMS32 tmp_params;
+
+ DBGINFO(("%s set_params32\n", info->device_name));
+ if (copy_from_user(_params, new_params, sizeof(struct
MGSL_PARAMS32)))
+ return -EFAULT;
+
+ spin_lock(>lock);
+ info->params.mode= tmp_params.mode;
+ info->params.loopback= tmp_params.loopback;
+ info->params.flags = tmp_params.flags;
+ info->params.encoding= tmp_params.encoding;
+ info->params.clock_speed = tmp_params.clock_speed;
+ info->params.addr_filter = tmp_params.addr_filter;
+ info->params.crc_type= tmp_params.crc_type;
+ info->params.preamble_length = tmp_params.preamble_length;
+ info->params.preamble= tmp_params.preamble;
+ info->params.data_rate = tmp_params.data_rate;
+ info->params.data_bits = tmp_params.data_bits;
+ info->params.stop_bits = tmp_params.stop_bits;
+ info->params.parity = tmp_params.parity;
+ spin_unlock(>lock);
+
+ change_params(info);
+
+ return 0;
+}
+
+static long slgt_compat_ioctl(struct tty_struct *tty, struct file *file,
+unsigned int cmd, unsigned long arg)
+{
+ struct slgt_info *info = tty->driver_data;
+ int rc = -ENOIOCTLCMD;
+
+ if (sanity_check(info, tty->name, "compat_ioctl"))
+ return -ENODEV;
+ DBGINFO(("%s compat_ioctl() cmd=%08X\n", info->device_
[PATCH] synclink_gt add compat_ioctl
Add support for 32 bit ioctl on 64 bit systems for synclink_gt
Cc: Arnd Bergmann [EMAIL PROTECTED]
Signed-off-by: Paul Fulghum [EMAIL PROTECTED]
--- a/include/linux/Kbuild 2007-04-25 22:08:32.0 -0500
+++ b/include/linux/Kbuild 2007-05-09 10:11:22.0 -0500
@@ -140,7 +140,6 @@ header-y += snmp.h
header-y += sockios.h
header-y += som.h
header-y += sound.h
-header-y += synclink.h
header-y += telephony.h
header-y += termios.h
header-y += ticable.h
@@ -315,6 +314,7 @@ unifdef-y += sonypi.h
unifdef-y += soundcard.h
unifdef-y += stat.h
unifdef-y += stddef.h
+unifdef-y += synclink.h
unifdef-y += sysctl.h
unifdef-y += tcp.h
unifdef-y += time.h
--- a/include/linux/synclink.h 2007-04-25 22:08:32.0 -0500
+++ b/include/linux/synclink.h 2007-05-09 10:37:20.0 -0500
@@ -291,4 +291,29 @@ struct gpio_desc {
#define MGSL_IOCGGPIO _IOR(MGSL_MAGIC_IOC,17,struct gpio_desc)
#define MGSL_IOCWAITGPIO _IOWR(MGSL_MAGIC_IOC,18,struct gpio_desc)
+#ifdef __KERNEL__
+/* provide 32 bit ioctl compatibility on 64 bit systems */
+#ifdef CONFIG_COMPAT
+#include linux/compat.h
+struct MGSL_PARAMS32
+{
+ compat_ulong_t mode;
+ unsigned char loopback;
+ unsigned short flags;
+ unsigned char encoding;
+ compat_ulong_t clock_speed;
+ unsigned char addr_filter;
+ unsigned short crc_type;
+ unsigned char preamble_length;
+ unsigned char preamble;
+ compat_ulong_t data_rate;
+ unsigned char data_bits;
+ unsigned char stop_bits;
+ unsigned char parity;
+};
+#define MGSL_IOCSPARAMS32 _IOW(MGSL_MAGIC_IOC,0,struct MGSL_PARAMS32)
+#define MGSL_IOCGPARAMS32 _IOR(MGSL_MAGIC_IOC,1,struct MGSL_PARAMS32)
+#endif
+#endif
+
#endif /* _SYNCLINK_H_ */
--- a/drivers/char/synclink_gt.c2007-04-25 22:08:32.0 -0500
+++ b/drivers/char/synclink_gt.c2007-05-09 10:12:29.0 -0500
@@ -1171,6 +1171,112 @@ static int ioctl(struct tty_struct *tty,
}
/*
+ * support for 32 bit ioctl calls on 64 bit systems
+ */
+#ifdef CONFIG_COMPAT
+static long get_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user
*user_params)
+{
+ struct MGSL_PARAMS32 tmp_params;
+
+ DBGINFO((%s get_params32\n, info-device_name));
+ tmp_params.mode= (compat_ulong_t)info-params.mode;
+ tmp_params.loopback= info-params.loopback;
+ tmp_params.flags = info-params.flags;
+ tmp_params.encoding= info-params.encoding;
+ tmp_params.clock_speed = (compat_ulong_t)info-params.clock_speed;
+ tmp_params.addr_filter = info-params.addr_filter;
+ tmp_params.crc_type= info-params.crc_type;
+ tmp_params.preamble_length = info-params.preamble_length;
+ tmp_params.preamble= info-params.preamble;
+ tmp_params.data_rate = (compat_ulong_t)info-params.data_rate;
+ tmp_params.data_bits = info-params.data_bits;
+ tmp_params.stop_bits = info-params.stop_bits;
+ tmp_params.parity = info-params.parity;
+ if (copy_to_user(user_params, tmp_params, sizeof(struct
MGSL_PARAMS32)))
+ return -EFAULT;
+ return 0;
+}
+
+static long set_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user
*new_params)
+{
+ struct MGSL_PARAMS32 tmp_params;
+
+ DBGINFO((%s set_params32\n, info-device_name));
+ if (copy_from_user(tmp_params, new_params, sizeof(struct
MGSL_PARAMS32)))
+ return -EFAULT;
+
+ spin_lock(info-lock);
+ info-params.mode= tmp_params.mode;
+ info-params.loopback= tmp_params.loopback;
+ info-params.flags = tmp_params.flags;
+ info-params.encoding= tmp_params.encoding;
+ info-params.clock_speed = tmp_params.clock_speed;
+ info-params.addr_filter = tmp_params.addr_filter;
+ info-params.crc_type= tmp_params.crc_type;
+ info-params.preamble_length = tmp_params.preamble_length;
+ info-params.preamble= tmp_params.preamble;
+ info-params.data_rate = tmp_params.data_rate;
+ info-params.data_bits = tmp_params.data_bits;
+ info-params.stop_bits = tmp_params.stop_bits;
+ info-params.parity = tmp_params.parity;
+ spin_unlock(info-lock);
+
+ change_params(info);
+
+ return 0;
+}
+
+static long slgt_compat_ioctl(struct tty_struct *tty, struct file *file,
+unsigned int cmd, unsigned long arg)
+{
+ struct slgt_info *info = tty-driver_data;
+ int rc = -ENOIOCTLCMD;
+
+ if (sanity_check(info, tty-name, compat_ioctl))
+ return -ENODEV;
+ DBGINFO((%s compat_ioctl() cmd=%08X\n, info-device_name, cmd));
+
+ switch (cmd) {
+
+ case MGSL_IOCSPARAMS32:
+ rc = set_params32(info, compat_ptr(arg));
+ break;
+
+ case
Re: [PATCH] Use tty_schedule in VT code.
Paul Fulghum wrote: As the tty flip buffer code has evolved, that delay value of 1 was carried along. It may have had some historical purpose, but I can't figure it out and it appears to have no use currently. I looked further back and in the 2.4 kernels this scheduling was done with the timer task queue (process receive data on next timer tick). I guess the schedule_delayed_work() with a time delay of 1 was the best approximation of the previous behavior. There is no logical reason to delay the first attempt at processing receive data so schedule_delayed_work() in tty_schedule_flip() should be changed to 0 (as was the case for con_schedule_flip). The schedule_delayed_work in flush_to_ldisc() will continue to use a delay of 1 if the ldisc can't accept more data. This allows the user app and ldisc to catch up. Subsequent calls to tty_schedule_flip won't affect this 'back off' delay because once the work is scheduled (with a delay of 1) new scheduling calls are ignored for the same work structure. I've been testing the change to 0 in tty_schedule_flip() under various loads and data rates with no ill effects. -- Paul Fulghum Microgate Systems, Ltd. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
James Simmons wrote: Great Here is the patch updated with the delay value set to zero. Acked-by: Paul Fulghum [EMAIL PROTECTED] You should submit this to Andrew Morton [EMAIL PROTECTED] so it can get into the mm tree. -- Paul - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
Arnd Bergmann wrote: To solve this, you can to change include/linux/Kbuild to list synclink.h as unifdef-y instead of header-y, and put the parts that you don't want to be in user space inside of #ifdef __KERNEL__. Alternatively, you can put these kernel-internal definitions into a private header file in drivers/char that does not get installed in the first place. That would be particularly useful if you can also move other parts of linux/synclink.h into the private header, when they are not part of the external ABI. Understood. That is the last piece to the puzzle. I think the first approach would be better as synclink.h is all interface definitions. The kernel specific parts are in the individual synclink drivers (such as register definitions, etc). The only exception to this is now the ioctl32 structures and I would hate to break out a whole new header just for that. Thanks again for your targeted and informative help. (Thanks to Andrew for your patience in dealing with a patch that never should have been submitted.) -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
On Sun, 2007-05-06 at 02:27 +0200, Arnd Bergmann wrote: > Now that you mention the duplication, this sounds wrong as well. The easiest > solution is probably to just put the definition of your data structure > inside of #ifdef CONFIG_COMPAT in the header file. The structure definition was already inside of #ifdef CONFIG_COMPAT The problem is including linux/compat.h inside of synclink.h causes an error on i386. The headerscheck facility is spitting out an error even if the #include is inside of #ifdef CONFIG_COMPAT make[3]: *** No rule to make target `/usr/src/devel/usr/include/linux/.check.synclink.h', needed by `__headerscheck'. Stop. linux/kexec.h includes linux/compat.h without a similar error, though that is inside of a #ifdef CONFIG_KEXEC Moving linux/compat.h from synclink.h to synclink_gt.c removes the error. This is the last error standing in my way and I'm trying to figure out the rules for when and where you are allowed to use compat.h, I'm not familiar with the headerscheck facility so I'm not sure what it is looking for and the error is not very helpful. There is nothing in Documentation covering it. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
On Tue, 2007-05-08 at 21:10 +0100, James Simmons wrote: > > This patch has the VT subsystem use tty_schedule_flip instead of > con_schedule_flip. There are two ways we can approach this. We can > do the below path or extend tty_schedule_flip to accept a time field. > Comments welcomed. This looks reasonable. I don't think a time field is necessary. In fact, I think the scheduled_delayed_work() in tty_schedule_flip() should use a time of 0 just like con_schedule_flip(). As the tty flip buffer code has evolved, that delay value of 1 was carried along. It may have had some historical purpose, but I can't figure it out and it appears to have no use currently. It would be better for performance to process the receive data as soon as possible (delay value of 0). -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use tty_schedule in VT code.
On Tue, 2007-05-08 at 21:10 +0100, James Simmons wrote: This patch has the VT subsystem use tty_schedule_flip instead of con_schedule_flip. There are two ways we can approach this. We can do the below path or extend tty_schedule_flip to accept a time field. Comments welcomed. This looks reasonable. I don't think a time field is necessary. In fact, I think the scheduled_delayed_work() in tty_schedule_flip() should use a time of 0 just like con_schedule_flip(). As the tty flip buffer code has evolved, that delay value of 1 was carried along. It may have had some historical purpose, but I can't figure it out and it appears to have no use currently. It would be better for performance to process the receive data as soon as possible (delay value of 0). -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
On Sun, 2007-05-06 at 02:27 +0200, Arnd Bergmann wrote: Now that you mention the duplication, this sounds wrong as well. The easiest solution is probably to just put the definition of your data structure inside of #ifdef CONFIG_COMPAT in the header file. The structure definition was already inside of #ifdef CONFIG_COMPAT The problem is including linux/compat.h inside of synclink.h causes an error on i386. The headerscheck facility is spitting out an error even if the #include linux/compat.h is inside of #ifdef CONFIG_COMPAT make[3]: *** No rule to make target `/usr/src/devel/usr/include/linux/.check.synclink.h', needed by `__headerscheck'. Stop. linux/kexec.h includes linux/compat.h without a similar error, though that is inside of a #ifdef CONFIG_KEXEC Moving linux/compat.h from synclink.h to synclink_gt.c removes the error. This is the last error standing in my way and I'm trying to figure out the rules for when and where you are allowed to use compat.h, I'm not familiar with the headerscheck facility so I'm not sure what it is looking for and the error is not very helpful. There is nothing in Documentation covering it. -- Paul Fulghum Microgate Systems, Ltd - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
Arnd Bergmann wrote: To solve this, you can to change include/linux/Kbuild to list synclink.h as unifdef-y instead of header-y, and put the parts that you don't want to be in user space inside of #ifdef __KERNEL__. Alternatively, you can put these kernel-internal definitions into a private header file in drivers/char that does not get installed in the first place. That would be particularly useful if you can also move other parts of linux/synclink.h into the private header, when they are not part of the external ABI. Understood. That is the last piece to the puzzle. I think the first approach would be better as synclink.h is all interface definitions. The kernel specific parts are in the individual synclink drivers (such as register definitions, etc). The only exception to this is now the ioctl32 structures and I would hate to break out a whole new header just for that. Thanks again for your targeted and informative help. (Thanks to Andrew for your patience in dealing with a patch that never should have been submitted.) -- Paul - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty flush flip buffer on ldisc input queue flush
Flush the tty flip buffer when the line discipline
input queue is flushed, including the user call
tcflush(TCIFLUSH/TCIOFLUSH). This prevents unexpected
stale data after a user application calls tcflush().
Cc: Alan Cox <[EMAIL PROTECTED]>
Cc: Antonino Ingargiola <[EMAIL PROTECTED]>
Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]>
--- a/drivers/char/tty_io.c 2007-05-04 05:46:55.0 -0500
+++ b/drivers/char/tty_io.c 2007-05-05 03:23:46.0 -0500
@@ -365,6 +365,29 @@ static void tty_buffer_free(struct tty_s
}
/**
+ * tty_buffer_flush- flush full tty buffers
+ * @tty: tty to flush
+ *
+ * flush all the buffers containing receive data
+ *
+ * Locking: none
+ */
+
+static void tty_buffer_flush(struct tty_struct *tty)
+{
+ struct tty_buffer *thead;
+ unsigned long flags;
+
+ spin_lock_irqsave(>buf.lock, flags);
+ while((thead = tty->buf.head) != NULL) {
+ tty->buf.head = thead->next;
+ tty_buffer_free(tty, thead);
+ }
+ tty->buf.tail = NULL;
+ spin_unlock_irqrestore(>buf.lock, flags);
+}
+
+/**
* tty_buffer_find - find a free tty buffer
* @tty: tty owning the buffer
* @size: characters wanted
@@ -1240,6 +1263,7 @@ void tty_ldisc_flush(struct tty_struct *
ld->flush_buffer(tty);
tty_ldisc_deref(ld);
}
+ tty_buffer_flush(tty);
}
EXPORT_SYMBOL_GPL(tty_ldisc_flush);
@@ -3336,6 +3360,15 @@ int tty_ioctl(struct inode * inode, stru
case TIOCMBIC:
case TIOCMBIS:
return tty_tiocmset(tty, file, cmd, p);
+ case TCFLSH:
+ switch (arg) {
+ case TCIFLUSH:
+ case TCIOFLUSH:
+ /* flush tty buffer and allow ldisc to process
ioctl */
+ tty_buffer_flush(tty);
+ break;
+ }
+ break;
}
if (tty->driver->ioctl) {
retval = (tty->driver->ioctl)(tty, file, cmd, arg);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty flush flip buffer on ldisc input queue flush
Flush the tty flip buffer when the line discipline
input queue is flushed, including the user call
tcflush(TCIFLUSH/TCIOFLUSH). This prevents unexpected
stale data after a user application calls tcflush().
Cc: Alan Cox [EMAIL PROTECTED]
Cc: Antonino Ingargiola [EMAIL PROTECTED]
Signed-off-by: Paul Fulghum [EMAIL PROTECTED]
--- a/drivers/char/tty_io.c 2007-05-04 05:46:55.0 -0500
+++ b/drivers/char/tty_io.c 2007-05-05 03:23:46.0 -0500
@@ -365,6 +365,29 @@ static void tty_buffer_free(struct tty_s
}
/**
+ * tty_buffer_flush- flush full tty buffers
+ * @tty: tty to flush
+ *
+ * flush all the buffers containing receive data
+ *
+ * Locking: none
+ */
+
+static void tty_buffer_flush(struct tty_struct *tty)
+{
+ struct tty_buffer *thead;
+ unsigned long flags;
+
+ spin_lock_irqsave(tty-buf.lock, flags);
+ while((thead = tty-buf.head) != NULL) {
+ tty-buf.head = thead-next;
+ tty_buffer_free(tty, thead);
+ }
+ tty-buf.tail = NULL;
+ spin_unlock_irqrestore(tty-buf.lock, flags);
+}
+
+/**
* tty_buffer_find - find a free tty buffer
* @tty: tty owning the buffer
* @size: characters wanted
@@ -1240,6 +1263,7 @@ void tty_ldisc_flush(struct tty_struct *
ld-flush_buffer(tty);
tty_ldisc_deref(ld);
}
+ tty_buffer_flush(tty);
}
EXPORT_SYMBOL_GPL(tty_ldisc_flush);
@@ -3336,6 +3360,15 @@ int tty_ioctl(struct inode * inode, stru
case TIOCMBIC:
case TIOCMBIS:
return tty_tiocmset(tty, file, cmd, p);
+ case TCFLSH:
+ switch (arg) {
+ case TCIFLUSH:
+ case TCIOFLUSH:
+ /* flush tty buffer and allow ldisc to process
ioctl */
+ tty_buffer_flush(tty);
+ break;
+ }
+ break;
}
if (tty-driver-ioctl) {
retval = (tty-driver-ioctl)(tty, file, cmd, arg);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [SOLVED] Serial buffer corruption [was Re: FTDI usb-serial possible bug]
Alan Cox wrote: On Sat, 5 May 2007 20:07:15 +0200 Oliver Neukum <[EMAIL PROTECTED]> wrote: should I understand this so that, if tty_buffer_request_room() returns less than requested, the rest of the data should be dropped on the floor? If it returns NULL then either there is > 64K buffered (we can adjust that if anyone shows need - its just for sanity) or the system is out of RAM. It sounds bad, but I think dropping the data make sense with the new tty buffering code. My interpretation of the tty buffering is that it is intended to be the main receive buffer facility for the driver. It simplifies and centralizes these functions so the driver does not need implement policies such as when to flush for user request, expand under load, crop when too large. It should not be the driver's responsibility to try and work around the tty buffering if it becomes full. That adds other complexity such as when the driver should attempt to push the data again: when more data is received? after a timeout? If the tty buffering runs dry, then maybe put out an error message. If the losses occur too often then the tty buffering code needs to be adjusted. -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [SOLVED] Serial buffer corruption [was Re: FTDI usb-serial possible bug]
Antonino Ingargiola wrote: For my use case would be more sensible to accept the new data and discard the older one in the tty buffer: the tty buffer would be a moving window of the most recent incoming data. This because if someone does not read the incoming data maybe he's not interested in it. When he finally reads the data (assuming there was buffer underrun) he's likely interested to the more recent chunk of incoming data and not to an "head" of the data firstly received. Since I acquire measurement data from serial this make perfect sense for me. But does this make sense in general too? There is no one policy here that will make everyone happy. Some will want all the data before some was lost, others the data after some was lost. The real goal is to minimize any data loss. However, whatever policy the buffer uses, the fundamental point it's that when I flush the input buffer I should be sure that each byte read after the flush is *new* (current) data and not old one. This because when the input stream can be stopped I can check that there are 0 byte in the buffer, but when the stream can't be stopped I must use a flush-and-sleep (multiple times) heuristic before I can read a single *reliable* byte. The flush minimizes stale data the application must process. As Alan said in his response there are other sources of stale data beyond the kernel's control. But we absolutely should be flushing all buffers we control. In the end, if more reliability is needed the application must implement its own discipline of framing (defining block boundaries) and checking (CRC) on the raw data stream. -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [SOLVED] Serial buffer corruption [was Re: FTDI usb-serial possible bug]
Antonino Ingargiola wrote: For my use case would be more sensible to accept the new data and discard the older one in the tty buffer: the tty buffer would be a moving window of the most recent incoming data. This because if someone does not read the incoming data maybe he's not interested in it. When he finally reads the data (assuming there was buffer underrun) he's likely interested to the more recent chunk of incoming data and not to an head of the data firstly received. Since I acquire measurement data from serial this make perfect sense for me. But does this make sense in general too? There is no one policy here that will make everyone happy. Some will want all the data before some was lost, others the data after some was lost. The real goal is to minimize any data loss. However, whatever policy the buffer uses, the fundamental point it's that when I flush the input buffer I should be sure that each byte read after the flush is *new* (current) data and not old one. This because when the input stream can be stopped I can check that there are 0 byte in the buffer, but when the stream can't be stopped I must use a flush-and-sleep (multiple times) heuristic before I can read a single *reliable* byte. The flush minimizes stale data the application must process. As Alan said in his response there are other sources of stale data beyond the kernel's control. But we absolutely should be flushing all buffers we control. In the end, if more reliability is needed the application must implement its own discipline of framing (defining block boundaries) and checking (CRC) on the raw data stream. -- Paul - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [SOLVED] Serial buffer corruption [was Re: FTDI usb-serial possible bug]
Alan Cox wrote: On Sat, 5 May 2007 20:07:15 +0200 Oliver Neukum [EMAIL PROTECTED] wrote: should I understand this so that, if tty_buffer_request_room() returns less than requested, the rest of the data should be dropped on the floor? If it returns NULL then either there is 64K buffered (we can adjust that if anyone shows need - its just for sanity) or the system is out of RAM. It sounds bad, but I think dropping the data make sense with the new tty buffering code. My interpretation of the tty buffering is that it is intended to be the main receive buffer facility for the driver. It simplifies and centralizes these functions so the driver does not need implement policies such as when to flush for user request, expand under load, crop when too large. It should not be the driver's responsibility to try and work around the tty buffering if it becomes full. That adds other complexity such as when the driver should attempt to push the data again: when more data is received? after a timeout? If the tty buffering runs dry, then maybe put out an error message. If the losses occur too often then the tty buffering code needs to be adjusted. -- Paul - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [SOLVED] Serial buffer corruption [was Re: FTDI usb-serial possible bug]
On Sat, 2007-05-05 at 18:58 +0200, Antonino Ingargiola wrote:
> This patch does not solve the problem with the cdc_acd driver. I still
> need to flush two times to really flush the input. And the "secondary
> buffer" still seems 26 bytes (as before).
I missed a bit, try this.
--- a/drivers/usb/class/cdc-acm.c 2007-04-25 22:08:32.0 -0500
+++ b/drivers/usb/class/cdc-acm.c 2007-05-05 12:03:19.0 -0500
@@ -335,8 +335,6 @@ static void acm_rx_tasklet(unsigned long
spin_lock_irqsave(>throttle_lock, flags);
throttled = acm->throttle;
spin_unlock_irqrestore(>throttle_lock, flags);
- if (throttled)
- return;
next_buffer:
spin_lock_irqsave(>read_lock, flags);
@@ -355,18 +353,9 @@ next_buffer:
spin_lock_irqsave(>throttle_lock, flags);
throttled = acm->throttle;
spin_unlock_irqrestore(>throttle_lock, flags);
- if (!throttled)
- tty_insert_flip_string(tty, buf->base, buf->size);
+ tty_insert_flip_string(tty, buf->base, buf->size);
tty_flip_buffer_push(tty);
- if (throttled) {
- dbg("Throttling noticed");
- spin_lock_irqsave(>read_lock, flags);
- list_add(>list, >filled_read_bufs);
- spin_unlock_irqrestore(>read_lock, flags);
- return;
- }
-
spin_lock_irqsave(>read_lock, flags);
list_add(>list, >spare_read_bufs);
spin_unlock_irqrestore(>read_lock, flags);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
