On Tue, Jun 09, 2015 at 04:41:33PM +0300, Dan Carpenter wrote:
> On Tue, Jun 09, 2015 at 10:54:02AM +0000, Gujulan Elango, Hari Prasath (H.)
> wrote:
> > On Tue, Jun 09, 2015 at 03:32:20PM +0530, Sudip Mukherjee wrote:
> > > On Tue, Jun 09, 2015 at 09:27:22AM +0000, Gujulan Elango, Hari Prasath
> > > (H.) wrote:
> > > > From: Hari Prasath Gujulan Elango <hguju...@visteon.com>
> > > >
> > > > Cleanup the device entry,device class & unregister the character device
> > > > in the module exit.All this cleanup is done already in the dgap_stop()
> > > > function.We need to call this in the cleanup module.
> > > No. this is wrong. The same thing what dgap_stop() does is being done in
> > > dgap_remove_one() which is the remove callback. So your patch is trying
> > > to destroy and unregister something which does not exist at the time
> > > dgap_stop()
> > > will be called.
> > >
> > > regards
> > > sudip
> > > >
> >
> > sudip,thanks for your comments.while I agree that my patch does the
> > same thing twice,I see that a similar thing is being done in the
> > failure path of the dgap_init_module().I am referring to the
> > goto err_unregister path where the pci_unregister_driver() and
> > dgap_stop() are invoked in succession.Going by your argument,should we
> > remove the redundant cleanup done in the dgap_stop() function.
>
> dgap_init_module() is done correctly.
>
> The module_exit should mirror the module_init. dgap_remove_one() should
> mirror dgap_init_one() but instead it removes bits of everything.
>
Currently the module_exit doesn't mirror the module_init.I will address
that.
> /*
> * dgap_cleanup_module()
> *
> * Module unload. This is where it all ends.
> */
>
> These comments are obvious. Delete.
Okay I will delete this comment.
>
> static void dgap_cleanup_module(void)
> {
> dgap_remove_driver_sysfiles(&dgap_driver);
> pci_unregister_driver(&dgap_driver);
> dgap_stop();
> }
>
This piece of code mirrors the module_init completely.
> But dgap_remove_one() is all kinds of terrible.
>
I will review this function.
> regards,
> dan carpenter
>
My original intent of sending this patch is to remove the cleanup
related stuff that are done twice,once in the driver remove function and
in the module exit.IMHO,the resources that are allocate in the driver
module init function should be cleaned up in the driver exit and it
should be the only place.
Regards
Hari Prasath
done twice
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
