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

Commit Message

Amitkumar Karwar Nov. 15, 2016, 1:36 p.m. UTC

From: Rajat Jain <[e-mail]>

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 <[e-mail]>
---
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(-)

Comments

Dmitry Torokhov Nov. 15, 2016, 5:35 p.m. UTC | #1

On Tue, Nov 15, 2016 at 07:06:04PM +0530, Amitkumar Karwar wrote:
> From: Rajat Jain <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 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 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),
>  			  MWIFIEX_ASYNC_CMD);
> +	mwifiex_disable_wake(adapter);
>  
>  	return 0;
>  }
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index c95f41f..7055282 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -79,67 +79,18 @@
>  	{ }
>  };
>  
> -static irqreturn_t mwifiex_wake_irq_wifi(int irq, void *priv)
> -{
> -	struct mwifiex_plt_wake_cfg *cfg = priv;
> -
> -	if (cfg->irq_wifi >= 0) {
> -		pr_info("%s: wake by wifi", __func__);
> -		cfg->wake_by_wifi = true;
> -		disable_irq_nosync(irq);
> -	}
> -
> -	/* Notify PM core we are wakeup source */
> -	pm_wakeup_event(cfg->dev, 0);
> -
> -	return IRQ_HANDLED;
> -}
> -
>  /* This function parse device tree node using mmc subnode devicetree API.
>   * The device node is saved in card->plt_of_node.
>   * if the device tree node exist and include interrupts attributes, this
>   * function will also request platform specific wakeup interrupt.
>   */
> -static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
> +static int mwifiex_sdio_probe_of(struct device *dev)
>  {
> -	struct mwifiex_plt_wake_cfg *cfg;
> -	int ret;
> -
>  	if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
>  		dev_err(dev, "required compatible string missing\n");
>  		return -EINVAL;
>  	}
>  
> -	card->plt_of_node = dev->of_node;
> -	card->plt_wake_cfg = devm_kzalloc(dev, sizeof(*card->plt_wake_cfg),
> -					  GFP_KERNEL);
> -	cfg = card->plt_wake_cfg;
> -	if (cfg && card->plt_of_node) {
> -		cfg->dev = dev;
> -		cfg->irq_wifi = irq_of_parse_and_map(card->plt_of_node, 0);
> -		if (!cfg->irq_wifi) {
> -			dev_dbg(dev,
> -				"fail to parse irq_wifi from device tree\n");
> -		} else {
> -			ret = devm_request_irq(dev, cfg->irq_wifi,
> -					       mwifiex_wake_irq_wifi,
> -					       IRQF_TRIGGER_LOW,
> -					       "wifi_wake", cfg);
> -			if (ret) {
> -				dev_dbg(dev,
> -					"Failed to request irq_wifi %d (%d)\n",
> -					cfg->irq_wifi, ret);
> -				card->plt_wake_cfg = NULL;
> -				return 0;
> -			}
> -			disable_irq(cfg->irq_wifi);
> -		}
> -	}
> -
> -	ret = device_init_wakeup(dev, true);
> -	if (ret)
> -		dev_err(dev, "fail to init wakeup for mwifiex");
> -
>  	return 0;
>  }
>  
> @@ -198,11 +149,9 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
>  
>  	/* device tree node parsing and platform specific configuration*/
>  	if (func->dev.of_node) {
> -		ret = mwifiex_sdio_probe_of(&func->dev, card);
> -		if (ret) {
> -			dev_err(&func->dev, "SDIO dt node parse failed\n");
> +		ret = mwifiex_sdio_probe_of(&func->dev);
> +		if (ret)
>  			goto err_disable;
> -		}
>  	}
>  
>  	ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
> @@ -267,12 +216,7 @@ static int mwifiex_sdio_resume(struct device *dev)
>  	mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),
>  			  MWIFIEX_SYNC_CMD);
>  
> -	/* Disable platform specific wakeup interrupt */
> -	if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
> -		disable_irq_wake(card->plt_wake_cfg->irq_wifi);
> -		if (!card->plt_wake_cfg->wake_by_wifi)
> -			disable_irq(card->plt_wake_cfg->irq_wifi);
> -	}
> +	mwifiex_disable_wake(adapter);
>  
>  	return 0;
>  }
> @@ -352,13 +296,7 @@ static int mwifiex_sdio_suspend(struct device *dev)
>  	}
>  
>  	adapter = card->adapter;
> -
> -	/* Enable platform specific wakeup interrupt */
> -	if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
> -		card->plt_wake_cfg->wake_by_wifi = false;
> -		enable_irq(card->plt_wake_cfg->irq_wifi);
> -		enable_irq_wake(card->plt_wake_cfg->irq_wifi);
> -	}
> +	mwifiex_enable_wake(adapter);
>  
>  	/* Enable the Host Sleep */
>  	if (!mwifiex_enable_hs(adapter)) {
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
> index 07cdd23..b9fbc5c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.h
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
> @@ -154,12 +154,6 @@
>  	a->mpa_rx.start_port = 0;					\
>  } while (0)
>  
> -struct mwifiex_plt_wake_cfg {
> -	struct device *dev;
> -	int irq_wifi;
> -	bool wake_by_wifi;
> -};
> -
>  /* data structure for SDIO MPA TX */
>  struct mwifiex_sdio_mpa_tx {
>  	/* multiport tx aggregation buffer pointer */
> @@ -243,8 +237,6 @@ struct mwifiex_sdio_card_reg {
>  struct sdio_mmc_card {
>  	struct sdio_func *func;
>  	struct mwifiex_adapter *adapter;
> -	struct device_node *plt_of_node;
> -	struct mwifiex_plt_wake_cfg *plt_wake_cfg;
>  
>  	const char *firmware;
>  	const struct mwifiex_sdio_card_reg *reg;
> -- 
> 1.9.1
> 

Thanks.

Brian Norris Nov. 15, 2016, 6:16 p.m. UTC | #2

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

Jeffy Chen July 3, 2017, 10:46 a.m. UTC | #3

Hi guys,

with this patch, the pci device's irq might be override by this wakeup 
irq when not using msi:


/**
  * of_irq_parse_pci - Resolve the interrupt for a PCI device
  * @pdev:       the device whose interrupt is to be resolved
  * @out_irq:    structure of_irq filled by this function
  *
  * This function resolves the PCI interrupt for a given PCI device. If a
  * device-node exists for a given pci_dev, it will use normal OF tree
  * walking. If not, it will implement standard swizzling and walk up the
  * PCI tree until an device-node is found, at which point it will finish
  * resolving using the OF tree walking.
  */
int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args 
*out_irq)
{
...
         /* Check if we have a device node, if yes, fallback to standard
          * device tree parsing
          */
         dn = pci_device_to_OF_node(pdev);
         if (dn) {
                 rc = of_irq_parse_one(dn, 0, out_irq); <--- this would 
take wifi wake irq as pci irq instead of fallback to PCI_INTERRUPT_PIN
                 if (!rc)
                         return rc;
         }

         /* Ok, we don't, time to have fun. Let's start by building up an
          * interrupt spec.  we assume #interrupt-cells is 1, which is 
standard
          * for PCI. If you do different, then don't use that routine.
          */
         rc = pci_read_config_byte(pdev, PCI_INTERRUPT_PIN, &pin);


but i have no idea how to fix it...



On 11/15/2016 09:36 PM, Amitkumar Karwar wrote:
> From: Rajat Jain <[e-mail]>
>
> 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 <[e-mail]>
> ---
> 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),
>   			  MWIFIEX_ASYNC_CMD);
> +	mwifiex_disable_wake(adapter);
>
>   	return 0;
>   }
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index c95f41f..7055282 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -79,67 +79,18 @@
>   	{ }
>   };
>
> -static irqreturn_t mwifiex_wake_irq_wifi(int irq, void *priv)
> -{
> -	struct mwifiex_plt_wake_cfg *cfg = priv;
> -
> -	if (cfg->irq_wifi >= 0) {
> -		pr_info("%s: wake by wifi", __func__);
> -		cfg->wake_by_wifi = true;
> -		disable_irq_nosync(irq);
> -	}
> -
> -	/* Notify PM core we are wakeup source */
> -	pm_wakeup_event(cfg->dev, 0);
> -
> -	return IRQ_HANDLED;
> -}
> -
>   /* This function parse device tree node using mmc subnode devicetree API.
>    * The device node is saved in card->plt_of_node.
>    * if the device tree node exist and include interrupts attributes, this
>    * function will also request platform specific wakeup interrupt.
>    */
> -static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
> +static int mwifiex_sdio_probe_of(struct device *dev)
>   {
> -	struct mwifiex_plt_wake_cfg *cfg;
> -	int ret;
> -
>   	if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
>   		dev_err(dev, "required compatible string missing\n");
>   		return -EINVAL;
>   	}
>
> -	card->plt_of_node = dev->of_node;
> -	card->plt_wake_cfg = devm_kzalloc(dev, sizeof(*card->plt_wake_cfg),
> -					  GFP_KERNEL);
> -	cfg = card->plt_wake_cfg;
> -	if (cfg && card->plt_of_node) {
> -		cfg->dev = dev;
> -		cfg->irq_wifi = irq_of_parse_and_map(card->plt_of_node, 0);
> -		if (!cfg->irq_wifi) {
> -			dev_dbg(dev,
> -				"fail to parse irq_wifi from device tree\n");
> -		} else {
> -			ret = devm_request_irq(dev, cfg->irq_wifi,
> -					       mwifiex_wake_irq_wifi,
> -					       IRQF_TRIGGER_LOW,
> -					       "wifi_wake", cfg);
> -			if (ret) {
> -				dev_dbg(dev,
> -					"Failed to request irq_wifi %d (%d)\n",
> -					cfg->irq_wifi, ret);
> -				card->plt_wake_cfg = NULL;
> -				return 0;
> -			}
> -			disable_irq(cfg->irq_wifi);
> -		}
> -	}
> -
> -	ret = device_init_wakeup(dev, true);
> -	if (ret)
> -		dev_err(dev, "fail to init wakeup for mwifiex");
> -
>   	return 0;
>   }
>
> @@ -198,11 +149,9 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
>
>   	/* device tree node parsing and platform specific configuration*/
>   	if (func->dev.of_node) {
> -		ret = mwifiex_sdio_probe_of(&func->dev, card);
> -		if (ret) {
> -			dev_err(&func->dev, "SDIO dt node parse failed\n");
> +		ret = mwifiex_sdio_probe_of(&func->dev);
> +		if (ret)
>   			goto err_disable;
> -		}
>   	}
>
>   	ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
> @@ -267,12 +216,7 @@ static int mwifiex_sdio_resume(struct device *dev)
>   	mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),
>   			  MWIFIEX_SYNC_CMD);
>
> -	/* Disable platform specific wakeup interrupt */
> -	if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
> -		disable_irq_wake(card->plt_wake_cfg->irq_wifi);
> -		if (!card->plt_wake_cfg->wake_by_wifi)
> -			disable_irq(card->plt_wake_cfg->irq_wifi);
> -	}
> +	mwifiex_disable_wake(adapter);
>
>   	return 0;
>   }
> @@ -352,13 +296,7 @@ static int mwifiex_sdio_suspend(struct device *dev)
>   	}
>
>   	adapter = card->adapter;
> -
> -	/* Enable platform specific wakeup interrupt */
> -	if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
> -		card->plt_wake_cfg->wake_by_wifi = false;
> -		enable_irq(card->plt_wake_cfg->irq_wifi);
> -		enable_irq_wake(card->plt_wake_cfg->irq_wifi);
> -	}
> +	mwifiex_enable_wake(adapter);
>
>   	/* Enable the Host Sleep */
>   	if (!mwifiex_enable_hs(adapter)) {
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
> index 07cdd23..b9fbc5c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.h
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
> @@ -154,12 +154,6 @@
>   	a->mpa_rx.start_port = 0;					\
>   } while (0)
>
> -struct mwifiex_plt_wake_cfg {
> -	struct device *dev;
> -	int irq_wifi;
> -	bool wake_by_wifi;
> -};
> -
>   /* data structure for SDIO MPA TX */
>   struct mwifiex_sdio_mpa_tx {
>   	/* multiport tx aggregation buffer pointer */
> @@ -243,8 +237,6 @@ struct mwifiex_sdio_card_reg {
>   struct sdio_mmc_card {
>   	struct sdio_func *func;
>   	struct mwifiex_adapter *adapter;
> -	struct device_node *plt_of_node;
> -	struct mwifiex_plt_wake_cfg *plt_wake_cfg;
>
>   	const char *firmware;
>   	const struct mwifiex_sdio_card_reg *reg;
>
>

Brian Norris July 7, 2017, 12:53 a.m. UTC | #4

On Mon, Jul 03, 2017 at 06:46:21PM +0800, Jeffy Chen wrote:
> Hi guys,
> 
> with this patch, the pci device's irq might be override by this
> wakeup irq when not using msi:

Hmm, good point. I believe I noticed this one at some point and then
didn't get to investigate further...

It kind of seems like we inadvertently conflicted with the PCI OF
interrupt spec [1]. There, the "interrupts" property for a device (if
present) is supposed to represent INT{A...D} with values of {1...4}.
IIUC, there should only be a single entry in this property.

If we were to extend this properly, I guess that would mean we'd need a
second "interrupts" entry, with a different parent. I think we can use
"interrupts-extended" for that.

So we'd need to document an optional "interrupt-names" for Marvell, and
have the driver try that first. The rough outline would be something
like this.

For the device tree (e.g., rk3399-gru):

-			interrupt-parent = <&gpio0>;
-			interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
+			interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
+			interrupt-names = "int-A", "wake";

Then mwifiex would need to check "byname" before trying "by index":

	adapter->irq_wakeup = of_irq_get_byname(adapter->dt_node, "wake");
	if (!adapter->irq_wakeup) {
		adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
		if (!adapter->irq_wakeup) {
			dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
			goto err_exit;
		}
	}

Or if we want to suggest the original binding was wrong and that we
should just ignore existing device trees that tried to use it, we can
skip the by-index fallback.

Brian

[1] Documentation/devicetree/bindings/pci/pci.txt points to
http://www.firmware.org/1275/practice/imap/imap0_9d.pdf
except that link is also dead now. I found the same doc here:
https://www.openfirmware.info/data/docs/rec.intmap.d09.pdf
Might want to update the binding doc... I've sent a patch for that
separately.

Jeffy Chen July 7, 2017, 3:04 a.m. UTC | #5

Hi brian,

On 07/07/2017 08:53 AM, Brian Norris wrote:
> On Mon, Jul 03, 2017 at 06:46:21PM +0800, Jeffy Chen wrote:
>> Hi guys,
>>
>> with this patch, the pci device's irq might be override by this
>> wakeup irq when not using msi:
>
> Hmm, good point. I believe I noticed this one at some point and then
> didn't get to investigate further...
>
> It kind of seems like we inadvertently conflicted with the PCI OF
> interrupt spec [1]. There, the "interrupts" property for a device (if
> present) is supposed to represent INT{A...D} with values of {1...4}.
> IIUC, there should only be a single entry in this property.
>
> If we were to extend this properly, I guess that would mean we'd need a
> second "interrupts" entry, with a different parent. I think we can use
> "interrupts-extended" for that.
>
> So we'd need to document an optional "interrupt-names" for Marvell, and
> have the driver try that first. The rough outline would be something
> like this.
>
> For the device tree (e.g., rk3399-gru):
>
> -			interrupt-parent = <&gpio0>;
> -			interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> +			interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
> +			interrupt-names = "int-A", "wake";
This is a great idea.

And how about also add a property to tell of_pci_irq to ignore of irq 
and force using PCI_INTERRUPT_PIN? Since there might be devices don't 
use pci irq, but using other irq(wowlan for example).

Then we can specify this property and add a name("wake") to the wifi 
wake irq here. And interrupts-extended would still be an available option.

>
> Then mwifiex would need to check "byname" before trying "by index":
>
> 	adapter->irq_wakeup = of_irq_get_byname(adapter->dt_node, "wake");
> 	if (!adapter->irq_wakeup) {
> 		adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
> 		if (!adapter->irq_wakeup) {
> 			dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
> 			goto err_exit;
> 		}
> 	}
>
> Or if we want to suggest the original binding was wrong and that we
> should just ignore existing device trees that tried to use it, we can
> skip the by-index fallback.
>
> Brian
>
> [1] Documentation/devicetree/bindings/pci/pci.txt points to
> http://www.firmware.org/1275/practice/imap/imap0_9d.pdf
> except that link is also dead now. I found the same doc here:
> https://www.openfirmware.info/data/docs/rec.intmap.d09.pdf
> Might want to update the binding doc... I've sent a patch for that
> separately.
>
>
>

Patch

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),
 			  MWIFIEX_ASYNC_CMD);
+	mwifiex_disable_wake(adapter);
 
 	return 0;
 }
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index c95f41f..7055282 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -79,67 +79,18 @@ 
 	{ }
 };
 
-static irqreturn_t mwifiex_wake_irq_wifi(int irq, void *priv)
-{
-	struct mwifiex_plt_wake_cfg *cfg = priv;
-
-	if (cfg->irq_wifi >= 0) {
-		pr_info("%s: wake by wifi", __func__);
-		cfg->wake_by_wifi = true;
-		disable_irq_nosync(irq);
-	}
-
-	/* Notify PM core we are wakeup source */
-	pm_wakeup_event(cfg->dev, 0);
-
-	return IRQ_HANDLED;
-}
-
 /* This function parse device tree node using mmc subnode devicetree API.
  * The device node is saved in card->plt_of_node.
  * if the device tree node exist and include interrupts attributes, this
  * function will also request platform specific wakeup interrupt.
  */
-static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
+static int mwifiex_sdio_probe_of(struct device *dev)
 {
-	struct mwifiex_plt_wake_cfg *cfg;
-	int ret;
-
 	if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
 		dev_err(dev, "required compatible string missing\n");
 		return -EINVAL;
 	}
 
-	card->plt_of_node = dev->of_node;
-	card->plt_wake_cfg = devm_kzalloc(dev, sizeof(*card->plt_wake_cfg),
-					  GFP_KERNEL);
-	cfg = card->plt_wake_cfg;
-	if (cfg && card->plt_of_node) {
-		cfg->dev = dev;
-		cfg->irq_wifi = irq_of_parse_and_map(card->plt_of_node, 0);
-		if (!cfg->irq_wifi) {
-			dev_dbg(dev,
-				"fail to parse irq_wifi from device tree\n");
-		} else {
-			ret = devm_request_irq(dev, cfg->irq_wifi,
-					       mwifiex_wake_irq_wifi,
-					       IRQF_TRIGGER_LOW,
-					       "wifi_wake", cfg);
-			if (ret) {
-				dev_dbg(dev,
-					"Failed to request irq_wifi %d (%d)\n",
-					cfg->irq_wifi, ret);
-				card->plt_wake_cfg = NULL;
-				return 0;
-			}
-			disable_irq(cfg->irq_wifi);
-		}
-	}
-
-	ret = device_init_wakeup(dev, true);
-	if (ret)
-		dev_err(dev, "fail to init wakeup for mwifiex");
-
 	return 0;
 }
 
@@ -198,11 +149,9 @@  static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
 
 	/* device tree node parsing and platform specific configuration*/
 	if (func->dev.of_node) {
-		ret = mwifiex_sdio_probe_of(&func->dev, card);
-		if (ret) {
-			dev_err(&func->dev, "SDIO dt node parse failed\n");
+		ret = mwifiex_sdio_probe_of(&func->dev);
+		if (ret)
 			goto err_disable;
-		}
 	}
 
 	ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
@@ -267,12 +216,7 @@  static int mwifiex_sdio_resume(struct device *dev)
 	mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),
 			  MWIFIEX_SYNC_CMD);
 
-	/* Disable platform specific wakeup interrupt */
-	if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
-		disable_irq_wake(card->plt_wake_cfg->irq_wifi);
-		if (!card->plt_wake_cfg->wake_by_wifi)
-			disable_irq(card->plt_wake_cfg->irq_wifi);
-	}
+	mwifiex_disable_wake(adapter);
 
 	return 0;
 }
@@ -352,13 +296,7 @@  static int mwifiex_sdio_suspend(struct device *dev)
 	}
 
 	adapter = card->adapter;
-
-	/* Enable platform specific wakeup interrupt */
-	if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
-		card->plt_wake_cfg->wake_by_wifi = false;
-		enable_irq(card->plt_wake_cfg->irq_wifi);
-		enable_irq_wake(card->plt_wake_cfg->irq_wifi);
-	}
+	mwifiex_enable_wake(adapter);
 
 	/* Enable the Host Sleep */
 	if (!mwifiex_enable_hs(adapter)) {
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
index 07cdd23..b9fbc5c 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.h
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
@@ -154,12 +154,6 @@ 
 	a->mpa_rx.start_port = 0;					\
 } while (0)
 
-struct mwifiex_plt_wake_cfg {
-	struct device *dev;
-	int irq_wifi;
-	bool wake_by_wifi;
-};
-
 /* data structure for SDIO MPA TX */
 struct mwifiex_sdio_mpa_tx {
 	/* multiport tx aggregation buffer pointer */
@@ -243,8 +237,6 @@  struct mwifiex_sdio_card_reg {
 struct sdio_mmc_card {
 	struct sdio_func *func;
 	struct mwifiex_adapter *adapter;
-	struct device_node *plt_of_node;
-	struct mwifiex_plt_wake_cfg *plt_wake_cfg;
 
 	const char *firmware;
 	const struct mwifiex_sdio_card_reg *reg;