Re: tty: closing n_gsm line discipline always times out

[Sascha Hauer]

On Wed, May 17, 2017 at 04:23:58PM +0100, Alan Cox wrote:
> On Wed, 17 May 2017 15:44:56 +0200
> Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote:
> 
> > Hi All,
> > 
> > When the n_gsm line discipline is closed it wants to shutdown the line
> > discipline properly and asks the link partner to end the mux protocol. This is
> > done in gsm_cleanup_mux() around line 2050 in n_gsm.c:
> > 
> > 	if (dlci) {
> > 		gc = gsm_control_send(gsm, CMD_CLD, NULL, 0);
> > 		if (gc)
> > 			gsm_control_wait(gsm, gc);
> > 	}
> > 
> > The call stack is like this:
> > 
> > (struct tty_ldisc_ops).close
> >   -> gsmld_close
> >     -> gsmld_detach_gsm
> >       -> gsm_cleanup_mux  
> > 
> > (struct tty_ldisc_ops).close is called at tty_release time or when the line
> > discipline is changed.  At tty_release time the tty is already shutdown, so we
> > cannot send anything anymore. In this case our link partner never closes the
> > mux protocol, which means we can never re-open it again.  When instead the line
> > discipline is changed back to N_TTY to close the protocol, gsm_control_send
> > works fine, but the tty_ldisc is locked and thus the chars received by the UART
> > never make it to the line discipline and gsm_control_wait times out.
> > 
> > There's obviously something wrong here. Any ideas how to fix that? When we are
> > not allowed to send/receive at tty_release time, do we need a new n_gsm specific
> > ioctl to shutdown the mux?
> 
> Probably - it used to work fine but other changes in the core tty code
> broke it, and I think because 99% of the users of that code are Android
> they've not yet caught up with this decade 8)
> 
> The other option would be to just remove the CLD sending and have the
> caller do whatever cleanup handling it wants in N_TTY ldisc. You can just
> write() the frame rather than having an ioctl.
> 
> That said there are power management cases where being able to turn it
> on/off without ldisc changing might be useful. However you've then got to
> deal with all the locking of active sessions against being in down state.
> 
> So I think just having the caller use a write() in N_TTY would be simpler
> since we'd have to change user space anyway.

This option doesn't sound very nice because then userspace needs to know
details of the protocol that it just wants to use. Also the cleanup
won't be done if the userspace process is killed for some reason.

I think I'll try and see where I end up with an ioctl. If the result is
not acceptable then we can still opt for your suggestion to write() the
close frame in userspace.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |