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

[Brian Norris]

On Tue, Nov 15, 2016 at 09:35:07AM -0800, Dmitry Torokhov wrote:
> On Tue, Nov 15, 2016 at 07:06:04PM +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;
> > +   disable_irq_nosync(irq);
> > +   }
> > +
> > +   /* 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;
> > +   }
> 
> I'd rather we used of_irq_get() here, because ti allows handling
> deferrals. of_irq_get_byname() would be even better, but I guess we
> already have bindings in the wild...

This function doesn't handle errors very gracefully at all; we just try,
and if we fail, we just skip the rest...

This could be an argument for rewriting the error handling to stop just
returning -1 in mwifiex_add_card() and use real Linux error codes.
Perhaps that can be a later cleanup?

> > +
> > +   ret = devm_request_irq(dev, adapter->irq_wakeup,
> > +  mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
> 
> irq_of_parse_and_map() (and of_irq_get()) will set trigger flags,
> why do we override them?
> 
> > +  "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;
> 
> Leaking interrupt (not forever, but if we are not using wakeup irq there
> is no need to have it claimed).
> 
> > +   }
> > +   return;
> > +
> > +err_exit:
> > +   adapter->irq_wakeup = 0;
> >  }
> 
> I also do not see anyone actually calling mwifiex_probe_of() in this
> patch?

It was added to mwifiex_add_card() in the previous patch, so all users
with a proper ->dt_node would call it.

> >  
> >  /*

Brian

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

[Dmitry Torokhov]

On Tue, Nov 15, 2016 at 07:06:04PM +0530, Amitkumar Karwar wrote:
> From: Rajat Jain 
> 
> Commit ce4f6f0c353b ("mwifiex: add platform specific wakeup interrupt
> support") added WoWLAN feature only for sdio. This patch moves that
> code to the common module so that all the interface drivers can use
> it for free. It enables pcie and sdio for its use currently.
> 
> Signed-off-by: Rajat Jain 
> ---
> v2: v1 doesn't apply smoothly on latest code due to recently merged
> patch "mwifiex: report error to PCIe for suspend failure". Minor
> conflict is resolved in v2
> v4: Same as v2, v3
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c | 41 
>  drivers/net/wireless/marvell/mwifiex/main.h | 25 ++
>  drivers/net/wireless/marvell/mwifiex/pcie.c |  2 +
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 72 
> ++---
>  drivers/net/wireless/marvell/mwifiex/sdio.h |  8 
>  5 files changed, 73 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index 835d330..948f5c2 100644
> --- 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;
> + disable_irq_nosync(irq);
> + }
> +
> + /* 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;
> + }

I'd rather we used of_irq_get() here, because ti allows handling
deferrals. of_irq_get_byname() would be even better, but I guess we
already have bindings in the wild...

> +
> + ret = devm_request_irq(dev, adapter->irq_wakeup,
> +mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,

irq_of_parse_and_map() (and of_irq_get()) will set trigger flags,
why do we override them?

> +"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;

Leaking interrupt (not forever, but if we are not using wakeup irq there
is no need to have it claimed).

> + }
> + return;
> +
> +err_exit:
> + adapter->irq_wakeup = 0;
>  }

I also do not see anyone actually calling mwifiex_probe_of() in this
patch?

>  
>  /*
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
> b/drivers/net/wireless/marvell/mwifiex/main.h
> index 549e1ba..ae5afe5 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)
> + 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 

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

[Amitkumar Karwar]

From: Rajat Jain 

Commit ce4f6f0c353b ("mwifiex: add platform specific wakeup interrupt
support") added WoWLAN feature only for sdio. This patch moves that
code to the common module so that all the interface drivers can use
it for free. It enables pcie and sdio for its use currently.

Signed-off-by: Rajat Jain 
---
v2: v1 doesn't apply smoothly on latest code due to recently merged
patch "mwifiex: report error to PCIe for suspend failure". Minor
conflict is resolved in v2
v4: Same as v2, v3
---
 drivers/net/wireless/marvell/mwifiex/main.c | 41 
 drivers/net/wireless/marvell/mwifiex/main.h | 25 ++
 drivers/net/wireless/marvell/mwifiex/pcie.c |  2 +
 drivers/net/wireless/marvell/mwifiex/sdio.c | 72 ++---
 drivers/net/wireless/marvell/mwifiex/sdio.h |  8 
 5 files changed, 73 insertions(+), 75 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index 835d330..948f5c2 100644
--- 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;
+   disable_irq_nosync(irq);
+   }
+
+   /* 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 549e1ba..ae5afe5 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)
+   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 *card, struct semaphore *sem,
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 745ecd6..e8f4f90 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -131,6 +131,7 @@ static int mwifiex_pcie_suspend(struct device *dev)
}
 
adapter = card->adapter;
+   mwifiex_enable_wake(adapter);
 
/* Enable the Host Sleep */
if (!mwifiex_enable_hs(adapter)) {
@@ -186,6 +187,7 @@ static int mwifiex_pcie_resume(struct device *dev)
 
mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),