* [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
* 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
* 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: 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
* 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
* [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: [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.