* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).