[3/4] mwifiex: Flag wake-up interrupt as IRQ_NOAUTOEN rather than disabling it too late
diff mbox series

Message ID 20190224140426.3267-4-marc.zyngier@arm.com
State New
Headers show
Series
  • mwifiex PCI/wake-up interrupt fixes
Related show

Commit Message

Marc Zyngier Feb. 24, 2019, 2:04 p.m. UTC
The mwifiex driver makes unsafe assumptions about the state of the
wake-up interrupt. It requests it and only then disable it. Of
course, the interrupt may be screaming for whatever reason at that
time, and the handler will then be called without the interrupt
having been registered with the PM/wakeup subsystem. Oops.

The right way to handle this kind of situation is to flag the
interrupt with IRQ_NOAUTOEN before requesting it. It will then
stay disabled until someone (the wake-up subsystem) enables it.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/net/wireless/marvell/mwifiex/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Brian Norris Feb. 26, 2019, 11:31 p.m. UTC | #1
Hi Marc,

On Sun, Feb 24, 2019 at 02:04:25PM +0000, Marc Zyngier wrote:
> The mwifiex driver makes unsafe assumptions about the state of the
> wake-up interrupt. It requests it and only then disable it. Of
> course, the interrupt may be screaming for whatever reason at that
> time, and the handler will then be called without the interrupt
> having been registered with the PM/wakeup subsystem. Oops.
> 
> The right way to handle this kind of situation is to flag the
> interrupt with IRQ_NOAUTOEN before requesting it. It will then
> stay disabled until someone (the wake-up subsystem) enables it.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

This could be:

Fixes: 853402a00823 ("mwifiex: Enable WoWLAN for both sdio and pcie")

although, that was just borrowing the existing antipattern from SDIO
code..

Reviewed-by: Brian Norris <briannorris@chromium.org>

Thanks.

> ---
>  drivers/net/wireless/marvell/mwifiex/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index 2105c2b7c627..82cf35e50579 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1610,6 +1610,7 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
>  			 "wake-up interrupt outside 'wake-up' subnode of %pOF\n",
>  			 adapter->dt_node);
>  
> +	irq_set_status_flags(adapter->irq_wakeup, IRQ_NOAUTOEN);
>  	ret = devm_request_irq(dev, adapter->irq_wakeup,
>  			       mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
>  			       "wifi_wake", adapter);
> @@ -1619,7 +1620,6 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
>  		goto err_exit;
>  	}
>  
> -	disable_irq(adapter->irq_wakeup);
>  	if (device_init_wakeup(dev, true)) {
>  		dev_err(dev, "fail to init wakeup for mwifiex\n");
>  		goto err_exit;
> -- 
> 2.20.1
>
Brian Norris Feb. 26, 2019, 11:34 p.m. UTC | #2
On Tue, Feb 26, 2019 at 03:31:31PM -0800, Brian Norris wrote:
> Hi Marc,
> 
> On Sun, Feb 24, 2019 at 02:04:25PM +0000, Marc Zyngier wrote:
> > The mwifiex driver makes unsafe assumptions about the state of the
> > wake-up interrupt. It requests it and only then disable it. Of
> > course, the interrupt may be screaming for whatever reason at that
> > time, and the handler will then be called without the interrupt
> > having been registered with the PM/wakeup subsystem. Oops.
> > 
> > The right way to handle this kind of situation is to flag the
> > interrupt with IRQ_NOAUTOEN before requesting it. It will then
> > stay disabled until someone (the wake-up subsystem) enables it.
> > 
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> This could be:
> 
> Fixes: 853402a00823 ("mwifiex: Enable WoWLAN for both sdio and pcie")

Also, this comes after a different change (that's not quite as clearly a
backport-able bugfix). Perhaps it should go first in the series?

Brian

> although, that was just borrowing the existing antipattern from SDIO
> code..
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> 
> Thanks.
> 
> > ---
> >  drivers/net/wireless/marvell/mwifiex/main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> > index 2105c2b7c627..82cf35e50579 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -1610,6 +1610,7 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> >  			 "wake-up interrupt outside 'wake-up' subnode of %pOF\n",
> >  			 adapter->dt_node);
> >  
> > +	irq_set_status_flags(adapter->irq_wakeup, IRQ_NOAUTOEN);
> >  	ret = devm_request_irq(dev, adapter->irq_wakeup,
> >  			       mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
> >  			       "wifi_wake", adapter);
> > @@ -1619,7 +1620,6 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> >  		goto err_exit;
> >  	}
> >  
> > -	disable_irq(adapter->irq_wakeup);
> >  	if (device_init_wakeup(dev, true)) {
> >  		dev_err(dev, "fail to init wakeup for mwifiex\n");
> >  		goto err_exit;
> > -- 
> > 2.20.1
> >
Kalle Valo April 4, 2019, 10:22 a.m. UTC | #3
Marc Zyngier <marc.zyngier@arm.com> wrote:

> The mwifiex driver makes unsafe assumptions about the state of the
> wake-up interrupt. It requests it and only then disable it. Of
> course, the interrupt may be screaming for whatever reason at that
> time, and the handler will then be called without the interrupt
> having been registered with the PM/wakeup subsystem. Oops.
> 
> The right way to handle this kind of situation is to flag the
> interrupt with IRQ_NOAUTOEN before requesting it. It will then
> stay disabled until someone (the wake-up subsystem) enables it.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Reviewed-by: Brian Norris <briannorris@chromium.org>

Failed to apply:

fatal: sha1 information is lacking or useless (drivers/net/wireless/marvell/mwifiex/main.c).
error: could not build fake ancestor
Applying: mwifiex: Flag wake-up interrupt as IRQ_NOAUTOEN rather than disabling it too late
Patch failed at 0001 mwifiex: Flag wake-up interrupt as IRQ_NOAUTOEN rather than disabling it too late
The copy of the patch that failed is found in: .git/rebase-apply/patch

Patch set to Changes Requested.

Patch
diff mbox series

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 2105c2b7c627..82cf35e50579 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1610,6 +1610,7 @@  static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
 			 "wake-up interrupt outside 'wake-up' subnode of %pOF\n",
 			 adapter->dt_node);
 
+	irq_set_status_flags(adapter->irq_wakeup, IRQ_NOAUTOEN);
 	ret = devm_request_irq(dev, adapter->irq_wakeup,
 			       mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
 			       "wifi_wake", adapter);
@@ -1619,7 +1620,6 @@  static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
 		goto err_exit;
 	}
 
-	disable_irq(adapter->irq_wakeup);
 	if (device_init_wakeup(dev, true)) {
 		dev_err(dev, "fail to init wakeup for mwifiex\n");
 		goto err_exit;