* [PATCH 2/5] iwlwifi: iwl3945 synchronize interrupt and tasklet for down iwlwifi @ 2008-01-09 11:02 Joonwoo Park 2008-01-10 19:10 ` [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand " Chatre, Reinette 0 siblings, 1 reply; 10+ messages in thread From: Joonwoo Park @ 2008-01-09 11:02 UTC (permalink / raw) To: Zhu Yi, netdev; +Cc: lkml, ipw3945-devel, linux-wireless, Joonwoo Park After disabling interrupts, it's possible irq & tasklet is pending or running This patch eleminates races for down iwlwifi Signed-off-by: Joonwoo Park <joonwpark81@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi 2008-01-09 11:02 [PATCH 2/5] iwlwifi: iwl3945 synchronize interrupt and tasklet for down iwlwifi Joonwoo Park @ 2008-01-10 19:10 ` Chatre, Reinette 2008-01-11 1:24 ` Joonwoo Park 0 siblings, 1 reply; 10+ messages in thread From: Chatre, Reinette @ 2008-01-10 19:10 UTC (permalink / raw) To: Joonwoo Park, Zhu, Yi, netdev; +Cc: linux-wireless, lkml, ipw3945-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi 2008-01-10 19:10 ` [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand " Chatre, Reinette @ 2008-01-11 1:24 ` Joonwoo Park 2008-01-11 1:42 ` Chatre, Reinette 0 siblings, 1 reply; 10+ messages in thread From: Joonwoo Park @ 2008-01-11 1:24 UTC (permalink / raw) To: Chatre, Reinette; +Cc: Zhu, Yi, netdev, linux-wireless, lkml, ipw3945-devel Hello Reinette, 2008/1/11, Chatre, Reinette <reinette.chatre@intel.com>: > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi 2008-01-11 1:24 ` Joonwoo Park @ 2008-01-11 1:42 ` Chatre, Reinette 2008-01-11 2:38 ` Joonwoo Park 0 siblings, 1 reply; 10+ messages in thread From: Chatre, Reinette @ 2008-01-11 1:42 UTC (permalink / raw) To: Joonwoo Park; +Cc: Zhu, Yi, linux-wireless, ipw3945-devel On Thursday, January 10, 2008 5:25 PM, Joonwoo Park wrote: > 2008/1/11, Chatre, Reinette <reinette.chatre@intel.com>: >> >> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi 2008-01-11 1:42 ` Chatre, Reinette @ 2008-01-11 2:38 ` Joonwoo Park 2008-01-14 8:35 ` Joonwoo Park 0 siblings, 1 reply; 10+ messages in thread From: Joonwoo Park @ 2008-01-11 2:38 UTC (permalink / raw) To: Chatre, Reinette; +Cc: Zhu, Yi, linux-wireless, ipw3945-devel 2008/1/11, Chatre, Reinette <reinette.chatre@intel.com>: > > On Thursday, January 10, 2008 5:25 PM, Joonwoo Park wrote: > > > 2008/1/11, Chatre, Reinette <reinette.chatre@intel.com>: > >> > >> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi 2008-01-11 2:38 ` Joonwoo Park @ 2008-01-14 8:35 ` Joonwoo Park 2008-01-14 9:42 ` Joonwoo Park 0 siblings, 1 reply; 10+ messages in thread From: Joonwoo Park @ 2008-01-14 8:35 UTC (permalink / raw) To: Zhu, Yi, netdev Cc: Chatre, Reinette, linux-wireless, ipw3945-devel, linux-kernel Joonwoo Park wrote: > 2008/1/11, Chatre, Reinette <reinette.chatre@intel.com>: >> 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 <joonwpark81@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi 2008-01-14 8:35 ` Joonwoo Park @ 2008-01-14 9:42 ` Joonwoo Park 2008-01-15 19:21 ` Chatre, Reinette 0 siblings, 1 reply; 10+ messages in thread From: Joonwoo Park @ 2008-01-14 9:42 UTC (permalink / raw) To: Zhu, Yi, netdev Cc: Chatre, Reinette, linux-wireless, ipw3945-devel, linux-kernel 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 <joonwpark81@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi 2008-01-14 9:42 ` Joonwoo Park @ 2008-01-15 19:21 ` Chatre, Reinette 2008-01-16 4:15 ` Joonwoo Park 0 siblings, 1 reply; 10+ messages in thread From: Chatre, Reinette @ 2008-01-15 19:21 UTC (permalink / raw) To: Joonwoo Park, Zhu, Yi; +Cc: linux-wireless, ipw3945-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi 2008-01-15 19:21 ` Chatre, Reinette @ 2008-01-16 4:15 ` Joonwoo Park 2008-01-16 16:37 ` Chatre, Reinette 0 siblings, 1 reply; 10+ messages in thread From: Joonwoo Park @ 2008-01-16 4:15 UTC (permalink / raw) To: Chatre, Reinette, Zhu, Yi; +Cc: linux-wireless, ipw3945-devel 2008/1/16, Chatre, Reinette <reinette.chatre@intel.com>: > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi 2008-01-16 4:15 ` Joonwoo Park @ 2008-01-16 16:37 ` Chatre, Reinette 0 siblings, 0 replies; 10+ messages in thread From: Chatre, Reinette @ 2008-01-16 16:37 UTC (permalink / raw) To: Joonwoo Park, Zhu, Yi; +Cc: linux-wireless, ipw3945-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2008-01-16 16:37 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-01-09 11:02 [PATCH 2/5] iwlwifi: iwl3945 synchronize interrupt and tasklet for down iwlwifi Joonwoo Park 2008-01-10 19:10 ` [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand " Chatre, Reinette 2008-01-11 1:24 ` Joonwoo Park 2008-01-11 1:42 ` Chatre, Reinette 2008-01-11 2:38 ` Joonwoo Park 2008-01-14 8:35 ` Joonwoo Park 2008-01-14 9:42 ` Joonwoo Park 2008-01-15 19:21 ` Chatre, Reinette 2008-01-16 4:15 ` Joonwoo Park 2008-01-16 16:37 ` Chatre, Reinette
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).