| Message ID | 1379331546-30617-2-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Daniel, Thanks for the patch. > Queue main_work in case mwifiex_main_process() bails due to an already > processed transaction. This is particularly necessary because > mwifiex_main_process() is called from both the SDIO interrupt handler > and the workqueue. In case an interrupt occurs while the main process > is currently executed from the workqueue, the interrupt is lost, > resulting in a command timeout and consequently a card reset. > > I'm marking this for stable kernel in version 3.7+, because on our > platform, the issue appears since 601216e12c ("mwifiex: process RX > packets in SDIO IRQ thread directly") went in. > > Signed-off-by: Daniel Mack <zonque@gmail.com> > Reported-by: Sven Neumann <s.neumann@raumfeld.com> > Reported-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com> > Cc: Bing Zhao <bzhao@marvell.com> > Cc: <stable@vger.kernel.org> [v3.7+] > --- > drivers/net/wireless/mwifiex/main.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/wireless/mwifiex/main.c b/drivers/net/wireless/mwifiex/main.c > index ff4ed96..0700bc2 100644 > --- a/drivers/net/wireless/mwifiex/main.c > +++ b/drivers/net/wireless/mwifiex/main.c > @@ -235,6 +235,7 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter) > /* Check if already processing */ > if (adapter->mwifiex_processing) { > spin_unlock_irqrestore(&adapter->main_proc_lock, flags); > + queue_work(adapter->workqueue, &adapter->main_work); This is specific to SDIO interface, so we'd add a check here. + if (adapter->iface_type == MWIFIEX_SDIO) + queue_work(adapter->workqueue, &adapter->main_work); Thanks, Bing > goto exit_main_proc; > } else { > adapter->mwifiex_processing = true; > -- > 1.8.3.1 -- 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
Hi Bing, On 16.09.2013 23:14, Bing Zhao wrote: >> diff --git a/drivers/net/wireless/mwifiex/main.c b/drivers/net/wireless/mwifiex/main.c >> index ff4ed96..0700bc2 100644 >> --- a/drivers/net/wireless/mwifiex/main.c >> +++ b/drivers/net/wireless/mwifiex/main.c >> @@ -235,6 +235,7 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter) >> /* Check if already processing */ >> if (adapter->mwifiex_processing) { >> spin_unlock_irqrestore(&adapter->main_proc_lock, flags); >> + queue_work(adapter->workqueue, &adapter->main_work); > > This is specific to SDIO interface, Is it really? By checking adapter->mwifiex_processing, the driver seems to expect mwifiex_main_process() to be called from multiple execution paths, and in that case, we will always loose one execution cycle in case we bail early. I actually wonder why this didn't hit us earlier, but I might miss a detail. OTOH, the worst thing that can happen if the function is executed too often is that it exits early and does nothing. > + if (adapter->iface_type == MWIFIEX_SDIO) > + queue_work(adapter->workqueue, &adapter->main_work); I can of course add this, but I don't fully understand why the driver takes care of concurrently running executing paths and then just bails silently in case a race is detected. Best regards, Daniel -- 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
Hi Daniel, > >> /* Check if already processing */ > >> if (adapter->mwifiex_processing) { > >> spin_unlock_irqrestore(&adapter->main_proc_lock, flags); > >> + queue_work(adapter->workqueue, &adapter->main_work); > > > > This is specific to SDIO interface, > > Is it really? By checking adapter->mwifiex_processing, the driver seems > to expect mwifiex_main_process() to be called from multiple execution > paths, and in that case, we will always loose one execution cycle in You are right. I overlooked it. > case we bail early. I actually wonder why this didn't hit us earlier, > but I might miss a detail. I guess, in your case, the interrupt comes in at line 363 where you have passed the int_status or RX_RCVD checking but the mwifiex_processing flag is still true. 361 if ((adapter->int_status) || IS_CARD_RX_RCVD(adapter)) 362 goto process_start; 363 364 spin_lock_irqsave(&adapter->main_proc_lock, flags); 365 adapter->mwifiex_processing = false; 366 spin_unlock_irqrestore(&adapter->main_proc_lock, flags); The interrupt thread exits because mwifiex_processing is true. Therefore the mwifiex_main_process misses this interrupt. > > OTOH, the worst thing that can happen if the function is executed too > often is that it exits early and does nothing. > > > + if (adapter->iface_type == MWIFIEX_SDIO) > > + queue_work(adapter->workqueue, &adapter->main_work); > > I can of course add this, but I don't fully understand why the driver > takes care of concurrently running executing paths and then just bails > silently in case a race is detected. No. Your original patch is fine. Could you resend it as [PATCH 3.12]? I will ACK in that thread. Thanks, Bing -- 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
diff --git a/drivers/net/wireless/mwifiex/main.c b/drivers/net/wireless/mwifiex/main.c index ff4ed96..0700bc2 100644 --- a/drivers/net/wireless/mwifiex/main.c +++ b/drivers/net/wireless/mwifiex/main.c @@ -235,6 +235,7 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter) /* Check if already processing */ if (adapter->mwifiex_processing) { spin_unlock_irqrestore(&adapter->main_proc_lock, flags); + queue_work(adapter->workqueue, &adapter->main_work); goto exit_main_proc; } else { adapter->mwifiex_processing = true;
Queue main_work in case mwifiex_main_process() bails due to an already processed transaction. This is particularly necessary because mwifiex_main_process() is called from both the SDIO interrupt handler and the workqueue. In case an interrupt occurs while the main process is currently executed from the workqueue, the interrupt is lost, resulting in a command timeout and consequently a card reset. I'm marking this for stable kernel in version 3.7+, because on our platform, the issue appears since 601216e12c ("mwifiex: process RX packets in SDIO IRQ thread directly") went in. Signed-off-by: Daniel Mack <zonque@gmail.com> Reported-by: Sven Neumann <s.neumann@raumfeld.com> Reported-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com> Cc: Bing Zhao <bzhao@marvell.com> Cc: <stable@vger.kernel.org> [v3.7+] --- drivers/net/wireless/mwifiex/main.c | 1 + 1 file changed, 1 insertion(+)