[PATCH] PM / devfreq: Don't delete sysfs group twice

[PATCH] PM / devfreq: Don't delete sysfs group twice

[Chris Diamand]

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

Re: [PATCH] PM / devfreq: Don't delete sysfs group twice

[Chanwoo Choi]

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

Re: [PATCH] PM / devfreq: Don't delete sysfs group twice

[Chanwoo Choi]

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

Re: [PATCH] PM / devfreq: Don't delete sysfs group twice

[Chanwoo Choi]

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

Re: [PATCH] PM / devfreq: Don't delete sysfs group twice

[Sudeep Holla]

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

Re: [PATCH] PM / devfreq: Don't delete sysfs group twice

[Chanwoo Choi]

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

Re: [PATCH] PM / devfreq: Don't delete sysfs group twice

[Chris Diamand]

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

Re: [PATCH] PM / devfreq: Don't delete sysfs group twice

[Chanwoo Choi]

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

Re: [PATCH] PM / devfreq: Don't delete sysfs group twice

[Chris Diamand]

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

Re: [PATCH] PM / devfreq: Don't delete sysfs group twice

[Chanwoo Choi]

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

[PATCH v2] PM / devfreq: Don't delete sysfs group twice

[Chris Diamand]

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

RE: Re: [PATCH] PM / devfreq: Don't delete sysfs group twice

[MyungJoo Ham]

[-- 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

[PATCH v2] PM / devfreq: Don't delete sysfs group twice

[chris.diamand]

From: Chris Diamand <chris.diamand@arm.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>
---
Hi all - this is a resend of my updated devfreq removal fix.

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.

Cheers!
Chris

 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

Re: [PATCH v2] PM / devfreq: Don't delete sysfs group twice

[Chanwoo Choi]

Hi Chris,

Looks good to me.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

On 2017년 01월 12일 23:57, chris.diamand@arm.com wrote:
> From: Chris Diamand <chris.diamand@arm.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>
> ---
> Hi all - this is a resend of my updated devfreq removal fix.
> 
> 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.
> 
> Cheers!
> Chris
> 
>  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;
>  }
> 


-- 
Best Regards,
Chanwoo Choi
S/W Center, Samsung Electronics

RE: [PATCH v2] PM / devfreq: Don't delete sysfs group twice

[MyungJoo Ham]

[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]

> From: Chris Diamand <chris.diamand@arm.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>
> ---
> Hi all - this is a resend of my updated devfreq removal fix.
> 
> 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.
> 
> Cheers!
> Chris

Ok, I'll update 
https://git.kernel.org/cgit/linux/kernel/git/mzx/devfreq.git/commit/?h=for-4.10-rc&id=1eb32280a81463b6e1e38e2d261710e0d7fd5dcf
with this one.

Cheers,
MyungJoo

Re: [PATCH v2] PM / devfreq: Don't delete sysfs group twice

[Chris Diamand]

On Fri, Jan 13, 2017 at 12:18:39AM +0000, Chanwoo Choi wrote:
> Hi Chris,
>
> Looks good to me.
>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

On Fri, Jan 13, 2017 at 01:15:06AM +0000, MyungJoo Ham wrote:
> Ok, I'll update
> https://git.kernel.org/cgit/linux/kernel/git/mzx/devfreq.git/commit/?h=for-4.10-rc&id=1eb32280a81463b6e1e38e2d261710e0d7fd5dcf
> with this one.

Thanks both!

Cheers,
Chris