This patch fixes the iwlwifi rfkill. It removes the input device from iwl3945,
adds support for RFKILL_STATE_HARD_BLOCKED and calls rfkill_force_state() to
update the state rather than accessing it directly.
The calls to iwl|iwl3945_rfkill_set_hw_state() had to be moved because
rfkill_force_state() cannot be called from an atomic context.
Tested on iwl3945 and seems to work fine.
Cc: Randy Dunlap <[email protected]>
Cc: Ivo van Doorn <[email protected]>
Cc: Fabien Crespel <[email protected]>
Cc: Zhu Yi <[email protected]>
Cc: John W. Linville <[email protected]>
Cc: Henrique de Moraes Holschuh <[email protected]>
Signed-off-by: Adel Gadllah <[email protected]>
---
drivers/net/wireless/iwlwifi/Kconfig | 5 +
drivers/net/wireless/iwlwifi/iwl-3945.h | 11 --
drivers/net/wireless/iwlwifi/iwl-core.h | 13 ++-
drivers/net/wireless/iwlwifi/iwl-dev.h | 2
drivers/net/wireless/iwlwifi/iwl-rfkill.c | 63 +++++++--------
drivers/net/wireless/iwlwifi/iwl-rfkill.h | 3
drivers/net/wireless/iwlwifi/iwl3945-base.c | 117 +++++++++++-----------------
drivers/net/wireless/iwlwifi/iwl4965-base.c | 8 -
8 files changed, 106 insertions(+), 116 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/Kconfig
b/drivers/net/wireless/iwlwifi/Kconfig
index b628a44..8305de6 100644
--- a/drivers/net/wireless/iwlwifi/Kconfig
+++ b/drivers/net/wireless/iwlwifi/Kconfig
@@ -104,6 +104,7 @@ config IWL3945
select IWLWIFI
select MAC80211_LEDS if IWL3945_LEDS
select LEDS_CLASS if IWL3945_LEDS
+ select RFKILL if IWL3945_RFKILL
---help---
Select to build the driver supporting the:
@@ -126,6 +127,10 @@ config IWL3945
say M here and read <file:Documentation/kbuild/modules.txt>. The
module will be called iwl3945.ko.
+config IWL3945_RFKILL
+ bool "Enable RF kill support in iwl3945 drivers"
+ depends on IWL3945
+
config IWL3945_SPECTRUM_MEASUREMENT
bool "Enable Spectrum Measurement in iwl3945 drivers"
depends on IWL3945
diff --git a/drivers/net/wireless/iwlwifi/iwl-3945.h
b/drivers/net/wireless/iwlwifi/iwl-3945.h
index a774978..9c0a09e 100644
--- a/drivers/net/wireless/iwlwifi/iwl-3945.h
+++ b/drivers/net/wireless/iwlwifi/iwl-3945.h
@@ -690,14 +690,9 @@ enum {
#endif
-#ifdef CONFIG_IWLWIFI_RFKILL
+#ifdef CONFIG_IWL3945_RFKILL
struct iwl3945_priv;
-struct iwl3945_rfkill_mngr {
- struct rfkill *rfkill;
- struct input_dev *input_dev;
-};
-
void iwl3945_rfkill_set_hw_state(struct iwl3945_priv *priv);
void iwl3945_rfkill_unregister(struct iwl3945_priv *priv);
int iwl3945_rfkill_init(struct iwl3945_priv *priv);
@@ -800,8 +795,8 @@ struct iwl3945_priv {
struct iwl3945_init_alive_resp card_alive_init;
struct iwl3945_alive_resp card_alive;
-#ifdef CONFIG_IWLWIFI_RFKILL
- struct iwl3945_rfkill_mngr rfkill_mngr;
+#ifdef CONFIG_IWL3945_RFKILL
+ struct rfkill *rfkill;
#endif
#ifdef CONFIG_IWL3945_LEDS
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.h
b/drivers/net/wireless/iwlwifi/iwl-core.h
index eb4abe1..dafd62c 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.h
+++ b/drivers/net/wireless/iwlwifi/iwl-core.h
@@ -356,10 +356,19 @@ static inline int iwl_is_init(struct iwl_priv *priv)
return test_bit(STATUS_INIT, &priv->status);
}
+static inline int iwl_is_rfkill_sw(struct iwl_priv *priv)
+{
+ return test_bit(STATUS_RF_KILL_SW, &priv->status);
+}
+
+static inline int iwl_is_rfkill_hw(struct iwl_priv *priv)
+{
+ return test_bit(STATUS_RF_KILL_HW, &priv->status);
+}
+
static inline int iwl_is_rfkill(struct iwl_priv *priv)
{
- return test_bit(STATUS_RF_KILL_HW, &priv->status) ||
- test_bit(STATUS_RF_KILL_SW, &priv->status);
+ return iwl_is_rfkill_hw(priv) || iwl_is_rfkill_sw(priv);
}
static inline int iwl_is_ready_rf(struct iwl_priv *priv)
diff --git a/drivers/net/wireless/iwlwifi/iwl-dev.h
b/drivers/net/wireless/iwlwifi/iwl-dev.h
index d1289cf..70e10ea 100644
--- a/drivers/net/wireless/iwlwifi/iwl-dev.h
+++ b/drivers/net/wireless/iwlwifi/iwl-dev.h
@@ -929,7 +929,7 @@ struct iwl_priv {
struct iwl_init_alive_resp card_alive_init;
struct iwl_alive_resp card_alive;
#ifdef CONFIG_IWLWIFI_RFKILL
- struct iwl_rfkill_mngr rfkill_mngr;
+ struct rfkill *rfkill;
#endif
#ifdef CONFIG_IWLWIFI_LEDS
diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
index aa9f31e..2b9a009 100644
--- a/drivers/net/wireless/iwlwifi/iwl-rfkill.c
+++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.c
@@ -44,7 +44,7 @@ static int iwl_rfkill_soft_rf_kill(void *data, enum
rfkill_state state)
struct iwl_priv *priv = data;
int err = 0;
- if (!priv->rfkill_mngr.rfkill)
+ if (!priv->rfkill)
return 0;
if (test_bit(STATUS_EXIT_PENDING, &priv->status))
@@ -55,20 +55,20 @@ static int iwl_rfkill_soft_rf_kill(void *data,
enum rfkill_state state)
switch (state) {
case RFKILL_STATE_UNBLOCKED:
- iwl_radio_kill_sw_enable_radio(priv);
- /* if HW rf-kill is set dont allow ON state */
- if (iwl_is_rfkill(priv))
+ if (iwl_is_rfkill_hw(priv)) {
err = -EBUSY;
+ goto out_unlock;
+ }
+ iwl_radio_kill_sw_enable_radio(priv);
break;
case RFKILL_STATE_SOFT_BLOCKED:
iwl_radio_kill_sw_disable_radio(priv);
- if (!iwl_is_rfkill(priv))
- err = -EBUSY;
break;
default:
IWL_WARNING("we recieved unexpected RFKILL state %d\n", state);
break;
}
+out_unlock:
mutex_unlock(&priv->mutex);
return err;
@@ -82,39 +82,35 @@ int iwl_rfkill_init(struct iwl_priv *priv)
BUG_ON(device == NULL);
IWL_DEBUG_RF_KILL("Initializing RFKILL.\n");
- priv->rfkill_mngr.rfkill = rfkill_allocate(device, RFKILL_TYPE_WLAN);
- if (!priv->rfkill_mngr.rfkill) {
+ priv->rfkill = rfkill_allocate(device, RFKILL_TYPE_WLAN);
+ if (!priv->rfkill) {
IWL_ERROR("Unable to allocate rfkill device.\n");
ret = -ENOMEM;
goto error;
}
- priv->rfkill_mngr.rfkill->name = priv->cfg->name;
- priv->rfkill_mngr.rfkill->data = priv;
- priv->rfkill_mngr.rfkill->state = RFKILL_STATE_ON;
- priv->rfkill_mngr.rfkill->toggle_radio = iwl_rfkill_soft_rf_kill;
- priv->rfkill_mngr.rfkill->user_claim_unsupported = 1;
+ priv->rfkill->name = priv->cfg->name;
+ priv->rfkill->data = priv;
+ priv->rfkill->state = RFKILL_STATE_UNBLOCKED;
+ priv->rfkill->toggle_radio = iwl_rfkill_soft_rf_kill;
+ priv->rfkill->user_claim_unsupported = 1;
- priv->rfkill_mngr.rfkill->dev.class->suspend = NULL;
- priv->rfkill_mngr.rfkill->dev.class->resume = NULL;
+ priv->rfkill->dev.class->suspend = NULL;
+ priv->rfkill->dev.class->resume = NULL;
- ret = rfkill_register(priv->rfkill_mngr.rfkill);
+ ret = rfkill_register(priv->rfkill);
if (ret) {
IWL_ERROR("Unable to register rfkill: %d\n", ret);
- goto unregister_rfkill;
+ goto freed_rfkill;
}
IWL_DEBUG_RF_KILL("RFKILL initialization complete.\n");
return ret;
-unregister_rfkill:
- rfkill_unregister(priv->rfkill_mngr.rfkill);
- priv->rfkill_mngr.rfkill = NULL;
-
freed_rfkill:
- if (priv->rfkill_mngr.rfkill != NULL)
- rfkill_free(priv->rfkill_mngr.rfkill);
- priv->rfkill_mngr.rfkill = NULL;
+ if (priv->rfkill != NULL)
+ rfkill_free(priv->rfkill);
+ priv->rfkill = NULL;
error:
IWL_DEBUG_RF_KILL("RFKILL initialization complete.\n");
@@ -125,22 +121,27 @@ EXPORT_SYMBOL(iwl_rfkill_init);
void iwl_rfkill_unregister(struct iwl_priv *priv)
{
- if (priv->rfkill_mngr.rfkill)
- rfkill_unregister(priv->rfkill_mngr.rfkill);
+ if (priv->rfkill)
+ rfkill_unregister(priv->rfkill);
- priv->rfkill_mngr.rfkill = NULL;
+ priv->rfkill = NULL;
}
EXPORT_SYMBOL(iwl_rfkill_unregister);
/* set rf-kill to the right state. */
void iwl_rfkill_set_hw_state(struct iwl_priv *priv)
{
- if (!priv->rfkill_mngr.rfkill)
+ if (!priv->rfkill)
return;
- if (!iwl_is_rfkill(priv))
- priv->rfkill_mngr.rfkill->state = RFKILL_STATE_ON;
+ if (iwl_is_rfkill_hw(priv)) {
+ rfkill_force_state(priv->rfkill, RFKILL_STATE_HARD_BLOCKED);
+ return;
+ }
+
+ if (!iwl_is_rfkill_sw(priv))
+ rfkill_force_state(priv->rfkill, RFKILL_STATE_UNBLOCKED);
else
- priv->rfkill_mngr.rfkill->state = RFKILL_STATE_OFF;
+ rfkill_force_state(priv->rfkill, RFKILL_STATE_SOFT_BLOCKED);
}
EXPORT_SYMBOL(iwl_rfkill_set_hw_state);
diff --git a/drivers/net/wireless/iwlwifi/iwl-rfkill.h
b/drivers/net/wireless/iwlwifi/iwl-rfkill.h
index 00692d2..402fd4c 100644
--- a/drivers/net/wireless/iwlwifi/iwl-rfkill.h
+++ b/drivers/net/wireless/iwlwifi/iwl-rfkill.h
@@ -33,9 +33,6 @@ struct iwl_priv;
#include <linux/rfkill.h>
#ifdef CONFIG_IWLWIFI_RFKILL
-struct iwl_rfkill_mngr {
- struct rfkill *rfkill;
-};
void iwl_rfkill_set_hw_state(struct iwl_priv *priv);
void iwl_rfkill_unregister(struct iwl_priv *priv);
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c
b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 3bc2644..f27c9fd 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -537,10 +537,20 @@ static inline int iwl3945_is_init(struct
iwl3945_priv *priv)
return test_bit(STATUS_INIT, &priv->status);
}
+static inline int iwl3945_is_rfkill_sw(struct iwl3945_priv *priv)
+{
+ return test_bit(STATUS_RF_KILL_SW, &priv->status);
+}
+
+static inline int iwl3945_is_rfkill_hw(struct iwl3945_priv *priv)
+{
+ return test_bit(STATUS_RF_KILL_HW, &priv->status);
+}
+
static inline int iwl3945_is_rfkill(struct iwl3945_priv *priv)
{
- return test_bit(STATUS_RF_KILL_HW, &priv->status) ||
- test_bit(STATUS_RF_KILL_SW, &priv->status);
+ return iwl3945_is_rfkill_hw(priv) ||
+ iwl3945_is_rfkill_sw(priv);
}
static inline int iwl3945_is_ready_rf(struct iwl3945_priv *priv)
@@ -6013,12 +6023,11 @@ static int __iwl3945_up(struct iwl3945_priv *priv)
else {
set_bit(STATUS_RF_KILL_HW, &priv->status);
if (!test_bit(STATUS_IN_SUSPEND, &priv->status)) {
- iwl3945_rfkill_set_hw_state(priv);
IWL_WARNING("Radio disabled by HW RF Kill switch\n");
return -ENODEV;
}
}
- iwl3945_rfkill_set_hw_state(priv);
+
iwl3945_write32(priv, CSR_INT, 0xFFFFFFFF);
rc = iwl3945_hw_nic_init(priv);
@@ -6143,8 +6152,8 @@ static void iwl3945_bg_rf_kill(struct work_struct *work)
"wireless networking to work.\n");
}
- iwl3945_rfkill_set_hw_state(priv);
mutex_unlock(&priv->mutex);
+ iwl3945_rfkill_set_hw_state(priv);
}
static void iwl3945_bg_set_monitor(struct work_struct *work)
@@ -6398,6 +6407,7 @@ static void iwl3945_bg_up(struct work_struct *data)
mutex_lock(&priv->mutex);
__iwl3945_up(priv);
mutex_unlock(&priv->mutex);
+ iwl3945_rfkill_set_hw_state(priv);
}
static void iwl3945_bg_restart(struct work_struct *data)
@@ -6618,6 +6628,8 @@ static int iwl3945_mac_start(struct ieee80211_hw *hw)
mutex_unlock(&priv->mutex);
+ iwl3945_rfkill_set_hw_state(priv);
+
if (ret)
goto out_release_irq;
@@ -8270,14 +8282,14 @@ static int iwl3945_pci_resume(struct pci_dev *pdev)
#endif /* CONFIG_PM */
/*************** RFKILL FUNCTIONS **********/
-#ifdef CONFIG_IWLWIFI_RFKILL
+#ifdef CONFIG_IWL3945_RFKILL
/* software rf-kill from user */
static int iwl3945_rfkill_soft_rf_kill(void *data, enum rfkill_state state)
{
struct iwl3945_priv *priv = data;
int err = 0;
- if (!priv->rfkill_mngr.rfkill)
+ if (!priv->rfkill)
return 0;
if (test_bit(STATUS_EXIT_PENDING, &priv->status))
@@ -8288,20 +8300,20 @@ static int iwl3945_rfkill_soft_rf_kill(void
*data, enum rfkill_state state)
switch (state) {
case RFKILL_STATE_UNBLOCKED:
- iwl3945_radio_kill_sw(priv, 0);
- /* if HW rf-kill is set dont allow ON state */
- if (iwl3945_is_rfkill(priv))
+ if (iwl3945_is_rfkill_hw(priv)) {
err = -EBUSY;
+ goto out_unlock;
+ }
+ iwl3945_radio_kill_sw(priv, 0);
break;
case RFKILL_STATE_SOFT_BLOCKED:
iwl3945_radio_kill_sw(priv, 1);
- if (!iwl3945_is_rfkill(priv))
- err = -EBUSY;
break;
default:
IWL_WARNING("we recieved unexpected RFKILL state %d\n", state);
break;
}
+out_unlock:
mutex_unlock(&priv->mutex);
return err;
@@ -8315,64 +8327,35 @@ int iwl3945_rfkill_init(struct iwl3945_priv *priv)
BUG_ON(device == NULL);
IWL_DEBUG_RF_KILL("Initializing RFKILL.\n");
- priv->rfkill_mngr.rfkill = rfkill_allocate(device, RFKILL_TYPE_WLAN);
- if (!priv->rfkill_mngr.rfkill) {
+ priv->rfkill = rfkill_allocate(device, RFKILL_TYPE_WLAN);
+ if (!priv->rfkill) {
IWL_ERROR("Unable to allocate rfkill device.\n");
ret = -ENOMEM;
goto error;
}
- priv->rfkill_mngr.rfkill->name = priv->cfg->name;
- priv->rfkill_mngr.rfkill->data = priv;
- priv->rfkill_mngr.rfkill->state = RFKILL_STATE_ON;
- priv->rfkill_mngr.rfkill->toggle_radio = iwl3945_rfkill_soft_rf_kill;
- priv->rfkill_mngr.rfkill->user_claim_unsupported = 1;
-
- priv->rfkill_mngr.rfkill->dev.class->suspend = NULL;
- priv->rfkill_mngr.rfkill->dev.class->resume = NULL;
+ priv->rfkill->name = priv->cfg->name;
+ priv->rfkill->data = priv;
+ priv->rfkill->state = RFKILL_STATE_UNBLOCKED;
+ priv->rfkill->toggle_radio = iwl3945_rfkill_soft_rf_kill;
+ priv->rfkill->user_claim_unsupported = 1;
- priv->rfkill_mngr.input_dev = input_allocate_device();
- if (!priv->rfkill_mngr.input_dev) {
- IWL_ERROR("Unable to allocate rfkill input device.\n");
- ret = -ENOMEM;
- goto freed_rfkill;
- }
+ priv->rfkill->dev.class->suspend = NULL;
+ priv->rfkill->dev.class->resume = NULL;
- priv->rfkill_mngr.input_dev->name = priv->cfg->name;
- priv->rfkill_mngr.input_dev->phys = wiphy_name(priv->hw->wiphy);
- priv->rfkill_mngr.input_dev->id.bustype = BUS_HOST;
- priv->rfkill_mngr.input_dev->id.vendor = priv->pci_dev->vendor;
- priv->rfkill_mngr.input_dev->dev.parent = device;
- priv->rfkill_mngr.input_dev->evbit[0] = BIT(EV_KEY);
- set_bit(KEY_WLAN, priv->rfkill_mngr.input_dev->keybit);
-
- ret = rfkill_register(priv->rfkill_mngr.rfkill);
+ ret = rfkill_register(priv->rfkill);
if (ret) {
IWL_ERROR("Unable to register rfkill: %d\n", ret);
- goto free_input_dev;
- }
-
- ret = input_register_device(priv->rfkill_mngr.input_dev);
- if (ret) {
- IWL_ERROR("Unable to register rfkill input device: %d\n", ret);
- goto unregister_rfkill;
+ goto freed_rfkill;
}
IWL_DEBUG_RF_KILL("RFKILL initialization complete.\n");
return ret;
-unregister_rfkill:
- rfkill_unregister(priv->rfkill_mngr.rfkill);
- priv->rfkill_mngr.rfkill = NULL;
-
-free_input_dev:
- input_free_device(priv->rfkill_mngr.input_dev);
- priv->rfkill_mngr.input_dev = NULL;
-
freed_rfkill:
- if (priv->rfkill_mngr.rfkill != NULL)
- rfkill_free(priv->rfkill_mngr.rfkill);
- priv->rfkill_mngr.rfkill = NULL;
+ if (priv->rfkill != NULL)
+ rfkill_free(priv->rfkill);
+ priv->rfkill = NULL;
error:
IWL_DEBUG_RF_KILL("RFKILL initialization complete.\n");
@@ -8381,28 +8364,28 @@ error:
void iwl3945_rfkill_unregister(struct iwl3945_priv *priv)
{
+ if (priv->rfkill)
+ rfkill_unregister(priv->rfkill);
- if (priv->rfkill_mngr.input_dev)
- input_unregister_device(priv->rfkill_mngr.input_dev);
-
- if (priv->rfkill_mngr.rfkill)
- rfkill_unregister(priv->rfkill_mngr.rfkill);
-
- priv->rfkill_mngr.input_dev = NULL;
- priv->rfkill_mngr.rfkill = NULL;
+ priv->rfkill = NULL;
}
/* set rf-kill to the right state. */
void iwl3945_rfkill_set_hw_state(struct iwl3945_priv *priv)
{
- if (!priv->rfkill_mngr.rfkill)
+ if (!priv->rfkill)
+ return;
+
+ if (iwl3945_is_rfkill_hw(priv)) {
+ rfkill_force_state(priv->rfkill, RFKILL_STATE_HARD_BLOCKED);
return;
+ }
- if (!iwl3945_is_rfkill(priv))
- priv->rfkill_mngr.rfkill->state = RFKILL_STATE_ON;
+ if (!iwl3945_is_rfkill_sw(priv))
+ rfkill_force_state(priv->rfkill, RFKILL_STATE_UNBLOCKED);
else
- priv->rfkill_mngr.rfkill->state = RFKILL_STATE_OFF;
+ rfkill_force_state(priv->rfkill, RFKILL_STATE_SOFT_BLOCKED);
}
#endif
diff --git a/drivers/net/wireless/iwlwifi/iwl4965-base.c
b/drivers/net/wireless/iwlwifi/iwl4965-base.c
index 60b7a64..7f65d91 100644
--- a/drivers/net/wireless/iwlwifi/iwl4965-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl4965-base.c
@@ -2187,13 +2187,11 @@ static int __iwl4965_up(struct iwl_priv *priv)
if (!test_bit(STATUS_IN_SUSPEND, &priv->status) &&
iwl_is_rfkill(priv)) {
- iwl_rfkill_set_hw_state(priv);
IWL_WARNING("Radio disabled by %s RF Kill switch\n",
test_bit(STATUS_RF_KILL_HW, &priv->status) ? "HW" : "SW");
return -ENODEV;
}
- iwl_rfkill_set_hw_state(priv);
iwl_write32(priv, CSR_INT, 0xFFFFFFFF);
ret = priv->cfg->ops->lib->alloc_shared_mem(priv);
@@ -2330,9 +2328,8 @@ static void iwl4965_bg_rf_kill(struct work_struct *work)
"Kill switch must be turned off for "
"wireless networking to work.\n");
}
- iwl_rfkill_set_hw_state(priv);
-
mutex_unlock(&priv->mutex);
+ iwl_rfkill_set_hw_state(priv);
}
static void iwl4965_bg_set_monitor(struct work_struct *work)
@@ -2390,6 +2387,7 @@ static void iwl4965_bg_up(struct work_struct *data)
mutex_lock(&priv->mutex);
__iwl4965_up(priv);
mutex_unlock(&priv->mutex);
+ iwl_rfkill_set_hw_state(priv);
}
static void iwl4965_bg_restart(struct work_struct *data)
@@ -2604,6 +2602,8 @@ static int iwl4965_mac_start(struct ieee80211_hw *hw)
mutex_unlock(&priv->mutex);
+ iwl_rfkill_set_hw_state(priv);
+
if (ret)
goto out_release_irq;
On Tue, 01 Jul 2008, Adel Gadllah wrote:
> The calls to iwl|iwl3945_rfkill_set_hw_state() had to be moved because
> rfkill_force_state() cannot be called from an atomic context.
Yeah, the joys of mutexes. If this is going to be a severe annoyance to
drivers, I don't see why rfkill could not be changed to use some other
locking primitive that does work on atomic contexes.
But I am not the right person to do it, I am still learning about all
the different locking flavours in Linux.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Wed, 02 Jul 2008, Zhu Yi wrote:
> On Tue, 2008-07-01 at 13:56 -0300, Henrique de Moraes Holschuh wrote:
> > On Tue, 01 Jul 2008, Adel Gadllah wrote:
> > > The calls to iwl|iwl3945_rfkill_set_hw_state() had to be moved
> > because rfkill_force_state() cannot be called from an atomic context.
>
> Yes, but what your patch changed is not in the atomic context. It is
> just inside the driver's priv->mutex. I don't see any problem if you
> call rfkill_force_state() inside it.
>
> > Yeah, the joys of mutexes. If this is going to be a severe annoyance
> > to drivers, I don't see why rfkill could not be changed to use some
> > other locking primitive that does work on atomic contexes.
>
> Allowing rfkill_force_state() to be called in the atomic context would
> be useful especially for hardware rfkill. Devices (i.e iwl4965) receive
> an interrupt when the hw-rfkill state changes. It's natural to update
> the rfkill state in this context.
>
> How about protect the rfkill->state by a spinlock and put the
> notifier_call_chain() into a workqueue in the rfkill subsystem?
That shouldn't be a problem. What are the spinlock primitives I should be
using on rfkill_force_state so that it would be compatible with most drivers
(and not cause issues when called in task context instead of interrupt
context)?
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Thu, 03 Jul 2008, Ivo van Doorn wrote:
> On Wednesday 02 July 2008, Henrique de Moraes Holschuh wrote:
> > On Wed, 02 Jul 2008, Ivo van Doorn wrote:
> > > Well actually it isn't that easy, the lock would also be used for the state change
> > > callback function toward the driver. And if that is done under a spinlock, USB
> > > drivers will start complaining since they can't access the hardware under atomic
> > > context...
> >
> > Would it be worth it to use a hybrid scheme?
>
> That would be nice if it could be done cleanly.
If it can't be done cleanly, it is best not done at all IMHO...
> > Callbacks and the notify chain would remain in task context, while the
> > locking is changed to spinlocks so that it can also work in interrupt
> > context when needed. We document better (in the text documentation,
> > include files and kernel-doc) the contextes so that people don't get
> > confused by the code and think that a spinlock means atomic context is OK
> > for these handlers.
>
> The mutex in the rfkill structure could become a spinlock, but the notifiers
> should probably be switched to the atomic versions as well so they could still
> be used at the current locations.
The kind of stuff we do in the notifiers usually benefits from task context,
I'd rather add a rfkill workqueue and call the notifiers from there when
needed (i.e. when we call from inside rfkill_force_state_atomic).
Other than calling LED triggers, everything else one typically would use the
notifier chain for (sending uevents, sending input evets) is best done from
task context, anyway.
> The global rfkill_mutex could remain a mutex to protect the rfkill list and all
> callback functions.
Yes, we could do that for the rfkill list, but shouldn't all callbacks and
all fields inside a rfkill struct be protected by the rfkill->mutex instead
of by the global mutex?
If a driver wants any more locking so as not to get two of its rfkill
callbacks (when it handles more than one rfkill device) called concurrently,
it is its business to do it.
> Now that I rechecked the code the current approach doesn't seem to protect the
> rfkill_toggle_radio callback function with consistent locking either.
> Sometimes it is called under the rfkill->mutex protection and at other times
> under the global rfkill_mutex.
> Both rfkill_state_store() and rfkill_resume() use the rfkill->mutex while most
> other functions use the global rfkill_mutex. Those 2 will have to switch over to
> the global mutex to prevent problems.
Actually, see above... Shouldn't we protect everything inside a rfkill
struct with rfkill->mutex, and outside stuff with the global mutex?
And I fear we could do better in the rfkill struct refcouting area, too.
The code I submitted does no refcounting, and it exposes the rfkill struct a
lot. Even under the protection of the mutexes, it was probably unwise for
me to do it that way without some refcounting paranoia. Especially when
schedule_work is involved...
> > For rfkill_force_state(), we'd either add a flags parameter that can take,
> > e.g., GFP_ATOMIC, to tell us whether it is being called from an interrupt
> > handler or a task context, or we could simply add rfkill_force_state_atomic().
>
> Neither sound like a good idea, because that would require 2 approaches to lock
> the rfkill structure which could be good cause for race conditions under certain
> situations.
[...]
> > This would be safer (because the state would still be updated synchronously
> > when one calls rfkill_force_state(_atomic)?) than adding a
> > rfkill_schedule_force_state() for drivers to call in interrupt context.
>
> Couldn't this cause race conditions in toggle_radio updates when 1 driver uses atomic
> context and the other in scheduled context.
Hmm, it could cause deadlocks I think. task context takes lock. interrupt
context tries to run, needs lock, keeps spinning forever waiting for it, and
if we are not SMP, task context never has a chance to release the lock, so
we get a deadlock. Is this correct?
If it is, we better make the atomic path lock-free. Argh. This means
rfkill->state (the only thing we care about in the atomic path) would have
to become an atomic variable, and the code changed to cope with its value
being "volatile", so that it can be used in a lockless fashion.
It is doable, I think, although it is quite annoying.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tue, 2008-07-01 at 13:56 -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 01 Jul 2008, Adel Gadllah wrote:
> > The calls to iwl|iwl3945_rfkill_set_hw_state() had to be moved
> because rfkill_force_state() cannot be called from an atomic context.
Yes, but what your patch changed is not in the atomic context. It is
just inside the driver's priv->mutex. I don't see any problem if you
call rfkill_force_state() inside it.
> Yeah, the joys of mutexes. If this is going to be a severe annoyance
> to drivers, I don't see why rfkill could not be changed to use some
> other locking primitive that does work on atomic contexes.
Allowing rfkill_force_state() to be called in the atomic context would
be useful especially for hardware rfkill. Devices (i.e iwl4965) receive
an interrupt when the hw-rfkill state changes. It's natural to update
the rfkill state in this context.
How about protect the rfkill->state by a spinlock and put the
notifier_call_chain() into a workqueue in the rfkill subsystem?
Thanks,
-yi
On Thu, 03 Jul 2008, Zhu Yi wrote:
> On Wed, 2008-07-02 at 22:53 -0300, Henrique de Moraes Holschuh wrote:
> > Hmm, it could cause deadlocks I think. task context takes lock.
> > interrupt context tries to run, needs lock, keeps spinning forever
> > waiting for it, and if we are not SMP, task context never has a chance
> > to release the lock, so we get a deadlock. Is this correct?
>
> Never heard of spin_lock_irqsave()?
Studying it now... I did say I was not the right person for the job because
I am still learning all the locks we have :-)
> > If it is, we better make the atomic path lock-free. Argh. This means
> > rfkill->state (the only thing we care about in the atomic path) would
> > have to become an atomic variable, and the code changed to cope with
> > its value being "volatile", so that it can be used in a lockless
> > fashion.
>
> Yup, if protecting rfkill->state is the only intention, a spinlock is
> too heavy. set|clear_bit is enough if organizing the state into bitmaps.
> BTW, read/write 32-bits aligned on x86 is already atomic.
I know rfkill->state is already atomic on x86, and I think it is also atomic
on every other platform Linux supports (I need to check this, since I am not
sure). However, explicitly making it atomic_t is a damn heavy hint to
anyone using rfkill about how to deal with rfkill->state, and that can be a
big advantage.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Wed, 02 Jul 2008, Ivo van Doorn wrote:
> Well actually it isn't that easy, the lock would also be used for the state change
> callback function toward the driver. And if that is done under a spinlock, USB
> drivers will start complaining since they can't access the hardware under atomic
> context...
Would it be worth it to use a hybrid scheme?
Callbacks and the notify chain would remain in task context, while the
locking is changed to spinlocks so that it can also work in interrupt
context when needed. We document better (in the text documentation,
include files and kernel-doc) the contextes so that people don't get
confused by the code and think that a spinlock means atomic context is OK
for these handlers.
For rfkill_force_state(), we'd either add a flags parameter that can take,
e.g., GFP_ATOMIC, to tell us whether it is being called from an interrupt
handler or a task context, or we could simply add rfkill_force_state_atomic().
This would be safer (because the state would still be updated synchronously
when one calls rfkill_force_state(_atomic)?) than adding a
rfkill_schedule_force_state() for drivers to call in interrupt context.
So far, the only function I can see that would have to work in
interrupt/atomic context would be rfkill_force_state_atomic.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Wednesday 02 July 2008, Henrique de Moraes Holschuh wrote:
> On Wed, 02 Jul 2008, Zhu Yi wrote:
> > On Tue, 2008-07-01 at 13:56 -0300, Henrique de Moraes Holschuh wrote:
> > > On Tue, 01 Jul 2008, Adel Gadllah wrote:
> > > > The calls to iwl|iwl3945_rfkill_set_hw_state() had to be moved
> > > because rfkill_force_state() cannot be called from an atomic context.
> >
> > Yes, but what your patch changed is not in the atomic context. It is
> > just inside the driver's priv->mutex. I don't see any problem if you
> > call rfkill_force_state() inside it.
> >
> > > Yeah, the joys of mutexes. If this is going to be a severe annoyance
> > > to drivers, I don't see why rfkill could not be changed to use some
> > > other locking primitive that does work on atomic contexes.
> >
> > Allowing rfkill_force_state() to be called in the atomic context would
> > be useful especially for hardware rfkill. Devices (i.e iwl4965) receive
> > an interrupt when the hw-rfkill state changes. It's natural to update
> > the rfkill state in this context.
> >
> > How about protect the rfkill->state by a spinlock and put the
> > notifier_call_chain() into a workqueue in the rfkill subsystem?
>
> That shouldn't be a problem. What are the spinlock primitives I should be
> using on rfkill_force_state so that it would be compatible with most drivers
> (and not cause issues when called in task context instead of interrupt
> context)?
Well actually it isn't that easy, the lock would also be used for the state change
callback function toward the driver. And if that is done under a spinlock, USB
drivers will start complaining since they can't access the hardware under atomic
context...
Ivo
On Wednesday 02 July 2008, Henrique de Moraes Holschuh wrote:
> On Wed, 02 Jul 2008, Ivo van Doorn wrote:
> > Well actually it isn't that easy, the lock would also be used for the state change
> > callback function toward the driver. And if that is done under a spinlock, USB
> > drivers will start complaining since they can't access the hardware under atomic
> > context...
>
> Would it be worth it to use a hybrid scheme?
That would be nice if it could be done cleanly.
> Callbacks and the notify chain would remain in task context, while the
> locking is changed to spinlocks so that it can also work in interrupt
> context when needed. We document better (in the text documentation,
> include files and kernel-doc) the contextes so that people don't get
> confused by the code and think that a spinlock means atomic context is OK
> for these handlers.
The mutex in the rfkill structure could become a spinlock, but the notifiers
should probably be switched to the atomic versions as well so they could still
be used at the current locations.
The global rfkill_mutex could remain a mutex to protect the rfkill list and all
callback functions.
Now that I rechecked the code the current approach doesn't seem to protect the
rfkill_toggle_radio callback function with consistent locking either.
Sometimes it is called under the rfkill->mutex protection and at other times
under the global rfkill_mutex.
Both rfkill_state_store() and rfkill_resume() use the rfkill->mutex while most
other functions use the global rfkill_mutex. Those 2 will have to switch over to
the global mutex to prevent problems.
> For rfkill_force_state(), we'd either add a flags parameter that can take,
> e.g., GFP_ATOMIC, to tell us whether it is being called from an interrupt
> handler or a task context, or we could simply add rfkill_force_state_atomic().
Neither sound like a good idea, because that would require 2 approaches to lock
the rfkill structure which could be good cause for race conditions under certain
situations.
> This would be safer (because the state would still be updated synchronously
> when one calls rfkill_force_state(_atomic)?) than adding a
> rfkill_schedule_force_state() for drivers to call in interrupt context.
Couldn't this cause race conditions in toggle_radio updates when 1 driver uses atomic
context and the other in scheduled context.
> So far, the only function I can see that would have to work in
> interrupt/atomic context would be rfkill_force_state_atomic.
Thats correct.
Ivo
On Wed, 2008-07-02 at 22:53 -0300, Henrique de Moraes Holschuh wrote:
> Hmm, it could cause deadlocks I think. task context takes lock.
> interrupt context tries to run, needs lock, keeps spinning forever
> waiting for it, and if we are not SMP, task context never has a chance
> to release the lock, so we get a deadlock. Is this correct?
Never heard of spin_lock_irqsave()?
> If it is, we better make the atomic path lock-free. Argh. This means
> rfkill->state (the only thing we care about in the atomic path) would
> have to become an atomic variable, and the code changed to cope with
> its value being "volatile", so that it can be used in a lockless
> fashion.
Yup, if protecting rfkill->state is the only intention, a spinlock is
too heavy. set|clear_bit is enough if organizing the state into bitmaps.
BTW, read/write 32-bits aligned on x86 is already atomic.
Thanks,
-yi