iwlwifi: fix oops in iwl3945_led_brightness_set

PATCH Superseded Marcin Slusarz 2 participants 4 messages Jun 8, 2008 Networking drivers (wireless)

Share & tools

Marcin Slusarz

author

2008-06-08 08:49 UTC #0 lore raw

PATCH 1/1 iwlwifi: fix oops in iwl3945_led_brightness_set

fix race between:
ieee80211_open->ieee80211_led_radio->led_trigger_event->led_set_brightness->iwl3945_led_brightness_set
(which assumes that "led->priv" is not NULL)
and iwl3945_pci_probe->iwl3945_setup_deferred_work->(...)->iwl3945_bg_alive_start->iwl3945_alive_start->iwl3945_led_register->iwl3945_led_register_led which sets priv field in struct iwl3945_led
after
led->led_dev.brightness_set = iwl3945_led_brightness_set;
(...)
led_classdev_register(device, &led->led_dev);

http://kerneloops.org/guilty.php?guilty=iwl3945_led_brightness_set&version=2.6.25-release&start=1671168&end=1703935&class=oops

Signed-off-by Marcin Slusarz <[email protected]>

Cc Zhu Yi <[email protected]>

Cc Reinette Chatre <[email protected]>

Cc Tomas Winkler <[email protected]>

Cc John W. Linville <[email protected]>

Cc [email protected]

---

Compile tested only. Please review and test on real hardware.

---
 drivers/net/wireless/iwlwifi/iwl-3945-led.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/net/wireless/iwlwifi/iwl-3945-led.c +2 -0

index d200d08..9f08568 100644
@@ -185,6 +185,8 @@ static void iwl3945_led_brightness_set(struct led_classdev *led_cdev,
 					       struct iwl3945_led, led_dev);
 	struct iwl3945_priv *priv = led->priv;
 
+	if (!led->registered)
+		return;
 	if (test_bit(STATUS_EXIT_PENDING, &priv->status))
 		return;

Signature · 1 line

1.5.4.5

Tomas Winkler

NAK

2008-06-08 10:18 UTC #1 lore raw

Quoted 42 lines · Marcin Slusarz

fix race between:
ieee80211_open->ieee80211_led_radio->led_trigger_event->led_set_brightness->iwl3945_led_brightness_set
(which assumes that "led->priv" is not NULL)
and iwl3945_pci_probe->iwl3945_setup_deferred_work->(...)->iwl3945_bg_alive_start->iwl3945_alive_start->iwl3945_led_register->iwl3945_led_register_led which sets priv field in struct iwl3945_led
after
led->led_dev.brightness_set = iwl3945_led_brightness_set;
(...)
led_classdev_register(device, &led->led_dev);
http://kerneloops.org/guilty.php?guilty=iwl3945_led_brightness_set&version=2.6.25-release&start=1671168&end=1703935&class=oops
Signed-off-by Marcin Slusarz <[email protected]>
Cc Zhu Yi <[email protected]>
Cc Reinette Chatre <[email protected]>
Cc Tomas Winkler <[email protected]>
Cc John W. Linville <[email protected]>
Cc [email protected]
Cc [email protected]
---
Compile tested only. Please review and test on real hardware.
---
 drivers/net/wireless/iwlwifi/iwl-3945-led.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/wireless/iwlwifi/iwl-3945-led.c +2 -1
index d200d08..9f08568 100644
@@ -185,6 +185,8 @@ static void iwl3945_led_brightness_set(struct led_classdev *led_cdev,
                                              struct iwl3945_led, led_dev);
       struct iwl3945_priv *priv = led->priv;
 
+       if (!led->registered)
+               return;
       if (test_bit(STATUS_EXIT_PENDING, &priv->status))
               return;
 
--

Quoted 2 lines

1.5.4.5

NACK
Just need to revert the order in registration command as done in iwl_leds_register_led.
Let me know if you want to provide that fix

Thanks
Tomas

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Marcin Slusarz

author

2008-06-08 11:13 UTC #2 lore raw

PATCH v2 1/1 iwlwifi: fix oops in iwl3945_led_brightness_set

Quoted 43 lines · Tomas Winkler

Quoted 41 lines · Marcin Slusarz
fix race between:
ieee80211_open->ieee80211_led_radio->led_trigger_event->led_set_brightness->iwl3945_led_brightness_set
(which assumes that "led->priv" is not NULL)
and iwl3945_pci_probe->iwl3945_setup_deferred_work->(...)->iwl3945_bg_alive_start->iwl3945_alive_start->iwl3945_led_register->iwl3945_led_register_led which sets priv field in struct iwl3945_led
after
led->led_dev.brightness_set = iwl3945_led_brightness_set;
(...)
led_classdev_register(device, &led->led_dev);
http://kerneloops.org/guilty.php?guilty=iwl3945_led_brightness_set&version=2.6.25-release&start=1671168&end=1703935&class=oops
Signed-off-by Marcin Slusarz <[email protected]>
Cc Zhu Yi <[email protected]>
Cc Reinette Chatre <[email protected]>
Cc Tomas Winkler <[email protected]>
Cc John W. Linville <[email protected]>
Cc [email protected]
Cc [email protected]
---
Compile tested only. Please review and test on real hardware.
---
 drivers/net/wireless/iwlwifi/iwl-3945-led.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/wireless/iwlwifi/iwl-3945-led.c +2 -0
index d200d08..9f08568 100644
@@ -185,6 +185,8 @@ static void iwl3945_led_brightness_set(struct led_classdev *led_cdev,
                                              struct iwl3945_led, led_dev);
       struct iwl3945_priv *priv = led->priv;
 
+       if (!led->registered)
+               return;
       if (test_bit(STATUS_EXIT_PENDING, &priv->status))
               return;
 

Quoted 6 lines

Quoted 3 lines
--
1.5.4.5
NACK
Just need to revert the order in registration command as done in iwl_leds_register_led.

Thanks for a review!
Updated patch below.

---
fix race between:
ieee80211_open->ieee80211_led_radio->led_trigger_event->led_set_brightness->iwl3945_led_brightness_set
(which assumes that "led->priv" is not NULL)
and iwl3945_pci_probe->iwl3945_setup_deferred_work->(...)->iwl3945_bg_alive_start->iwl3945_alive_start->iwl3945_led_register->iwl3945_led_register_led which sets priv field in struct iwl3945_led
after
led->led_dev.brightness_set = iwl3945_led_brightness_set;
(...)
led_classdev_register(device, &led->led_dev);

http://kerneloops.org/guilty.php?guilty=iwl3945_led_brightness_set&version=2.6.25-release&start=1671168&end=1703935&class=oops

Signed-off-by Marcin Slusarz <[email protected]>

Cc Zhu Yi <[email protected]>

Cc Reinette Chatre <[email protected]>

Cc Tomas Winkler <[email protected]>

Cc John W. Linville <[email protected]>

Cc [email protected]

---
 drivers/net/wireless/iwlwifi/iwl-3945-led.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

drivers/net/wireless/iwlwifi/iwl-3945-led.c +3 -2

index d200d08..8b1528e 100644
@@ -229,14 +229,15 @@ static int iwl3945_led_register_led(struct iwl3945_priv *priv,
 	led->led_dev.brightness_set = iwl3945_led_brightness_set;
 	led->led_dev.default_trigger = trigger;
 
+	led->priv = priv;
+	led->type = type;
+
 	ret = led_classdev_register(device, &led->led_dev);
 	if (ret) {
 		IWL_ERROR("Error: failed to register led handler.\n");
 		return ret;
 	}
 
-	led->priv = priv;
-	led->type = type;
 	led->registered = 1;
 
 	if (set_led && led->led_on)

Signature · 1 line

1.5.4.5

Tomas Winkler

2008-06-08 15:33 UTC #3 lore raw

Quoted 102 lines · Marcin Slusarz

Quoted 43 lines · Tomas Winkler
Quoted 41 lines · Marcin Slusarz
fix race between:
ieee80211_open->ieee80211_led_radio->led_trigger_event->led_set_brightness->iwl3945_led_brightness_set
(which assumes that "led->priv" is not NULL)
and iwl3945_pci_probe->iwl3945_setup_deferred_work->(...)->iwl3945_bg_alive_start->iwl3945_alive_start->iwl3945_led_register->iwl3945_led_register_led which sets priv field in struct iwl3945_led
after
led->led_dev.brightness_set = iwl3945_led_brightness_set;
(...)
led_classdev_register(device, &led->led_dev);
http://kerneloops.org/guilty.php?guilty=iwl3945_led_brightness_set&version=2.6.25-release&start=1671168&end=1703935&class=oops
Signed-off-by Marcin Slusarz <[email protected]>
Cc Zhu Yi <[email protected]>
Cc Reinette Chatre <[email protected]>
Cc Tomas Winkler <[email protected]>
Cc John W. Linville <[email protected]>
Cc [email protected]
Cc [email protected]
---
Compile tested only. Please review and test on real hardware.
---
 drivers/net/wireless/iwlwifi/iwl-3945-led.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/wireless/iwlwifi/iwl-3945-led.c +2 -0
index d200d08..9f08568 100644
@@ -185,6 +185,8 @@ static void iwl3945_led_brightness_set(struct led_classdev *led_cdev,
                                              struct iwl3945_led, led_dev);
       struct iwl3945_priv *priv = led->priv;
 
+       if (!led->registered)
+               return;
       if (test_bit(STATUS_EXIT_PENDING, &priv->status))
               return;
 
Quoted 6 lines
Quoted 3 lines
--
1.5.4.5
NACK
Just need to revert the order in registration command as done in iwl_leds_register_led.
Thanks for a review!
Updated patch below.
---
fix race between:
ieee80211_open->ieee80211_led_radio->led_trigger_event->led_set_brightness->iwl3945_led_brightness_set
(which assumes that "led->priv" is not NULL)
and iwl3945_pci_probe->iwl3945_setup_deferred_work->(...)->iwl3945_bg_alive_start->iwl3945_alive_start->iwl3945_led_register->iwl3945_led_register_led which sets priv field in struct iwl3945_led
after
led->led_dev.brightness_set = iwl3945_led_brightness_set;
(...)
led_classdev_register(device, &led->led_dev);
http://kerneloops.org/guilty.php?guilty=iwl3945_led_brightness_set&version=2.6.25-release&start=1671168&end=1703935&class=oops
Signed-off-by Marcin Slusarz <[email protected]>
Cc Zhu Yi <[email protected]>
Cc Reinette Chatre <[email protected]>
Cc Tomas Winkler <[email protected]>
Cc John W. Linville <[email protected]>
Cc [email protected]
Cc [email protected]
---
 drivers/net/wireless/iwlwifi/iwl-3945-led.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/wireless/iwlwifi/iwl-3945-led.c +3 -3
index d200d08..8b1528e 100644
@@ -229,14 +229,15 @@ static int iwl3945_led_register_led(struct iwl3945_priv *priv,
       led->led_dev.brightness_set = iwl3945_led_brightness_set;
       led->led_dev.default_trigger = trigger;
 
+       led->priv = priv;
+       led->type = type;
+
       ret = led_classdev_register(device, &led->led_dev);
       if (ret) {
               IWL_ERROR("Error: failed to register led handler.\n");
               return ret;
       }
 
-       led->priv = priv;
-       led->type = type;
       led->registered = 1;
 
       if (set_led && led->led_on)
--

Quoted 1 line

1.5.4.5

ACK

Tomas