[PATCH v2 1/3] mwifiex: Allow mwifiex early access to device structure

[PATCH v2 1/3] mwifiex: Allow mwifiex early access to device structure

[Amitkumar Karwar]

From: Rajat Jain <rajatja@google.com>

Today all the interface drivers (usb/pcie/sdio) assign the
adapter->dev in the register_dev() callback, although they
have this piece of info well before hand.

This patch makes the device structure available for mwifiex
right at the beginning, so that it can be used for early
initialization if needed.

This is needed for subsequent patches in this patchset that
intend to unify and consolidate some of the code that would
otherwise have to be duplicated among the interface drivers
(sdio, pcie, usb).

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: Same as v1
---
 drivers/net/wireless/marvell/mwifiex/main.c | 4 +++-
 drivers/net/wireless/marvell/mwifiex/main.h | 3 ++-
 drivers/net/wireless/marvell/mwifiex/pcie.c | 4 +---
 drivers/net/wireless/marvell/mwifiex/sdio.c | 5 +----
 drivers/net/wireless/marvell/mwifiex/usb.c  | 3 +--
 5 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 2478ccd..dcceab2 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1567,7 +1567,8 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
  */
 int
 mwifiex_add_card(void *card, struct semaphore *sem,
-		 struct mwifiex_if_ops *if_ops, u8 iface_type)
+		 struct mwifiex_if_ops *if_ops, u8 iface_type,
+		 struct device *dev)
 {
 	struct mwifiex_adapter *adapter;
 
@@ -1579,6 +1580,7 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
 		goto err_init_sw;
 	}
 
+	adapter->dev = dev;
 	adapter->iface_type = iface_type;
 	adapter->card_sem = sem;
 
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index d61fe3a..2664513 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1412,7 +1412,8 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
 
 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);
+int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,
+		     struct device *);
 int mwifiex_remove_card(struct mwifiex_adapter *, struct semaphore *);
 
 void mwifiex_get_version(struct mwifiex_adapter *adapter, char *version,
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index fed6a1d..de6939c 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -231,7 +231,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
 	}
 
 	ret = mwifiex_add_card(card, &add_remove_card_sem, &pcie_ops,
-			       MWIFIEX_PCIE);
+			       MWIFIEX_PCIE, &pdev->dev);
 	if (ret) {
 		pr_err("%s failed\n", __func__);
 		goto err_free;
@@ -2999,11 +2999,9 @@ static void mwifiex_pcie_get_fw_name(struct mwifiex_adapter *adapter)
 static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
 {
 	struct pcie_service_card *card = adapter->card;
-	struct pci_dev *pdev = card->dev;
 
 	/* save adapter pointer in card */
 	card->adapter = adapter;
-	adapter->dev = &pdev->dev;
 
 	if (mwifiex_pcie_request_irq(adapter))
 		return -1;
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 807af13..c95f41f 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -206,7 +206,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
 	}
 
 	ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
-			       MWIFIEX_SDIO);
+			       MWIFIEX_SDIO, &func->dev);
 	if (ret) {
 		dev_err(&func->dev, "add card failed\n");
 		goto err_disable;
@@ -2106,9 +2106,6 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
 		return ret;
 	}
 
-
-	adapter->dev = &func->dev;
-
 	strcpy(adapter->fw_name, card->firmware);
 	if (card->fw_dump_enh) {
 		adapter->mem_type_mapping_tbl = generic_mem_type_map;
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 73eb084..f847fff 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -476,7 +476,7 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
 	usb_set_intfdata(intf, card);
 
 	ret = mwifiex_add_card(card, &add_remove_card_sem, &usb_ops,
-			       MWIFIEX_USB);
+			       MWIFIEX_USB, &card->udev->dev);
 	if (ret) {
 		pr_err("%s: mwifiex_add_card failed: %d\n", __func__, ret);
 		usb_reset_device(udev);
@@ -932,7 +932,6 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
 	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
 
 	card->adapter = adapter;
-	adapter->dev = &card->udev->dev;
 
 	switch (le16_to_cpu(card->udev->descriptor.idProduct)) {
 	case USB8997_PID_1:
-- 
1.9.1

[PATCH v2 2/3] mwifiex: Introduce mwifiex_probe_of() to parse common properties

[Amitkumar Karwar]

From: Rajat Jain <rajatja@google.com>

Introduce function mwifiex_probe_of() to parse common properties.
Since the interface drivers get to decide whether or not the device
tree node was a valid one (depending on the compatible property),
let the interface drivers pass a flag to indicate whether the device
tree node was a valid one.

The function mwifiex_probe_of()nodetself is currently only a place
holder with the next patch adding content to it.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: Same as v1
---
 drivers/net/wireless/marvell/mwifiex/main.c    | 15 ++++++++++++++-
 drivers/net/wireless/marvell/mwifiex/main.h    |  2 +-
 drivers/net/wireless/marvell/mwifiex/pcie.c    |  4 +++-
 drivers/net/wireless/marvell/mwifiex/sdio.c    |  4 +++-
 drivers/net/wireless/marvell/mwifiex/sta_cmd.c |  5 +----
 drivers/net/wireless/marvell/mwifiex/usb.c     |  2 +-
 6 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index dcceab2..b2f3d96 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1552,6 +1552,16 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
 }
 EXPORT_SYMBOL_GPL(mwifiex_do_flr);
 
+static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
+{
+	struct device *dev = adapter->dev;
+
+	if (!dev->of_node)
+		return;
+
+	adapter->dt_node = dev->of_node;
+}
+
 /*
  * This function adds the card.
  *
@@ -1568,7 +1578,7 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
 int
 mwifiex_add_card(void *card, struct semaphore *sem,
 		 struct mwifiex_if_ops *if_ops, u8 iface_type,
-		 struct device *dev)
+		 struct device *dev, bool of_node_valid)
 {
 	struct mwifiex_adapter *adapter;
 
@@ -1581,6 +1591,9 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
 	}
 
 	adapter->dev = dev;
+	if (of_node_valid)
+		mwifiex_probe_of(adapter);
+
 	adapter->iface_type = iface_type;
 	adapter->card_sem = sem;
 
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 2664513..0c07434 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1413,7 +1413,7 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
 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,
-		     struct device *);
+		     struct device *, bool);
 int mwifiex_remove_card(struct mwifiex_adapter *, struct semaphore *);
 
 void mwifiex_get_version(struct mwifiex_adapter *adapter, char *version,
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index de6939c..fe7f3b0 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -201,6 +201,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
 					const struct pci_device_id *ent)
 {
 	struct pcie_service_card *card;
+	bool valid_of_node = false;
 	int ret;
 
 	pr_debug("info: vendor=0x%4.04X device=0x%4.04X rev=%d\n",
@@ -228,10 +229,11 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
 		ret = mwifiex_pcie_probe_of(&pdev->dev);
 		if (ret)
 			goto err_free;
+		valid_of_node = true;
 	}
 
 	ret = mwifiex_add_card(card, &add_remove_card_sem, &pcie_ops,
-			       MWIFIEX_PCIE, &pdev->dev);
+			       MWIFIEX_PCIE, &pdev->dev, valid_of_node);
 	if (ret) {
 		pr_err("%s failed\n", __func__);
 		goto err_free;
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index c95f41f..558743a 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -156,6 +156,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
 {
 	int ret;
 	struct sdio_mmc_card *card = NULL;
+	bool valid_of_node = false;
 
 	pr_debug("info: vendor=0x%4.04X device=0x%4.04X class=%d function=%d\n",
 		 func->vendor, func->device, func->class, func->num);
@@ -203,10 +204,11 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
 			dev_err(&func->dev, "SDIO dt node parse failed\n");
 			goto err_disable;
 		}
+		valid_of_node = true;
 	}
 
 	ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
-			       MWIFIEX_SDIO, &func->dev);
+			       MWIFIEX_SDIO, &func->dev, valid_of_node);
 	if (ret) {
 		dev_err(&func->dev, "add card failed\n");
 		goto err_disable;
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index b697b61..bcd6408 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -2235,10 +2235,7 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init)
 		 * The cal-data can be read from device tree and/or
 		 * a configuration file and downloaded to firmware.
 		 */
-		if ((priv->adapter->iface_type == MWIFIEX_SDIO ||
-		    priv->adapter->iface_type == MWIFIEX_PCIE) &&
-		    adapter->dev->of_node) {
-			adapter->dt_node = adapter->dev->of_node;
+		if (adapter->dt_node) {
 			if (of_property_read_u32(adapter->dt_node,
 						 "marvell,wakeup-pin",
 						 &data) == 0) {
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index f847fff..11c9629 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -476,7 +476,7 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
 	usb_set_intfdata(intf, card);
 
 	ret = mwifiex_add_card(card, &add_remove_card_sem, &usb_ops,
-			       MWIFIEX_USB, &card->udev->dev);
+			       MWIFIEX_USB, &card->udev->dev, false);
 	if (ret) {
 		pr_err("%s: mwifiex_add_card failed: %d\n", __func__, ret);
 		usb_reset_device(udev);
-- 
1.9.1

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

[Amitkumar Karwar]

From: Rajat Jain <rajatja@google.com>

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 <rajatja@google.com>
---
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
---
 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 b2f3d96..20c9b77 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 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)
+			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,
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index fe7f3b0..a38994e 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 558743a..ff5bb45 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;
 }
 
@@ -199,11 +150,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;
-		}
 		valid_of_node = true;
 	}
 
@@ -269,12 +218,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;
 }
@@ -354,13 +298,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

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

[Brian Norris]

On Fri, Nov 11, 2016 at 04:45:11PM +0530, Amitkumar Karwar wrote:
> From: Rajat Jain <rajatja@google.com>
> 
> 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 <rajatja@google.com>
> ---
> 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
> ---

In case you haven't noticed my comments elsewhere, I'll repeat them
here: you're copy-and-pasting buggy code. That can be OK I suppose, but
please fix these issues, possibly in a follow-up patch.

>  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 b2f3d96..20c9b77 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;

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.

> +	}
> +
> +	/* 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.

> +			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,
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index fe7f3b0..a38994e 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)) {

^^^ What if this fails? Then you'll leave the wake IRQ enabled.

(You've propagated this error from the SDIO driver.)

> @@ -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 558743a..ff5bb45 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;
>  }
>  
> @@ -199,11 +150,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;
> -		}
>  		valid_of_node = true;
>  	}
>  
> @@ -269,12 +218,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;
>  }
> @@ -354,13 +298,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)) {

Same problem here. Existing issue.

Brian

> 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
> 

Re: [PATCH v2 2/3] mwifiex: Introduce mwifiex_probe_of() to parse common properties

[Brian Norris]

On Fri, Nov 11, 2016 at 04:45:10PM +0530, Amitkumar Karwar wrote:
> From: Rajat Jain <rajatja@google.com>
> 
> Introduce function mwifiex_probe_of() to parse common properties.
> Since the interface drivers get to decide whether or not the device
> tree node was a valid one (depending on the compatible property),
> let the interface drivers pass a flag to indicate whether the device
> tree node was a valid one.

Wait, what? I don't understand why this is needed. The current of_node
user (SDIO) always checks dev->of_node (if !NULL), and if it's not
matching, it rejects the device and doesn't even try to register the
card at all. That's a common pattern for DT-based drivers, and I don't
see why we need to do differently for any other driver (e.g., PCIe).

So...isn't 'dev->of_node != NULL' an equivalent test to 'of_node_valid'?
Or put another way, mwifiex_add_card() should never see a 'struct
device' whose of_node is not compatible. Do you agree?

> The function mwifiex_probe_of()nodetself is currently only a place
> holder with the next patch adding content to it.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v2: Same as v1
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c    | 15 ++++++++++++++-
>  drivers/net/wireless/marvell/mwifiex/main.h    |  2 +-
>  drivers/net/wireless/marvell/mwifiex/pcie.c    |  4 +++-
>  drivers/net/wireless/marvell/mwifiex/sdio.c    |  4 +++-
>  drivers/net/wireless/marvell/mwifiex/sta_cmd.c |  5 +----
>  drivers/net/wireless/marvell/mwifiex/usb.c     |  2 +-
>  6 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index dcceab2..b2f3d96 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1552,6 +1552,16 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
>  }
>  EXPORT_SYMBOL_GPL(mwifiex_do_flr);
>  
> +static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> +{
> +	struct device *dev = adapter->dev;
> +
> +	if (!dev->of_node)
> +		return;
> +
> +	adapter->dt_node = dev->of_node;
> +}
> +
>  /*
>   * This function adds the card.
>   *
> @@ -1568,7 +1578,7 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
>  int
>  mwifiex_add_card(void *card, struct semaphore *sem,
>  		 struct mwifiex_if_ops *if_ops, u8 iface_type,
> -		 struct device *dev)
> +		 struct device *dev, bool of_node_valid)
>  {
>  	struct mwifiex_adapter *adapter;
>  
> @@ -1581,6 +1591,9 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
>  	}
>  
>  	adapter->dev = dev;
> +	if (of_node_valid)
> +		mwifiex_probe_of(adapter);
> +
>  	adapter->iface_type = iface_type;
>  	adapter->card_sem = sem;
>  
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 2664513..0c07434 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1413,7 +1413,7 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
>  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,
> -		     struct device *);
> +		     struct device *, bool);

IOW, I think this extra bool flag is a bad idea.

Brian

>  int mwifiex_remove_card(struct mwifiex_adapter *, struct semaphore *);
>  
>  void mwifiex_get_version(struct mwifiex_adapter *adapter, char *version,
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index de6939c..fe7f3b0 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -201,6 +201,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
>  					const struct pci_device_id *ent)
>  {
>  	struct pcie_service_card *card;
> +	bool valid_of_node = false;
>  	int ret;
>  
>  	pr_debug("info: vendor=0x%4.04X device=0x%4.04X rev=%d\n",
> @@ -228,10 +229,11 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
>  		ret = mwifiex_pcie_probe_of(&pdev->dev);
>  		if (ret)
>  			goto err_free;
> +		valid_of_node = true;
>  	}
>  
>  	ret = mwifiex_add_card(card, &add_remove_card_sem, &pcie_ops,
> -			       MWIFIEX_PCIE, &pdev->dev);
> +			       MWIFIEX_PCIE, &pdev->dev, valid_of_node);
>  	if (ret) {
>  		pr_err("%s failed\n", __func__);
>  		goto err_free;
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index c95f41f..558743a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -156,6 +156,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
>  {
>  	int ret;
>  	struct sdio_mmc_card *card = NULL;
> +	bool valid_of_node = false;
>  
>  	pr_debug("info: vendor=0x%4.04X device=0x%4.04X class=%d function=%d\n",
>  		 func->vendor, func->device, func->class, func->num);
> @@ -203,10 +204,11 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
>  			dev_err(&func->dev, "SDIO dt node parse failed\n");
>  			goto err_disable;
>  		}
> +		valid_of_node = true;
>  	}
>  
>  	ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
> -			       MWIFIEX_SDIO, &func->dev);
> +			       MWIFIEX_SDIO, &func->dev, valid_of_node);
>  	if (ret) {
>  		dev_err(&func->dev, "add card failed\n");
>  		goto err_disable;
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> index b697b61..bcd6408 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> @@ -2235,10 +2235,7 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init)
>  		 * The cal-data can be read from device tree and/or
>  		 * a configuration file and downloaded to firmware.
>  		 */
> -		if ((priv->adapter->iface_type == MWIFIEX_SDIO ||
> -		    priv->adapter->iface_type == MWIFIEX_PCIE) &&
> -		    adapter->dev->of_node) {
> -			adapter->dt_node = adapter->dev->of_node;
> +		if (adapter->dt_node) {
>  			if (of_property_read_u32(adapter->dt_node,
>  						 "marvell,wakeup-pin",
>  						 &data) == 0) {
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
> index f847fff..11c9629 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -476,7 +476,7 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
>  	usb_set_intfdata(intf, card);
>  
>  	ret = mwifiex_add_card(card, &add_remove_card_sem, &usb_ops,
> -			       MWIFIEX_USB, &card->udev->dev);
> +			       MWIFIEX_USB, &card->udev->dev, false);
>  	if (ret) {
>  		pr_err("%s: mwifiex_add_card failed: %d\n", __func__, ret);
>  		usb_reset_device(udev);
> -- 
> 1.9.1
> 

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

[Brian Norris]

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,

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

[Amitkumar Karwar]

Hi Brian,

> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Saturday, November 12, 2016 2:13 AM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> rajatja@google.com; dmitry.torokhov@gmail.com
> Subject: Re: [PATCH v2 3/3] mwifiex: Enable WoWLAN for both sdio and
> pcie
> 
> On Fri, Nov 11, 2016 at 04:45:11PM +0530, Amitkumar Karwar wrote:
> > From: Rajat Jain <rajatja@google.com>
> >
> > 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 <rajatja@google.com>
> > ---
> > 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
> > ---
> 
> In case you haven't noticed my comments elsewhere, I'll repeat them
> here: you're copy-and-pasting buggy code. That can be OK I suppose, but
> please fix these issues, possibly in a follow-up patch.

Thanks for pointing this. I will post separate follow-up patch which address this issue as per your suggestion.

Regards,
Amitkumar

RE: [PATCH v2 2/3] mwifiex: Introduce mwifiex_probe_of() to parse common properties

[Amitkumar Karwar]

Hi Brian,

> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Saturday, November 12, 2016 2:20 AM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> rajatja@google.com; dmitry.torokhov@gmail.com
> Subject: Re: [PATCH v2 2/3] mwifiex: Introduce mwifiex_probe_of() to
> parse common properties
> 
> On Fri, Nov 11, 2016 at 04:45:10PM +0530, Amitkumar Karwar wrote:
> > From: Rajat Jain <rajatja@google.com>
> >
> > Introduce function mwifiex_probe_of() to parse common properties.
> > Since the interface drivers get to decide whether or not the device
> > tree node was a valid one (depending on the compatible property), let
> > the interface drivers pass a flag to indicate whether the device tree
> > node was a valid one.
> 
> Wait, what? I don't understand why this is needed. The current of_node
> user (SDIO) always checks dev->of_node (if !NULL), and if it's not
> matching, it rejects the device and doesn't even try to register the
> card at all. That's a common pattern for DT-based drivers, and I don't
> see why we need to do differently for any other driver (e.g., PCIe).
> 
> So...isn't 'dev->of_node != NULL' an equivalent test to
> 'of_node_valid'?
> Or put another way, mwifiex_add_card() should never see a 'struct
> device' whose of_node is not compatible. Do you agree?

I agree. 'of_node_valid' seems to be redundant here. I will remove it and post updated version.

Regards,
Amitkumar