Re: patch "TTY: remove tty_locked" added to tty tree

Re: patch "TTY: remove tty_locked" added to tty tree

[Arnd Bergmann]

On Wednesday 24 August 2011, Jiri Slaby wrote:
> On 08/24/2011 01:20 PM, Arnd Bergmann wrote:
> > It's not clear to me what state->mutex protects in the serial_core, but
> > it has been around forever (used to be called state->sem)
> 
> It was actually moved in uart_close back in 2003. Formerly (when there
> was only a coarse grained port_sem) it was right before uart_shutdown.
> But there were some flags to handle some races. I'm not sure whether the
> flags protected any race here though.

ok

> > and is held in
> > all uart functions, which is at least consistent. IIRC what Alan's plan
> > for this was, uart_close should eventually get changed to use
> > tty_port_close_start or even tty_port_close. Maybe the time for that has
> > come now, lacking better alternatives?
> 
> Yes, I have such a patch in my queue. But it's not easy to get there.
> You may take a look at:
> http://decibel.fi.muni.cz/gitweb/?p=linux.git;a=shortlog;h=refs/heads/devel
> 
> But it's still far from ready. And yet, in the queue, I still have
> port->mutex locked before tty_port_close_start like it is now.

Ah, right. I still don't see why the port->mutex is or is not needed there,
and I think that's the main issue.

By comparison, getting *_wait_until_sent to be called without BTM seems
easy -- we know that all callers from ->close() hold it, while the ones
from ->ioctl() don't. We could have a helper like

void tty_wait_until_sent_from_close(struct tty_struct *tty, long timeout)
{
	tty_unlock(); /* tty->ops->close holds the BTM, drop it while waiting */
	tty_wait_until_sent(tty, timeout);
	tty_lock();
}

to deal with that, if only we can sort the lock ordering with port->mutex.

BTW, I saw that the three m68k serial port drivers (amiserial, 68328, 68360)
all call *_wait_until_sent with interrupts disabled, which is even more
broken.

	Arnd

Re: patch "TTY: remove tty_locked" added to tty tree

[Jiri Slaby]

On 08/24/2011 04:35 PM, Arnd Bergmann wrote:
> On Wednesday 24 August 2011, Jiri Slaby wrote:
>> On 08/24/2011 01:20 PM, Arnd Bergmann wrote:
>>> It's not clear to me what state->mutex protects in the serial_core, but
>>> it has been around forever (used to be called state->sem)
>>
>> It was actually moved in uart_close back in 2003. Formerly (when there
>> was only a coarse grained port_sem) it was right before uart_shutdown.
>> But there were some flags to handle some races. I'm not sure whether the
>> flags protected any race here though.
> 
> ok
> 
>>> and is held in
>>> all uart functions, which is at least consistent. IIRC what Alan's plan
>>> for this was, uart_close should eventually get changed to use
>>> tty_port_close_start or even tty_port_close. Maybe the time for that has
>>> come now, lacking better alternatives?
>>
>> Yes, I have such a patch in my queue. But it's not easy to get there.
>> You may take a look at:
>> http://decibel.fi.muni.cz/gitweb/?p=linux.git;a=shortlog;h=refs/heads/devel
>>
>> But it's still far from ready. And yet, in the queue, I still have
>> port->mutex locked before tty_port_close_start like it is now.
> 
> Ah, right. I still don't see why the port->mutex is or is not needed there,
> and I think that's the main issue.
> 
> By comparison, getting *_wait_until_sent to be called without BTM seems
> easy -- we know that all callers from ->close() hold it, while the ones
> from ->ioctl() don't. We could have a helper like
> 
> void tty_wait_until_sent_from_close(struct tty_struct *tty, long timeout)
> {
> 	tty_unlock(); /* tty->ops->close holds the BTM, drop it while waiting */
> 	tty_wait_until_sent(tty, timeout);
> 	tty_lock();
> }
> 
> to deal with that, if only we can sort the lock ordering with .

Ah, it looks like I just got the reason why port->mutex is locked in the
top of uart_close. In uart, TTY_CLOSING flag is not used. So there is
nothing to protect against races between ->close (the code between the
two spinlock critical sections corresponding to port_close_start and
_end) and ->open (block_til_ready).

Other than that I see no point for the lock to be in the beginning. So
if we introduce CLOSING flag (I do that in my patches implicitly),
everything should be OK:
* port->count etc is and always was protected by the spinlock,
* ->stop_rx stands as I wrote earlier.
* uart_wait_until_sent -- that one is already called without port->mutex
from set_termios and tty_set_ldisc.

So it looks like we should:
- introduce CLOSING flag
- move the lock below, before shutdown
- introduce your magic _from_close helper
- use it

Doing this after we have all the helpers in place would be easier. There
would be no need to play with CLOSING bit. But there will be no option
to backport this to stable trees then. And I know I will have to do that
at least for 3.0.

Note that we may use the _from_close helper from tty_port_close_start
almost instantly. All users should not hold port->mutex over
tty_port_close_start. But I need to check. Tomorrow.

In the meantime, comments welcome.

> BTW, I saw that the three m68k serial port drivers (amiserial, 68328, 68360)
> all call *_wait_until_sent with interrupts disabled, which is even more
> broken.

Blah, yes.

-- 
js
suse labs

Re: patch "TTY: remove tty_locked" added to tty tree

[Greg KH]

On Wed, Aug 24, 2011 at 11:27:19PM +0200, Jiri Slaby wrote:
> On 08/24/2011 04:35 PM, Arnd Bergmann wrote:
> > On Wednesday 24 August 2011, Jiri Slaby wrote:
> >> On 08/24/2011 01:20 PM, Arnd Bergmann wrote:
> >>> It's not clear to me what state->mutex protects in the serial_core, but
> >>> it has been around forever (used to be called state->sem)
> >>
> >> It was actually moved in uart_close back in 2003. Formerly (when there
> >> was only a coarse grained port_sem) it was right before uart_shutdown.
> >> But there were some flags to handle some races. I'm not sure whether the
> >> flags protected any race here though.
> > 
> > ok
> > 
> >>> and is held in
> >>> all uart functions, which is at least consistent. IIRC what Alan's plan
> >>> for this was, uart_close should eventually get changed to use
> >>> tty_port_close_start or even tty_port_close. Maybe the time for that has
> >>> come now, lacking better alternatives?
> >>
> >> Yes, I have such a patch in my queue. But it's not easy to get there.
> >> You may take a look at:
> >> http://decibel.fi.muni.cz/gitweb/?p=linux.git;a=shortlog;h=refs/heads/devel
> >>
> >> But it's still far from ready. And yet, in the queue, I still have
> >> port->mutex locked before tty_port_close_start like it is now.
> > 
> > Ah, right. I still don't see why the port->mutex is or is not needed there,
> > and I think that's the main issue.
> > 
> > By comparison, getting *_wait_until_sent to be called without BTM seems
> > easy -- we know that all callers from ->close() hold it, while the ones
> > from ->ioctl() don't. We could have a helper like
> > 
> > void tty_wait_until_sent_from_close(struct tty_struct *tty, long timeout)
> > {
> > 	tty_unlock(); /* tty->ops->close holds the BTM, drop it while waiting */
> > 	tty_wait_until_sent(tty, timeout);
> > 	tty_lock();
> > }
> > 
> > to deal with that, if only we can sort the lock ordering with .
> 
> Ah, it looks like I just got the reason why port->mutex is locked in the
> top of uart_close. In uart, TTY_CLOSING flag is not used. So there is
> nothing to protect against races between ->close (the code between the
> two spinlock critical sections corresponding to port_close_start and
> _end) and ->open (block_til_ready).
> 
> Other than that I see no point for the lock to be in the beginning. So
> if we introduce CLOSING flag (I do that in my patches implicitly),
> everything should be OK:
> * port->count etc is and always was protected by the spinlock,
> * ->stop_rx stands as I wrote earlier.
> * uart_wait_until_sent -- that one is already called without port->mutex
> from set_termios and tty_set_ldisc.
> 
> So it looks like we should:
> - introduce CLOSING flag
> - move the lock below, before shutdown
> - introduce your magic _from_close helper
> - use it
> 
> Doing this after we have all the helpers in place would be easier. There
> would be no need to play with CLOSING bit. But there will be no option
> to backport this to stable trees then. And I know I will have to do that
> at least for 3.0.
> 
> Note that we may use the _from_close helper from tty_port_close_start
> almost instantly. All users should not hold port->mutex over
> tty_port_close_start. But I need to check. Tomorrow.
> 
> In the meantime, comments welcome.

So, is your original patch you sent in this thread still needed?

confused,

greg k-h

Re: patch "TTY: remove tty_locked" added to tty tree

[Jiri Slaby]

On 08/24/2011 11:42 PM, Greg KH wrote:
> So, is your original patch you sent in this thread still needed?

If you mean "TTY: remove tty_locked", then yes, it's fine to see
tty_locked go. It looks like we won't need it to fix the stalls in the
end. If you mean TTY-fix-stalls-on-BTM-when-waiting-until-sent, that one
will be superseded by a new one.

thanks,
-- 
js
suse labs

Re: patch "TTY: remove tty_locked" added to tty tree

[Greg KH]

On Wed, Aug 24, 2011 at 11:48:22PM +0200, Jiri Slaby wrote:
> On 08/24/2011 11:42 PM, Greg KH wrote:
> > So, is your original patch you sent in this thread still needed?
> 
> If you mean "TTY: remove tty_locked", then yes, it's fine to see
> tty_locked go.

Good, that's already in my tree :)

> It looks like we won't need it to fix the stalls in the end. If you
> mean TTY-fix-stalls-on-BTM-when-waiting-until-sent, that one will be
> superseded by a new one.

Ok, I'll wait for that one then, thanks.

greg k-h