On Sat, 22 Aug 2009, Alan Cox wrote:
> > What about protecting the use counter? In tty_port.c it's always
> > protected by port->lock, but not in serial_open(). Is that a mistake?
>
> Ah good an easy question to begin with
>
> Yes it is in error.
Okay, I'll fix it.
> The core of both hangup and open are still BKL protected against
> one another (ugly - wants fixing), release_one_dev() liekwise. This is
> probably inadequate as they may well sleep in various spots
Would you consider ideas for changing the protection to something else?
I don't have anything in mind at the moment -- I need to study the code
some more to understand it better. But eventually a possibility may
suggest itself.
> > > clean up resources
> > > if (last && test_clear INITIALIZED)
> >
> > How do you check for "last"? Doesn't the fact that we are here mean
> > that there are no remaining open references?
>
> It means there are no remaining file references to the handle, but you
> may have multiple file handles referencing the same tty
So basically this amounts to testing whether port->count == 0? But
isn't that already implicit when tty_port_close_start() returns 0?
It sounds like this is another little thingy needed only by drivers
that don't use the new helpers.
> > P.S.: Consider this code in tty_port_block_til_ready():
> >
> > /* if non-blocking mode is set we can pass directly to open unless
> > the port has just hung up or is in another error state */
> > if ((filp->f_flags & O_NONBLOCK) ||
> > (tty->flags & (1 << TTY_IO_ERROR))) {
> > port->flags |= ASYNC_NORMAL_ACTIVE;
> > return 0;
> > }
> >
> > The comment doesn't agree with the logic of the test. Which is wrong?
>
> The code and comment were copied from the original drivers (and occur in
> several places ;))
>
> The intended logic is
>
> if O_NONBLOCK is set
> succeed immediately
> if there is a hangup (or other pending error)
> succeed immediately
Got it -- the comment is wrong. It should say something like:
/* If non-blocking mode is set or the port is in an error state
* then we can return directly; tty_open() will handle everything.
*/
Thanks,
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
