Re: [PATCH v2 3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[Brian Norris <briannorris@chromium.org>]

Hi,

One correction to my review:

On Fri, Nov 11, 2016 at 12:42:36PM -0800, Brian Norris wrote:
> On Fri, Nov 11, 2016 at 04:45:11PM +0530, Amitkumar Karwar wrote:
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -1552,14 +1552,55 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
> >  }
> >  EXPORT_SYMBOL_GPL(mwifiex_do_flr);
> >  
> > +static irqreturn_t mwifiex_irq_wakeup_handler(int irq, void *priv)
> > +{
> > +	struct mwifiex_adapter *adapter = priv;
> > +
> > +	if (adapter->irq_wakeup >= 0) {
> > +		dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> > +		adapter->wake_by_wifi = true;
> 
> This flag is unecessary and buggy. IIUC, you're trying to avoid calling
> disable_irq() in resume(), if you've called it here?
> 
> > +		disable_irq_nosync(irq);
> 
> ...but this is unnecessary, I think, unless you're trying to make up for
> buggy wakeup interrupts that keep firing? See my suggestion below.

I think I figured out some of the logic here, and my suggestion was
somewhat wrong. This 'wake_by_wifi' flag seems to be used here to assist
with the fact that we're requesting a level-triggered interrupt, but
the card doesn't deassert the interrupt until much later -- when we
finish the wakeup handshake.

So I guess it is necessary (or at least, expedient) to disable the
interrupt here.

> > +	}
> > +
> > +	/* Notify PM core we are wakeup source */
> > +	pm_wakeup_event(adapter->dev, 0);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> >  {
> > +	int ret;
> >  	struct device *dev = adapter->dev;
> >  
> >  	if (!dev->of_node)
> >  		return;
> >  
> >  	adapter->dt_node = dev->of_node;
> > +	adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
> > +	if (!adapter->irq_wakeup) {
> > +		dev_info(dev, "fail to parse irq_wakeup from device tree\n");
> > +		return;
> > +	}
> > +
> > +	ret = devm_request_irq(dev, adapter->irq_wakeup,
> > +			       mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
> > +			       "wifi_wake", adapter);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to request irq_wakeup %d (%d)\n",
> > +			adapter->irq_wakeup, ret);
> > +		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;
> > +	}
> > +	return;
> > +
> > +err_exit:
> > +	adapter->irq_wakeup = 0;
> >  }
> >  
> >  /*
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> > index 0c07434..11abc49 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.h
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> > @@ -1011,6 +1011,10 @@ struct mwifiex_adapter {
> >  	bool usb_mc_setup;
> >  	struct cfg80211_wowlan_nd_info *nd_info;
> >  	struct ieee80211_regdomain *regd;
> > +
> > +	/* Wake-on-WLAN (WoWLAN) */
> > +	int irq_wakeup;
> > +	bool wake_by_wifi;
> >  };
> >  
> >  void mwifiex_process_tx_queue(struct mwifiex_adapter *adapter);
> > @@ -1410,6 +1414,27 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
> >  	return false;
> >  }
> >  
> > +/* Disable platform specific wakeup interrupt */
> > +static inline void mwifiex_disable_wake(struct mwifiex_adapter *adapter)
> > +{
> > +	if (adapter->irq_wakeup >= 0) {
> > +		disable_irq_wake(adapter->irq_wakeup);
> > +		if (!adapter->wake_by_wifi)
> 
> You're depending on the wake IRQ handler to set this flag, so you don't
> disable the IRQ twice (the calls nest). But what if the IRQ is being
> serviced concurrently with this? Then you'll double-disable the IRQ.
> 
> I believe you can just remove the disable_irq_nosync() from the handler,
> kill the above flag, and just unconditionally disable the IRQ.

So my suggestion here was wrong; we shouldn't completely kill the check
for ->wake_by_wifi I don't think, but we *should* wait for the interrupt
handler to complete before checking the flag. i.e., synchronize_irq():

diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c
index 4063086ab5b8..e9446eeafb9d 100644
--- a/drivers/net/wireless/marvell/mwifiex/util.c
+++ b/drivers/net/wireless/marvell/mwifiex/util.c
@@ -841,6 +841,11 @@ void mwifiex_disable_wake(struct mwifiex_plt_wake_cfg *wake_cfg)
 {
 	if (wake_cfg && wake_cfg->irq_wifi >= 0) {
 		disable_irq_wake(wake_cfg->irq_wifi);
+		/*
+		 * Disable the wake IRQ only if it didn't already fire (and
+		 * disable itself).
+		 */
+		synchronize_irq(wake_cfg->irq_wifi);
 		if (!wake_cfg->wake_by_wifi)
 			disable_irq(wake_cfg->irq_wifi);
 	}

I'd appreciate if you bugfix this, either before or after this patch.

Brian

> > +			disable_irq(adapter->irq_wakeup);
> > +	}
> > +}
> > +
> > +/* Enable platform specific wakeup interrupt */
> > +static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter)
> > +{
> > +	/* Enable platform specific wakeup interrupt */
> > +	if (adapter->irq_wakeup >= 0) {
> > +		adapter->wake_by_wifi = false;
> > +		enable_irq(adapter->irq_wakeup);
> > +		enable_irq_wake(adapter->irq_wakeup);
> > +	}
> > +}
> > +
> >  int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
> >  			     u32 func_init_shutdown);
> >  int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,