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, &tty->flags)) || >>> +(ret = 0, tty_hung_up_p(file)) || >>> +(rbuf = n_hdlc_buf_get(&n_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 &ret and &rbuf 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(&n_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(&tty->write_wait, &wait);
>
> for (;;) {
> - set_current_state(TASK_INTERRUPTIBLE);
> -
> tbuf = n_hdlc_buf_get(&n_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/
[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: 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/
[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)
-
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(&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 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
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/
[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 i
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(&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: 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] 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: 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] 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: [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, &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_i
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/
[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]
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(&acm->throttle_lock, flags);
throttled = acm->throttle;
spin_unlock_irqrestore(&acm->throttle_lock, flags);
- if (throttled)
- return;
next_buffer:
spin_lock_irqsave(&acm->read_lock, flags);
@@ -355,18 +353,9 @@ next_buffer:
spin_lock_irqsave(&acm->throttle_lock, flags);
throttled = acm->throttle;
spin_unlock_irqrestore(&acm->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(&acm->read_lock, flags);
- list_add(&buf->list, &acm->filled_read_bufs);
- spin_unlock_irqrestore(&acm->read_lock, flags);
- return;
- }
-
spin_lock_irqsave(&acm->read_lock, flags);
list_add(&buf->list, &acm->spare_read_bufs);
spin_unlock_irqrestore(&acm->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/
Re: [SOLVED] Serial buffer corruption [was Re: FTDI usb-serial possible bug]
On Sat, 2007-05-05 at 18:46 +0200, Oliver Neukum wrote: > Am Samstag, 5. Mai 2007 18:08 schrieb Paul Fulghum: > > I would argue that cdc-acm should do the same as the serial driver. > > Has this been tested? > If so we could reduce the complexity of the throtteling logic in the usb > drivers. Antonino is doing so now. I think Alan nailed it: with the old tty buffering the extra logic was required to avoid data loss. The new tty buffering handles large blocks of data with no problem. Removing this part of the throttle logic makes the input flushing mechanism complete. -- 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 11:08 -0500, Paul Fulghum wrote:
> I would argue that cdc-acm should do the same as the serial driver.
Try this patch for the usb ports. (I don't have that hardware)
--- 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 11:12:10.0 -0500
@@ -355,18 +355,9 @@ next_buffer:
spin_lock_irqsave(&acm->throttle_lock, flags);
throttled = acm->throttle;
spin_unlock_irqrestore(&acm->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(&acm->read_lock, flags);
- list_add(&buf->list, &acm->filled_read_bufs);
- spin_unlock_irqrestore(&acm->read_lock, flags);
- return;
- }
-
spin_lock_irqsave(&acm->read_lock, flags);
list_add(&buf->list, &acm->spare_read_bufs);
spin_unlock_irqrestore(&acm->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/
Re: [PATCH] synclink_gt add compat_ioctl
Arnd Bergmann wrote: On Friday 04 May 2007, Paul Fulghum wrote: - It is fishy that apart from one outlier in kexec.h, synclink.h is the only header file which uses compat_ulong_t. Are we doing this right? Arnd, do you have any comment on this? I think most others just define the compat data structures in the same file that implements the headers, inside the same #ifdef that hides the functions using them. This makes sense, because the data structures here don't define an interface, but rather describe what the interface looks like in the 32 bit case. OK, moving the compatible structure declarations from the header to the individual source files will fix all the header mess and the odd compilation errors on i386 when using the compat.h header inside of another header. That declaration will need to be duplicated in each driver that uses it (4 drivers in my case). In that sense (a structure declaration used by multiple code modules) it does seem like an interface definition. If that is what is needed, I will do it. -- 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 Fri, 2007-05-04 at 17:30 -0600, Paul Fulghum wrote:
> OK, this behavior is so unexpected I must be missing
> something basic.
And so I was. Try this patch.
--- 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]
On Sat, 2007-05-05 at 10:43 -0600, Paul Fulghum wrote: > There is not an input flush method for the tty driver > and individual drivers don't process that ioctl. > The tty drivers I've seen immediately pass receive data to the > tty buffering and I'm not sure why a driver would > behave otherwise. cdc-acm does its own buffering. In your case, the line discipline throttled the tty device because the ldisc buffer was full. If the line discipline throttles the driver input, the cdc-acm driver stops giving data to the tty buffering and instead stores them internally. In the serial driver this usually just results in dropping RTS to signal the remote end to stop sending. The serial driver always immediately gives receive data to the tty buffering without regard to the throttled state. I would argue that cdc-acm should do the same as the serial driver. -- 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: The patch now boot properly and solves completely the testcase with two serial lines: Good, thanks for testing. I think this patch should be included in mainline, since if one flushes the input buffer, really want to flush the entire buffer chain and doesn't want to read any old data _after_ a flush. I will submit the patch, it's clearly needed. However I also tested a usb-serial device (that uses the cdc-acm driver) and in this case I still need _two_ flushInput() to totally flush the input buffer. There can be another *secondary buffer* in the usb-serial driver? Can this buffer be emptied out too? Very possible, but I'm not familiar with that. There is not an input flush method for the tty driver and individual drivers don't process that ioctl. The tty drivers I've seen immediately pass receive data to the tty buffering and I'm not sure why a driver would behave otherwise. -- 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
Andrew Morton wrote: On Thu, 03 May 2007 13:01:17 -0500 Paul Fulghum <[EMAIL PROTECTED]> wrote: Add compat_ioctl handler to synclink_gt driver. i386 allmodconfig: make[3]: *** No rule to make target `/usr/src/devel/usr/include/linux/.check.synclink.h', needed by `__headerscheck'. Stop. I got tired of this patch - I think I'll drop it. This all seems to be related to the use of compat_ulong_t Since my original patch worked fine using unsigned int, how about I go back to that? -- 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: Can you try two tests (with my patch applied): 1. comment out the tty_flush_buffer() call in tty_ldisc_flush() and test 2. uncomment (reenable) the above call and comment out the tty_flush_buffer() call in tty_ioctl() and test 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: [SOLVED] Serial buffer corruption [was Re: FTDI usb-serial possible bug]
Antonino Ingargiola wrote: "With the patch, flushing the input results effectively in a complete flush. However after doing the flush I can't read [further] chars [sent to the serial port] without closing and reopening the port. I've verified this behavior both communicating between two serial ports and both communicating with an usb-serial device (driver cdc-acm)." OK, this behavior is so unexpected I must be missing something basic. Not being able to reproduce this myself is a problem. If I think of something I'll post. -- 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 add compat_ioctl
Arnd Bergmann wrote: - The return value of the new compat_ioctl methods should probably 'int', not 'long'. We've had the discussion before and then decided not to change the existing compat_ioctl and unlocked_ioctl functions -- even though int is more appropriate, but having the same prototype has the advantage that a driver can use the same function for both ->ioctl and ->compat_ioctl if all calls are compatible. Any comments on this or should I assume that compat_ioctl() will stay with ulong return type? -- 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
Andrew Morton wrote: In file included from drivers/char/synclink_gt.c:85: include/linux/synclink.h:175: error: expected specifier-qualifier-list before 'compat_ulong_t' - We might as well do the same ifdef-avoidery trick around compat_ioctl() too. That required that it be renamed. - It is fishy that apart from one outlier in kexec.h, synclink.h is the only header file which uses compat_ulong_t. Are we doing this right? Arnd, do you have any comment on this? It seems like the compatible types should be available in something that is already commonly used like linux/types.h I'm fine with it either way. I'm not in a position to be making those kinds of decisions for widely used infrastructure, so I'll leave that for someone further up the food chain. -- 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: [SOLVED] Serial buffer corruption [was Re: FTDI usb-serial possible bug]
On Fri, 2007-05-04 at 21:06 +0200, Antonino Ingargiola wrote:
> Filling with echo console-screen.sh I've found that the blocking command is:
>
> unicode_start 2> /dev/null || true
>
> or at least the echo before this command is the last shown.
I still don't know what is blocking.
It is possible some tty device is operating with an improperly
initialized tty structure. I vaguely remember some console code
creating its own minimally initialized 'dummy' tty structure.
This might be causing the new flush code to hang.
Try this patch which does not call the flush unless a line
discipline is attached to the tty. That should indicate
a normally initialized tty structure.
--- a/drivers/char/tty_io.c 2007-04-25 22:08:32.0 -0500
+++ b/drivers/char/tty_io.c 2007-05-04 12:15:18.0 -0500
@@ -365,6 +365,28 @@ 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);
+ }
+ spin_unlock_irqrestore(&tty->buf.lock, flags);
+}
+
+/**
* tty_buffer_find - find a free tty buffer
* @tty: tty owning the buffer
* @size: characters wanted
@@ -1236,8 +1258,10 @@ void tty_ldisc_flush(struct tty_struct *
{
struct tty_ldisc *ld = tty_ldisc_ref(tty);
if(ld) {
- if(ld->flush_buffer)
+ if(ld->flush_buffer) {
ld->flush_buffer(tty);
+ tty_buffer_flush(tty);
+ }
tty_ldisc_deref(ld);
}
}
@@ -3336,6 +3360,20 @@ 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 */
+ ld = tty_ldisc_ref_wait(tty);
+ if (ld) {
+ if (ld->ioctl)
+ tty_buffer_flush(tty);
+ tty_ldisc_deref(ld);
+ }
+ 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_set_ldisc receive_room fix
Fix tty_set_ldisc in tty_io.c so that tty->receive_room is only cleared if actually changing line disciplines. Without this fix a problem occurs when requesting the line discipline to change to the same line discipline. In this case tty->receive_room is cleared but ldisc->open() is not called to set tty->receive_room back to a sane value. The result is that tty->receive_room is stuck at 0 preventing the tty flip buffer from passing receive data to the line discipline. For example: a switch from N_TTY to N_TTY followed by a select() call for read input results in data never being received because tty->receive_room is stuck at zero. A switch from N_TTY to N_TTY followed by a read() call works because the read() call itself sets tty->receive_room correctly (but select does not). Previously (< 2.6.18) this was not a problem because the tty flip buffer pushed data to the line discipline without regard for tty->receive room. Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]> --- a/drivers/char/tty_io.c 2007-04-25 22:08:32.0 -0500 +++ b/drivers/char/tty_io.c 2007-05-04 14:25:08.0 -0500 @@ -936,13 +936,6 @@ restart: return -EINVAL; /* -* No more input please, we are switching. The new ldisc -* will update this value in the ldisc open function -*/ - - tty->receive_room = 0; - - /* * Problem: What do we do if this blocks ? */ @@ -953,6 +946,13 @@ restart: return 0; } + /* +* No more input please, we are switching. The new ldisc +* will update this value in the ldisc open function +*/ + + tty->receive_room = 0; + o_ldisc = tty->ldisc; o_tty = tty->link; - 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 Fri, 2007-05-04 at 19:25 +0200, Antonino Ingargiola wrote: > No. Ehmm ... I've an usb keybord and vga monitor on a standard desktop pc. OK, I'm stumped. I don't see how my patch could cause the machine to not boot and I'm not seeing that behavior here. -- 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: [SOLVED] Serial buffer corruption [was Re: FTDI usb-serial possible bug]
On Fri, 2007-05-04 at 19:13 +0200, Antonino Ingargiola wrote: > On 5/4/07, Paul Fulghum <[EMAIL PROTECTED]> wrote: > > Antonino Ingargiola wrote: > > > The system blocks during booting. I can unblock it with SysReq+K but > > > then I'm unable to log into X. > > > > Hmmm, it tests here with no problem. > > > > Does reversing the patch and rebuilding the kernel fix the boot? > > The kernel to which I applied the patch was vanilla 2.6.21.1, and it > is the one I'm currently running (no config changes or other patches). Are you using a serial port as console? -- 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: [SOLVED] Serial buffer corruption [was Re: FTDI usb-serial possible bug]
Antonino Ingargiola wrote: The system blocks during booting. I can unblock it with SysReq+K but then I'm unable to log into X. Hmmm, it tests here with no problem. Does reversing the patch and rebuilding the kernel fix the boot? -- 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: [SOLVED] Serial buffer corruption [was Re: FTDI usb-serial possible bug]
Here is a patch against 2.6.21 which flushes the tty flip buffer
on ioctl(TCFLSH) for input.
--- a/drivers/char/tty_io.c 2007-04-25 22:08:32.0 -0500
+++ b/drivers/char/tty_io.c 2007-05-04 09:30:01.0 -0500
@@ -365,6 +365,28 @@ 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);
+ }
+ 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 +1262,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 +3359,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]
Paul Fulghum wrote: A new function to flush the tty flip buffer needs to be added and then called from tty_io.c:tty_ldisc_flush(). That is not enough. It looks like tty_ioctl() needs to process ioctl(TCFLSH) to clear the flip buffer before passing that ioctl to the line discipline. -- 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: [SOLVED] Serial buffer corruption [was Re: FTDI usb-serial possible bug]
Antonino Ingargiola wrote: Nope. In python I use the flushInput() method of the serial object defined by the pyserial library[0]. The method does just this system call: termios.tcflush(self.fd, TERMIOS.TCIFLUSH) that I think is correct. There is intermediate buffering between the driver and the line discipline called the tty flip buffer. receive data flow: driver --> tty flip --> line discipline --> application When you flush input, the line disciplines flush_buffer() method is called to clear any data residing the in the line discipline. This does not affect the tty flip buffer or hardware receive FIFOs. I suspect the biggest problem is the data in the tty flip buffer. A new function to flush the tty flip buffer needs to be added and then called from tty_io.c:tty_ldisc_flush(). Then a call to tcflush(TCIFLUSH) will clear both buffers. This still would not clear any data in the hardware receive FIFOs. -- 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 add compat_ioctl
Paul Fulghum wrote: Arnd Bergmann wrote: - In your driver you don't get the big kernel lock in the compat_ioctl function. I assume that this is correct for the particular driver, but it may be nice if you could consequently also add an unlocked_ioctl function that can be used without the BKL for native ioctls. It would be good to hear an opinon on this from someone who has an insight in tty locking issues though, so I'm Cc:ing some people who have touched that recently. I don't count on higher level locking for synchronization issues specific to the driver. I thought the current compat_ioctl() was already meant to *not* have the BKL just like unlocked_ioctl. My thought was that any driver getting a recent update like compat_ioctl() would need to be reviewed for BKL safety and take the lock manually if necessary. Nevermind. I misread what you wrote (I'm tired). Yes, adding an unlocked_ioctl() makes sense. -- 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 add compat_ioctl
Arnd Bergmann wrote: - The return value of the new compat_ioctl methods should probably 'int', not 'long'. We've had the discussion before and then decided not to change the existing compat_ioctl and unlocked_ioctl functions -- even though int is more appropriate, but having the same prototype has the advantage that a driver can use the same function for both ->ioctl and ->compat_ioctl if all calls are compatible. I noticed that but thought the change in return value type had some higher purpose I had not perceived. If it can be int that would be the way to go. - In your driver you don't get the big kernel lock in the compat_ioctl function. I assume that this is correct for the particular driver, but it may be nice if you could consequently also add an unlocked_ioctl function that can be used without the BKL for native ioctls. It would be good to hear an opinon on this from someone who has an insight in tty locking issues though, so I'm Cc:ing some people who have touched that recently. I don't count on higher level locking for synchronization issues specific to the driver. I thought the current compat_ioctl() was already meant to *not* have the BKL just like unlocked_ioctl. My thought was that any driver getting a recent update like compat_ioctl() would need to be reviewed for BKL safety and take the lock manually if necessary. Drivers that are falling behind wont have a compat_ioctl defined at all. -- 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 add compat_ioctl
Add compat_ioctl handler to synclink_gt driver.
The one case requiring a separate 32 bit handler could be
removed by redefining the associated structure in
a way compatible with both 32 and 64 bit systems. But that
approach would break existing native 64 bit user applications.
Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]>
--- a/include/linux/synclink.h 2007-04-25 22:08:32.0 -0500
+++ b/include/linux/synclink.h 2007-05-03 12:44:09.0 -0500
@@ -9,6 +9,8 @@
* the terms of the GNU Public License (GPL)
*/
+#include
+
#ifndef _SYNCLINK_H_
#define _SYNCLINK_H_
#define SYNCLINK_H_VERSION 3.6
@@ -167,6 +169,24 @@ typedef struct _MGSL_PARAMS
} MGSL_PARAMS, *PMGSL_PARAMS;
+/* provide 32 bit ioctl compatibility on 64 bit systems */
+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 MICROGATE_VENDOR_ID 0x13c0
#define SYNCLINK_DEVICE_ID 0x0010
#define MGSCC_DEVICE_ID 0x0020
@@ -276,6 +296,8 @@ struct gpio_desc {
#define MGSL_MAGIC_IOC 'm'
#define MGSL_IOCSPARAMS_IOW(MGSL_MAGIC_IOC,0,struct
_MGSL_PARAMS)
#define MGSL_IOCGPARAMS_IOR(MGSL_MAGIC_IOC,1,struct
_MGSL_PARAMS)
+#define MGSL_IOCSPARAMS32 _IOW(MGSL_MAGIC_IOC,0,struct MGSL_PARAMS32)
+#define MGSL_IOCGPARAMS32 _IOR(MGSL_MAGIC_IOC,1,struct MGSL_PARAMS32)
#define MGSL_IOCSTXIDLE_IO(MGSL_MAGIC_IOC,2)
#define MGSL_IOCGTXIDLE_IO(MGSL_MAGIC_IOC,3)
#define MGSL_IOCTXENABLE _IO(MGSL_MAGIC_IOC,4)
--- a/drivers/char/synclink_gt.c2007-04-25 22:08:32.0 -0500
+++ b/drivers/char/synclink_gt.c2007-05-03 12:40:36.0 -0500
@@ -73,6 +73,7 @@
#include
#include
#include
+#include
#include
#include
@@ -530,6 +531,10 @@ static int set_interface(struct slgt_in
static int set_gpio(struct slgt_info *info, struct gpio_desc __user *gpio);
static int get_gpio(struct slgt_info *info, struct gpio_desc __user *gpio);
static int wait_gpio(struct slgt_info *info, struct gpio_desc __user *gpio);
+#ifdef CONFIG_COMPAT
+static long set_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user
*new_params);
+static long get_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user
*new_params);
+#endif
/*
* driver functions
@@ -1170,6 +1175,55 @@ static int ioctl(struct tty_struct *tty,
return 0;
}
+#ifdef CONFIG_COMPAT
+static long 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 MGSL_IOCGPARAMS32:
+ rc = get_params32(info, compat_ptr(arg));
+ break;
+
+ case MGSL_IOCGPARAMS:
+ case MGSL_IOCSPARAMS:
+ case MGSL_IOCGTXIDLE:
+ case MGSL_IOCGSTATS:
+ case MGSL_IOCWAITEVENT:
+ case MGSL_IOCGIF:
+ case MGSL_IOCSGPIO:
+ case MGSL_IOCGGPIO:
+ case MGSL_IOCWAITGPIO:
+ case TIOCGICOUNT:
+ rc = ioctl(tty, file, cmd, (unsigned long)(compat_ptr(arg)));
+ break;
+
+ case MGSL_IOCSTXIDLE:
+ case MGSL_IOCTXENABLE:
+ case MGSL_IOCRXENABLE:
+ case MGSL_IOCTXABORT:
+ case TIOCMIWAIT:
+ case MGSL_IOCSIF:
+ rc = ioctl(tty, file, cmd, arg);
+ break;
+ }
+
+ DBGINFO(("%s compat_ioctl() cmd=%08X rc=%d\n", info->device_name, cmd,
rc));
+ return rc;
+}
+#endif
+
/*
* proc fs support
*/
@@ -2507,6 +2561,60 @@ static int set_params(struct slgt_info *
return 0;
}
+#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
[PATCH] 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.
Based on patch by Arnd Bergmann:
http://www.uwsg.iu.edu/hypermail/linux/kernel/0511.0/1732.html
Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]>
CC: Arnd Bergmann <[EMAIL PROTECTED]>
--- a/drivers/char/n_tty.c 2007-04-25 22:08:32.0 -0500
+++ b/drivers/char/n_tty.c 2007-05-01 11:00:47.0 -0500
@@ -1544,21 +1544,18 @@ static unsigned int normal_poll(struct t
}
struct tty_ldisc tty_ldisc_N_TTY = {
- TTY_LDISC_MAGIC,/* magic */
- "n_tty",/* name */
- 0, /* num */
- 0, /* flags */
- n_tty_open, /* open */
- n_tty_close,/* close */
- n_tty_flush_buffer, /* flush_buffer */
- n_tty_chars_in_buffer, /* chars_in_buffer */
- read_chan, /* read */
- write_chan, /* write */
- n_tty_ioctl,/* ioctl */
- n_tty_set_termios, /* set_termios */
- normal_poll,/* poll */
- NULL, /* hangup */
- n_tty_receive_buf, /* receive_buf */
- n_tty_write_wakeup /* write_wakeup */
+ .magic = TTY_LDISC_MAGIC,
+ .name= "n_tty",
+ .open= n_tty_open,
+ .close = n_tty_close,
+ .flush_buffer= n_tty_flush_buffer,
+ .chars_in_buffer = n_tty_chars_in_buffer,
+ .read= read_chan,
+ .write = write_chan,
+ .ioctl = n_tty_ioctl,
+ .set_termios = n_tty_set_termios,
+ .poll= normal_poll,
+ .receive_buf = n_tty_receive_buf,
+ .write_wakeup= n_tty_write_wakeup
};
--- a/include/linux/tty_driver.h2007-04-25 22:08:32.0 -0500
+++ b/include/linux/tty_driver.h2007-05-03 10:03:52.0 -0500
@@ -52,6 +52,11 @@
* This routine allows the tty driver to implement
* device-specific ioctl's. If the ioctl number passed in cmd
* is not recognized by the driver, it should return ENOIOCTLCMD.
+ *
+ * long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
+ * unsigned int cmd, unsigned long arg);
+ *
+ * implement ioctl processing for 32 bit process on 64 bit system
*
* void (*set_termios)(struct tty_struct *tty, struct ktermios * old);
*
@@ -132,6 +137,8 @@ struct tty_operations {
int (*chars_in_buffer)(struct tty_struct *tty);
int (*ioctl)(struct tty_struct *tty, struct file * file,
unsigned int cmd, unsigned long arg);
+ long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
+unsigned int cmd, unsigned long arg);
void (*set_termios)(struct tty_struct *tty, struct ktermios * old);
void (*throttle)(struct tty_struct * tty);
void (*unthrottle)(struct tty_struct * tty);
@@ -193,6 +200,8 @@ struct tty_driver {
int (*chars_in_buffer)(struct tty_struct *tty);
int (*ioctl)(struct tty_struct *tty, struct file * file,
unsigned int cmd, unsigned long arg);
+ long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
+unsigned int cmd, unsigned long arg);
void (*set_termios)(struct tty_struct *tty, struct ktermios * old);
void (*throttle)(struct tty_struct * tty);
void (*unthrottle)(struct tty_struct * tty);
--- a/drivers/char/tty_io.c 2007-04-25 22:08:32.0 -0500
+++ b/drivers/char/tty_io.c 2007-05-03 10:42:35.0 -0500
@@ -153,6 +153,11 @@ static int tty_open(struct inode *, stru
static int tty_release(struct inode *, struct file *);
int tty_ioctl(struct inode * inode, struct file * file,
unsigned int cmd, unsigned long arg);
+#ifdef CONFIG_COMPAT
+long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg);
+#else
+#define tty_compat_ioctl NULL
+#endif
static int tty_fasync(int fd, struct file * filp, int on);
static void release_tty(struct tty_struct *tty, int idx);
static struct pid *__proc_set_tty(struct task_struct *tsk,
@@ -1145,8 +1150,8 @@ static unsigned int hung_up_tty_poll(str
return POLLIN | POLLOUT | POLLERR | POLLHUP | POLLRDNORM | POLLWRNORM;
}
-static int hung_up_tty_ioctl(struct inode * inode, struct file * file,
-unsigned int cmd, unsigned long arg)
+static long hung_up_tty_ioctl(struct file * file,
+ unsigned int cmd, unsigned long arg)
{
return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
}
@@ -1157,6 +1162,7 @@ static const struct file_operations tty_
.write = tty_write,
.poll = tty_poll,
.ioctl = tty_ioctl,
+ .compat_ioct
Re: [PATCH] synclink_gt use dynamic tty device registration
On Thu, 2007-05-03 at 00:05 -0700, Andrew Morton wrote:
> On Wed, 02 May 2007 11:17:33 -0500 Paul Fulghum <[EMAIL PROTECTED]> wrote:
>
> > Change synclink_gt driver to use dynamic tty device registration.
> >
> > ...
> >
> > + for (i=0; i < port_count; ++i)
> > + tty_register_device(serial_driver, port_array[i]->line,
> > &(port_array[i]->pdev->dev));
> > ...
> > + for (info=slgt_device_list ; info != NULL ;
> > info=info->next_device)
> > + tty_unregister_device(serial_driver, info->line);
> > ...
> > + if ((rc = pci_register_driver(&pci_driver)) < 0) {
>
> hm, not a big fan of kernel coding style, I see.
It varies. If this idiom bothers you, I can split it into 2 lines.
> What's going to happen here if tty_register_device() fails?
Then the device will not be accessible as a tty device.
It may still be accessible as a network device.
On driver unload, tty_unregister_device() does nothing because
the device was never created.
In this case, tracking the return value does not change anything.
I could add a printk on error to better inform the user that the
kernel's plumbing went south.
--
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: The same function contains a copy_from_user(), which cannot be called with interrupts disabled, so yes, I am very certain it will not change. Good point. -- 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:
+ case MGSL_IOCGPARAMS32:
+ rc = get_params32(info, (struct MGSL_PARAMS32 __user
*)compat_ptr(arg));
+ break;
No need for the cast here.
OK, for some reason I remember getting a warning
error without it. I must have been mistaken.
+
+ spin_lock_irqsave(&info->lock, flags);
no need for _irqsave, just use spin_{,un}lock_irq() when you know that
interrupts are enabled.
That makes me a little uneasy. The locking
mechanisms (and just about everything else) above the driver
seem to change frequently. This involves not just the VFS but
the tty core as well.
If you are confident this will not change, I will
switch to spin_lock(). I used spin_lock_irqsave() to be
more robust against changes to behavior outside my driver.
+/* provide 32 bit ioctl compatibility on 64 bit systems */
+struct MGSL_PARAMS32
+{
+ unsigned intmode;
+ unsigned char loopback;
+ unsigned short flags;
+ unsigned char encoding;
+ unsigned intclock_speed;
+ unsigned char addr_filter;
+ unsigned short crc_type;
+ unsigned char preamble_length;
+ unsigned char preamble;
+ unsigned intdata_rate;
+ unsigned char data_bits;
+ unsigned char stop_bits;
+ unsigned char parity;
+};
The definition is correct, but by convention it would be better to use
compat_ulong_t instead of unsigned int for those fields that are an
unsigned long in native mode.
OK
--
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 add compat_ioctl method
Arnd Bergmann wrote: Looks ok mostly. Just some details: ... +#ifdef CONFIG_COMPAT + long (*compat_ioctl)(struct tty_struct *tty, struct file * file, +unsigned int cmd, unsigned long arg); +#endif I wouldn't hide this inside of an #ifdef. The structures are all static and therefore a single field per driver doesn't add much bload, but being able to always assign the .compat_ioctl pointer makes the code somewhat nicer OK --- a/drivers/char/tty_io.c 2006-11-29 15:57:37.0 -0600 +++ b/drivers/char/tty_io.c 2007-04-30 14:51:01.0 -0500 @@ -151,6 +151,9 @@ static int tty_open(struct inode *, stru static int tty_release(struct inode *, struct file *); int tty_ioctl(struct inode * inode, struct file * file, unsigned int cmd, unsigned long arg); +#ifdef CONFIG_COMPAT +long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg); +#endif static int tty_fasync(int fd, struct file * filp, int on); static void release_mem(struct tty_struct *tty, int idx); declarations should never be hidden inside of an #ifdef. If you want to be extra clever here, you can do OK, I have no problem with that. A declaration without implementation won't generate a warning? #ifdef CONFIG_COMPAT long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg); #else #define tty_compat_ioctl NULL #endif then you don't need an #ifdef in the code setting the function pointers. OK + tty = (struct tty_struct *)file->private_data; no need for the cast, ->private_data is void*. Yes, an easy fix. -- 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 add compat_ioctl
Add compat_ioctl handler to synclink_gt driver.
The one case requiring a separate 32 bit handler could be
removed by redefining the associated structure in
a way compatible with both 32 and 64 bit systems. But that
approach would break existing native 64 bit user applications.
Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]>
--- a/drivers/char/synclink_gt.c2007-04-25 22:08:32.0 -0500
+++ b/drivers/char/synclink_gt.c2007-05-02 14:52:48.0 -0500
@@ -73,6 +73,7 @@
#include
#include
#include
+#include
#include
#include
@@ -530,6 +531,10 @@ static int set_interface(struct slgt_in
static int set_gpio(struct slgt_info *info, struct gpio_desc __user *gpio);
static int get_gpio(struct slgt_info *info, struct gpio_desc __user *gpio);
static int wait_gpio(struct slgt_info *info, struct gpio_desc __user *gpio);
+#ifdef CONFIG_COMPAT
+static long set_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user
*new_params);
+static long get_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user
*new_params);
+#endif
/*
* driver functions
@@ -1170,6 +1175,55 @@ static int ioctl(struct tty_struct *tty,
return 0;
}
+#ifdef CONFIG_COMPAT
+static long 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, (struct MGSL_PARAMS32 __user
*)compat_ptr(arg));
+ break;
+
+ case MGSL_IOCGPARAMS32:
+ rc = get_params32(info, (struct MGSL_PARAMS32 __user
*)compat_ptr(arg));
+ break;
+
+ case MGSL_IOCGPARAMS:
+ case MGSL_IOCSPARAMS:
+ case MGSL_IOCGTXIDLE:
+ case MGSL_IOCGSTATS:
+ case MGSL_IOCWAITEVENT:
+ case MGSL_IOCGIF:
+ case MGSL_IOCSGPIO:
+ case MGSL_IOCGGPIO:
+ case MGSL_IOCWAITGPIO:
+ case TIOCGICOUNT:
+ rc = ioctl(tty, file, cmd, (unsigned long)(compat_ptr(arg)));
+ break;
+
+ case MGSL_IOCSTXIDLE:
+ case MGSL_IOCTXENABLE:
+ case MGSL_IOCRXENABLE:
+ case MGSL_IOCTXABORT:
+ case TIOCMIWAIT:
+ case MGSL_IOCSIF:
+ rc = ioctl(tty, file, cmd, arg);
+ break;
+ }
+
+ DBGINFO(("%s compat_ioctl() cmd=%08X rc=%d\n", info->device_name, cmd,
rc));
+ return rc;
+}
+#endif
+
/*
* proc fs support
*/
@@ -2507,6 +2561,61 @@ static int set_params(struct slgt_info *
return 0;
}
+#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= (unsigned int)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 = (unsigned int)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 = (unsigned int)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)
+{
+ unsigned long flags;
+ 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_irqsave(&info->lock, flags);
+ 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_leng
[PATCH] tty add compat_ioctl method
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.
Based on patch by Arnd Bergmann:
http://www.uwsg.iu.edu/hypermail/linux/kernel/0511.0/1732.html
This patch does not remove tty ioctl entries in compat_ioctl.[ch]
That will be implemented in a separate patch.
Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]>
CC: Arnd Bergmann <[EMAIL PROTECTED]>
--- a/drivers/char/n_tty.c 2007-04-30 15:13:19.0 -0500
+++ b/drivers/char/n_tty.c 2007-04-30 15:17:28.0 -0500
@@ -1545,21 +1545,18 @@ static unsigned int normal_poll(struct t
}
struct tty_ldisc tty_ldisc_N_TTY = {
- TTY_LDISC_MAGIC,/* magic */
- "n_tty",/* name */
- 0, /* num */
- 0, /* flags */
- n_tty_open, /* open */
- n_tty_close,/* close */
- n_tty_flush_buffer, /* flush_buffer */
- n_tty_chars_in_buffer, /* chars_in_buffer */
- read_chan, /* read */
- write_chan, /* write */
- n_tty_ioctl,/* ioctl */
- n_tty_set_termios, /* set_termios */
- normal_poll,/* poll */
- NULL, /* hangup */
- n_tty_receive_buf, /* receive_buf */
- n_tty_write_wakeup /* write_wakeup */
+ .magic = TTY_LDISC_MAGIC,
+ .name= "n_tty",
+ .open= n_tty_open,
+ .close = n_tty_close,
+ .flush_buffer= n_tty_flush_buffer,
+ .chars_in_buffer = n_tty_chars_in_buffer,
+ .read= read_chan,
+ .write = write_chan,
+ .ioctl = n_tty_ioctl,
+ .set_termios = n_tty_set_termios,
+ .poll= normal_poll,
+ .receive_buf = n_tty_receive_buf,
+ .write_wakeup= n_tty_write_wakeup
};
--- a/include/linux/tty_driver.h2006-11-29 15:57:37.0 -0600
+++ b/include/linux/tty_driver.h2007-04-30 14:05:02.0 -0500
@@ -132,6 +132,10 @@ struct tty_operations {
int (*chars_in_buffer)(struct tty_struct *tty);
int (*ioctl)(struct tty_struct *tty, struct file * file,
unsigned int cmd, unsigned long arg);
+#ifdef CONFIG_COMPAT
+ long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
+unsigned int cmd, unsigned long arg);
+#endif
void (*set_termios)(struct tty_struct *tty, struct termios * old);
void (*throttle)(struct tty_struct * tty);
void (*unthrottle)(struct tty_struct * tty);
@@ -193,6 +197,10 @@ struct tty_driver {
int (*chars_in_buffer)(struct tty_struct *tty);
int (*ioctl)(struct tty_struct *tty, struct file * file,
unsigned int cmd, unsigned long arg);
+#ifdef CONFIG_COMPAT
+ long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
+unsigned int cmd, unsigned long arg);
+#endif
void (*set_termios)(struct tty_struct *tty, struct termios * old);
void (*throttle)(struct tty_struct * tty);
void (*unthrottle)(struct tty_struct * tty);
--- a/drivers/char/tty_io.c 2006-11-29 15:57:37.0 -0600
+++ b/drivers/char/tty_io.c 2007-04-30 14:51:01.0 -0500
@@ -151,6 +151,9 @@ static int tty_open(struct inode *, stru
static int tty_release(struct inode *, struct file *);
int tty_ioctl(struct inode * inode, struct file * file,
unsigned int cmd, unsigned long arg);
+#ifdef CONFIG_COMPAT
+long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg);
+#endif
static int tty_fasync(int fd, struct file * filp, int on);
static void release_mem(struct tty_struct *tty, int idx);
@@ -1153,8 +1156,8 @@ static unsigned int hung_up_tty_poll(str
return POLLIN | POLLOUT | POLLERR | POLLHUP | POLLRDNORM | POLLWRNORM;
}
-static int hung_up_tty_ioctl(struct inode * inode, struct file * file,
-unsigned int cmd, unsigned long arg)
+static long hung_up_tty_ioctl(struct file * file,
+ unsigned int cmd, unsigned long arg)
{
return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
}
@@ -1165,6 +1168,9 @@ static const struct file_operations tty_
.write = tty_write,
.poll = tty_poll,
.ioctl = tty_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = tty_compat_ioctl,
+#endif
.open = tty_open,
.release= tty_release,
.fasync = tty_fasync,
@@ -1177,6 +1183,9 @@ static const struct file_operations ptmx
.write = tty_write,
.poll = tty_poll,
.ioctl = tty_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = tty_compat_ioctl,
+#endif
.open
[PATCH] synclink_gt use dynamic tty device registration
Change synclink_gt driver to use dynamic tty device registration.
Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]>
--- a/drivers/char/synclink_gt.c2007-04-25 22:08:32.0 -0500
+++ b/drivers/char/synclink_gt.c2007-05-02 11:11:34.0 -0500
@@ -3415,6 +3415,9 @@ static void device_init(int adapter_num,
}
}
}
+
+ for (i=0; i < port_count; ++i)
+ tty_register_device(serial_driver, port_array[i]->line,
&(port_array[i]->pdev->dev));
}
static int __devinit init_one(struct pci_dev *dev,
@@ -3466,6 +3469,8 @@ static void slgt_cleanup(void)
printk("unload %s %s\n", driver_name, driver_version);
if (serial_driver) {
+ for (info=slgt_device_list ; info != NULL ;
info=info->next_device)
+ tty_unregister_device(serial_driver, info->line);
if ((rc = tty_unregister_driver(serial_driver)))
DBGERR(("tty_unregister_driver error=%d\n", rc));
put_tty_driver(serial_driver);
@@ -3506,23 +3511,10 @@ static int __init slgt_init(void)
printk("%s %s\n", driver_name, driver_version);
- slgt_device_count = 0;
- if ((rc = pci_register_driver(&pci_driver)) < 0) {
- printk("%s pci_register_driver error=%d\n", driver_name, rc);
- return rc;
- }
- pci_registered = 1;
-
- if (!slgt_device_list) {
- printk("%s no devices found\n",driver_name);
- pci_unregister_driver(&pci_driver);
- return -ENODEV;
- }
-
serial_driver = alloc_tty_driver(MAX_DEVICES);
if (!serial_driver) {
- rc = -ENOMEM;
- goto error;
+ printk("%s can't allocate tty driver\n", driver_name);
+ return -ENOMEM;
}
/* Initialize the tty_driver structure */
@@ -3539,7 +3531,7 @@ static int __init slgt_init(void)
B9600 | CS8 | CREAD | HUPCL | CLOCAL;
serial_driver->init_termios.c_ispeed = 9600;
serial_driver->init_termios.c_ospeed = 9600;
- serial_driver->flags = TTY_DRIVER_REAL_RAW;
+ serial_driver->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
tty_set_operations(serial_driver, &ops);
if ((rc = tty_register_driver(serial_driver)) < 0) {
DBGERR(("%s can't register serial driver\n", driver_name));
@@ -3552,6 +3544,16 @@ static int __init slgt_init(void)
driver_name, driver_version,
serial_driver->major);
+ slgt_device_count = 0;
+ if ((rc = pci_register_driver(&pci_driver)) < 0) {
+ printk("%s pci_register_driver error=%d\n", driver_name, rc);
+ goto error;
+ }
+ pci_registered = 1;
+
+ if (!slgt_device_list)
+ printk("%s no devices found\n",driver_name);
+
return 0;
error:
-
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: compat_ioctl question
Arnd Bergmann wrote: So you are interested in the MGSL_* set of ioctls, right? AFAICS, they are all compatible, with the exception of MGSL_IOCGPARAMS and MGSL_IOCSPARAMS. Fortunately, these two have different ioctl numbers on 64 bit, so you can define a new #define MGSL_IOCSPARAMS32 _IOR(MGSL_MAGIC_IOC,0,struct _MGSL_PARAMS32) #define MGSL_IOCGPARAMS32 _IOR(MGSL_MAGIC_IOC,1,struct _MGSL_PARAMS32) and handle both versions in the ioctl function. I missed that approach, thanks. Yes, that would be the right solution. I've started this some time ago, but never finished it: http://www.uwsg.iu.edu/hypermail/linux/kernel/0511.0/1732.html Currently the tty file ops do not include that and tty_io.c does not register a compat_ioctl(), instead relying on compat_ioctl.h and compat_ioctl.c Just adding the hook in tty_io.c should be trivial, please do that. If you like, you can also move the vt ioctls in order to reduce the size of fs/compat_ioctl.c. I'll look at that. You have given me precisely the information I need. I just wanted to be sure I did not pursue a dead end and have people go 'e... why did you do it that way?' 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: compat_ioctl question
Arnd Bergmann wrote: It depends a lot on what your specific driver does in the ioctl handler, but normally you should define a compat_ioctl() function. What driver are you talking about? drivers/char/synclink.c drivers/char/synclinkmp.c drivers/char/synclink_gt.c drivers/char/pcmcia/synclink_cs.c All use the same set of ioctl() codes that are peculiar to the synclink drivers. Defining compat_ioctl() seems to be the best way, but that will require modifying the base tty code to allow the individual tty drivers to register compat_ioctl(). Currently the tty file ops do not include that and tty_io.c does not register a compat_ioctl(), instead relying on compat_ioctl.h and compat_ioctl.c -- 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/
compat_ioctl question
I need to add ioctl translations for my driver to allow 32 bit access on 64 bit systems. After digging through the kernel code there seems to be 3 methods of doing this: 1. define compat_ioctl() file operation for device and implement translation code in individual driver 2. add COMPATIBLE_IOCTL entry to include/linux/compat_ioctl.h to mark an ioctl code as the same in any environment 3. add HANDLE_IOCTL entry to fs/compat_ioctl.c with translation code implemented in the same file There is no way to implement #1 for a tty driver without modifying the kernel tty code to allow registration of a compat_ioctl() handler. #3 would put a lot of driver specific stuff in a common kernel file. This method also seems to break if there is an ioctl code collision. All of these methods involve changes to code outside of my driver. -- Before I spend a lot of time on this I need to know what the officially sanctioned method is. I haven't found any definitive documentation and a review of mailing list archives does not suggest a prevailing opinion. Does anyone have pointers on which way would be most likely to be accepted as a patch? 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: question on tty open and close
Alan Cox wrote: It breaks if any existing driver is doing no cleanup in ->open() when it fails but relying upon ->close() being called. That is what needs auditing first of all. Yes, I did not think of 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: question on tty open and close
Alan Cox wrote: I'm also not aware of any reason other than history, which means if someone cares to double check the other drivers there really shouldn't be an obstacle to "fixing" this behaviour. Unless anyone knows different ? As long as the new behavior continues to call driver->close() if driver->open() succeeds then I see no problem. If individual drivers are correctly doing internal reference counting to handle driver->close() calls without a preceding successful driver->open() call (usually just doing nothing), then they should continue to operate correctly without the extra and unnecessary driver->close(). -- 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 related oops
On Thu, Feb 22, 2007 at 03:02:46PM +, Jose Goncalves wrote: What I find real hard to understand is why a hardware fault happens always in the same software instruction! I would expect a hardware fault to hit randomly... I've experienced just such a hardware fault. The Infineon DSCC4 serial controller has a hardware bug in the PCI request/grant handling that can lead to the device driving the PCI bus in conflict with another device. While the results were random (as the oops in this problem seem to be), the trigger was always activating certain devices in combination. In your case, altering the timing/behavior of the serial device during open may be provoking the hardware fault. -- 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: how to limit flip buffer size in tty driver?
Arjan van de Ven wrote: On Tue, 2007-02-20 at 15:50 +0300, Mockern wrote: Thank you Alan for your respond, Could you help me with a problem which I have with my tty driver, please? I suspect Alan would be able to help you a whole lot better if you actually included the full source code of your driver... Not to mention responding to the previous advice of others (including myself and Theodore) who stated there is no magic function in a driver that directly supports the user mode cat program, and that he should look at the tty settings controllable by stty. (in particular clocal) I also suggested running cat on a standard serial port and getting that working first. Then applying those stty settings to his custom device/driver. (Background: he says other user mode programs work to read/write but that cat < /dev/foocustom does nothing.) -- 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 and tty driver
Mockern wrote: I have a question, what is really difference between serial and tty drivers? As I understand tty is high level and communicates with user space. The serial core implements many of the details of a tty driver in a common place so that individual hardware drivers (serial drivers) only need implement the hardware specific code. This prevents duplicating tty logic in many drivers, with the possibility of mistakes/inconsistency in the different tty drivers. The stand alone tty drivers are mostly legacy code from the time before serial core that have not been ported to be a serial drivers. -- 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: line discipline and tty driver
Mockern wrote: > When it's necessary to use line discipline in tty driver > (to call ldisc.write(), ldisc.read(), ldisc.receive_buf()) > and when I can ignore line discipline in my tty driver? You should not call any of these from your driver. transmit data is sent first to the line discipline ldisc.write() which then calls into your driver write() method your driver passes receive data to the ldisc through the tty flip buffer functions. -- 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: [BUG]: 2.6.19.2: Weird serial core issue
Aubrey Li wrote: But When I type "ENTER" key, I got cflag = 0x1cb1, old = 0x80001cb1 cflag = 0x1cb1, old = 0x1cb1 That means terminal setting is back to crtscts disabled. But here I didn't do anything to disable it, I just type "ENTER", crtscts setting should keep enabled. What makes it back to disabled? Now I guess there must be something wrong with serial core. Is there an mgetty or similar program monitoring the same serial port? -- 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: Strange problem with tty layer
Lennart Sorensen wrote: I am not sure actually. I just open /dev/ttyn0 and /dev/ttyn1 and write to one, and read from the other. I didn't even know about the line diciplines actually. How do I tell which one I am using? ioctl(TIOCSETD/TIOCGETD) sets/returns an integer identifier that can be compared agains the N_XXX macros. If you are not explicitly setting this then is is probably the default N_TTY. Also at the application level, look at tcsetattr() for setting the termios features. Look specifically at the c_cc[VTIME] and c_cc[VMIN] members of the termios structure. These settings control how much data must be available before returning data to a read(). Try VTIME=0 and VMIN=1. Since your 'missing' data is always on the tail end, maybe VMIN is set to 64 or something. -- 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: Strange problem with tty layer
Lennart Sorensen wrote: Well it turns out that didn't help. Neither does 2.6.18 (that one was the easiest newer one to try). It does seem as if the error rate is lower with 2.6.18 than with 2.6.16, so perhaps there was more than one place that could cause losses in the tty buffering. I had only 2 failures in 15 hours with 2.6.18, rather than a whole lot of failures with 2.6.16. I guess I will have to try 2.6.19 or even something newer. You can eliminate the tty buffering altogether by observing what gets passed to the line discipline. I assume you are using the default line discipline N_TTY. Look at what is passed to drivers/char/n_tty.c:n_tty_receive_buf() If all the data gets that far, then there is some issue with the line discipline or something further downstream. If not, then the problem is with the tty buffering (assuming you are correct that all data gets to the tty buffering code followed by a tty_flip_buffer_push call). -- 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: Strange problem with tty layer
Lennart Sorensen wrote: I have confirmed that the driver does in fact receive all the characters, and that they are correct, and that they are being passed to the tty layer using tty_insert_flip_string, and that it returns that all the characters have been passed to the tty layer. The user space application however still doesn't see the last few characters (when it fails). The problem seems to occour every few hours of testing on a 266MHz Geode SC1200. When I change the clock to 133MHz, it happens every few minutes instead (so much more frequently). I suspect there is some race condition that allows the tty layer to not get around to processing all the data in the buffer, even when asked for data by the application (which is waiting on the serial port using select, with a 4s timeout). In 2.6.16 the tty buffering pushes data to the line discipline without regard to tty->receive_room. If the line discipline can't keep up, the data gets dropped. I observed this data loss at higher speeds when placing the system under heavy load. 2.6.18 added code to respect tty->receive_room. This may or may not be your problem, but you should be able to check by adding a conditional printk to drivers/char/tty_io.c:flush_to_ldisc() If tty->receive_room is less than the size of the buffer passed to disc->receive_buf() then you are losing data. -- 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: tty->low_latency + irq context
On Tue, 2007-01-02 at 11:17 -0600, Hollis Blanchard wrote: > On Tue, 2006-12-26 at 01:08 +0059, Jiri Slaby wrote: > > * Queue a push of the terminal flip buffers to the line discipline. > > This > > * function must not be called from IRQ context if tty->low_latency is > > set. > > > > But some drivers (mxser, nozomi, hvsi...) sets low_latency to 1 in _open and > > calls tty_flip_buffer_push in isr or in functions, which are called from > > isr. > > Is the comment correct or the drivers? > > The comment would be true if tty_flip_buffer_push() attempted to block > with tty->low_latency set, but it doesn't AFAICS. One possibility for > deadlock is if the tty->buf.lock spinlock is taken on behalf of a user > process... There is no deadlock on tty->buf.lock, which is always acquired with spin_lock_irqsave() and is only used by the tty buffering code. The only deadlock I know of with the current tty buffering code is calling tty_flip_buffer_push() with low_latency set and from the ISR of a driver that has a problem with the line discipline calling back into the driver. The standard serial core has (or at least had the last time I looked) this problem with the N_TTY ldisc: driver gets internal spinlock in ISR driver calls tty_flip_buffer_push with low_latency = 1 flush_to_ldisc() immediately passes data to line discipline line discipline calls back into driver driver tries again to get internal spinlock With low_latency == 1, flush_to_ldisc() is deferred until the ISR is complete and the internal spinlock is released. I forget the exact driver callback that caused this. -- 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: tty->low_latency + irq context
Paul Fulghum wrote: With low_latency == 1, flush_to_ldisc() is deferred until the ISR is complete and the internal spinlock is released. Oops, I meant low_latency == 0 of course. -- 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] generic HDLC synclink config mismatch fix
Fix compile errors on mismatch between generic HDLC
and synclink drivers.
Notes:
generic HDLC support for synclink drivers is *optional*
so you can't just use depend on in Kconfig
This solution is deemed the best after 7 months of review
and criticism by many developers including AKPM. Read the
threads on LKML before posting about this solution.
Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]>
--- a/drivers/char/synclink.c 2006-11-29 15:57:37.0 -0600
+++ b/drivers/char/synclink.c 2006-12-01 14:38:13.0 -0600
@@ -101,8 +101,10 @@
#include
#include
-#ifdef CONFIG_HDLC_MODULE
-#define CONFIG_HDLC 1
+#if defined(CONFIG_HDLC) || (defined(CONFIG_HDLC_MODULE) &&
defined(CONFIG_SYNCLINK_MODULE))
+#define SYNCLINK_GENERIC_HDLC 1
+#else
+#define SYNCLINK_GENERIC_HDLC 0
#endif
#define GET_USER(error,value,addr) error = get_user(value,addr)
@@ -320,7 +322,7 @@ struct mgsl_struct {
int dosyncppp;
spinlock_t netlock;
-#ifdef CONFIG_HDLC
+#if SYNCLINK_GENERIC_HDLC
struct net_device *netdev;
#endif
};
@@ -728,7 +730,7 @@ static void usc_loopmode_send_done( stru
static int mgsl_ioctl_common(struct mgsl_struct *info, unsigned int cmd,
unsigned long arg);
-#ifdef CONFIG_HDLC
+#if SYNCLINK_GENERIC_HDLC
#define dev_to_port(D) (dev_to_hdlc(D)->priv)
static void hdlcdev_tx_done(struct mgsl_struct *info);
static void hdlcdev_rx(struct mgsl_struct *info, char *buf, int size);
@@ -1276,7 +1278,7 @@ static void mgsl_isr_transmit_status( st
info->drop_rts_on_tx_done = 0;
}
-#ifdef CONFIG_HDLC
+#if SYNCLINK_GENERIC_HDLC
if (info->netcount)
hdlcdev_tx_done(info);
else
@@ -1341,7 +1343,7 @@ static void mgsl_isr_io_pin( struct mgsl
info->input_signal_events.dcd_up++;
} else
info->input_signal_events.dcd_down++;
-#ifdef CONFIG_HDLC
+#if SYNCLINK_GENERIC_HDLC
if (info->netcount) {
if (status & MISCSTATUS_DCD)
netif_carrier_on(info->netdev);
@@ -4312,7 +4314,7 @@ static void mgsl_add_device( struct mgsl
info->max_frame_size );
}
-#ifdef CONFIG_HDLC
+#if SYNCLINK_GENERIC_HDLC
hdlcdev_init(info);
#endif
@@ -4470,7 +4472,7 @@ static void synclink_cleanup(void)
info = mgsl_device_list;
while(info) {
-#ifdef CONFIG_HDLC
+#if SYNCLINK_GENERIC_HDLC
hdlcdev_exit(info);
#endif
mgsl_release_resources(info);
@@ -6644,7 +6646,7 @@ static int mgsl_get_rx_frame(struct mgsl
return_frame = 1;
}
framesize = 0;
-#ifdef CONFIG_HDLC
+#if SYNCLINK_GENERIC_HDLC
{
struct net_device_stats *stats =
hdlc_stats(info->netdev);
stats->rx_errors++;
@@ -6720,7 +6722,7 @@ static int mgsl_get_rx_frame(struct mgsl
*ptmp);
}
-#ifdef CONFIG_HDLC
+#if SYNCLINK_GENERIC_HDLC
if (info->netcount)
hdlcdev_rx(info,info->intermediate_rxbuffer,framesize);
else
@@ -7624,7 +7626,7 @@ static void mgsl_tx_timeout(unsigned lon
spin_unlock_irqrestore(&info->irq_spinlock,flags);
-#ifdef CONFIG_HDLC
+#if SYNCLINK_GENERIC_HDLC
if (info->netcount)
hdlcdev_tx_done(info);
else
@@ -7700,7 +7702,7 @@ static int usc_loopmode_active( struct m
return usc_InReg( info, CCSR ) & BIT7 ? 1 : 0 ;
}
-#ifdef CONFIG_HDLC
+#if SYNCLINK_GENERIC_HDLC
/**
* called by generic HDLC layer when protocol selected (PPP, frame relay, etc.)
--- a/drivers/char/pcmcia/synclink_cs.c 2006-11-29 15:57:37.0 -0600
+++ b/drivers/char/pcmcia/synclink_cs.c 2006-12-01 14:38:25.0 -0600
@@ -75,8 +75,10 @@
#include
#include
-#ifdef CONFIG_HDLC_MODULE
-#define CONFIG_HDLC 1
+#if defined(CONFIG_HDLC) || (defined(CONFIG_HDLC_MODULE) &&
defined(CONFIG_SYNCLINK_CS_MODULE))
+#define SYNCLINK_GENERIC_HDLC 1
+#else
+#define SYNCLINK_GENERIC_HDLC 0
#endif
#define GET_USER(error,value,addr) error = get_user(value,addr)
@@ -235,7 +237,7 @@ typedef struct _mgslpc_info {
int dosyncppp;
spinlock_t netlock;
-#ifdef CONFIG_HDLC
+#if SYNCLINK_GENERIC_HDLC
struct net_device *netdev;
#endif
@@ -392,7 +394,7 @@ static void tx_timeout(unsigned long con
static int ioctl_common(MGSLPC_INFO *info, unsigned int cmd, unsigned long
arg);
-#ifdef CONFIG_HDLC
+#if SYNCLINK_GENERIC_HDLC
#define dev_to_port(D) (dev_to_hdlc(D)->priv)
static void hdlcdev_tx_done(MGSLPC_INFO *info);
static void hdlcdev_rx(MGSLPC_INFO *info, char *buf, int size);
@@ -1060,7 +1062,7 @@
