question on tty open and close

question on tty open and close

[Oliver Neukum]

Hi,

in tty_io.c::tty_open():

	if (!retval) {
		if (tty->driver->open)
			retval = tty->driver->open(tty, filp);
		else
			retval = -ENODEV;
	}
	filp->f_flags = saved_flags;

	if (!retval && test_bit(TTY_EXCLUSIVE, &tty->flags) && !capable(CAP_SYS_ADMIN))
		retval = -EBUSY;

	if (retval) {
#ifdef TTY_DEBUG_HANGUP
		printk(KERN_DEBUG "error %d in opening %s...", retval,
		       tty->name);
#endif
		release_dev(filp);

We find that a failure in open() leads to release_dev() being called.
release_dev() calls close():

	if (tty->driver->close)
		tty->driver->close(tty, filp);

So we have a file that's closed although open() never succeeded?
What's the reason for that?

	Regards
		Oliver

RE: question on tty open and close

[Stuart MacDonald]

From: linux-kernel-owner@vger.kernel.org 
> in tty_io.c::tty_open():
[snip]
> We find that a failure in open() leads to release_dev() being called.
> release_dev() calls close():
> 
> 	if (tty->driver->close)
> 		tty->driver->close(tty, filp);
> 
> So we have a file that's closed although open() never succeeded?

That's correct! It's been a pain in my butt for years.

> What's the reason for that?

No idea. Would *love* to hear an explanation.

..Stu

Re: question on tty open and close

[Oliver Neukum]

Am Mittwoch, 28. März 2007 15:10 schrieb Stuart MacDonald:
> > We find that a failure in open() leads to release_dev() being called.
> > release_dev() calls close():
> > 
> >       if (tty->driver->close)
> >               tty->driver->close(tty, filp);
> > 
> > So we have a file that's closed although open() never succeeded?
> 
> That's correct! It's been a pain in my butt for years.

How did you deal with that proctological issue?

	Regards
		Oliver

RE: question on tty open and close

[Stuart MacDonald]

From: Oliver Neukum [mailto:oliver@neukum.org] 
> Am Mittwoch, 28. März 2007 15:10 schrieb Stuart MacDonald:
> > > We find that a failure in open() leads to release_dev() 
> being called.
> > > release_dev() calls close():
> > > 
> > >       if (tty->driver->close)
> > >               tty->driver->close(tty, filp);
> > > 
> > > So we have a file that's closed although open() never succeeded?
> > 
> > That's correct! It's been a pain in my butt for years.
> 
> How did you deal with that proctological issue?

Just make sure the close() handles the situation properly. It makes
reference counting...  fun.

The serial driver has always handled it like this.

..Stu

Re: question on tty open and close

[Alan Cox]

> > > > So we have a file that's closed although open() never succeeded?
> > > 
> > > That's correct! It's been a pain in my butt for years.
> > 
> > How did you deal with that proctological issue?
> 
> Just make sure the close() handles the situation properly. It makes
> reference counting...  fun.
> 
> The serial driver has always handled it like this.

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 ?

Alan

Re: question on tty open and close

[Paul Fulghum]

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.

Re: question on tty open and close

[Alan Cox]

> As long as the new behavior continues to call
> driver->close() if driver->open() succeeds
> then I see no problem.

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.

Re: question on tty open and close

[Marcel Holtmann]

Hi Alan,

> > As long as the new behavior continues to call
> > driver->close() if driver->open() succeeds
> > then I see no problem.
> 
> 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.

I know at least that the Bluetooth TTY emulation (RFCOMM) will break
since I have a big fat warning in my code:

        /* We don't leak this refcount. For reasons which are not entirely
           clear, the TTY layer will call our ->close() method even if the
           open fails. We decrease the refcount there, and decreasing it
           here too would cause breakage. */
        dev = rfcomm_dev_get(id);
        if (!dev)
                return -ENODEV;

Regards

Marcel

Re: question on tty open and close

[Paul Fulghum]

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.

Re: question on tty open and close

[Kilau, Scott]

> > > > > So we have a file that's closed although open() never
succeeded?
> > > > 
> > > > That's correct! It's been a pain in my butt for years.
> > > 
> > > How did you deal with that proctological issue?
> > 
> > Just make sure the close() handles the situation properly. It makes
> > reference counting...  fun.
> > 
> > The serial driver has always handled it like this.
> 
> 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 ?

Hi Alan!

>From our (Digi's) drivers history, I believe you tried "fixing" it back
in the Red Hat 7.1 days. =)

Sadly, our drivers still carry this legacy fix around.

    o Red Hat 7.1 -- Kernel Compatibility Issues
         Some 2.4 kernel-based distributions (Red Hat 7.1 included) have
a patch
         applied to them which modifies the behavior of Linux when an
open of
         a serial port is canceled (for instance, if an application is
waiting
         for the carrier signal and a user hits CTRL-C to kill the
application)
        
         With this behavior change, the device driver is unable to
cleanup its
         internal data structures and the sane functioning of the driver
is
         compromised.  The classic symptom of this problem is that the
command
         "lsmod", which (among other things) will return a count of the
         applications using the device driver, will return a non-zero
value
         even if all applications associated with the serial ports
         are killed.   
            
         Unfortunately, it is impossible (from within the device driver)
to
         determine which behavior is implemented in the running kernel.
         However, Digi now provides a workaround to allow customers with
this
         problem to change the Digi behavior to be compatible with these
         "patched" kernels.


I know backwards compatibility in the kernel isn't a concern at all,
which has been made abundantly clear, (ie, stable_api_nonsense.txt),
but this will impact *all* out of tree serial drivers.

Just wanted to toss this out there.
Scott Kilau