[RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[Helmut Schaa]

Currently iwlagn is not able to report hw-killswitch events while the
interface is down. However iwl4965 and iwl5000 are able to report killswitch
changes via interrupts even if no firmware is loaded.

Thus enable the device and interrupts already in iwl_pci_probe instead of
iwl_up to get notified when the hw-killswitch state changes. Furthermore
move the pci deactivation code from iwl_mac_stop to iwl_pci_remove and
update the killswitch state in the interrupt handler. iwl_bg_rf_kill will
care about restarting the card if the killswitch was activated while the
interface was left up.

The patch at least does what it is supposed to do and I used it the last
few days without noticing any issues. Nevertheless sending as RFC/RFT as I'm
not completely sure if everything is done correctly.

Thanks for reviewing/commenting.

Btw the patch is against current wireless-testing.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>

---
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index 9d50fad..d57b103 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -1413,12 +1413,15 @@ static void iwl_irq_tasklet(struct iwl_priv *priv)
 
 		/* driver only loads ucode once setting the interface up.
 		 * the driver as well won't allow loading if RFKILL is set
-		 * therefore no need to restart the driver from this handler
+		 * therefore no need to restart the driver from this handler.
+		 * However the killswitch state should be updated.
 		 */
-		if (!hw_rf_kill && !test_bit(STATUS_ALIVE, &priv->status)) {
-			clear_bit(STATUS_RF_KILL_HW, &priv->status);
-			if (priv->is_open && !iwl_is_rfkill(priv))
-				queue_work(priv->workqueue, &priv->up);
+		if (!test_bit(STATUS_ALIVE, &priv->status)) {
+			if (hw_rf_kill)
+				set_bit(STATUS_RF_KILL_HW, &priv->status);
+			else
+				clear_bit(STATUS_RF_KILL_HW, &priv->status);
+			queue_work(priv->workqueue, &priv->rf_kill);
 		}
 
 		handled |= CSR_INT_BIT_RF_KILL;
@@ -2044,12 +2047,6 @@ static int __iwl_up(struct iwl_priv *priv)
 		return -EIO;
 	}
 
-	/* If platform's RF_KILL switch is NOT set to KILL */
-	if (iwl_read32(priv, CSR_GP_CNTRL) & CSR_GP_CNTRL_REG_FLAG_HW_RF_KILL_SW)
-		clear_bit(STATUS_RF_KILL_HW, &priv->status);
-	else
-		set_bit(STATUS_RF_KILL_HW, &priv->status);
-
 	if (iwl_is_rfkill(priv)) {
 		iwl_enable_interrupts(priv);
 		IWL_WARNING("Radio disabled by %s RF Kill switch\n",
@@ -2167,7 +2164,7 @@ static void iwl_bg_rf_kill(struct work_struct *work)
 		IWL_DEBUG(IWL_DL_RF_KILL,
 			  "HW and/or SW RF Kill no longer active, restarting "
 			  "device\n");
-		if (!test_bit(STATUS_EXIT_PENDING, &priv->status))
+		if (!test_bit(STATUS_EXIT_PENDING, &priv->status) && test_bit(STATUS_ALIVE, &priv->status))
 			queue_work(priv->workqueue, &priv->restart);
 	} else {
 		/* make sure mac80211 stop sending Tx frame */
@@ -2366,31 +2363,9 @@ static int iwl_mac_start(struct ieee80211_hw *hw)
 {
 	struct iwl_priv *priv = hw->priv;
 	int ret;
-	u16 pci_cmd;
 
 	IWL_DEBUG_MAC80211("enter\n");
 
-	if (pci_enable_device(priv->pci_dev)) {
-		IWL_ERROR("Fail to pci_enable_device\n");
-		return -ENODEV;
-	}
-	pci_restore_state(priv->pci_dev);
-	pci_enable_msi(priv->pci_dev);
-
-	/* enable interrupts if needed: hw bug w/a */
-	pci_read_config_word(priv->pci_dev, PCI_COMMAND, &pci_cmd);
-	if (pci_cmd & PCI_COMMAND_INTX_DISABLE) {
-		pci_cmd &= ~PCI_COMMAND_INTX_DISABLE;
-		pci_write_config_word(priv->pci_dev, PCI_COMMAND, pci_cmd);
-	}
-
-	ret = request_irq(priv->pci_dev->irq, iwl_isr, IRQF_SHARED,
-			  DRV_NAME, priv);
-	if (ret) {
-		IWL_ERROR("Error allocating IRQ %d\n", priv->pci_dev->irq);
-		goto out_disable_msi;
-	}
-
 	/* we should be verifying the device is ready to be opened */
 	mutex_lock(&priv->mutex);
 
@@ -2403,7 +2378,7 @@ static int iwl_mac_start(struct ieee80211_hw *hw)
 		if (ret) {
 			IWL_ERROR("Could not read microcode: %d\n", ret);
 			mutex_unlock(&priv->mutex);
-			goto out_release_irq;
+			return ret;
 		}
 	}
 
@@ -2414,7 +2389,7 @@ static int iwl_mac_start(struct ieee80211_hw *hw)
 	iwl_rfkill_set_hw_state(priv);
 
 	if (ret)
-		goto out_release_irq;
+		return ret;
 
 	if (iwl_is_rfkill(priv))
 		goto out;
@@ -2433,8 +2408,7 @@ static int iwl_mac_start(struct ieee80211_hw *hw)
 		if (!test_bit(STATUS_READY, &priv->status)) {
 			IWL_ERROR("START_ALIVE timeout after %dms.\n",
 				jiffies_to_msecs(UCODE_READY_TIMEOUT));
-			ret = -ETIMEDOUT;
-			goto out_release_irq;
+			return -ETIMEDOUT;
 		}
 	}
 
@@ -2442,15 +2416,6 @@ out:
 	priv->is_open = 1;
 	IWL_DEBUG_MAC80211("leave\n");
 	return 0;
-
-out_release_irq:
-	free_irq(priv->pci_dev->irq, priv);
-out_disable_msi:
-	pci_disable_msi(priv->pci_dev);
-	pci_disable_device(priv->pci_dev);
-	priv->is_open = 0;
-	IWL_DEBUG_MAC80211("leave - failed\n");
-	return ret;
 }
 
 static void iwl_mac_stop(struct ieee80211_hw *hw)
@@ -2478,10 +2443,10 @@ static void iwl_mac_stop(struct ieee80211_hw *hw)
 	iwl_down(priv);
 
 	flush_workqueue(priv->workqueue);
-	free_irq(priv->pci_dev->irq, priv);
-	pci_disable_msi(priv->pci_dev);
-	pci_save_state(priv->pci_dev);
-	pci_disable_device(priv->pci_dev);
+
+	/* enable interrupts again in order to receive rfkill changes*/
+	iwl_write32(priv, CSR_INT, 0xFFFFFFFF);
+	iwl_enable_interrupts(priv);
 
 	IWL_DEBUG_MAC80211("leave\n");
 }
@@ -3778,6 +3743,7 @@ static int iwl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	struct ieee80211_hw *hw;
 	struct iwl_cfg *cfg = (struct iwl_cfg *)(ent->driver_data);
 	unsigned long flags;
+	u16 pci_cmd;
 	DECLARE_MAC_BUF(mac);
 
 	/************************
@@ -3923,6 +3889,15 @@ static int iwl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	spin_lock_irqsave(&priv->lock, flags);
 	iwl_disable_interrupts(priv);
 	spin_unlock_irqrestore(&priv->lock, flags);
+	
+	pci_enable_msi(priv->pci_dev);
+
+	err = request_irq(priv->pci_dev->irq, iwl_isr, IRQF_SHARED,
+			  DRV_NAME, priv);
+	if (err) {
+		IWL_ERROR("Error allocating IRQ %d\n", priv->pci_dev->irq);
+		goto out_disable_msi;
+	}
 
 	err = sysfs_create_group(&pdev->dev.kobj, &iwl_attribute_group);
 	if (err) {
@@ -3930,20 +3905,22 @@ static int iwl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_uninit_drv;
 	}
 
-
 	iwl_setup_deferred_work(priv);
 	iwl_setup_rx_handlers(priv);
 
-	/********************
-	 * 9. Conclude
-	 ********************/
-	pci_save_state(pdev);
-	pci_disable_device(pdev);
-
 	/**********************************
 	 * 10. Setup and register mac80211
 	 **********************************/
 
+	/* enable interrupts if needed: hw bug w/a */
+	pci_read_config_word(priv->pci_dev, PCI_COMMAND, &pci_cmd);
+	if (pci_cmd & PCI_COMMAND_INTX_DISABLE) {
+		pci_cmd &= ~PCI_COMMAND_INTX_DISABLE;
+		pci_write_config_word(priv->pci_dev, PCI_COMMAND, pci_cmd);
+	}
+
+	iwl_enable_interrupts(priv);
+
 	err = iwl_setup_mac(priv);
 	if (err)
 		goto out_remove_sysfs;
@@ -3952,15 +3929,30 @@ static int iwl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (err)
 		IWL_ERROR("failed to create debugfs files\n");
 
+	/* If platform's RF_KILL switch is NOT set to KILL */
+	if (iwl_read32(priv, CSR_GP_CNTRL) & CSR_GP_CNTRL_REG_FLAG_HW_RF_KILL_SW)
+		clear_bit(STATUS_RF_KILL_HW, &priv->status);
+	else
+		set_bit(STATUS_RF_KILL_HW, &priv->status);
+
+	IWL_DEBUG(IWL_DL_RF_KILL, "RF_KILL bit is %s.\n",
+			test_bit(STATUS_RF_KILL_HW, &priv->status) ? "disable radio" : "enable radio");
+
 	err = iwl_rfkill_init(priv);
 	if (err)
 		IWL_ERROR("Unable to initialize RFKILL system. "
 				  "Ignoring error: %d\n", err);
+	else
+		iwl_rfkill_set_hw_state(priv);
+
 	iwl_power_initialize(priv);
 	return 0;
 
  out_remove_sysfs:
 	sysfs_remove_group(&pdev->dev.kobj, &iwl_attribute_group);
+ out_disable_msi:
+	pci_disable_msi(priv->pci_dev);
+	pci_disable_device(priv->pci_dev);
  out_uninit_drv:
 	iwl_uninit_drv(priv);
  out_free_eeprom:
@@ -4032,6 +4024,9 @@ static void __devexit iwl_pci_remove(struct pci_dev *pdev)
 	destroy_workqueue(priv->workqueue);
 	priv->workqueue = NULL;
 
+	free_irq(priv->pci_dev->irq, priv);
+	pci_disable_msi(priv->pci_dev);
+	
 	pci_iounmap(pdev, priv->hw_base);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
@@ -4057,6 +4052,8 @@ static int iwl_pci_suspend(struct pci_dev *pdev, pm_message_t state)
 		priv->is_open = 1;
 	}
 
+	pci_save_state(pdev);
+	pci_disable_device(pdev);
 	pci_set_power_state(pdev, PCI_D3hot);
 
 	return 0;
@@ -4067,6 +4064,9 @@ static int iwl_pci_resume(struct pci_dev *pdev)
 	struct iwl_priv *priv = pci_get_drvdata(pdev);
 
 	pci_set_power_state(pdev, PCI_D0);
+	pci_enable_device(pdev);
+	pci_restore_state(pdev);	
+	iwl_enable_interrupts(priv);
 
 	if (priv->is_open)
 		iwl_mac_start(priv->hw);

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[mohamed salim abbas]

the interrupt moved from pci_probe to mac_start for power saving. once
the interface is up the driver will read some register to know rfkill
status, if the interface in down the driver don't care to keep track
of rfkill switch. I wonder what the purpose of changing this behavior?


On Tue, Dec 16, 2008 at 4:07 AM, Helmut Schaa
<helmut.schaa@googlemail.com> wrote:
> Currently iwlagn is not able to report hw-killswitch events while the
> interface is down. However iwl4965 and iwl5000 are able to report killswitch
> changes via interrupts even if no firmware is loaded.
>
> Thus enable the device and interrupts already in iwl_pci_probe instead of
> iwl_up to get notified when the hw-killswitch state changes. Furthermore
> move the pci deactivation code from iwl_mac_stop to iwl_pci_remove and
> update the killswitch state in the interrupt handler. iwl_bg_rf_kill will
> care about restarting the card if the killswitch was activated while the
> interface was left up.
>
> The patch at least does what it is supposed to do and I used it the last
> few days without noticing any issues. Nevertheless sending as RFC/RFT as I'm
> not completely sure if everything is done correctly.
>
> Thanks for reviewing/commenting.
>
> Btw the patch is against current wireless-testing.
>
> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
>
> ---
> diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
> index 9d50fad..d57b103 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-agn.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
> @@ -1413,12 +1413,15 @@ static void iwl_irq_tasklet(struct iwl_priv *priv)
>
>                /* driver only loads ucode once setting the interface up.
>                 * the driver as well won't allow loading if RFKILL is set
> -                * therefore no need to restart the driver from this handler
> +                * therefore no need to restart the driver from this handler.
> +                * However the killswitch state should be updated.
>                 */
> -               if (!hw_rf_kill && !test_bit(STATUS_ALIVE, &priv->status)) {
> -                       clear_bit(STATUS_RF_KILL_HW, &priv->status);
> -                       if (priv->is_open && !iwl_is_rfkill(priv))
> -                               queue_work(priv->workqueue, &priv->up);
> +               if (!test_bit(STATUS_ALIVE, &priv->status)) {
> +                       if (hw_rf_kill)
> +                               set_bit(STATUS_RF_KILL_HW, &priv->status);
> +                       else
> +                               clear_bit(STATUS_RF_KILL_HW, &priv->status);
> +                       queue_work(priv->workqueue, &priv->rf_kill);
>                }
>
>                handled |= CSR_INT_BIT_RF_KILL;
> @@ -2044,12 +2047,6 @@ static int __iwl_up(struct iwl_priv *priv)
>                return -EIO;
>        }
>
> -       /* If platform's RF_KILL switch is NOT set to KILL */
> -       if (iwl_read32(priv, CSR_GP_CNTRL) & CSR_GP_CNTRL_REG_FLAG_HW_RF_KILL_SW)
> -               clear_bit(STATUS_RF_KILL_HW, &priv->status);
> -       else
> -               set_bit(STATUS_RF_KILL_HW, &priv->status);
> -
>        if (iwl_is_rfkill(priv)) {
>                iwl_enable_interrupts(priv);
>                IWL_WARNING("Radio disabled by %s RF Kill switch\n",
> @@ -2167,7 +2164,7 @@ static void iwl_bg_rf_kill(struct work_struct *work)
>                IWL_DEBUG(IWL_DL_RF_KILL,
>                          "HW and/or SW RF Kill no longer active, restarting "
>                          "device\n");
> -               if (!test_bit(STATUS_EXIT_PENDING, &priv->status))
> +               if (!test_bit(STATUS_EXIT_PENDING, &priv->status) && test_bit(STATUS_ALIVE, &priv->status))
>                        queue_work(priv->workqueue, &priv->restart);
>        } else {
>                /* make sure mac80211 stop sending Tx frame */
> @@ -2366,31 +2363,9 @@ static int iwl_mac_start(struct ieee80211_hw *hw)
>  {
>        struct iwl_priv *priv = hw->priv;
>        int ret;
> -       u16 pci_cmd;
>
>        IWL_DEBUG_MAC80211("enter\n");
>
> -       if (pci_enable_device(priv->pci_dev)) {
> -               IWL_ERROR("Fail to pci_enable_device\n");
> -               return -ENODEV;
> -       }
> -       pci_restore_state(priv->pci_dev);
> -       pci_enable_msi(priv->pci_dev);
> -
> -       /* enable interrupts if needed: hw bug w/a */
> -       pci_read_config_word(priv->pci_dev, PCI_COMMAND, &pci_cmd);
> -       if (pci_cmd & PCI_COMMAND_INTX_DISABLE) {
> -               pci_cmd &= ~PCI_COMMAND_INTX_DISABLE;
> -               pci_write_config_word(priv->pci_dev, PCI_COMMAND, pci_cmd);
> -       }
> -
> -       ret = request_irq(priv->pci_dev->irq, iwl_isr, IRQF_SHARED,
> -                         DRV_NAME, priv);
> -       if (ret) {
> -               IWL_ERROR("Error allocating IRQ %d\n", priv->pci_dev->irq);
> -               goto out_disable_msi;
> -       }
> -
>        /* we should be verifying the device is ready to be opened */
>        mutex_lock(&priv->mutex);
>
> @@ -2403,7 +2378,7 @@ static int iwl_mac_start(struct ieee80211_hw *hw)
>                if (ret) {
>                        IWL_ERROR("Could not read microcode: %d\n", ret);
>                        mutex_unlock(&priv->mutex);
> -                       goto out_release_irq;
> +                       return ret;
>                }
>        }
>
> @@ -2414,7 +2389,7 @@ static int iwl_mac_start(struct ieee80211_hw *hw)
>        iwl_rfkill_set_hw_state(priv);
>
>        if (ret)
> -               goto out_release_irq;
> +               return ret;
>
>        if (iwl_is_rfkill(priv))
>                goto out;
> @@ -2433,8 +2408,7 @@ static int iwl_mac_start(struct ieee80211_hw *hw)
>                if (!test_bit(STATUS_READY, &priv->status)) {
>                        IWL_ERROR("START_ALIVE timeout after %dms.\n",
>                                jiffies_to_msecs(UCODE_READY_TIMEOUT));
> -                       ret = -ETIMEDOUT;
> -                       goto out_release_irq;
> +                       return -ETIMEDOUT;
>                }
>        }
>
> @@ -2442,15 +2416,6 @@ out:
>        priv->is_open = 1;
>        IWL_DEBUG_MAC80211("leave\n");
>        return 0;
> -
> -out_release_irq:
> -       free_irq(priv->pci_dev->irq, priv);
> -out_disable_msi:
> -       pci_disable_msi(priv->pci_dev);
> -       pci_disable_device(priv->pci_dev);
> -       priv->is_open = 0;
> -       IWL_DEBUG_MAC80211("leave - failed\n");
> -       return ret;
>  }
>
>  static void iwl_mac_stop(struct ieee80211_hw *hw)
> @@ -2478,10 +2443,10 @@ static void iwl_mac_stop(struct ieee80211_hw *hw)
>        iwl_down(priv);
>
>        flush_workqueue(priv->workqueue);
> -       free_irq(priv->pci_dev->irq, priv);
> -       pci_disable_msi(priv->pci_dev);
> -       pci_save_state(priv->pci_dev);
> -       pci_disable_device(priv->pci_dev);
> +
> +       /* enable interrupts again in order to receive rfkill changes*/
> +       iwl_write32(priv, CSR_INT, 0xFFFFFFFF);
> +       iwl_enable_interrupts(priv);
>
>        IWL_DEBUG_MAC80211("leave\n");
>  }
> @@ -3778,6 +3743,7 @@ static int iwl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>        struct ieee80211_hw *hw;
>        struct iwl_cfg *cfg = (struct iwl_cfg *)(ent->driver_data);
>        unsigned long flags;
> +       u16 pci_cmd;
>        DECLARE_MAC_BUF(mac);
>
>        /************************
> @@ -3923,6 +3889,15 @@ static int iwl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>        spin_lock_irqsave(&priv->lock, flags);
>        iwl_disable_interrupts(priv);
>        spin_unlock_irqrestore(&priv->lock, flags);
> +
> +       pci_enable_msi(priv->pci_dev);
> +
> +       err = request_irq(priv->pci_dev->irq, iwl_isr, IRQF_SHARED,
> +                         DRV_NAME, priv);
> +       if (err) {
> +               IWL_ERROR("Error allocating IRQ %d\n", priv->pci_dev->irq);
> +               goto out_disable_msi;
> +       }
>
>        err = sysfs_create_group(&pdev->dev.kobj, &iwl_attribute_group);
>        if (err) {
> @@ -3930,20 +3905,22 @@ static int iwl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>                goto out_uninit_drv;
>        }
>
> -
>        iwl_setup_deferred_work(priv);
>        iwl_setup_rx_handlers(priv);
>
> -       /********************
> -        * 9. Conclude
> -        ********************/
> -       pci_save_state(pdev);
> -       pci_disable_device(pdev);
> -
>        /**********************************
>         * 10. Setup and register mac80211
>         **********************************/
>
> +       /* enable interrupts if needed: hw bug w/a */
> +       pci_read_config_word(priv->pci_dev, PCI_COMMAND, &pci_cmd);
> +       if (pci_cmd & PCI_COMMAND_INTX_DISABLE) {
> +               pci_cmd &= ~PCI_COMMAND_INTX_DISABLE;
> +               pci_write_config_word(priv->pci_dev, PCI_COMMAND, pci_cmd);
> +       }
> +
> +       iwl_enable_interrupts(priv);
> +
>        err = iwl_setup_mac(priv);
>        if (err)
>                goto out_remove_sysfs;
> @@ -3952,15 +3929,30 @@ static int iwl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>        if (err)
>                IWL_ERROR("failed to create debugfs files\n");
>
> +       /* If platform's RF_KILL switch is NOT set to KILL */
> +       if (iwl_read32(priv, CSR_GP_CNTRL) & CSR_GP_CNTRL_REG_FLAG_HW_RF_KILL_SW)
> +               clear_bit(STATUS_RF_KILL_HW, &priv->status);
> +       else
> +               set_bit(STATUS_RF_KILL_HW, &priv->status);
> +
> +       IWL_DEBUG(IWL_DL_RF_KILL, "RF_KILL bit is %s.\n",
> +                       test_bit(STATUS_RF_KILL_HW, &priv->status) ? "disable radio" : "enable radio");
> +
>        err = iwl_rfkill_init(priv);
>        if (err)
>                IWL_ERROR("Unable to initialize RFKILL system. "
>                                  "Ignoring error: %d\n", err);
> +       else
> +               iwl_rfkill_set_hw_state(priv);
> +
>        iwl_power_initialize(priv);
>        return 0;
>
>  out_remove_sysfs:
>        sysfs_remove_group(&pdev->dev.kobj, &iwl_attribute_group);
> + out_disable_msi:
> +       pci_disable_msi(priv->pci_dev);
> +       pci_disable_device(priv->pci_dev);
>  out_uninit_drv:
>        iwl_uninit_drv(priv);
>  out_free_eeprom:
> @@ -4032,6 +4024,9 @@ static void __devexit iwl_pci_remove(struct pci_dev *pdev)
>        destroy_workqueue(priv->workqueue);
>        priv->workqueue = NULL;
>
> +       free_irq(priv->pci_dev->irq, priv);
> +       pci_disable_msi(priv->pci_dev);
> +
>        pci_iounmap(pdev, priv->hw_base);
>        pci_release_regions(pdev);
>        pci_disable_device(pdev);
> @@ -4057,6 +4052,8 @@ static int iwl_pci_suspend(struct pci_dev *pdev, pm_message_t state)
>                priv->is_open = 1;
>        }
>
> +       pci_save_state(pdev);
> +       pci_disable_device(pdev);
>        pci_set_power_state(pdev, PCI_D3hot);
>
>        return 0;
> @@ -4067,6 +4064,9 @@ static int iwl_pci_resume(struct pci_dev *pdev)
>        struct iwl_priv *priv = pci_get_drvdata(pdev);
>
>        pci_set_power_state(pdev, PCI_D0);
> +       pci_enable_device(pdev);
> +       pci_restore_state(pdev);
> +       iwl_enable_interrupts(priv);
>
>        if (priv->is_open)
>                iwl_mac_start(priv->hw);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[John W. Linville]

On Wed, Dec 17, 2008 at 12:10:11PM -0800, mohamed salim abbas wrote:
> the interrupt moved from pci_probe to mac_start for power saving. once
> the interface is up the driver will read some register to know rfkill
> status, if the interface in down the driver don't care to keep track
> of rfkill switch. I wonder what the purpose of changing this behavior?

I think it still isn't settled in everyone's minds whether rfkill
only matters if the device is "up" or if it is something that
e.g. NetworkManager might want to monitor as a clue to bring the
device up or down in response to rfkill changes.

John
-- 
John W. Linville		Linux should be at the core
linville@tuxdriver.com			of your literate lifestyle.

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[Dan Williams]

On Wed, 2008-12-17 at 15:29 -0500, John W. Linville wrote:
> On Wed, Dec 17, 2008 at 12:10:11PM -0800, mohamed salim abbas wrote:
> > the interrupt moved from pci_probe to mac_start for power saving. once
> > the interface is up the driver will read some register to know rfkill
> > status, if the interface in down the driver don't care to keep track
> > of rfkill switch. I wonder what the purpose of changing this behavior?
> 
> I think it still isn't settled in everyone's minds whether rfkill
> only matters if the device is "up" or if it is something that
> e.g. NetworkManager might want to monitor as a clue to bring the
> device up or down in response to rfkill changes.

The question is:  does NetworkManager just always keep the device 'up'
irregardless of whether it's supposed to be associated with anything
just so we can get rfkill events?

We have to keep ethernet devices up because some don't report carrier
status unless they are up (when the PHY is powered on).  Yeah, that
consumes a trivial amount of power.  But lots better to get
notifications of ethernet carrier events dynamically than having to
either (a) poll the carrier by IFF_UP every 5 seconds or (b) not getting
carrier events at all.

I guess I'll treat rfkill the same as ethernet carrier.  If we cannot
rely on rfkill notifications when the device is down (we already can't,
since iwl3945 simply can't do it) then I guess we just have to keep the
wifi device up all the time, even when it's rfkilled, and set the tx
power off when wireless is supposed to be disabled.

It is simply a hard requirement to be able to get rfkill events when the
switch gets flipped, irrespective of whether some blocks of the silicon
are still powered or not (which I assume is the motivation for
completely unloading the firmware in the first place).  We're not going
to special-case a certain chips in NetworkManager; if there are quirks
for devices, those quirks need to live in the driver, not worked around
in userspace.

Dan

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[Helmut Schaa]

Am Mittwoch, 17. Dezember 2008 schrieb mohamed salim abbas:
> the interrupt moved from pci_probe to mac_start for power saving.

Out of curiosity, do you have any specific numbers of how much power can
be saved by disabling interrupts?

Thanks,
Helmut

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[Helmut Schaa]

Am Mittwoch, 17. Dezember 2008 schrieb John W. Linville:
> I think it still isn't settled in everyone's minds whether rfkill
> only matters if the device is "up" or if it is something that
> e.g. NetworkManager might want to monitor as a clue to bring the
> device up or down in response to rfkill changes.

I guess we need a concrete definition if the rfkill state is bound to
iface up/down or not. However I think it's somwhat awkward to expose an
(maybe) incorrect rfkill state if the interface is down. Either the state
should be correct or not exposed at all.

Helmut

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[Helmut Schaa]

Am Mittwoch, 17. Dezember 2008 schrieb Dan Williams:
> On Wed, 2008-12-17 at 15:29 -0500, John W. Linville wrote:
> > On Wed, Dec 17, 2008 at 12:10:11PM -0800, mohamed salim abbas wrote:
> > > the interrupt moved from pci_probe to mac_start for power saving. once
> > > the interface is up the driver will read some register to know rfkill
> > > status, if the interface in down the driver don't care to keep track
> > > of rfkill switch. I wonder what the purpose of changing this behavior?
> > 
> > I think it still isn't settled in everyone's minds whether rfkill
> > only matters if the device is "up" or if it is something that
> > e.g. NetworkManager might want to monitor as a clue to bring the
> > device up or down in response to rfkill changes.
> 
> The question is:  does NetworkManager just always keep the device 'up'
> irregardless of whether it's supposed to be associated with anything
> just so we can get rfkill events?

Another question is: is it worth to keep the interface up (and thus the
firmware loaded) even if the transceiver is killed by a hardware switch?
Wouldn't that consume even more power than just listening to rfkill
interrupts (or polling the killswitch state in case of 3945) with no
firmware loaded.

> I guess I'll treat rfkill the same as ethernet carrier.  If we cannot
> rely on rfkill notifications when the device is down (we already can't,
> since iwl3945 simply can't do it) 

I've just checked the 3945 and it is indeed possible to poll the killswitch
state even if the firmware is not loaded. Hence 3945 could also expose
the killswitch state while the interface is down (of course the driver would
have to poll for that information).

> if there are quirks for devices, those quirks need to live in the driver,
> not worked around in userspace.

Agreed.

Helmut

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[Dan Williams]

On Mon, 2009-01-05 at 15:56 +0100, Helmut Schaa wrote:
> Am Mittwoch, 17. Dezember 2008 schrieb Dan Williams:
> > On Wed, 2008-12-17 at 15:29 -0500, John W. Linville wrote:
> > > On Wed, Dec 17, 2008 at 12:10:11PM -0800, mohamed salim abbas wrote:
> > > > the interrupt moved from pci_probe to mac_start for power saving. once
> > > > the interface is up the driver will read some register to know rfkill
> > > > status, if the interface in down the driver don't care to keep track
> > > > of rfkill switch. I wonder what the purpose of changing this behavior?
> > > 
> > > I think it still isn't settled in everyone's minds whether rfkill
> > > only matters if the device is "up" or if it is something that
> > > e.g. NetworkManager might want to monitor as a clue to bring the
> > > device up or down in response to rfkill changes.
> > 
> > The question is:  does NetworkManager just always keep the device 'up'
> > irregardless of whether it's supposed to be associated with anything
> > just so we can get rfkill events?
> 
> Another question is: is it worth to keep the interface up (and thus the
> firmware loaded) even if the transceiver is killed by a hardware switch?
> Wouldn't that consume even more power than just listening to rfkill
> interrupts (or polling the killswitch state in case of 3945) with no
> firmware loaded.
> 
> > I guess I'll treat rfkill the same as ethernet carrier.  If we cannot
> > rely on rfkill notifications when the device is down (we already can't,
> > since iwl3945 simply can't do it) 
> 
> I've just checked the 3945 and it is indeed possible to poll the killswitch
> state even if the firmware is not loaded. Hence 3945 could also expose
> the killswitch state while the interface is down (of course the driver would
> have to poll for that information).

Even polling the state once every 2 - 4 seconds would be perfectly
acceptable latency for me.  It doesn't have to be instantaneous, so we
can certainly trade off latency for fewer wakeups.  Care to propose a
patch?  it'll make a lot of people happy :)

Dan

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[Marcel Holtmann]

Hi Dan,

> > > > > the interrupt moved from pci_probe to mac_start for power saving. once
> > > > > the interface is up the driver will read some register to know rfkill
> > > > > status, if the interface in down the driver don't care to keep track
> > > > > of rfkill switch. I wonder what the purpose of changing this behavior?
> > > > 
> > > > I think it still isn't settled in everyone's minds whether rfkill
> > > > only matters if the device is "up" or if it is something that
> > > > e.g. NetworkManager might want to monitor as a clue to bring the
> > > > device up or down in response to rfkill changes.
> > > 
> > > The question is:  does NetworkManager just always keep the device 'up'
> > > irregardless of whether it's supposed to be associated with anything
> > > just so we can get rfkill events?
> > 
> > Another question is: is it worth to keep the interface up (and thus the
> > firmware loaded) even if the transceiver is killed by a hardware switch?
> > Wouldn't that consume even more power than just listening to rfkill
> > interrupts (or polling the killswitch state in case of 3945) with no
> > firmware loaded.
> > 
> > > I guess I'll treat rfkill the same as ethernet carrier.  If we cannot
> > > rely on rfkill notifications when the device is down (we already can't,
> > > since iwl3945 simply can't do it) 
> > 
> > I've just checked the 3945 and it is indeed possible to poll the killswitch
> > state even if the firmware is not loaded. Hence 3945 could also expose
> > the killswitch state while the interface is down (of course the driver would
> > have to poll for that information).
> 
> Even polling the state once every 2 - 4 seconds would be perfectly
> acceptable latency for me.  It doesn't have to be instantaneous, so we
> can certainly trade off latency for fewer wakeups.  Care to propose a
> patch?  it'll make a lot of people happy :)

if we can't get the rfkill state change via interrupt, then the kernel
driver has to poll for it (in the no-firmware cases). Or we have to
remove the rfkill support completely. Everything else is just not
acceptable. It is _not_ the responsibility of the userspace to make this
work with quirks.

Regards

Marcel

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[Dan Williams]

On Tue, 2009-01-06 at 17:41 +0100, Marcel Holtmann wrote:
> Hi Dan,
> 
> > > > > > the interrupt moved from pci_probe to mac_start for power saving. once
> > > > > > the interface is up the driver will read some register to know rfkill
> > > > > > status, if the interface in down the driver don't care to keep track
> > > > > > of rfkill switch. I wonder what the purpose of changing this behavior?
> > > > > 
> > > > > I think it still isn't settled in everyone's minds whether rfkill
> > > > > only matters if the device is "up" or if it is something that
> > > > > e.g. NetworkManager might want to monitor as a clue to bring the
> > > > > device up or down in response to rfkill changes.
> > > > 
> > > > The question is:  does NetworkManager just always keep the device 'up'
> > > > irregardless of whether it's supposed to be associated with anything
> > > > just so we can get rfkill events?
> > > 
> > > Another question is: is it worth to keep the interface up (and thus the
> > > firmware loaded) even if the transceiver is killed by a hardware switch?
> > > Wouldn't that consume even more power than just listening to rfkill
> > > interrupts (or polling the killswitch state in case of 3945) with no
> > > firmware loaded.
> > > 
> > > > I guess I'll treat rfkill the same as ethernet carrier.  If we cannot
> > > > rely on rfkill notifications when the device is down (we already can't,
> > > > since iwl3945 simply can't do it) 
> > > 
> > > I've just checked the 3945 and it is indeed possible to poll the killswitch
> > > state even if the firmware is not loaded. Hence 3945 could also expose
> > > the killswitch state while the interface is down (of course the driver would
> > > have to poll for that information).
> > 
> > Even polling the state once every 2 - 4 seconds would be perfectly
> > acceptable latency for me.  It doesn't have to be instantaneous, so we
> > can certainly trade off latency for fewer wakeups.  Care to propose a
> > patch?  it'll make a lot of people happy :)
> 
> if we can't get the rfkill state change via interrupt, then the kernel
> driver has to poll for it (in the no-firmware cases). Or we have to
> remove the rfkill support completely. Everything else is just not
> acceptable. It is _not_ the responsibility of the userspace to make this
> work with quirks.

Right; I meant polling in *iwl3945* every 2 - 4 seconds or so, not from
userspace.  Userspace should still listen for the rfkill uevents (not
that HAL does yet, but...)

Dan

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[Marcel Holtmann]

Hi Dan,

> > > > > > > the interrupt moved from pci_probe to mac_start for power saving. once
> > > > > > > the interface is up the driver will read some register to know rfkill
> > > > > > > status, if the interface in down the driver don't care to keep track
> > > > > > > of rfkill switch. I wonder what the purpose of changing this behavior?
> > > > > > 
> > > > > > I think it still isn't settled in everyone's minds whether rfkill
> > > > > > only matters if the device is "up" or if it is something that
> > > > > > e.g. NetworkManager might want to monitor as a clue to bring the
> > > > > > device up or down in response to rfkill changes.
> > > > > 
> > > > > The question is:  does NetworkManager just always keep the device 'up'
> > > > > irregardless of whether it's supposed to be associated with anything
> > > > > just so we can get rfkill events?
> > > > 
> > > > Another question is: is it worth to keep the interface up (and thus the
> > > > firmware loaded) even if the transceiver is killed by a hardware switch?
> > > > Wouldn't that consume even more power than just listening to rfkill
> > > > interrupts (or polling the killswitch state in case of 3945) with no
> > > > firmware loaded.
> > > > 
> > > > > I guess I'll treat rfkill the same as ethernet carrier.  If we cannot
> > > > > rely on rfkill notifications when the device is down (we already can't,
> > > > > since iwl3945 simply can't do it) 
> > > > 
> > > > I've just checked the 3945 and it is indeed possible to poll the killswitch
> > > > state even if the firmware is not loaded. Hence 3945 could also expose
> > > > the killswitch state while the interface is down (of course the driver would
> > > > have to poll for that information).
> > > 
> > > Even polling the state once every 2 - 4 seconds would be perfectly
> > > acceptable latency for me.  It doesn't have to be instantaneous, so we
> > > can certainly trade off latency for fewer wakeups.  Care to propose a
> > > patch?  it'll make a lot of people happy :)
> > 
> > if we can't get the rfkill state change via interrupt, then the kernel
> > driver has to poll for it (in the no-firmware cases). Or we have to
> > remove the rfkill support completely. Everything else is just not
> > acceptable. It is _not_ the responsibility of the userspace to make this
> > work with quirks.
> 
> Right; I meant polling in *iwl3945* every 2 - 4 seconds or so, not from
> userspace.  Userspace should still listen for the rfkill uevents (not
> that HAL does yet, but...)

you actually could use libudev for it directly. That is what I am doing
right now. Works pretty smooth.

Regards

Marcel

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[Dan Williams]

On Tue, 2009-01-06 at 18:05 +0100, Marcel Holtmann wrote:
> Hi Dan,
> 
> > > > > > > > the interrupt moved from pci_probe to mac_start for power saving. once
> > > > > > > > the interface is up the driver will read some register to know rfkill
> > > > > > > > status, if the interface in down the driver don't care to keep track
> > > > > > > > of rfkill switch. I wonder what the purpose of changing this behavior?
> > > > > > > 
> > > > > > > I think it still isn't settled in everyone's minds whether rfkill
> > > > > > > only matters if the device is "up" or if it is something that
> > > > > > > e.g. NetworkManager might want to monitor as a clue to bring the
> > > > > > > device up or down in response to rfkill changes.
> > > > > > 
> > > > > > The question is:  does NetworkManager just always keep the device 'up'
> > > > > > irregardless of whether it's supposed to be associated with anything
> > > > > > just so we can get rfkill events?
> > > > > 
> > > > > Another question is: is it worth to keep the interface up (and thus the
> > > > > firmware loaded) even if the transceiver is killed by a hardware switch?
> > > > > Wouldn't that consume even more power than just listening to rfkill
> > > > > interrupts (or polling the killswitch state in case of 3945) with no
> > > > > firmware loaded.
> > > > > 
> > > > > > I guess I'll treat rfkill the same as ethernet carrier.  If we cannot
> > > > > > rely on rfkill notifications when the device is down (we already can't,
> > > > > > since iwl3945 simply can't do it) 
> > > > > 
> > > > > I've just checked the 3945 and it is indeed possible to poll the killswitch
> > > > > state even if the firmware is not loaded. Hence 3945 could also expose
> > > > > the killswitch state while the interface is down (of course the driver would
> > > > > have to poll for that information).
> > > > 
> > > > Even polling the state once every 2 - 4 seconds would be perfectly
> > > > acceptable latency for me.  It doesn't have to be instantaneous, so we
> > > > can certainly trade off latency for fewer wakeups.  Care to propose a
> > > > patch?  it'll make a lot of people happy :)
> > > 
> > > if we can't get the rfkill state change via interrupt, then the kernel
> > > driver has to poll for it (in the no-firmware cases). Or we have to
> > > remove the rfkill support completely. Everything else is just not
> > > acceptable. It is _not_ the responsibility of the userspace to make this
> > > work with quirks.
> > 
> > Right; I meant polling in *iwl3945* every 2 - 4 seconds or so, not from
> > userspace.  Userspace should still listen for the rfkill uevents (not
> > that HAL does yet, but...)
> 
> you actually could use libudev for it directly. That is what I am doing
> right now. Works pretty smooth.

Do what with libudev?  Poll the register directly?  Or hit some sysfs
file that eventually polls the register?  And you said "it's not the
responsibility of userspace to make this work with quirks" which to me
says you'd prefer this polling to be done in the driver, not
userspace...  no?

Dan

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[Marcel Holtmann]

Hi Dan,

> > > > > > > > > the interrupt moved from pci_probe to mac_start for power saving. once
> > > > > > > > > the interface is up the driver will read some register to know rfkill
> > > > > > > > > status, if the interface in down the driver don't care to keep track
> > > > > > > > > of rfkill switch. I wonder what the purpose of changing this behavior?
> > > > > > > > 
> > > > > > > > I think it still isn't settled in everyone's minds whether rfkill
> > > > > > > > only matters if the device is "up" or if it is something that
> > > > > > > > e.g. NetworkManager might want to monitor as a clue to bring the
> > > > > > > > device up or down in response to rfkill changes.
> > > > > > > 
> > > > > > > The question is:  does NetworkManager just always keep the device 'up'
> > > > > > > irregardless of whether it's supposed to be associated with anything
> > > > > > > just so we can get rfkill events?
> > > > > > 
> > > > > > Another question is: is it worth to keep the interface up (and thus the
> > > > > > firmware loaded) even if the transceiver is killed by a hardware switch?
> > > > > > Wouldn't that consume even more power than just listening to rfkill
> > > > > > interrupts (or polling the killswitch state in case of 3945) with no
> > > > > > firmware loaded.
> > > > > > 
> > > > > > > I guess I'll treat rfkill the same as ethernet carrier.  If we cannot
> > > > > > > rely on rfkill notifications when the device is down (we already can't,
> > > > > > > since iwl3945 simply can't do it) 
> > > > > > 
> > > > > > I've just checked the 3945 and it is indeed possible to poll the killswitch
> > > > > > state even if the firmware is not loaded. Hence 3945 could also expose
> > > > > > the killswitch state while the interface is down (of course the driver would
> > > > > > have to poll for that information).
> > > > > 
> > > > > Even polling the state once every 2 - 4 seconds would be perfectly
> > > > > acceptable latency for me.  It doesn't have to be instantaneous, so we
> > > > > can certainly trade off latency for fewer wakeups.  Care to propose a
> > > > > patch?  it'll make a lot of people happy :)
> > > > 
> > > > if we can't get the rfkill state change via interrupt, then the kernel
> > > > driver has to poll for it (in the no-firmware cases). Or we have to
> > > > remove the rfkill support completely. Everything else is just not
> > > > acceptable. It is _not_ the responsibility of the userspace to make this
> > > > work with quirks.
> > > 
> > > Right; I meant polling in *iwl3945* every 2 - 4 seconds or so, not from
> > > userspace.  Userspace should still listen for the rfkill uevents (not
> > > that HAL does yet, but...)
> > 
> > you actually could use libudev for it directly. That is what I am doing
> > right now. Works pretty smooth.
> 
> Do what with libudev?  Poll the register directly?  Or hit some sysfs
> file that eventually polls the register?  And you said "it's not the
> responsibility of userspace to make this work with quirks" which to me
> says you'd prefer this polling to be done in the driver, not
> userspace...  no?

with libudev you can listen on udev events that translate directly from
uevent from the kernel. Don't confuse libudev with the broken libsysfs
attempt.

And this comment was meant in response for HAL not being able to notify
you of rfkill state changes. You don't need HAL for this if you want
proper support for rfkill events.

The iwl3945 driver has to do the polling of the rfkill register if the
firmware is not loaded. There is nothing we can or should do in
userspace.

Regards

Marcel

Re: [RFC][RFT] fix iwlagn hw-rfkill while the interface is down

[Helmut Schaa]

Am Dienstag, 6. Januar 2009 schrieb Dan Williams:
> On Mon, 2009-01-05 at 15:56 +0100, Helmut Schaa wrote:
> > Am Mittwoch, 17. Dezember 2008 schrieb Dan Williams:
> > > On Wed, 2008-12-17 at 15:29 -0500, John W. Linville wrote:
> > > > On Wed, Dec 17, 2008 at 12:10:11PM -0800, mohamed salim abbas wrote:
> > > > > the interrupt moved from pci_probe to mac_start for power saving. once
> > > > > the interface is up the driver will read some register to know rfkill
> > > > > status, if the interface in down the driver don't care to keep track
> > > > > of rfkill switch. I wonder what the purpose of changing this behavior?
> > > > 
> > > > I think it still isn't settled in everyone's minds whether rfkill
> > > > only matters if the device is "up" or if it is something that
> > > > e.g. NetworkManager might want to monitor as a clue to bring the
> > > > device up or down in response to rfkill changes.
> > > 
> > > The question is:  does NetworkManager just always keep the device 'up'
> > > irregardless of whether it's supposed to be associated with anything
> > > just so we can get rfkill events?
> > 
> > Another question is: is it worth to keep the interface up (and thus the
> > firmware loaded) even if the transceiver is killed by a hardware switch?
> > Wouldn't that consume even more power than just listening to rfkill
> > interrupts (or polling the killswitch state in case of 3945) with no
> > firmware loaded.
> > 
> > > I guess I'll treat rfkill the same as ethernet carrier.  If we cannot
> > > rely on rfkill notifications when the device is down (we already can't,
> > > since iwl3945 simply can't do it) 
> > 
> > I've just checked the 3945 and it is indeed possible to poll the killswitch
> > state even if the firmware is not loaded. Hence 3945 could also expose
> > the killswitch state while the interface is down (of course the driver would
> > have to poll for that information).
> 
> Even polling the state once every 2 - 4 seconds would be perfectly
> acceptable latency for me.  It doesn't have to be instantaneous, so we
> can certainly trade off latency for fewer wakeups.  Care to propose a
> patch?

Will do that soon.

Helmut