After disabling interrupts, it's possible irq & tasklet is pending or running
This patch eleminates races for down iwlwifi
Signed-off-by: Joonwoo Park <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl3945-base.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index c97448d..3986aaf 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -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
Hello Reinette,
2008/1/11, Chatre, Reinette <[email protected]>:
>
> Could synchronize_irq() be moved into iwl_disable_interrupts() ? I am
At this time, iwl_disable_interrupts() can be called with irq
disabled, so for do that I think additional modification would be
needed.
> also wondering if we cannot call tasklet_kill() before
> iwl_disable_interrupts() ... thus preventing it from being scheduled
> when we are going down.
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
On Monday, January 14, 2008 1:43 AM, Joonwoo Park wrote:
> -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);
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/11, Chatre, Reinette <[email protected]>:
>
> On Thursday, January 10, 2008 5:25 PM, Joonwoo Park wrote:
>
> > 2008/1/11, Chatre, Reinette <[email protected]>:
> >>
> >> Could synchronize_irq() be moved into iwl_disable_interrupts() ? I am
> >
> > At this time, iwl_disable_interrupts() can be called with irq
> > disabled, so for do that I think additional modification would be
> > needed.
>
> 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.
I agree with you and it's what I want. the meaning 'irq disabled' was local irq.
I'm sorry for confusing.
>
> What modification are you considering?
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;
}
>
> >> also wondering if we cannot call tasklet_kill() before
> >> iwl_disable_interrupts() ... thus preventing it from being scheduled
> >> when we are going down.
> >
> > Thanks for your catch, it seems tasklet can re-enable interrupts.
> > I'll handle and make an another patch for them at this weekend :)
>
> Please think it through also as I am exploring with you ...
Thanks again your comments.
Thanks,
Joonwoo
On , Joonwoo Park wrote:
> --- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
> +++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
> @@ -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);
> +
Could synchronize_irq() be moved into iwl_disable_interrupts() ? I am
also wondering if we cannot call tasklet_kill() before
iwl_disable_interrupts() ... thus preventing it from being scheduled
when we are going down.
Reinette
2008/1/16, Chatre, Reinette <[email protected]>:
> On Monday, January 14, 2008 1:43 AM, Joonwoo Park wrote:
>
> > -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);
>
> 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.
>
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'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.
Signed-off-by: Joonwoo Park <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl3945-base.c | 33 ++++++++++++++++++++++-----
1 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 6e20725..f98cd4f 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -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
On Tuesday, January 15, 2008 8:15 PM, Joonwoo Park wrote:
> 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.
I've modified the patch to include this.
> Do you want version of patch for base code?
> Can you ACK for this patch?
Your patch looks good and is currently being tested.
Thanks!
Reinette
Joonwoo Park wrote:
> 2008/1/11, Chatre, Reinette <[email protected]>:
>> On Thursday, January 10, 2008 5:25 PM, Joonwoo Park wrote:
>
>> What modification are you considering?
>
> 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;
> }
>
>
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.
Signed-off-by: Joonwoo Park <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl3945-base.c | 33 ++++++++++++++++++++++-----
1 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 6e20725..f98cd4f 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -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
On Thursday, January 10, 2008 5:25 PM, Joonwoo Park wrote:
> 2008/1/11, Chatre, Reinette <[email protected]>:
>>
>> Could synchronize_irq() be moved into iwl_disable_interrupts() ? I am
>
> At this time, iwl_disable_interrupts() can be called with irq
> disabled, so for do that I think additional modification would be
> needed.
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?
>> also wondering if we cannot call tasklet_kill() before
>> iwl_disable_interrupts() ... thus preventing it from being scheduled
>> when we are going down.
>
> Thanks for your catch, it seems tasklet can re-enable interrupts.
> I'll handle and make an another patch for them at this weekend :)
Please think it through also as I am exploring with you ...
Reinette