* [PATCH] PM / devfreq: Don't delete sysfs group twice @ 2016-12-13 17:09 ` Chris Diamand 2016-12-14 1:37 ` Chanwoo Choi 2016-12-14 1:38 ` Chanwoo Choi 0 siblings, 2 replies; 13+ messages in thread From: Chris Diamand @ 2016-12-13 17:09 UTC (permalink / raw) To: linux-pm, MyungJoo Ham; +Cc: Kyungmin Park, Nishanth Menon, Cai Zhiyong The 'userspace' governor adds a sysfs entry, which is removed when the governor is changed, or the devfreq device is released. However, when the latter occurs via device_unregister(), device_del() is called first, which removes the sysfs entries recursively and deletes the kobject. This means we get an Oops when the governor calls sysfs_remove_group() on the deleted kobject. Fix this by explicitly stopping the governor in devfreq_remove_device(), *before* the devfreq entry is removed. Note that we can't just remove the call to sysfs_remove_group() in the userspace governor, as it's needed for when the governor is changed to one without a sysfs entry. Signed-off-by: Chris Diamand <chris.diamand@arm.com> --- drivers/devfreq/devfreq.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 478006b..d8a817c 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -622,6 +622,12 @@ int devfreq_remove_device(struct devfreq *devfreq) if (!devfreq) return -EINVAL; + if (devfreq->governor) { + devfreq->governor->event_handler(devfreq, + DEVFREQ_GOV_STOP, NULL); + devfreq->governor = NULL; + } + device_unregister(&devfreq->dev); return 0; -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] PM / devfreq: Don't delete sysfs group twice 2016-12-13 17:09 ` [PATCH] PM / devfreq: Don't delete sysfs group twice Chris Diamand @ 2016-12-14 1:37 ` Chanwoo Choi 2016-12-14 1:38 ` Chanwoo Choi 1 sibling, 0 replies; 13+ messages in thread From: Chanwoo Choi @ 2016-12-14 1:37 UTC (permalink / raw) To: Chris Diamand, linux-pm, MyungJoo Ham Cc: Kyungmin Park, Nishanth Menon, Cai Zhiyong Hi Chris, On 2016년 12월 14일 02:09, Chris Diamand wrote: > The 'userspace' governor adds a sysfs entry, which is removed when > the governor is changed, or the devfreq device is released. However, > when the latter occurs via device_unregister(), device_del() is > called first, which removes the sysfs entries recursively and deletes > the kobject. > > This means we get an Oops when the governor calls > sysfs_remove_group() on the deleted kobject. Fix this by explicitly > stopping the governor in devfreq_remove_device(), *before* the > devfreq entry is removed. > > Note that we can't just remove the call to sysfs_remove_group() in > the userspace governor, as it's needed for when the governor is > changed to one without a sysfs entry. > > Signed-off-by: Chris Diamand <chris.diamand@arm.com> > --- > drivers/devfreq/devfreq.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 478006b..d8a817c 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -622,6 +622,12 @@ int devfreq_remove_device(struct devfreq *devfreq) > if (!devfreq) > return -EINVAL; > > + if (devfreq->governor) { > + devfreq->governor->event_handler(devfreq, > + DEVFREQ_GOV_STOP, NULL); > + devfreq->governor = NULL; > + } The devfreq_remove_device has following description: - "* The oppsite of devfreq_add_device()" I think that we should call the '_remove_devfreq()' function in the devfreq_remove_device() because '_remove_devfreq()' is in charge of releasing the devfreq instance. The '_remove_devfreq() ' function includes the code to stop the governor and following works: - remove the devfreq instance from devfreq list - call the profile->exit function - destroy the muxte of devfreq instance - Free the devfreq memory. Best Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PM / devfreq: Don't delete sysfs group twice 2016-12-13 17:09 ` [PATCH] PM / devfreq: Don't delete sysfs group twice Chris Diamand 2016-12-14 1:37 ` Chanwoo Choi @ 2016-12-14 1:38 ` Chanwoo Choi 2016-12-14 2:48 ` Chanwoo Choi 1 sibling, 1 reply; 13+ messages in thread From: Chanwoo Choi @ 2016-12-14 1:38 UTC (permalink / raw) To: Chris Diamand, linux-pm, MyungJoo Ham Cc: Kyungmin Park, Nishanth Menon, Cai Zhiyong, cpgs (cpgs@samsung.com) Hi Chris, On 2016년 12월 14일 02:09, Chris Diamand wrote: > The 'userspace' governor adds a sysfs entry, which is removed when > the governor is changed, or the devfreq device is released. However, > when the latter occurs via device_unregister(), device_del() is > called first, which removes the sysfs entries recursively and deletes > the kobject. > > This means we get an Oops when the governor calls > sysfs_remove_group() on the deleted kobject. Fix this by explicitly > stopping the governor in devfreq_remove_device(), *before* the > devfreq entry is removed. > > Note that we can't just remove the call to sysfs_remove_group() in > the userspace governor, as it's needed for when the governor is > changed to one without a sysfs entry. > > Signed-off-by: Chris Diamand <chris.diamand@arm.com> > --- > drivers/devfreq/devfreq.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 478006b..d8a817c 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -622,6 +622,12 @@ int devfreq_remove_device(struct devfreq *devfreq) > if (!devfreq) > return -EINVAL; > > + if (devfreq->governor) { > + devfreq->governor->event_handler(devfreq, > + DEVFREQ_GOV_STOP, NULL); > + devfreq->governor = NULL; > + } > + > The devfreq_remove_device has following description: - "* The oppsite of devfreq_add_device()" I think that we should call the '_remove_devfreq()' function in the devfreq_remove_device() because '_remove_devfreq()' is in charge of releasing the devfreq instance. The '_remove_devfreq() ' function includes the code to stop the governor and following works: - remove the devfreq instance from devfreq list - call the profile->exit function - destroy the muxte of devfreq instance - Free the devfreq memory. Best Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PM / devfreq: Don't delete sysfs group twice 2016-12-14 1:38 ` Chanwoo Choi @ 2016-12-14 2:48 ` Chanwoo Choi 2016-12-14 9:29 ` Sudeep Holla 0 siblings, 1 reply; 13+ messages in thread From: Chanwoo Choi @ 2016-12-14 2:48 UTC (permalink / raw) To: Chris Diamand, linux-pm, MyungJoo Ham Cc: Kyungmin Park, Nishanth Menon, Cai Zhiyong, cpgs (cpgs@samsung.com) Hi Chris, On 2016년 12월 14일 10:38, Chanwoo Choi wrote: > Hi Chris, > > On 2016년 12월 14일 02:09, Chris Diamand wrote: >> The 'userspace' governor adds a sysfs entry, which is removed when >> the governor is changed, or the devfreq device is released. However, >> when the latter occurs via device_unregister(), device_del() is >> called first, which removes the sysfs entries recursively and deletes >> the kobject. >> >> This means we get an Oops when the governor calls >> sysfs_remove_group() on the deleted kobject. Fix this by explicitly >> stopping the governor in devfreq_remove_device(), *before* the >> devfreq entry is removed. >> >> Note that we can't just remove the call to sysfs_remove_group() in >> the userspace governor, as it's needed for when the governor is >> changed to one without a sysfs entry. >> >> Signed-off-by: Chris Diamand <chris.diamand@arm.com> >> --- >> drivers/devfreq/devfreq.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 478006b..d8a817c 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -622,6 +622,12 @@ int devfreq_remove_device(struct devfreq *devfreq) >> if (!devfreq) >> return -EINVAL; >> >> + if (devfreq->governor) { >> + devfreq->governor->event_handler(devfreq, >> + DEVFREQ_GOV_STOP, NULL); >> + devfreq->governor = NULL; >> + } >> + >> > The devfreq_remove_device has following description: > - "* The oppsite of devfreq_add_device()" > > I think that we should call the '_remove_devfreq()' function in the devfreq_remove_device() > because '_remove_devfreq()' is in charge of releasing the devfreq instance. > > The '_remove_devfreq() ' function includes the code to stop the governor and following works: > - remove the devfreq instance from devfreq list > - call the profile->exit function > - destroy the muxte of devfreq instance > - Free the devfreq memory. How about following patch? I think that this patch covers the all of case when removing the devfreq device. diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 8a456dd55a8d..eea406df0037 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -496,6 +496,7 @@ static void _remove_devfreq(struct devfreq *devfreq) devfreq->profile->exit(devfreq->dev.parent); mutex_destroy(&devfreq->lock); + device_unregister(&devfreq->dev); kfree(devfreq); } @@ -634,7 +635,7 @@ int devfreq_remove_device(struct devfreq *devfreq) if (!devfreq) return -EINVAL; - device_unregister(&devfreq->dev); + _remove_devfreq(devfreq); return 0; } Regards, Chanwoo Choi ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] PM / devfreq: Don't delete sysfs group twice 2016-12-14 2:48 ` Chanwoo Choi @ 2016-12-14 9:29 ` Sudeep Holla 2016-12-14 10:10 ` Chanwoo Choi [not found] ` <172e4f85358b4f4e9206c4a54b5db056@AM4PR0802MB2210.eurprd08.prod.outlook.com> 0 siblings, 2 replies; 13+ messages in thread From: Sudeep Holla @ 2016-12-14 9:29 UTC (permalink / raw) To: Chanwoo Choi, Chris Diamand, linux-pm, MyungJoo Ham Cc: Sudeep Holla, Kyungmin Park, Nishanth Menon, Cai Zhiyong, cpgs (cpgs@samsung.com) On 14/12/16 02:48, Chanwoo Choi wrote: > Hi Chris, > > On 2016년 12월 14일 10:38, Chanwoo Choi wrote: >> Hi Chris, >> >> On 2016년 12월 14일 02:09, Chris Diamand wrote: >>> The 'userspace' governor adds a sysfs entry, which is removed when >>> the governor is changed, or the devfreq device is released. However, >>> when the latter occurs via device_unregister(), device_del() is >>> called first, which removes the sysfs entries recursively and deletes >>> the kobject. >>> >>> This means we get an Oops when the governor calls >>> sysfs_remove_group() on the deleted kobject. Fix this by explicitly >>> stopping the governor in devfreq_remove_device(), *before* the >>> devfreq entry is removed. >>> >>> Note that we can't just remove the call to sysfs_remove_group() in >>> the userspace governor, as it's needed for when the governor is >>> changed to one without a sysfs entry. >>> >>> Signed-off-by: Chris Diamand <chris.diamand@arm.com> >>> --- >>> drivers/devfreq/devfreq.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index 478006b..d8a817c 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -622,6 +622,12 @@ int devfreq_remove_device(struct devfreq *devfreq) >>> if (!devfreq) >>> return -EINVAL; >>> >>> + if (devfreq->governor) { >>> + devfreq->governor->event_handler(devfreq, >>> + DEVFREQ_GOV_STOP, NULL); >>> + devfreq->governor = NULL; >>> + } >>> + >>> >> The devfreq_remove_device has following description: >> - "* The oppsite of devfreq_add_device()" >> >> I think that we should call the '_remove_devfreq()' function in the devfreq_remove_device() >> because '_remove_devfreq()' is in charge of releasing the devfreq instance. >> >> The '_remove_devfreq() ' function includes the code to stop the governor and following works: >> - remove the devfreq instance from devfreq list >> - call the profile->exit function >> - destroy the muxte of devfreq instance >> - Free the devfreq memory. > > How about following patch? > I think that this patch covers the all of case when removing the devfreq device. > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 8a456dd55a8d..eea406df0037 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -496,6 +496,7 @@ static void _remove_devfreq(struct devfreq *devfreq) > devfreq->profile->exit(devfreq->dev.parent); > > mutex_destroy(&devfreq->lock); > + device_unregister(&devfreq->dev); > kfree(devfreq); > } > Won't this result in device_unregister->put_device->kobject_put->kref_put-> kobject_release->kobject_cleanup->release->device_unregister Is that fine ? -- Regards, Sudeep ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PM / devfreq: Don't delete sysfs group twice 2016-12-14 9:29 ` Sudeep Holla @ 2016-12-14 10:10 ` Chanwoo Choi [not found] ` <172e4f85358b4f4e9206c4a54b5db056@AM4PR0802MB2210.eurprd08.prod.outlook.com> 1 sibling, 0 replies; 13+ messages in thread From: Chanwoo Choi @ 2016-12-14 10:10 UTC (permalink / raw) To: Sudeep Holla, Chris Diamand, linux-pm, MyungJoo Ham Cc: Kyungmin Park, Nishanth Menon, Cai Zhiyong, cpgs (cpgs@samsung.com) Hi Sudeep, On 2016년 12월 14일 18:29, Sudeep Holla wrote: > > > On 14/12/16 02:48, Chanwoo Choi wrote: >> Hi Chris, >> >> On 2016년 12월 14일 10:38, Chanwoo Choi wrote: >>> Hi Chris, >>> >>> On 2016년 12월 14일 02:09, Chris Diamand wrote: >>>> The 'userspace' governor adds a sysfs entry, which is removed when >>>> the governor is changed, or the devfreq device is released. However, >>>> when the latter occurs via device_unregister(), device_del() is >>>> called first, which removes the sysfs entries recursively and deletes >>>> the kobject. >>>> >>>> This means we get an Oops when the governor calls >>>> sysfs_remove_group() on the deleted kobject. Fix this by explicitly >>>> stopping the governor in devfreq_remove_device(), *before* the >>>> devfreq entry is removed. >>>> >>>> Note that we can't just remove the call to sysfs_remove_group() in >>>> the userspace governor, as it's needed for when the governor is >>>> changed to one without a sysfs entry. >>>> >>>> Signed-off-by: Chris Diamand <chris.diamand@arm.com> >>>> --- >>>> drivers/devfreq/devfreq.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>> index 478006b..d8a817c 100644 >>>> --- a/drivers/devfreq/devfreq.c >>>> +++ b/drivers/devfreq/devfreq.c >>>> @@ -622,6 +622,12 @@ int devfreq_remove_device(struct devfreq *devfreq) >>>> if (!devfreq) >>>> return -EINVAL; >>>> >>>> + if (devfreq->governor) { >>>> + devfreq->governor->event_handler(devfreq, >>>> + DEVFREQ_GOV_STOP, NULL); >>>> + devfreq->governor = NULL; >>>> + } >>>> + >>>> >>> The devfreq_remove_device has following description: >>> - "* The oppsite of devfreq_add_device()" >>> >>> I think that we should call the '_remove_devfreq()' function in the devfreq_remove_device() >>> because '_remove_devfreq()' is in charge of releasing the devfreq instance. >>> >>> The '_remove_devfreq() ' function includes the code to stop the governor and following works: >>> - remove the devfreq instance from devfreq list >>> - call the profile->exit function >>> - destroy the muxte of devfreq instance >>> - Free the devfreq memory. >> >> How about following patch? >> I think that this patch covers the all of case when removing the devfreq device. >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 8a456dd55a8d..eea406df0037 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -496,6 +496,7 @@ static void _remove_devfreq(struct devfreq *devfreq) >> devfreq->profile->exit(devfreq->dev.parent); >> >> mutex_destroy(&devfreq->lock); >> + device_unregister(&devfreq->dev); >> kfree(devfreq); >> } >> > > Won't this result in device_unregister->put_device->kobject_put->kref_put-> > kobject_release->kobject_cleanup->release->device_unregister > > Is that fine ? > My suggestion is incorrect. The original suggestion is right with removing the code related to DEVFREQ_GOV_STOP from _remove_devfreq(). I'm sorry for confusion. diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 8a456dd55a8d..3e9eb7fa9e72 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -488,10 +488,6 @@ static void _remove_devfreq(struct devfreq *devfreq) list_del(&devfreq->node); mutex_unlock(&devfreq_list_lock); - if (devfreq->governor) - devfreq->governor->event_handler(devfreq, - DEVFREQ_GOV_STOP, NULL); - if (devfreq->profile->exit) devfreq->profile->exit(devfreq->dev.parent); @@ -634,6 +630,10 @@ int devfreq_remove_device(struct devfreq *devfreq) if (!devfreq) return -EINVAL; + if (devfreq->governor) + devfreq->governor->event_handler(devfreq, + DEVFREQ_GOV_STOP, NULL); + device_unregister(&devfreq->dev); return 0; -- Regards, Chanwoo Choi ^ permalink raw reply related [flat|nested] 13+ messages in thread
[parent not found: <172e4f85358b4f4e9206c4a54b5db056@AM4PR0802MB2210.eurprd08.prod.outlook.com>]
* Re: [PATCH] PM / devfreq: Don't delete sysfs group twice [not found] ` <172e4f85358b4f4e9206c4a54b5db056@AM4PR0802MB2210.eurprd08.prod.outlook.com> @ 2016-12-14 11:32 ` Chris Diamand 2016-12-14 13:48 ` Chanwoo Choi [not found] ` <7a342ead78be4f32a4a37b541fde412b@AM4PR0802MB2210.eurprd08.prod.outlook.com> 0 siblings, 2 replies; 13+ messages in thread From: Chris Diamand @ 2016-12-14 11:32 UTC (permalink / raw) To: Chanwoo Choi Cc: Sudeep Holla, linux-pm, MyungJoo Ham, Kyungmin Park, Nishanth Menon, Cai Zhiyong, cpgs (cpgs@samsung.com) Hi all, Thanks for your replies. > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 8a456dd55a8d..3e9eb7fa9e72 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -488,10 +488,6 @@ static void _remove_devfreq(struct devfreq *devfreq) > list_del(&devfreq->node); > mutex_unlock(&devfreq_list_lock); > > - if (devfreq->governor) > - devfreq->governor->event_handler(devfreq, > - DEVFREQ_GOV_STOP, NULL); > - > if (devfreq->profile->exit) > devfreq->profile->exit(devfreq->dev.parent); I was thinking of removing this too, but wasn't completely sure - is there any situation where the devfreq feature's reference count would go to 0, *without* going via devfreq_remove_device()? If there is, we might need to keep it, although the userspace governor would still be broken for those cases. The one case I can think of is if there's an error in devfreq_add_device(), where it calls device_unregister() directly. Except I think we're OK here, because the governor won't have started yet. Cheers, Chris ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PM / devfreq: Don't delete sysfs group twice 2016-12-14 11:32 ` Chris Diamand @ 2016-12-14 13:48 ` Chanwoo Choi [not found] ` <7a342ead78be4f32a4a37b541fde412b@AM4PR0802MB2210.eurprd08.prod.outlook.com> 1 sibling, 0 replies; 13+ messages in thread From: Chanwoo Choi @ 2016-12-14 13:48 UTC (permalink / raw) To: Chris Diamand Cc: Sudeep Holla, linux-pm, MyungJoo Ham, Kyungmin Park, Nishanth Menon, Cai Zhiyong, cpgs (cpgs@samsung.com) Hi, On 2016년 12월 14일 20:32, Chris Diamand wrote: > Hi all, > > Thanks for your replies. > >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 8a456dd55a8d..3e9eb7fa9e72 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -488,10 +488,6 @@ static void _remove_devfreq(struct devfreq *devfreq) >> list_del(&devfreq->node); >> mutex_unlock(&devfreq_list_lock); >> >> - if (devfreq->governor) >> - devfreq->governor->event_handler(devfreq, >> - DEVFREQ_GOV_STOP, NULL); >> - >> if (devfreq->profile->exit) >> devfreq->profile->exit(devfreq->dev.parent); > > I was thinking of removing this too, but wasn't completely sure - is there > any situation where the devfreq feature's reference count would go to 0, > *without* going via devfreq_remove_device()? I only considered the case which calling the devfreq_remove_device(). As you mentioned, it is right to remain this code in _remove_devfreq() for other case to unregister devfreq device except for devfreq_remove_device(). I'm sorry to make you confused. > > If there is, we might need to keep it, although the userspace governor would > still be broken for those cases. > > The one case I can think of is if there's an error in devfreq_add_device(), > where it calls device_unregister() directly. Except I think we're OK here, > because the governor won't have started yet. -- Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <7a342ead78be4f32a4a37b541fde412b@AM4PR0802MB2210.eurprd08.prod.outlook.com>]
* Re: [PATCH] PM / devfreq: Don't delete sysfs group twice [not found] ` <7a342ead78be4f32a4a37b541fde412b@AM4PR0802MB2210.eurprd08.prod.outlook.com> @ 2016-12-15 10:41 ` Chris Diamand 2016-12-16 6:48 ` Chanwoo Choi [not found] ` <f4620f8efbf247119640470ec12810fe@AM4PR0802MB2210.eurprd08.prod.outlook.com> 2016-12-16 2:17 ` Re: [PATCH] " MyungJoo Ham 1 sibling, 2 replies; 13+ messages in thread From: Chris Diamand @ 2016-12-15 10:41 UTC (permalink / raw) To: Chanwoo Choi Cc: Sudeep Holla, linux-pm, MyungJoo Ham, Kyungmin Park, Nishanth Menon, Cai Zhiyong, cpgs (cpgs@samsung.com) Hi! > As you mentioned, it is right to remain this code in _remove_devfreq() > for other case to unregister devfreq device except for devfreq_remove_device(). Sure, I'll send out another patch with a comment. Although, if anyone does unregister devfreq without devfreq_remove_device(), the userspace governor will still be broken. I wonder if it would be better to fix this in the governor instead, because this problem only affects things using sysfs entries. Something like this: --- a/drivers/devfreq/governor_userspace.c +++ b/drivers/devfreq/governor_userspace.c @@ -112,7 +112,13 @@ out: static void userspace_exit(struct devfreq *devfreq) { - sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group); + /* + * Remove the sysfs entry, unless this is being called after + * device_del(), which should have done this already via kobject_del(). + */ + if (devfreq->dev.kobj.sd) + sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group); + kfree(devfreq->data); devfreq->data = NULL; } It's a bit uglier, but would fix it in all cases - what do you think? Cheers, Chris ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PM / devfreq: Don't delete sysfs group twice 2016-12-15 10:41 ` Chris Diamand @ 2016-12-16 6:48 ` Chanwoo Choi [not found] ` <f4620f8efbf247119640470ec12810fe@AM4PR0802MB2210.eurprd08.prod.outlook.com> 1 sibling, 0 replies; 13+ messages in thread From: Chanwoo Choi @ 2016-12-16 6:48 UTC (permalink / raw) To: Chris Diamand Cc: Sudeep Holla, linux-pm, MyungJoo Ham, Kyungmin Park, Nishanth Menon, Cai Zhiyong, cpgs (cpgs@samsung.com) Hi Chris, On 2016년 12월 15일 19:41, Chris Diamand wrote: > Hi! > >> As you mentioned, it is right to remain this code in _remove_devfreq() >> for other case to unregister devfreq device except for devfreq_remove_device(). > > Sure, I'll send out another patch with a comment. > > Although, if anyone does unregister devfreq without devfreq_remove_device(), > the userspace governor will still be broken. > > I wonder if it would be better to fix this in the governor instead, because > this problem only affects things using sysfs entries. Something like this: > > --- a/drivers/devfreq/governor_userspace.c > +++ b/drivers/devfreq/governor_userspace.c > @@ -112,7 +112,13 @@ out: > > static void userspace_exit(struct devfreq *devfreq) > { > - sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group); > + /* > + * Remove the sysfs entry, unless this is being called after > + * device_del(), which should have done this already via kobject_del(). > + */ > + if (devfreq->dev.kobj.sd) > + sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group); > + > kfree(devfreq->data); > devfreq->data = NULL; > } > > It's a bit uglier, but would fix it in all cases - what do you think?-- I think so too. But, I agree new patch. Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <f4620f8efbf247119640470ec12810fe@AM4PR0802MB2210.eurprd08.prod.outlook.com>]
* [PATCH v2] PM / devfreq: Don't delete sysfs group twice [not found] ` <f4620f8efbf247119640470ec12810fe@AM4PR0802MB2210.eurprd08.prod.outlook.com> @ 2016-12-16 19:09 ` Chris Diamand 0 siblings, 0 replies; 13+ messages in thread From: Chris Diamand @ 2016-12-16 19:09 UTC (permalink / raw) To: Chanwoo Choi, linux-pm Cc: Sudeep Holla, MyungJoo Ham, Kyungmin Park, Nishanth Menon, Cai Zhiyong, cpgs (cpgs@samsung.com) The 'userspace' governor adds a sysfs entry, which is removed when the governor is changed, or the devfreq device is released. However, when the latter occurs via device_unregister(), device_del() is called first, which removes the sysfs entries recursively and deletes the kobject. This means we get an Oops when the governor calls sysfs_remove_group() on the deleted kobject. Fix this by only doing the call when kobj *hasn't* been kobject_del()'d. Note that we can't just remove the call to sysfs_remove_group() entirely - it's needed for when the governor is changed to one which doesn't need a sysfs entry. Signed-off-by: Chris Diamand <chris.diamand@arm.com> --- This takes a different approach to v1, and fixes the issue in the governor instead. This means reference counting should still work as usual, so anything which doesn't call devfreq_remove_device() should still work. drivers/devfreq/governor_userspace.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c index 35de6e8..9db4d6f 100644 --- a/drivers/devfreq/governor_userspace.c +++ b/drivers/devfreq/governor_userspace.c @@ -112,7 +112,13 @@ static int userspace_init(struct devfreq *devfreq) static void userspace_exit(struct devfreq *devfreq) { - sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group); + /* + * Remove the sysfs entry, unless this is being called after + * device_del(), which should have done this already via kobject_del(). + */ + if (devfreq->dev.kobj.sd) + sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group); + kfree(devfreq->data); devfreq->data = NULL; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* RE: Re: [PATCH] PM / devfreq: Don't delete sysfs group twice [not found] ` <7a342ead78be4f32a4a37b541fde412b@AM4PR0802MB2210.eurprd08.prod.outlook.com> 2016-12-15 10:41 ` Chris Diamand @ 2016-12-16 2:17 ` MyungJoo Ham 1 sibling, 0 replies; 13+ messages in thread From: MyungJoo Ham @ 2016-12-16 2:17 UTC (permalink / raw) To: Chris Diamand, Chanwoo Choi Cc: Sudeep Holla, linux-pm, Kyungmin Park, Nishanth Menon, Cai Zhiyong, cpgs . [-- Attachment #1: Type: text/plain, Size: 1184 bytes --] > Sure, I'll send out another patch with a comment. > > Although, if anyone does unregister devfreq without devfreq_remove_device(), > the userspace governor will still be broken. > > I wonder if it would be better to fix this in the governor instead, because > this problem only affects things using sysfs entries. Something like this: > > --- a/drivers/devfreq/governor_userspace.c > +++ b/drivers/devfreq/governor_userspace.c > @@ -112,7 +112,13 @@ out: > > static void userspace_exit(struct devfreq *devfreq) > { > - sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group); > + /* > + * Remove the sysfs entry, unless this is being called after > + * device_del(), which should have done this already via kobject_del(). > + */ > + if (devfreq->dev.kobj.sd) > + sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group); > + > kfree(devfreq->data); > devfreq->data = NULL; > } > > It's a bit uglier, but would fix it in all cases - what do you think? This looks better because the problem here is on undoing what a governor has in its own functions at the governor's entering function. Cheers, MyungJoo > > Cheers, > Chris ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <CGME20161213170941epcas2p1267d3da4c7034777c8de117ed3c0f9fe@epcas2p1.samsung.com>]
* RE: [PATCH] PM / devfreq: Don't delete sysfs group twice [not found] <CGME20161213170941epcas2p1267d3da4c7034777c8de117ed3c0f9fe@epcas2p1.samsung.com> @ 2016-12-14 0:45 ` MyungJoo Ham 0 siblings, 0 replies; 13+ messages in thread From: MyungJoo Ham @ 2016-12-14 0:45 UTC (permalink / raw) To: Chris Diamand, linux-pm; +Cc: Kyungmin Park, Nishanth Menon, Cai Zhiyong [-- Attachment #1: Type: text/plain, Size: 1431 bytes --] >The 'userspace' governor adds a sysfs entry, which is removed when >the governor is changed, or the devfreq device is released. However, >when the latter occurs via device_unregister(), device_del() is >called first, which removes the sysfs entries recursively and deletes >the kobject. > >This means we get an Oops when the governor calls >sysfs_remove_group() on the deleted kobject. Fix this by explicitly >stopping the governor in devfreq_remove_device(), *before* the >devfreq entry is removed. > >Note that we can't just remove the call to sysfs_remove_group() in >the userspace governor, as it's needed for when the governor is >changed to one without a sysfs entry. > >Signed-off-by: Chris Diamand <chris.diamand@arm.com> Thanks! Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com> Cheers, MyungJoo >--- > drivers/devfreq/devfreq.c | 6 ++++++ > 1 file changed, 6 insertions(+) > >diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >index 478006b..d8a817c 100644 >--- a/drivers/devfreq/devfreq.c >+++ b/drivers/devfreq/devfreq.c >@@ -622,6 +622,12 @@ int devfreq_remove_device(struct devfreq *devfreq) > if (!devfreq) > return -EINVAL; > >+ if (devfreq->governor) { >+ devfreq->governor->event_handler(devfreq, >+ DEVFREQ_GOV_STOP, NULL); >+ devfreq->governor = NULL; >+ } >+ > device_unregister(&devfreq->dev); > > return 0; >-- >1.9.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-12-16 19:09 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20161213171211epcas3p3ed8807883967daca28b7abe211446739@epcas3p3.samsung.com> 2016-12-13 17:09 ` [PATCH] PM / devfreq: Don't delete sysfs group twice Chris Diamand 2016-12-14 1:37 ` Chanwoo Choi 2016-12-14 1:38 ` Chanwoo Choi 2016-12-14 2:48 ` Chanwoo Choi 2016-12-14 9:29 ` Sudeep Holla 2016-12-14 10:10 ` Chanwoo Choi [not found] ` <172e4f85358b4f4e9206c4a54b5db056@AM4PR0802MB2210.eurprd08.prod.outlook.com> 2016-12-14 11:32 ` Chris Diamand 2016-12-14 13:48 ` Chanwoo Choi [not found] ` <7a342ead78be4f32a4a37b541fde412b@AM4PR0802MB2210.eurprd08.prod.outlook.com> 2016-12-15 10:41 ` Chris Diamand 2016-12-16 6:48 ` Chanwoo Choi [not found] ` <f4620f8efbf247119640470ec12810fe@AM4PR0802MB2210.eurprd08.prod.outlook.com> 2016-12-16 19:09 ` [PATCH v2] " Chris Diamand 2016-12-16 2:17 ` Re: [PATCH] " MyungJoo Ham [not found] <CGME20161213170941epcas2p1267d3da4c7034777c8de117ed3c0f9fe@epcas2p1.samsung.com> 2016-12-14 0:45 ` MyungJoo Ham
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.