lkml.org 
[lkml]   [2004]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH][RFC] 2.6.6 tty_io.c hangup locking
From
Date
On Fri, 2004-05-28 at 18:06, Andrew Morton wrote:
> Obviously the current locking is pointless...

Agreed

> ... and the only useful locking we have in there is lock_kernel().

For calling write_wakeup, even that is useless.

After more examination, I see write_wakeup can
be called in any context (even hard interrupt) with no
locking (external to write_wakeup) at all.

write_wakeup *must* provide all required locking internally,
because none is provided externally (in a consistent way
by all callers). write_wakeup implementations requiring
external locking are broken.
Planned or not, this is the current situation.

I'm still poking around the flush_buffer calls
but the same appears to apply here as well.


> The reason why a patch such as yours wasn't applied is that it was all kept
> as a reminder that we suck. Someone needs to get down and audit what's
> actually happening in there. It seems that you've now done that via
> comparison with other callers, but that is not necessarily a good approach
> when it comes to the tty layer ;)

My analysis of callers of these functions was to show that
disabling interrupts to make these calls in do_tty_hangup()
is useless, because not all callers follow this convention.
It is not an indication of the correctness
of individual tty drivers or line disciplines.

Removing the interrupt disabling won't make
individual tty drivers or line disciplines any more or
less broken. (but may expose existing problems more readily)


> We need to itemise all the affected memory storage in all impementations of
> ->flush_buffer() and ->write_wakeup() and then make sure that all _other_
> users of those fields (whether or not they lie in the ->flush_buffer() and
> ->write_wakeup() codepaths) are using the same locking. Is that something
> you could do?

I see what you are getting at, but what question are we
trying to answer? We already know flush_buffer/write_wakeup
implementations requiring external locking
are broken as no such locking currently exists, and
interrupt disabling in do_tty_hangup does not fix this.
If external locking is not required, then interrupt
disabling in do_tty_hangup serves no purpose.

Apart from that question, auditing the correctness of internal
details of the 17 line disciplines and 64 tty drivers would be
nice but would take a very long time for one person :-)
It is probably better that:
* Broken, unmaintained drivers remain broken and are eventually culled.
* Broken, maintained drivers are identified and fixed by the maintainer.

I understand that without such an audit you may want
to let interrupt disabling in do_tty_hangup() stand in
case it is helping broken drivers or line disciplines
to limp along in some cases.


As for the softirq.c warning, this is not a problem.
Currently ppp_synctty does not allow for the fact that
write_wakeup can be called in hard interrupt context.
A patch from Paul Mackerras has already been accepted
to correct the same problem for ppp_async.c and I will
adapt his modifications to ppp_synctty.c
These modifications have the side effect of making
the interrupt enable/disable state of the write_wakeup
caller irrelevant.

--
Paul Fulghum
paulkf@microgate.com



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:03    [W:0.098 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site