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,
