On Wed, Sep 25, 2013 at 6:29 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> 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
Alrighty, thanks. :)
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
