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

[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

[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