* [PATCH] [PATCH] extcon: driver model release call not needed @ 2012-10-18 17:12 anish kumar 2012-10-20 1:57 ` Chanwoo Choi 2012-10-22 19:57 ` Greg KH 0 siblings, 2 replies; 7+ messages in thread From: anish kumar @ 2012-10-18 17:12 UTC (permalink / raw) To: gregkh, myungjoo.ham, cw00.choi; +Cc: linux-kernel, anish kumar From: anish kumar <anish198519851985@gmail.com> We don't need a release call in this file as we are doing everything needed in unregister call and we don't have any more pointer to free up. Signed-off-by: anish kumar <anish198519851985@gmail.com> --- drivers/extcon/extcon-class.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-) diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c index 946a318..cf30eb1 100644 --- a/drivers/extcon/extcon-class.c +++ b/drivers/extcon/extcon-class.c @@ -585,9 +585,7 @@ static void extcon_cleanup(struct extcon_dev *edev, bool skip) static void extcon_dev_release(struct device *dev) { - struct extcon_dev *edev = (struct extcon_dev *) dev_get_drvdata(dev); - - extcon_cleanup(edev, true); + /* We don't have any thing to free here */ } static const char *muex_name = "mutually_exclusive"; -- 1.7.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] [PATCH] extcon: driver model release call not needed 2012-10-18 17:12 [PATCH] [PATCH] extcon: driver model release call not needed anish kumar @ 2012-10-20 1:57 ` Chanwoo Choi 2012-10-20 2:30 ` anish kumar 2012-10-22 19:57 ` Greg KH 1 sibling, 1 reply; 7+ messages in thread From: Chanwoo Choi @ 2012-10-20 1:57 UTC (permalink / raw) To: anish kumar; +Cc: gregkh, myungjoo.ham, linux-kernel On 10/19/2012 02:12 AM, anish kumar wrote: > From: anish kumar <anish198519851985@gmail.com> > > We don't need a release call in this file as we are doing > everything needed in unregister call and we don't have any > more pointer to free up. > > Signed-off-by: anish kumar <anish198519851985@gmail.com> > --- > drivers/extcon/extcon-class.c | 4 +--- > 1 files changed, 1 insertions(+), 3 deletions(-) > > diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c > index 946a318..cf30eb1 100644 > --- a/drivers/extcon/extcon-class.c > +++ b/drivers/extcon/extcon-class.c > @@ -585,9 +585,7 @@ static void extcon_cleanup(struct extcon_dev *edev, bool skip) > > static void extcon_dev_release(struct device *dev) > { > - struct extcon_dev *edev = (struct extcon_dev *) dev_get_drvdata(dev); > - > - extcon_cleanup(edev, true); > + /* We don't have any thing to free here */ > } > > static const char *muex_name = "mutually_exclusive"; I can't agree this patch. The extcon_dev_release() function is used for dev->release. If some case without calling extcon_dev_unregister(), I think dev->release function is needed to free memory of edev->dev. The edev->dev->release store the function pointer of extcon_dev_release() in extcon_dev_register(). edev->dev->parent = dev; edev->dev->class = extcon_class; edev->dev->release = extcon_dev_release; Thanks, Chanwoo Choi ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] [PATCH] extcon: driver model release call not needed 2012-10-20 1:57 ` Chanwoo Choi @ 2012-10-20 2:30 ` anish kumar 2012-10-20 2:37 ` Chanwoo Choi 0 siblings, 1 reply; 7+ messages in thread From: anish kumar @ 2012-10-20 2:30 UTC (permalink / raw) To: Chanwoo Choi; +Cc: gregkh, myungjoo.ham, linux-kernel On Sat, 2012-10-20 at 10:57 +0900, Chanwoo Choi wrote: > On 10/19/2012 02:12 AM, anish kumar wrote: > > From: anish kumar <anish198519851985@gmail.com> > > > > We don't need a release call in this file as we are doing > > everything needed in unregister call and we don't have any > > more pointer to free up. > > > > Signed-off-by: anish kumar <anish198519851985@gmail.com> > > --- > > drivers/extcon/extcon-class.c | 4 +--- > > 1 files changed, 1 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c > > index 946a318..cf30eb1 100644 > > --- a/drivers/extcon/extcon-class.c > > +++ b/drivers/extcon/extcon-class.c > > @@ -585,9 +585,7 @@ static void extcon_cleanup(struct extcon_dev *edev, bool skip) > > > > static void extcon_dev_release(struct device *dev) > > { > > - struct extcon_dev *edev = (struct extcon_dev *) dev_get_drvdata(dev); > > - > > - extcon_cleanup(edev, true); > > + /* We don't have any thing to free here */ > > } > > > > static const char *muex_name = "mutually_exclusive"; > > I can't agree this patch. The extcon_dev_release() function is used > for dev->release. If some case without calling extcon_dev_unregister(), > I think dev->release function is needed to free memory of edev->dev. Is it not being released by extcon_dev_unregister? I think it is released by that and we will do two times free and list_del(&edev->entry) as it is called by extcon_dev_release also. > > The edev->dev->release store the function pointer of extcon_dev_release() > in extcon_dev_register(). > edev->dev->parent = dev; > edev->dev->class = extcon_class; > edev->dev->release = extcon_dev_release; > > > Thanks, > Chanwoo Choi ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] [PATCH] extcon: driver model release call not needed 2012-10-20 2:30 ` anish kumar @ 2012-10-20 2:37 ` Chanwoo Choi 2012-10-20 3:33 ` anish kumar 0 siblings, 1 reply; 7+ messages in thread From: Chanwoo Choi @ 2012-10-20 2:37 UTC (permalink / raw) To: anish kumar; +Cc: gregkh, myungjoo.ham, linux-kernel On 10/20/2012 11:30 AM, anish kumar wrote: > On Sat, 2012-10-20 at 10:57 +0900, Chanwoo Choi wrote: >> On 10/19/2012 02:12 AM, anish kumar wrote: >>> From: anish kumar <anish198519851985@gmail.com> >>> >>> We don't need a release call in this file as we are doing >>> everything needed in unregister call and we don't have any >>> more pointer to free up. >>> >>> Signed-off-by: anish kumar <anish198519851985@gmail.com> >>> --- >>> drivers/extcon/extcon-class.c | 4 +--- >>> 1 files changed, 1 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c >>> index 946a318..cf30eb1 100644 >>> --- a/drivers/extcon/extcon-class.c >>> +++ b/drivers/extcon/extcon-class.c >>> @@ -585,9 +585,7 @@ static void extcon_cleanup(struct extcon_dev *edev, bool skip) >>> >>> static void extcon_dev_release(struct device *dev) >>> { >>> - struct extcon_dev *edev = (struct extcon_dev *) dev_get_drvdata(dev); >>> - >>> - extcon_cleanup(edev, true); >>> + /* We don't have any thing to free here */ >>> } >>> >>> static const char *muex_name = "mutually_exclusive"; >> >> I can't agree this patch. The extcon_dev_release() function is used >> for dev->release. If some case without calling extcon_dev_unregister(), >> I think dev->release function is needed to free memory of edev->dev. > Is it not being released by extcon_dev_unregister? > I think it is released by that and we will do two times free and > list_del(&edev->entry) as it is called by extcon_dev_release also. I think that this patch should modify it as below patch to remove two call of kfree(). How about you? diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c index e717bbc..efca0b4 100644 --- a/drivers/extcon/extcon-class.c +++ b/drivers/extcon/extcon-class.c @@ -597,9 +597,8 @@ static void extcon_cleanup(struct extcon_dev *edev, bool skip) #endif device_unregister(edev->dev); put_device(edev->dev); - } - - kfree(edev->dev); + } else { + kfree(edev->dev); ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] [PATCH] extcon: driver model release call not needed 2012-10-20 2:37 ` Chanwoo Choi @ 2012-10-20 3:33 ` anish kumar 2012-10-20 4:00 ` Chanwoo Choi 0 siblings, 1 reply; 7+ messages in thread From: anish kumar @ 2012-10-20 3:33 UTC (permalink / raw) To: Chanwoo Choi; +Cc: gregkh, myungjoo.ham, linux-kernel On Sat, 2012-10-20 at 11:37 +0900, Chanwoo Choi wrote: > On 10/20/2012 11:30 AM, anish kumar wrote: > > On Sat, 2012-10-20 at 10:57 +0900, Chanwoo Choi wrote: > >> On 10/19/2012 02:12 AM, anish kumar wrote: > >>> From: anish kumar <anish198519851985@gmail.com> > >>> > >>> We don't need a release call in this file as we are doing > >>> everything needed in unregister call and we don't have any > >>> more pointer to free up. > >>> > >>> Signed-off-by: anish kumar <anish198519851985@gmail.com> > >>> --- > >>> drivers/extcon/extcon-class.c | 4 +--- > >>> 1 files changed, 1 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c > >>> index 946a318..cf30eb1 100644 > >>> --- a/drivers/extcon/extcon-class.c > >>> +++ b/drivers/extcon/extcon-class.c > >>> @@ -585,9 +585,7 @@ static void extcon_cleanup(struct extcon_dev *edev, bool skip) > >>> > >>> static void extcon_dev_release(struct device *dev) > >>> { > >>> - struct extcon_dev *edev = (struct extcon_dev *) dev_get_drvdata(dev); > >>> - > >>> - extcon_cleanup(edev, true); > >>> + /* We don't have any thing to free here */ > >>> } > >>> > >>> static const char *muex_name = "mutually_exclusive"; > >> > >> I can't agree this patch. The extcon_dev_release() function is used > >> for dev->release. If some case without calling extcon_dev_unregister(), > >> I think dev->release function is needed to free memory of edev->dev. > > Is it not being released by extcon_dev_unregister? > > I think it is released by that and we will do two times free and > > list_del(&edev->entry) as it is called by extcon_dev_release also. > > I think that this patch should modify it as below patch to remove > two call of kfree(). How about you? > > diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c > index e717bbc..efca0b4 100644 > --- a/drivers/extcon/extcon-class.c > +++ b/drivers/extcon/extcon-class.c > @@ -597,9 +597,8 @@ static void extcon_cleanup(struct extcon_dev *edev, bool skip) > #endif > device_unregister(edev->dev); > put_device(edev->dev); > - } > - > - kfree(edev->dev); > + } else { > + kfree(edev->dev) How about below? diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c index 946a318..62e4ecb 100644 --- a/drivers/extcon/extcon-class.c +++ b/drivers/extcon/extcon-class.c @@ -551,43 +551,9 @@ static int create_extcon_class(void) return 0; } -static void extcon_cleanup(struct extcon_dev *edev, bool skip) -{ - mutex_lock(&extcon_dev_list_lock); - list_del(&edev->entry); - mutex_unlock(&extcon_dev_list_lock); - - if (!skip && get_device(edev->dev)) { - int index; - - if (edev->mutually_exclusive && edev->max_supported) { - for (index = 0; edev->mutually_exclusive[index]; - index++) - kfree(edev->d_attrs_muex[index].attr.name); - kfree(edev->d_attrs_muex); - kfree(edev->attrs_muex); - } - - for (index = 0; index < edev->max_supported; index++) - kfree(edev->cables[index].attr_g.name); - - if (edev->max_supported) { - kfree(edev->extcon_dev_type.groups); - kfree(edev->cables); - } - - device_unregister(edev->dev); - put_device(edev->dev); - } - - kfree(edev->dev); -} - static void extcon_dev_release(struct device *dev) { - struct extcon_dev *edev = (struct extcon_dev *) dev_get_drvdata(dev); - - extcon_cleanup(edev, true); + kfree(edev->dev); } static const char *muex_name = "mutually_exclusive"; @@ -813,7 +779,32 @@ EXPORT_SYMBOL_GPL(extcon_dev_register); */ void extcon_dev_unregister(struct extcon_dev *edev) { - extcon_cleanup(edev, false); + mutex_lock(&extcon_dev_list_lock); + list_del(&edev->entry); + mutex_unlock(&extcon_dev_list_lock); + + if (!skip && get_device(edev->dev)) { + int index; + + if (edev->mutually_exclusive && edev->max_supported) { + for (index = 0; edev->mutually_exclusive[index]; + index++) + kfree(edev->d_attrs_muex[index].attr.name); + kfree(edev->d_attrs_muex); + kfree(edev->attrs_muex); + } + + for (index = 0; index < edev->max_supported; index++) + kfree(edev->cables[index].attr_g.name); + + if (edev->max_supported) { + kfree(edev->extcon_dev_type.groups); + kfree(edev->cables); + } + + device_unregister(edev->dev); + put_device(edev->dev); + } } EXPORT_SYMBOL_GPL(extcon_dev_unregister); > ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] [PATCH] extcon: driver model release call not needed 2012-10-20 3:33 ` anish kumar @ 2012-10-20 4:00 ` Chanwoo Choi 0 siblings, 0 replies; 7+ messages in thread From: Chanwoo Choi @ 2012-10-20 4:00 UTC (permalink / raw) To: anish kumar; +Cc: gregkh, myungjoo.ham, linux-kernel On 10/20/2012 12:33 PM, anish kumar wrote: [....] > How about below? I agree, but there were some minor issues, [....] > > static const char *muex_name = "mutually_exclusive"; > @@ -813,7 +779,32 @@ EXPORT_SYMBOL_GPL(extcon_dev_register); > */ > void extcon_dev_unregister(struct extcon_dev *edev) > { > - extcon_cleanup(edev, false); > + mutex_lock(&extcon_dev_list_lock); > + list_del(&edev->entry); > + mutex_unlock(&extcon_dev_list_lock); > + > + if (!skip && get_device(edev->dev)) { 'skip' variable isn't anymore used and fix indentation as below code [...] ----- diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c index e717bbc..ec7cc84 100644 --- a/drivers/extcon/extcon-class.c +++ b/drivers/extcon/extcon-class.c @@ -566,47 +566,9 @@ static int create_extcon_class(void) return 0; } -static void extcon_cleanup(struct extcon_dev *edev, bool skip) -{ - mutex_lock(&extcon_dev_list_lock); - list_del(&edev->entry); - mutex_unlock(&extcon_dev_list_lock); - - if (!skip && get_device(edev->dev)) { - int index; - - if (edev->mutually_exclusive && edev->max_supported) { - for (index = 0; edev->mutually_exclusive[index]; - index++) - kfree(edev->d_attrs_muex[index].attr.name); - kfree(edev->d_attrs_muex); - kfree(edev->attrs_muex); - } - - for (index = 0; index < edev->max_supported; index++) - kfree(edev->cables[index].attr_g.name); - - if (edev->max_supported) { - kfree(edev->extcon_dev_type.groups); - kfree(edev->cables); - } - -#if defined(CONFIG_ANDROID) - if (switch_class) - class_compat_remove_link(switch_class, edev->dev, NULL); -#endif - device_unregister(edev->dev); - put_device(edev->dev); - } - - kfree(edev->dev); -} - static void extcon_dev_release(struct device *dev) { - struct extcon_dev *edev = (struct extcon_dev *) dev_get_drvdata(dev); - - extcon_cleanup(edev, true); + kfree(edev->dev); } static const char *muex_name = "mutually_exclusive"; @@ -832,7 +794,37 @@ EXPORT_SYMBOL_GPL(extcon_dev_register); */ void extcon_dev_unregister(struct extcon_dev *edev) { - extcon_cleanup(edev, false); + int index; + + mutex_lock(&extcon_dev_list_lock); + list_del(&edev->entry); + mutex_unlock(&extcon_dev_list_lock); + + if (!get_device(edev->dev)) + return; + + if (edev->mutually_exclusive && edev->max_supported) { + for (index = 0; edev->mutually_exclusive[index]; + index++) + kfree(edev->d_attrs_muex[index].attr.name); + kfree(edev->d_attrs_muex); + kfree(edev->attrs_muex); + } + + for (index = 0; index < edev->max_supported; index++) + kfree(edev->cables[index].attr_g.name); + + if (edev->max_supported) { + kfree(edev->extcon_dev_type.groups); + kfree(edev->cables); + } + +#if defined(CONFIG_ANDROID) + if (switch_class) + class_compat_remove_link(switch_class, edev->dev, NULL); +#endif + device_unregister(edev->dev); + put_device(edev->dev); } EXPORT_SYMBOL_GPL(extcon_dev_unregister); Thanks, Chanwoo Choi ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] [PATCH] extcon: driver model release call not needed 2012-10-18 17:12 [PATCH] [PATCH] extcon: driver model release call not needed anish kumar 2012-10-20 1:57 ` Chanwoo Choi @ 2012-10-22 19:57 ` Greg KH 1 sibling, 0 replies; 7+ messages in thread From: Greg KH @ 2012-10-22 19:57 UTC (permalink / raw) To: anish kumar; +Cc: myungjoo.ham, cw00.choi, linux-kernel On Fri, Oct 19, 2012 at 02:12:06AM +0900, anish kumar wrote: > From: anish kumar <anish198519851985@gmail.com> > > We don't need a release call in this file as we are doing > everything needed in unregister call and we don't have any > more pointer to free up. > > Signed-off-by: anish kumar <anish198519851985@gmail.com> > --- > drivers/extcon/extcon-class.c | 4 +--- > 1 files changed, 1 insertions(+), 3 deletions(-) > > diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c > index 946a318..cf30eb1 100644 > --- a/drivers/extcon/extcon-class.c > +++ b/drivers/extcon/extcon-class.c > @@ -585,9 +585,7 @@ static void extcon_cleanup(struct extcon_dev *edev, bool skip) > > static void extcon_dev_release(struct device *dev) > { > - struct extcon_dev *edev = (struct extcon_dev *) dev_get_drvdata(dev); > - > - extcon_cleanup(edev, true); > + /* We don't have any thing to free here */ > } In the future (I know you fixed this up), doing an empty release() function will get you in big trouble. Please read the kobject documentation for more information about why this is. greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-10-22 19:57 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-18 17:12 [PATCH] [PATCH] extcon: driver model release call not needed anish kumar 2012-10-20 1:57 ` Chanwoo Choi 2012-10-20 2:30 ` anish kumar 2012-10-20 2:37 ` Chanwoo Choi 2012-10-20 3:33 ` anish kumar 2012-10-20 4:00 ` Chanwoo Choi 2012-10-22 19:57 ` Greg KH
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.