On Wed, Sep 25, 2013 at 06:13:47PM -0400, Lidza Louina wrote:
> On Wed, Sep 25, 2013 at 2:34 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > On Wed, Sep 25, 2013 at 01:22:08PM -0400, Lidza Louina wrote:
> >>
> >> I looked at other uses of the function alloc_tty_driver() in
> >> the kernel and none of them seem to follow up with a
> >> call to kfree().
> >
> > Read my first response again. I showed how to do this. Your setting
> > up a bunch of things in a line. If any of them fail you need to
> > cleanup by releasing any allocations.
> >
> > If you have an allocation from alloc_tty_driver() then you can't release
> > it with kfree() you need to use put_tty_driver().
>
> Alrighty.
>
> These are the examples I'd found in the kernel.
>
> Case 1: tty/synclink.c: mgsl_init_tty(): The serial_driver is
> allocated, it checks for an error and returns -ENOMEM:
>
> serial_driver = alloc_tty_driver(128);
> if (!serial_driver)
> return -ENOMEM;
>
It's not allocated, the allocation failed. It does call
put_tty_driver() if the tty_register_driver() call fails so this
function is correct.
> The code doesn't call put_tty_driver until synclink_cleanup() is
> called. In synclink, the put_tty_driver only gets called when
> serial_driver is not null:
>
> if (serial_driver) {
> if ((rc = tty_unregister_driver(serial_driver)))
> printk("%s(%d) failed to unregister tty driver err=%d\n",
> __FILE__,__LINE__,rc);
> put_tty_driver(serial_driver);
> }
>
> This is the case for most of the drivers I found, it returns -ENOMEM
> when the alloc fails, and calls put_tty_driver when something fails
> afterward (like when registering the device fails).
Yes. That's correct.
>
> Case 2: tty/rocket.c: rp_init(): rocket_driver is allocated using
> alloc_tty_driver, and we return ret:
> int ret = -ENOMEM, pci_boards_found, isa_boards_found, i;
>
> rocket_driver = alloc_tty_driver(MAX_RP_PORTS);
> if (!rocket_driver)
> goto err;
> .............(some code).............
> err:
> return ret;
>
> put_tty_driver() gets called when we can't find an IO region:
>
> if (controller && (!request_region(controller, 4, "Comtrol RocketPort"))) {
> printk(KERN_ERR "Unable to reserve IO region for first "
> "configured ISA RocketPort controller 0x%lx. "
> "Driver exiting\n", controller);
> ret = -EBUSY;
> goto err_tty;
> }
> .............(some code).............
> err_tty:
> put_tty_driver(rocket_driver);
>
> And after setting rocket_driver's flags, termios info, type, subtype,
> etc., it tries to register the driver:
>
> ret = tty_register_driver(rocket_driver);
> if (ret < 0) {
> printk(KERN_ERR "Couldn't install tty RocketPort driver\n");
> goto err_controller;
> }
> .............(some code).............
> err_controller:
> if (controller)
> release_region(controller, 4);
>
> I would think that err_controller would have a call to put_tty_driver.
> Also I'd think that err_tty would go with the failed register_driver()
> call and the err_controller would math the failed request_region. Bad
> names? >_<
This function is also fine. The names ok-ish.
err_tty puts the tty.
err_controller releases the controller region.
err_ttyu unregisters the tty.
The names are not great. "ttyu" is a too short and who would know that
the 'u' stands for "unregister?" It would be better to use
"err_unregister_tty:"
>
> Case 3: tty/serial/msm_smd_tty.c: smd_tty_init(): This doesn't have a
> matching put_tty_driver after alloc_tty_driver.
>
This one is buggy. If tty_register_driver() fails then it should call
put_tty_driver().
> Case 4: tty/vt/vt.c: vty_init(): This code allocates the driver, then
> calls a panic function:
>
> console_driver = alloc_tty_driver(MAX_NR_CONSOLES);
> if (!console_driver)
> panic("Couldn't allocate console driver\n");
>
> The code doesn't call put_tty_driver at any time, and I'm not sure
> what the panic function does. I grepped thru the tty drivers and
> couldn't find a declaration or definition for it.
>
panic() means the kernel dies. If you can't load the vt module then
there is no point in continueing and nothing you can do to recover.
vt is special and essential.
> There are more drivers I didn't look at, but I figured this would be
> enough for now.
>
> Out of the 18 drivers I checked:
> - Most of them returned -ENOMEM when allocating failed and most used
> put_tty_driver when registering, requesting a region, or using kthread
> failed (not all)
> - One called put_tty_driver when the module_exit function was being
> called: tty/hvc/hvc_console.c
The hvc_console.c driver is fine.
> - One had no put_tty_driver call after it was allocated
> - One had a panic function when it encountered an error and I don't
> know what panic() does, but it doesnt seem to call put_tty_driver
>
> I think I was just looking at the bad ones. >_< Do the ones I caught
> need fixing? :)
Only smd_tty_init(). The others are all ok.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
