[PATCH 2/5] iwlwifi: iwl3945 synchronize interrupt and tasklet for down iwlwifi - Kernel

This is a discussion on [PATCH 2/5] iwlwifi: iwl3945 synchronize interrupt and tasklet for down iwlwifi - Kernel ; After disabling interrupts, it's possible irq & tasklet is pending or running This patch eleminates races for down iwlwifi Signed-off-by: Joonwoo Park --- 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 ...

+ Reply to Thread
Results 1 to 5 of 5

Thread: [PATCH 2/5] iwlwifi: iwl3945 synchronize interrupt and tasklet for down iwlwifi

  1. [PATCH 2/5] 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

    Signed-off-by: Joonwoo Park
    ---
    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. RE: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi

    Hello Reinette,

    2008/1/11, Chatre, Reinette :
    >
    > 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi

    Joonwoo Park wrote:
    > 2008/1/11, Chatre, Reinette :
    >> 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
    ---
    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [ipw3945-devel] [PATCH 2/5] iwlwifi: iwl3945 synchronize interruptand tasklet for down iwlwifi

    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
    ---
    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread