Hi,
I have been working on various serial drivers and I notice that physical
driver close routine is called in all cases even if the physical driver
open routine fails. That suggests to me that a lot of the MOD_DEC/INC_COUNT
logic in serial.c and other physical serial drivers is incorrect. As
serial.c seems usually to be compiled into the kernel the issue
is not so important, but a lot of the other logic associated with
open counts also seems incorrect. Is this observation correct?
Joachim Martillo
> I have been working on various serial drivers and I notice that physical
> driver close routine is called in all cases even if the physical driver
> open routine fails. That suggests to me that a lot of the MOD_DEC/INC_COUNT
> logic in serial.c and other physical serial drivers is incorrect. As
> serial.c seems usually to be compiled into the kernel the issue
> is not so important, but a lot of the other logic associated with
> open counts also seems incorrect. Is this observation correct?
Possibly so. But everyone who sent me a 2.2 patch to redo it broke stuff and
caused crashes and panics. Its worth doing for 2.5.x tho - along with proper
refcounting and killing the BKL
On Tue, Dec 18, 2001 at 07:58:46AM -0500, [email protected] wrote:
> I have been working on various serial drivers and I notice that physical
> driver close routine is called in all cases even if the physical driver
> open routine fails. That suggests to me that a lot of the MOD_DEC/INC_COUNT
> logic in serial.c and other physical serial drivers is incorrect. As
> serial.c seems usually to be compiled into the kernel the issue
> is not so important, but a lot of the other logic associated with
> open counts also seems incorrect. Is this observation correct?
Absolutely 100% correct.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Tue, Dec 18, 2001 at 02:16:35PM +0000, Alan Cox wrote:
> Possibly so. But everyone who sent me a 2.2 patch to redo it broke stuff and
> caused crashes and panics. Its worth doing for 2.5.x tho - along with proper
> refcounting and killing the BKL
The module use accounting for the tty layer are *disgusting* beyond
belief, caused by the fact that if an open fails, the close method
is called. Let me paste the following code from the replacement
serial drivers open method (note that the old driver only has to
worry about itself):
/*
* tty->driver.num won't change, so we won't fail here with
* tty->driver_data set to something non-NULL (and therefore
* we won't get caught by uart_close()).
*/
retval = -ENODEV;
if (line >= tty->driver.num)
goto fail;
/*
* If we fail to increment the module use count, we can't have
* any other users of this tty (since this implies that the module
* is about to be unloaded). Therefore, it is safe to set
* tty->driver_data to be NULL, so uart_close() doesn't bite us.
*/
if (!try_inc_mod_count(drv->owner)) {
tty->driver_data = NULL;
goto fail;
}
/*
* FIXME: This one isn't fun. We can't guarantee that the tty isn't
* already in open, nor can we guarantee the state of tty->driver_data
*/
info = uart_get(drv, line);
retval = -ENOMEM;
if (!info) {
if (tty->driver_data)
goto out;
else
goto fail;
}
I don't like the second entry because it the potential to cause a null
pointer dereference on a SMP machine.
I definitely don't like the third either.
However, to add to the dilemas, what if tty->driver_data has been set,
the module unloaded, reloaded (with a different port configuration - ie,
smaller tty->driver.num). tty->driver_data may not be NULL.
rs_close/uart_close use tty->driver_data to indicate whether the port has
been opened, and whether we need to therefore decrement the count. It's
broken, but I regard the tty layer as broken for calling the driver close
method when the open method has failed.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html