iwlwifi: iwl3945 synchronize interrupt and tasklet for down iwlwifi

After disabling interrupts, it's possible irq & tasklet is pending or running This patch eleminates races for down iwlwifi

---
 drivers/net/wireless/iwlwifi/iwl3945-base.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

index c97448d..3986aaf 100644
@@ -6262,6 +6262,10 @@ static void __iwl_down(struct iwl_priv *priv)
 	/* tell the device to stop sending interrupts */
 	iwl_disable_interrupts(priv);
 
+	/* synchronize irq and tasklet */
+	synchronize_irq(priv->pci_dev->irq);
+	tasklet_kill(&priv->irq_tasklet);
+
 	if (priv->mac80211_registered)
 		ieee80211_stop_queues(priv->hw);
 

1.5.3.rc5


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Hello Reinette,

2008/1/11, Chatre, Reinette <[email protected]>:

At this time, iwl_disable_interrupts() can be called with irq disabled, so for do that I think additional modification would be needed.

Thanks for your catch, it seems tasklet can re-enable interrupts. I'll handle and make an another patch for them at this weekend :)

Thanks,
Joonwoo

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

If this is the case where iwl_disable_interrupts() is called while in the ISR (where interrupts are disabled), then this behavior may be what we want as synchronize_irq() (as I understand) waits for the handler to complete irrespective of irq enable/disable.

What modification are you considering?

Please think it through also as I am exploring with you ...

Reinette

2008/1/11, Chatre, Reinette <[email protected]>:

I agree with you and it's what I want. the meaning 'irq disabled' was local irq. I'm sorry for confusing.

Roughly, I'm considering make synchronize_irq() be moved into iwl_disable_interrupts() and fix iwl_irq_tasket not to call iwl_disable_interrupts with irq disabled.
For now iwl_irq_tasklet calls iwl_disable_interrupts() with local irq disabled. like this:

static void iwl_irq_tasklet(struct iwl_priv *priv)

{
        ...
        spin_lock_irqsave(&priv->lock, flags);

        ...
        /* Now service all interrupt bits discovered above. */
        if (inta & CSR_INT_BIT_HW_ERR) {
                IWL_ERROR("Microcode HW error detected.  Restarting.\n");

                /* Tell the device to stop sending interrupts */
                iwl_disable_interrupts(priv);
                ...
                spin_unlock_irqrestore(&priv->lock, flags);
                return;
        }

Thanks again your comments.

Thanks,
Joonwoo

After disabling interrupts, it's possible irq & tasklet is pending or running This patch eleminates races for down iwlwifi

Since synchronize_irq can introduce iwl_irq_tasklet, tasklet_kill should be called after doing synchronize_irq
To avoid races between iwl_synchronize_irq and iwl_irq_tasklet STATUS_INT_ENABLED flag is needed.

---
  drivers/net/wireless/iwlwifi/iwl3945-base.c |   33 ++++++++++++++++++++++-----
  1 files changed, 27 insertions(+), 6 deletions(-)

index 6e20725..f98cd4f 100644
@@ -4405,6 +4405,14 @@ static void iwl_print_rx_config_cmd(struct iwl_rxon_cmd *rxon)
  }
  #endif
 
+static void iwl_synchronize_interrupts(struct iwl_priv *priv)
+{
+	synchronize_irq(priv->pci_dev->irq);
+	/* synchornize_irq introduces irq_tasklet,
+	 * tasklet_kill should be called after doing synchronize_irq */
+	tasklet_kill(&priv->irq_tasklet);
+}
+
  static void iwl_enable_interrupts(struct iwl_priv *priv)
  {
  	IWL_DEBUG_ISR("Enabling interrupts\n");

@@ -4413,7 +4421,7 @@ static void iwl_enable_interrupts(struct iwl_priv *priv)
  	iwl_flush32(priv, CSR_INT_MASK);
  }
 
-static inline void iwl_disable_interrupts(struct iwl_priv *priv)
+static inline void __iwl_disable_interrupts(struct iwl_priv *priv)
  {
  	clear_bit(STATUS_INT_ENABLED, &priv->status);
 

@@ -4427,6 +4435,13 @@ static inline void iwl_disable_interrupts(struct iwl_priv *priv)
  	iwl_flush32(priv, CSR_INT);
  	iwl_write32(priv, CSR_FH_INT_STATUS, 0xffffffff);
  	iwl_flush32(priv, CSR_FH_INT_STATUS);
+}
+
+static inline void iwl_disable_interrupts(struct iwl_priv *priv)
+{
+	__iwl_disable_interrupts(priv);
+
+	iwl_synchronize_interrupts(priv);
 
  	IWL_DEBUG_ISR("Disabled interrupts\n");
  }

@@ -4708,7 +4723,8 @@ static void iwl_irq_tasklet(struct iwl_priv *priv)
  		IWL_ERROR("Microcode HW error detected.  Restarting.\n");
 
  		/* Tell the device to stop sending interrupts */
-		iwl_disable_interrupts(priv);
+		__iwl_disable_interrupts(priv);
+		IWL_DEBUG_ISR("Disabled interrupts\n");
 
  		iwl_irq_handle_error(priv);
 

@@ -4814,8 +4830,11 @@ static void iwl_irq_tasklet(struct iwl_priv *priv)
  		IWL_WARNING("   with FH_INT = 0x%08x\n", inta_fh);
  	}
 
-	/* Re-enable all interrupts */
-	iwl_enable_interrupts(priv);
+	/* To avoid race when device goes down,
+	 * it should be discarded to enable interrupts */
+	if (test_bit(STATUS_INT_ENABLED, &priv->status))
+		/* Re-enable all interrupts */
+		iwl_enable_interrupts(priv);
 
  #ifdef CONFIG_IWLWIFI_DEBUG
  	if (iwl_debug_level & (IWL_DL_ISR)) {

@@ -4876,8 +4895,10 @@ unplugged:
  	return IRQ_HANDLED;
 
   none:
-	/* re-enable interrupts here since we don't have anything to service. */
-	iwl_enable_interrupts(priv);
+	if (test_bit(STATUS_INT_ENABLED, &priv->status))
+		/* re-enable interrupts here since we don't have anything
+		 * to service. */
+		iwl_enable_interrupts(priv);
  	spin_unlock(&priv->lock);
  	return IRQ_NONE;
  }

---

Thanks,
Joonwoo

I'm so sorry for mangled patch.
resending patch with preformat html from thunderbird.

After disabling interrupts, it's possible irq & tasklet is pending or running This patch eleminates races for down iwlwifi.

Since synchronize_irq can introduce iwl_irq_tasklet, tasklet_kill should be called after doing synchronize_irq. To avoid races between iwl_synchronize_irq and iwl_irq_tasklet
STATUS_INT_ENABLED flag is needed.

---
drivers/net/wireless/iwlwifi/iwl3945-base.c | 33 ++++++++++++++++++++++-----

1 files changed, 27 insertions(+), 6 deletions(-)

index 6e20725..f98cd4f 100644
@@ -4405,6 +4405,14 @@ static void iwl_print_rx_config_cmd(struct iwl_rxon_cmd *rxon)

}
#endif

+static void iwl_synchronize_interrupts(struct iwl_priv *priv) +{
+ synchronize_irq(priv->pci_dev->irq);
+ /* synchornize_irq introduces irq_tasklet,
+ * tasklet_kill should be called after doing synchronize_irq */
+ tasklet_kill(&priv->irq_tasklet);
+}
+
static void iwl_enable_interrupts(struct iwl_priv *priv)

{
	IWL_DEBUG_ISR("Enabling interrupts\n");
@@ -4413,7 +4421,7 @@ static void iwl_enable_interrupts(struct iwl_priv *priv)
	iwl_flush32(priv, CSR_INT_MASK);
}

-static inline void iwl_disable_interrupts(struct iwl_priv *priv) +static inline void __iwl_disable_interrupts(struct iwl_priv *priv)

{
	clear_bit(STATUS_INT_ENABLED, &priv->status);

@@ -4427,6 +4435,13 @@ static inline void iwl_disable_interrupts(struct iwl_priv *priv)

	iwl_flush32(priv, CSR_INT);
	iwl_write32(priv, CSR_FH_INT_STATUS, 0xffffffff);
	iwl_flush32(priv, CSR_FH_INT_STATUS);

+}
+
+static inline void iwl_disable_interrupts(struct iwl_priv *priv) +{
+ __iwl_disable_interrupts(priv);
+
+ iwl_synchronize_interrupts(priv);

	IWL_DEBUG_ISR("Disabled interrupts\n");
}
@@ -4708,7 +4723,8 @@ static void iwl_irq_tasklet(struct iwl_priv *priv)
		IWL_ERROR("Microcode HW error detected.  Restarting.\n");

		/* Tell the device to stop sending interrupts */

- iwl_disable_interrupts(priv);
+ __iwl_disable_interrupts(priv);
+ IWL_DEBUG_ISR("Disabled interrupts\n");

		iwl_irq_handle_error(priv);

@@ -4814,8 +4830,11 @@ static void iwl_irq_tasklet(struct iwl_priv *priv)

		IWL_WARNING("   with FH_INT = 0x%08x\n", inta_fh);
	}

- /* Re-enable all interrupts */
- iwl_enable_interrupts(priv);
+ /* To avoid race when device goes down,
+ * it should be discarded to enable interrupts */
+ if (test_bit(STATUS_INT_ENABLED, &priv->status))
+ /* Re-enable all interrupts */
+ iwl_enable_interrupts(priv);

#ifdef CONFIG_IWLWIFI_DEBUG
	if (iwl_debug_level & (IWL_DL_ISR)) {
@@ -4876,8 +4895,10 @@ unplugged:
	return IRQ_HANDLED;

none:
- /* re-enable interrupts here since we don't have anything to service. */
- iwl_enable_interrupts(priv);
+ if (test_bit(STATUS_INT_ENABLED, &priv->status))
+ /* re-enable interrupts here since we don't have anything
+ * to service. */

+		iwl_enable_interrupts(priv);
	spin_unlock(&priv->lock);
	return IRQ_NONE;
}

---

Thanks,
Joonwoo

Could the call to iwl_disable_interrupts in iwl_pci_probe be changed to __iwl_disable_interrupts? The isr is only activated when the interface is brought up so we do not need to do the extra steps (eg. synchronize_irq()) during device probe.

Reinette

2008/1/16, Chatre, Reinette <[email protected]>:

Hi Reinette,
Thank you for your advice, it seems recent base code's commit inserted iwl_disable_interrupts() into iwl_pci_probe.
As like you said, for base code __iwl_disable_interrupts() should better than iwl_disable_interrupts for iwl_pci_probe.

Zhu,
Do you want version of patch for base code?
Can you ACK for this patch?

Thanks,
Joonwoo

I've modified the patch to include this.

Your patch looks good and is currently being tested.

Thanks!

Reinette