[patch]hp_accel: Fix race in device removal

[patch]hp_accel: Fix race in device removal

[Oliver Neukum]

>From 64a9e0d8e585c65526248e6cf1659a13fb01a93a Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oliver@neukum.org>
Date: Fri, 7 May 2010 17:41:59 +0200
Subject: [PATCH] hp_accel: Fix race in device removal

The work queue has to be flushed after the device has been made
inaccessible.

Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
 drivers/hwmon/hp_accel.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
index c8ab505..6f6e05d 100644
--- a/drivers/hwmon/hp_accel.c
+++ b/drivers/hwmon/hp_accel.c
@@ -328,9 +328,9 @@ static int lis3lv02d_remove(struct acpi_device *device, int type)
 	lis3lv02d_joystick_disable();
 	lis3lv02d_poweroff(&lis3_dev);
 
-	flush_work(&hpled_led.work);
 	led_classdev_unregister(&hpled_led.led_classdev);
-
+	flush_work(&hpled_led.work);
+	
 	return lis3lv02d_remove_fs(&lis3_dev);
 }
 
-- 
1.6.4.2

[patch]hp_accel: Fix race in device removal

[Oliver Neukum]

>From 505056649f5ecb1ebb086981243b9870ce961a87 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oliver@neukum.org>
Date: Fri, 7 May 2010 18:02:07 +0200
Subject: [PATCH] hp_accel: Fix race in device removal

The work queue has to be flushed after the device has been made
inaccessible.

Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
 drivers/hwmon/hp_accel.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
index c8ab505..7580f55 100644
--- a/drivers/hwmon/hp_accel.c
+++ b/drivers/hwmon/hp_accel.c
@@ -328,8 +328,8 @@ static int lis3lv02d_remove(struct acpi_device *device, int type)
 	lis3lv02d_joystick_disable();
 	lis3lv02d_poweroff(&lis3_dev);
 
-	flush_work(&hpled_led.work);
 	led_classdev_unregister(&hpled_led.led_classdev);
+	flush_work(&hpled_led.work);
 
 	return lis3lv02d_remove_fs(&lis3_dev);
 }
-- 
1.6.4.2

Re: [patch]hp_accel: Fix race in device removal

[Pavel Machek]

On Fri 2010-05-07 18:04:20, Oliver Neukum wrote:
> From 505056649f5ecb1ebb086981243b9870ce961a87 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oliver@neukum.org>
> Date: Fri, 7 May 2010 18:02:07 +0200
> Subject: [PATCH] hp_accel: Fix race in device removal
> 
> The work queue has to be flushed after the device has been made
> inaccessible.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.de>

ACK.
> ---
>  drivers/hwmon/hp_accel.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
> index c8ab505..7580f55 100644
> --- a/drivers/hwmon/hp_accel.c
> +++ b/drivers/hwmon/hp_accel.c
> @@ -328,8 +328,8 @@ static int lis3lv02d_remove(struct acpi_device *device, int type)
>  	lis3lv02d_joystick_disable();
>  	lis3lv02d_poweroff(&lis3_dev);
>  
> -	flush_work(&hpled_led.work);
>  	led_classdev_unregister(&hpled_led.led_classdev);
> +	flush_work(&hpled_led.work);
>  
>  	return lis3lv02d_remove_fs(&lis3_dev);
>  }

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch]hp_accel: Fix race in device removal

[Éric Piel]

Op 07-05-10 18:04, Oliver Neukum schreef:
> From 505056649f5ecb1ebb086981243b9870ce961a87 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oliver@neukum.org>
> Date: Fri, 7 May 2010 18:02:07 +0200
> Subject: [PATCH] hp_accel: Fix race in device removal
> 
> The work queue has to be flushed after the device has been made
> inaccessible.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.de>
Ah, flushing the work before unregistering the device seemed more
logical... but now that you mention it, I understand the race.

Thanks

Acked-by: Eric Piel <eric.piel@tremplin-utc.net>


> ---
>  drivers/hwmon/hp_accel.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
> index c8ab505..7580f55 100644
> --- a/drivers/hwmon/hp_accel.c
> +++ b/drivers/hwmon/hp_accel.c
> @@ -328,8 +328,8 @@ static int lis3lv02d_remove(struct acpi_device *device, int type)
>  	lis3lv02d_joystick_disable();
>  	lis3lv02d_poweroff(&lis3_dev);
>  
> -	flush_work(&hpled_led.work);
>  	led_classdev_unregister(&hpled_led.led_classdev);
> +	flush_work(&hpled_led.work);
>  
>  	return lis3lv02d_remove_fs(&lis3_dev);
>  }

Re: [patch]hp_accel: Fix race in device removal

[Andrew Morton]

On Fri, 7 May 2010 18:04:20 +0200
Oliver Neukum <oneukum@suse.de> wrote:

> >From 505056649f5ecb1ebb086981243b9870ce961a87 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oliver@neukum.org>
> Date: Fri, 7 May 2010 18:02:07 +0200
> Subject: [PATCH] hp_accel: Fix race in device removal
> 
> The work queue has to be flushed after the device has been made
> inaccessible.
> 

You cc'ed stable@kernel.org so presumably you think it's a pretty
serious bug.

But alas, I (and presumably others) cannot confirm that, because you
didn't describe the bug :( This also means that if someone hits a
problem with this driver, they won't easily be able to work out whether
this patch will fix it.


Under what circumstances does it trigger?

What are the observed effects when it triggers?

Please always include this sort of information when sending bugfixes, thanks.

Re: [patch]hp_accel: Fix race in device removal

[Andrew Morton]

On Tue, 11 May 2010 07:55:07 +0200 Oliver Neukum <oneukum@suse.de> wrote:

> Am Dienstag, 11. Mai 2010 00:46:32 schrieb Andrew Morton:
> > But alas, I (and presumably others) cannot confirm that, because you
> > didn't describe the bug :( This also means that if someone hits a
> > problem with this driver, they won't easily be able to work out whether
> > this patch will fix it.
> > 
> > 
> > Under what circumstances does it trigger?
> > 
> > What are the observed effects when it triggers?
> > 
> > Please always include this sort of information when sending bugfixes, thanks.
> 
> Sorry,
> 
> the patch closes a window during which a work queue might remain
> active after the device is removed and would then lead to ACPI calls
> with undefined behavior.

Had anyone observed this in the field?  Was a machine crashing, perhaps?

Re: [patch]hp_accel: Fix race in device removal

[Oliver Neukum]

Am Dienstag, 11. Mai 2010 00:46:32 schrieb Andrew Morton:
> But alas, I (and presumably others) cannot confirm that, because you
> didn't describe the bug :( This also means that if someone hits a
> problem with this driver, they won't easily be able to work out whether
> this patch will fix it.
> 
> 
> Under what circumstances does it trigger?
> 
> What are the observed effects when it triggers?
> 
> Please always include this sort of information when sending bugfixes, thanks.

Sorry,

the patch closes a window during which a work queue might remain
active after the device is removed and would then lead to ACPI calls
with undefined behavior.

	Regards
		Oliver

Re: [patch]hp_accel: Fix race in device removal

[Oliver Neukum]

Am Dienstag, 11. Mai 2010 04:57:25 schrieb Andrew Morton:
> On Tue, 11 May 2010 07:55:07 +0200 Oliver Neukum <oneukum@suse.de> wrote:
> 
> > Am Dienstag, 11. Mai 2010 00:46:32 schrieb Andrew Morton:
> > > But alas, I (and presumably others) cannot confirm that, because you
> > > didn't describe the bug :( This also means that if someone hits a
> > > problem with this driver, they won't easily be able to work out whether
> > > this patch will fix it.
> > > 
> > > 
> > > Under what circumstances does it trigger?
> > > 
> > > What are the observed effects when it triggers?
> > > 
> > > Please always include this sort of information when sending bugfixes, thanks.
> > 
> > Sorry,
> > 
> > the patch closes a window during which a work queue might remain
> > active after the device is removed and would then lead to ACPI calls
> > with undefined behavior.
> 
> Had anyone observed this in the field?  Was a machine crashing, perhaps?

No, this was found during a code review.

	Regards
		Oliver