Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650050 > unrolled thread

[PATCH 01/14] mwifiex: pcie: properly synchronize, disable interrupts in driver callbacks

Back to article view | Back to linux.kernel

Contents

  [PATCH 01/14] mwifiex: pcie: properly synchronize, disable interrupts in driver callbacks Brian Norris <briannorris@chromium.org> - 2017-05-25 02:20 +0200
    [PATCH 13/14] mwifiex: drop 'add_tail' param from mwifiex_insert_cmd_to_pending_q() Brian Norris <briannorris@chromium.org> - 2017-05-25 02:20 +0200
    [PATCH 06/14] mwifiex: don't short-circuit netdev notifiers on interface deletion Brian Norris <briannorris@chromium.org> - 2017-05-25 02:20 +0200
    [PATCH 10/14] mwifiex: pcie: stop masking interrupts in FW downloader Brian Norris <briannorris@chromium.org> - 2017-05-25 02:20 +0200
    [PATCH 09/14] mwifiex: pcie: remove redundant synchronize_irq() Brian Norris <briannorris@chromium.org> - 2017-05-25 02:20 +0200
    [PATCH 14/14] mwifiex: pcie: fix whitespace Brian Norris <briannorris@chromium.org> - 2017-05-25 02:20 +0200
    [PATCH 11/14] mwifiex: utilize netif_tx_{wake,stop}_all_queues() Brian Norris <briannorris@chromium.org> - 2017-05-25 02:20 +0200
    Re: [PATCH 01/14] mwifiex: pcie: properly synchronize, disable  interrupts in driver callbacks Brian Norris <briannorris@chromium.org> - 2017-05-31 19:20 +0200
      Re: [PATCH 01/14] mwifiex: pcie: properly synchronize, disable  interrupts in driver callbacks Xinming Hu <huxm@marvell.com> - 2017-06-05 14:00 +0200

#1650050 — [PATCH 01/14] mwifiex: pcie: properly synchronize, disable interrupts in driver callbacks

It seems that the implicit assumption of the mwifiex
{enable,disable}_int() callbacks is that after ->disable_int(), all
interrupt handling should be complete (synchronized) and not fire again
until after ->enable_int(). Also, interrupts should not be serviced
until after the first ->enable_int().

However, the PCIe driver does none of this. First, the existing
interrupt mask programming appears to only have an effect for legacy
interrupts. It doesn't actually prevent MSI/MSI-X interrupts. Second,
even when it might mask interrupts, we're doing nothing to ensure that
pending IRQs have finished processing; they could be already in-flight
when a CPU masks them.

Another quirk of this driver's design is the use of a racy
"surprise_removed" check in mwifiex_pcie_interrupt(). This appears to
act like a racy poor-man's version of masking our interrupts -- it
allows us to short-circuit the ISR if it fires when we're not prepared
to handle more work.

We can resolve this all by:
(a) disabling our IRQs after requesting them
(b) call {enable,disable}_irq() in the {enable,disable}_int() callbacks
(c) remove the racy '->surprise_removed' hack from
    mwifiex_pcie_interrupt()
(d) document the effect (or lack thereof) of PCIE_HOST_INT_MASK, to
    clarify and possibly prevent future misuse

Along the way, I decided to use underscores to prefix the driver-private
forms of "disabling interrupts" (instead of the awkward "_noerr" suffix
used already), partly to discourage their use.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 70 ++++++++++++++++++++---------
 1 file changed, 49 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 394224d6c219..ea75315bf19d 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -505,12 +505,10 @@ static int mwifiex_pm_wakeup_card_complete(struct mwifiex_adapter *adapter)
 }
 
 /*
- * This function disables the host interrupt.
- *
- * The host interrupt mask is read, the disable bit is reset and
- * written back to the card host interrupt mask register.
+ * This function masks the host interrupt. Effective only for legacy PCI
+ * interrupts.
  */
-static int mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
+static int __mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
 {
 	if (mwifiex_pcie_ok_to_access_hw(adapter)) {
 		if (mwifiex_write_reg(adapter, PCIE_HOST_INT_MASK,
@@ -525,18 +523,30 @@ static int mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
 	return 0;
 }
 
-static void mwifiex_pcie_disable_host_int_noerr(struct mwifiex_adapter *adapter)
+/*
+ * Disable interrupts, synchronizing with any outstanding interrupts.
+ */
+static void mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
 {
-	WARN_ON(mwifiex_pcie_disable_host_int(adapter));
+	struct pcie_service_card *card = adapter->card;
+	int i;
+
+	WARN_ON(__mwifiex_pcie_disable_host_int(adapter));
+
+	if (card->msix_enable) {
+		for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++) {
+			disable_irq(card->msix_entries[i].vector);
+		}
+	} else {
+		disable_irq(card->dev->irq);
+	}
 }
 
 /*
- * This function enables the host interrupt.
- *
- * The host interrupt enable mask is written to the card
- * host interrupt mask register.
+ * This function unmasks the host interrupt. Effective only for legacy PCI
+ * interrupts.
  */
-static int mwifiex_pcie_enable_host_int(struct mwifiex_adapter *adapter)
+static int __mwifiex_pcie_enable_host_int(struct mwifiex_adapter *adapter)
 {
 	if (mwifiex_pcie_ok_to_access_hw(adapter)) {
 		/* Simply write the mask to the register */
@@ -551,6 +561,26 @@ static int mwifiex_pcie_enable_host_int(struct mwifiex_adapter *adapter)
 	return 0;
 }
 
+static int mwifiex_pcie_enable_host_int(struct mwifiex_adapter *adapter)
+{
+	struct pcie_service_card *card = adapter->card;
+	int i, ret;
+
+	ret = __mwifiex_pcie_enable_host_int(adapter);
+	if (ret)
+		return ret;
+
+	if (card->msix_enable) {
+		for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++) {
+			enable_irq(card->msix_entries[i].vector);
+		}
+	} else {
+		enable_irq(card->dev->irq);
+	}
+
+	return 0;
+}
+
 /*
  * This function initializes TX buffer ring descriptors
  */
@@ -1738,7 +1768,7 @@ static int mwifiex_pcie_process_cmd_complete(struct mwifiex_adapter *adapter)
 			while (reg->sleep_cookie && (count++ < 10) &&
 			       mwifiex_pcie_ok_to_access_hw(adapter))
 				usleep_range(50, 60);
-			mwifiex_pcie_enable_host_int(adapter);
+			__mwifiex_pcie_enable_host_int(adapter);
 			mwifiex_process_sleep_confirm_resp(adapter, skb->data,
 							   skb->len);
 		} else {
@@ -2081,7 +2111,7 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter,
 		    "info: Downloading FW image (%d bytes)\n",
 		    firmware_len);
 
-	if (mwifiex_pcie_disable_host_int(adapter)) {
+	if (__mwifiex_pcie_disable_host_int(adapter)) {
 		mwifiex_dbg(adapter, ERROR,
 			    "%s: Disabling interrupts failed.\n", __func__);
 		return -1;
@@ -2335,8 +2365,7 @@ static void mwifiex_interrupt_status(struct mwifiex_adapter *adapter,
 		if ((pcie_ireg == 0xFFFFFFFF) || !pcie_ireg)
 			return;
 
-
-		mwifiex_pcie_disable_host_int(adapter);
+		__mwifiex_pcie_disable_host_int(adapter);
 
 		/* Clear the pending interrupts */
 		if (mwifiex_write_reg(adapter, PCIE_HOST_INT_STATUS,
@@ -2387,9 +2416,6 @@ static irqreturn_t mwifiex_pcie_interrupt(int irq, void *context)
 	}
 	adapter = card->adapter;
 
-	if (adapter->surprise_removed)
-		goto exit;
-
 	if (card->msix_enable)
 		mwifiex_interrupt_status(adapter, ctx->msg_id);
 	else
@@ -2494,7 +2520,7 @@ static int mwifiex_process_pcie_int(struct mwifiex_adapter *adapter)
 		    "info: cmd_sent=%d data_sent=%d\n",
 		    adapter->cmd_sent, adapter->data_sent);
 	if (!card->msi_enable && adapter->ps_state != PS_STATE_SLEEP)
-		mwifiex_pcie_enable_host_int(adapter);
+		__mwifiex_pcie_enable_host_int(adapter);
 
 	return 0;
 }
@@ -3055,6 +3081,7 @@ static int mwifiex_pcie_request_irq(struct mwifiex_adapter *adapter)
 						  &card->msix_ctx[i]);
 				if (ret)
 					break;
+				disable_irq(card->msix_entries[i].vector);
 			}
 
 			if (ret) {
@@ -3087,6 +3114,7 @@ static int mwifiex_pcie_request_irq(struct mwifiex_adapter *adapter)
 		pr_err("request_irq failed: ret=%d\n", ret);
 		return -1;
 	}
+	disable_irq(pdev->irq);
 
 	return 0;
 }
@@ -3244,7 +3272,7 @@ static struct mwifiex_if_ops pcie_ops = {
 	.register_dev =			mwifiex_register_dev,
 	.unregister_dev =		mwifiex_unregister_dev,
 	.enable_int =			mwifiex_pcie_enable_host_int,
-	.disable_int =			mwifiex_pcie_disable_host_int_noerr,
+	.disable_int =			mwifiex_pcie_disable_host_int,
 	.process_int_status =		mwifiex_process_int_status,
 	.host_to_card =			mwifiex_pcie_host_to_card,
 	.wakeup =			mwifiex_pm_wakeup_card,
-- 
2.13.0.219.gdb65acc882-goog

[toc] | [next] | [standalone]

#1650051 — [PATCH 13/14] mwifiex: drop 'add_tail' param from mwifiex_insert_cmd_to_pending_q()

It's always called with 'true' -- we only determine it 'false' locally
within this function. So drop the parameter.

Also, this should be 'bool' (since we use true/false), not 'u32'.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/cmdevt.c | 5 +++--
 drivers/net/wireless/marvell/mwifiex/main.h   | 3 +--
 drivers/net/wireless/marvell/mwifiex/scan.c   | 5 ++---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 95221306a4e5..2d94104fe7ea 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -667,7 +667,7 @@ int mwifiex_send_cmd(struct mwifiex_private *priv, u16 cmd_no,
 	    cmd_no == HostCmd_CMD_802_11_SCAN_EXT) {
 		mwifiex_queue_scan_cmd(priv, cmd_node);
 	} else {
-		mwifiex_insert_cmd_to_pending_q(adapter, cmd_node, true);
+		mwifiex_insert_cmd_to_pending_q(adapter, cmd_node);
 		queue_work(adapter->workqueue, &adapter->main_work);
 		if (cmd_node->wait_q_enabled)
 			ret = mwifiex_wait_queue_complete(adapter, cmd_node);
@@ -685,11 +685,12 @@ int mwifiex_send_cmd(struct mwifiex_private *priv, u16 cmd_no,
  */
 void
 mwifiex_insert_cmd_to_pending_q(struct mwifiex_adapter *adapter,
-				struct cmd_ctrl_node *cmd_node, u32 add_tail)
+				struct cmd_ctrl_node *cmd_node)
 {
 	struct host_cmd_ds_command *host_cmd = NULL;
 	u16 command;
 	unsigned long flags;
+	bool add_tail = true;
 
 	host_cmd = (struct host_cmd_ds_command *) (cmd_node->cmd_skb->data);
 	if (!host_cmd) {
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index c1d96c64af74..96d68ca9394f 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1071,8 +1071,7 @@ void mwifiex_recycle_cmd_node(struct mwifiex_adapter *adapter,
 			      struct cmd_ctrl_node *cmd_node);
 
 void mwifiex_insert_cmd_to_pending_q(struct mwifiex_adapter *adapter,
-				     struct cmd_ctrl_node *cmd_node,
-				     u32 addtail);
+				     struct cmd_ctrl_node *cmd_node);
 
 int mwifiex_exec_next_cmd(struct mwifiex_adapter *adapter);
 int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter);
diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
index ce6936d0c5c0..2ed6a22d5247 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -1534,8 +1534,7 @@ int mwifiex_scan_networks(struct mwifiex_private *priv,
 			list_del(&cmd_node->list);
 			spin_unlock_irqrestore(&adapter->scan_pending_q_lock,
 					       flags);
-			mwifiex_insert_cmd_to_pending_q(adapter, cmd_node,
-							true);
+			mwifiex_insert_cmd_to_pending_q(adapter, cmd_node);
 			queue_work(adapter->workqueue, &adapter->main_work);
 
 			/* Perform internal scan synchronously */
@@ -2033,7 +2032,7 @@ static void mwifiex_check_next_scan_command(struct mwifiex_private *priv)
 					    struct cmd_ctrl_node, list);
 		list_del(&cmd_node->list);
 		spin_unlock_irqrestore(&adapter->scan_pending_q_lock, flags);
-		mwifiex_insert_cmd_to_pending_q(adapter, cmd_node, true);
+		mwifiex_insert_cmd_to_pending_q(adapter, cmd_node);
 	}
 
 	return;
-- 
2.13.0.219.gdb65acc882-goog

[toc] | [prev] | [next] | [standalone]

#1650052 — [PATCH 06/14] mwifiex: don't short-circuit netdev notifiers on interface deletion

When we leave the delete interface function, there are still netdev
hooks that might try to process the device. We're short-circuiting some
of that by changing the interface type and clearing ieee80211_ptr. This
means we skip NETDEV_UNREGISTER_FINAL in cfg80211. Fortunately, that is
currently a no-op.

We don't need most of the cleanup here anyway:

 * the connection state will get (un)set as part of the disconnect
   process (which cfg80211 already initiates for us)
 * the interface type doesn't actually need to be cleared at all (it'll
   trigger a WARN_ON() in cfg80211 if we do)
 * the iee80211_ptr isn't really "ours" to clear anyway

So stop resetting those 3 things.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 025bc06a19d6..c3a25b7f2b48 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -3129,11 +3129,7 @@ int mwifiex_del_virtual_intf(struct wiphy *wiphy, struct wireless_dev *wdev)
 		priv->dfs_chan_sw_workqueue = NULL;
 	}
 	/* Clear the priv in adapter */
-	priv->netdev->ieee80211_ptr = NULL;
 	priv->netdev = NULL;
-	priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
-
-	priv->media_connected = false;
 
 	switch (priv->bss_mode) {
 	case NL80211_IFTYPE_UNSPECIFIED:
-- 
2.13.0.219.gdb65acc882-goog

[toc] | [prev] | [next] | [standalone]

#1650053 — [PATCH 10/14] mwifiex: pcie: stop masking interrupts in FW downloader

The upper layers already have disabled our interrupts (haven't called
->enable_int() yet for probe(), and we call ->disable_int() before
trying to reset), so this is redundant.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 7d5a4f2a9a22..c86119b05f52 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2113,12 +2113,6 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter,
 		    "info: Downloading FW image (%d bytes)\n",
 		    firmware_len);
 
-	if (__mwifiex_pcie_disable_host_int(adapter)) {
-		mwifiex_dbg(adapter, ERROR,
-			    "%s: Disabling interrupts failed.\n", __func__);
-		return -1;
-	}
-
 	skb = dev_alloc_skb(MWIFIEX_UPLD_SIZE);
 	if (!skb) {
 		ret = -ENOMEM;
-- 
2.13.0.219.gdb65acc882-goog

[toc] | [prev] | [next] | [standalone]

#1650054 — [PATCH 09/14] mwifiex: pcie: remove redundant synchronize_irq()

free_irq() already calls synchronize_irq() in a non-racy manner. Calling
synchronize_irq() here is redundant.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 3d5c29d79609..7d5a4f2a9a22 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -3209,9 +3209,6 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
 
 	if (card->msix_enable) {
 		for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
-			synchronize_irq(card->msix_entries[i].vector);
-
-		for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
 			free_irq(card->msix_entries[i].vector,
 				 &card->msix_ctx[i]);
 
-- 
2.13.0.219.gdb65acc882-goog

[toc] | [prev] | [next] | [standalone]

#1650056 — [PATCH 14/14] mwifiex: pcie: fix whitespace

This keeps annoying me.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index c86119b05f52..4f1d946ea460 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -3211,7 +3211,7 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
 	} else {
 		mwifiex_dbg(adapter, INFO,
 			    "%s(): calling free_irq()\n", __func__);
-	       free_irq(card->dev->irq, &card->share_irq_ctx);
+		free_irq(card->dev->irq, &card->share_irq_ctx);
 
 		if (card->msi_enable)
 			pci_disable_msi(pdev);
-- 
2.13.0.219.gdb65acc882-goog

[toc] | [prev] | [next] | [standalone]

#1650057 — [PATCH 11/14] mwifiex: utilize netif_tx_{wake,stop}_all_queues()

We're open-coding these. Just use the helpers.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/init.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index 80bdf1c5f77f..d3b8ca402d08 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -332,17 +332,9 @@ void mwifiex_wake_up_net_dev_queue(struct net_device *netdev,
 					struct mwifiex_adapter *adapter)
 {
 	unsigned long dev_queue_flags;
-	unsigned int i;
 
 	spin_lock_irqsave(&adapter->queue_lock, dev_queue_flags);
-
-	for (i = 0; i < netdev->num_tx_queues; i++) {
-		struct netdev_queue *txq = netdev_get_tx_queue(netdev, i);
-
-		if (netif_tx_queue_stopped(txq))
-			netif_tx_wake_queue(txq);
-	}
-
+	netif_tx_wake_all_queues(netdev);
 	spin_unlock_irqrestore(&adapter->queue_lock, dev_queue_flags);
 }
 
@@ -353,17 +345,9 @@ void mwifiex_stop_net_dev_queue(struct net_device *netdev,
 					struct mwifiex_adapter *adapter)
 {
 	unsigned long dev_queue_flags;
-	unsigned int i;
 
 	spin_lock_irqsave(&adapter->queue_lock, dev_queue_flags);
-
-	for (i = 0; i < netdev->num_tx_queues; i++) {
-		struct netdev_queue *txq = netdev_get_tx_queue(netdev, i);
-
-		if (!netif_tx_queue_stopped(txq))
-			netif_tx_stop_queue(txq);
-	}
-
+	netif_tx_stop_all_queues(netdev);
 	spin_unlock_irqrestore(&adapter->queue_lock, dev_queue_flags);
 }
 
-- 
2.13.0.219.gdb65acc882-goog

[toc] | [prev] | [next] | [standalone]

#1654469 — Re: [PATCH 01/14] mwifiex: pcie: properly synchronize, disable interrupts in driver callbacks

By the way, this had a few review comments elsewhere, which I'll
summarize here, since I plan to resubmit a new version sometime.

On Wed, May 24, 2017 at 05:11:06PM -0700, Brian Norris wrote:
> It seems that the implicit assumption of the mwifiex
> {enable,disable}_int() callbacks is that after ->disable_int(), all
> interrupt handling should be complete (synchronized) and not fire again
> until after ->enable_int(). Also, interrupts should not be serviced
> until after the first ->enable_int().
> 
> However, the PCIe driver does none of this. First, the existing
> interrupt mask programming appears to only have an effect for legacy
> interrupts. It doesn't actually prevent MSI/MSI-X interrupts. Second,
> even when it might mask interrupts, we're doing nothing to ensure that
> pending IRQs have finished processing; they could be already in-flight
> when a CPU masks them.
> 
> Another quirk of this driver's design is the use of a racy
> "surprise_removed" check in mwifiex_pcie_interrupt(). This appears to
> act like a racy poor-man's version of masking our interrupts -- it
> allows us to short-circuit the ISR if it fires when we're not prepared
> to handle more work.
> 
> We can resolve this all by:
> (a) disabling our IRQs after requesting them
> (b) call {enable,disable}_irq() in the {enable,disable}_int() callbacks
> (c) remove the racy '->surprise_removed' hack from
>     mwifiex_pcie_interrupt()
> (d) document the effect (or lack thereof) of PCIE_HOST_INT_MASK, to
>     clarify and possibly prevent future misuse
> 
> Along the way, I decided to use underscores to prefix the driver-private
> forms of "disabling interrupts" (instead of the awkward "_noerr" suffix
> used already), partly to discourage their use.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 70 ++++++++++++++++++++---------
>  1 file changed, 49 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 394224d6c219..ea75315bf19d 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -505,12 +505,10 @@ static int mwifiex_pm_wakeup_card_complete(struct mwifiex_adapter *adapter)
>  }
>  
>  /*
> - * This function disables the host interrupt.
> - *
> - * The host interrupt mask is read, the disable bit is reset and
> - * written back to the card host interrupt mask register.
> + * This function masks the host interrupt. Effective only for legacy PCI
> + * interrupts.
>   */
> -static int mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
> +static int __mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
>  {
>  	if (mwifiex_pcie_ok_to_access_hw(adapter)) {
>  		if (mwifiex_write_reg(adapter, PCIE_HOST_INT_MASK,
> @@ -525,18 +523,30 @@ static int mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
>  	return 0;
>  }
>  
> -static void mwifiex_pcie_disable_host_int_noerr(struct mwifiex_adapter *adapter)
> +/*
> + * Disable interrupts, synchronizing with any outstanding interrupts.
> + */
> +static void mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
>  {
> -	WARN_ON(mwifiex_pcie_disable_host_int(adapter));
> +	struct pcie_service_card *card = adapter->card;
> +	int i;
> +
> +	WARN_ON(__mwifiex_pcie_disable_host_int(adapter));
> +
> +	if (card->msix_enable) {
> +		for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++) {
> +			disable_irq(card->msix_entries[i].vector);
> +		}
> +	} else {
> +		disable_irq(card->dev->irq);

This approach is not safe for the non-MSI-X case, since we actually
requested this IRQ with IRQF_SHARED. That's likely mostly for the legacy
PCI interrupt case (where we *have* to support shared interrupts) and
could probably be modified, but at any rate, this is unsafe as written.

Also, I've fielded objections to using the host-level IRQ masking for
disabling MSI interrupts here. I'm still not completely sure *why* the
objection, but I'm investigating whether there's any device-level
mechanism for disabling MSI interrupts on the Wif card. (Marvell folks,
feel free to speak up here.)

> +	}
>  }
>  
>  /*
> - * This function enables the host interrupt.
> - *
> - * The host interrupt enable mask is written to the card
> - * host interrupt mask register.
> + * This function unmasks the host interrupt. Effective only for legacy PCI
> + * interrupts.
>   */
> -static int mwifiex_pcie_enable_host_int(struct mwifiex_adapter *adapter)
> +static int __mwifiex_pcie_enable_host_int(struct mwifiex_adapter *adapter)
>  {
>  	if (mwifiex_pcie_ok_to_access_hw(adapter)) {
>  		/* Simply write the mask to the register */
> @@ -551,6 +561,26 @@ static int mwifiex_pcie_enable_host_int(struct mwifiex_adapter *adapter)
>  	return 0;
>  }
>  
> +static int mwifiex_pcie_enable_host_int(struct mwifiex_adapter *adapter)
> +{
> +	struct pcie_service_card *card = adapter->card;
> +	int i, ret;
> +
> +	ret = __mwifiex_pcie_enable_host_int(adapter);
> +	if (ret)
> +		return ret;
> +
> +	if (card->msix_enable) {
> +		for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++) {
> +			enable_irq(card->msix_entries[i].vector);
> +		}
> +	} else {
> +		enable_irq(card->dev->irq);
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * This function initializes TX buffer ring descriptors
>   */
> @@ -1738,7 +1768,7 @@ static int mwifiex_pcie_process_cmd_complete(struct mwifiex_adapter *adapter)
>  			while (reg->sleep_cookie && (count++ < 10) &&
>  			       mwifiex_pcie_ok_to_access_hw(adapter))
>  				usleep_range(50, 60);
> -			mwifiex_pcie_enable_host_int(adapter);
> +			__mwifiex_pcie_enable_host_int(adapter);
>  			mwifiex_process_sleep_confirm_resp(adapter, skb->data,
>  							   skb->len);
>  		} else {
> @@ -2081,7 +2111,7 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter,
>  		    "info: Downloading FW image (%d bytes)\n",
>  		    firmware_len);
>  
> -	if (mwifiex_pcie_disable_host_int(adapter)) {
> +	if (__mwifiex_pcie_disable_host_int(adapter)) {
>  		mwifiex_dbg(adapter, ERROR,
>  			    "%s: Disabling interrupts failed.\n", __func__);
>  		return -1;
> @@ -2335,8 +2365,7 @@ static void mwifiex_interrupt_status(struct mwifiex_adapter *adapter,
>  		if ((pcie_ireg == 0xFFFFFFFF) || !pcie_ireg)
>  			return;
>  
> -
> -		mwifiex_pcie_disable_host_int(adapter);
> +		__mwifiex_pcie_disable_host_int(adapter);
>  
>  		/* Clear the pending interrupts */
>  		if (mwifiex_write_reg(adapter, PCIE_HOST_INT_STATUS,
> @@ -2387,9 +2416,6 @@ static irqreturn_t mwifiex_pcie_interrupt(int irq, void *context)
>  	}
>  	adapter = card->adapter;
>  
> -	if (adapter->surprise_removed)
> -		goto exit;
> -
>  	if (card->msix_enable)
>  		mwifiex_interrupt_status(adapter, ctx->msg_id);
>  	else
> @@ -2494,7 +2520,7 @@ static int mwifiex_process_pcie_int(struct mwifiex_adapter *adapter)
>  		    "info: cmd_sent=%d data_sent=%d\n",
>  		    adapter->cmd_sent, adapter->data_sent);
>  	if (!card->msi_enable && adapter->ps_state != PS_STATE_SLEEP)
> -		mwifiex_pcie_enable_host_int(adapter);
> +		__mwifiex_pcie_enable_host_int(adapter);
>  
>  	return 0;
>  }
> @@ -3055,6 +3081,7 @@ static int mwifiex_pcie_request_irq(struct mwifiex_adapter *adapter)
>  						  &card->msix_ctx[i]);
>  				if (ret)
>  					break;
> +				disable_irq(card->msix_entries[i].vector);

Also, if we're really dealing with spurious interrupts at init time,
then this leaves a window of time in between request_irq() and this
disable_irq() in which we could still receive a bad IRQ. So this should
be reworked to do one of:
(a) move the request_irq() later, until we're really able to handle
interrupts
(b) set the IRQ_NOAUTOEN flag (for the non-shared case), to avoid
enabling IRQs initially
(c) use some sort of (yet-unknown) device-level mask for MSI interrupts.

I'm looking to address these problems in a v2. Many of the other patches
are likely independent. I'll plan to resubmit them in the next series
(if they aren't applied before then), to avoid conflicts with those that
aren't independent, and because I intentionally put bugfixes (like this
patch) first in the series.

Brian

>  			}
>  
>  			if (ret) {
> @@ -3087,6 +3114,7 @@ static int mwifiex_pcie_request_irq(struct mwifiex_adapter *adapter)
>  		pr_err("request_irq failed: ret=%d\n", ret);
>  		return -1;
>  	}
> +	disable_irq(pdev->irq);
>  
>  	return 0;
>  }
> @@ -3244,7 +3272,7 @@ static struct mwifiex_if_ops pcie_ops = {
>  	.register_dev =			mwifiex_register_dev,
>  	.unregister_dev =		mwifiex_unregister_dev,
>  	.enable_int =			mwifiex_pcie_enable_host_int,
> -	.disable_int =			mwifiex_pcie_disable_host_int_noerr,
> +	.disable_int =			mwifiex_pcie_disable_host_int,
>  	.process_int_status =		mwifiex_process_int_status,
>  	.host_to_card =			mwifiex_pcie_host_to_card,
>  	.wakeup =			mwifiex_pm_wakeup_card,
> -- 
> 2.13.0.219.gdb65acc882-goog
> 

[toc] | [prev] | [next] | [standalone]

#1657535 — Re: [PATCH 01/14] mwifiex: pcie: properly synchronize, disable interrupts in driver callbacks

Hi Brian,

> -----Original Message-----
> From: linux-wireless-owner@vger.kernel.org
> [mailto:linux-wireless-owner@vger.kernel.org] On Behalf Of Brian Norris
> Sent: 2017年6月1日 1:11
> To: Ganapathi Bhat; Nishant Sarmukadam
> Cc: linux-kernel@vger.kernel.org; Dmitry Torokhov; Amitkumar Karwar; Kalle
> Valo; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 01/14] mwifiex: pcie: properly synchronize, disable
> interrupts in driver callbacks
> 
> By the way, this had a few review comments elsewhere, which I'll summarize
> here, since I plan to resubmit a new version sometime.
> 
> On Wed, May 24, 2017 at 05:11:06PM -0700, Brian Norris wrote:
> > It seems that the implicit assumption of the mwifiex
> > {enable,disable}_int() callbacks is that after ->disable_int(), all
> > interrupt handling should be complete (synchronized) and not fire
> > again until after ->enable_int(). Also, interrupts should not be
> > serviced until after the first ->enable_int().
> >
> > However, the PCIe driver does none of this. First, the existing
> > interrupt mask programming appears to only have an effect for legacy
> > interrupts. It doesn't actually prevent MSI/MSI-X interrupts. Second,
> > even when it might mask interrupts, we're doing nothing to ensure that
> > pending IRQs have finished processing; they could be already in-flight
> > when a CPU masks them.
> >
> > Another quirk of this driver's design is the use of a racy
> > "surprise_removed" check in mwifiex_pcie_interrupt(). This appears to
> > act like a racy poor-man's version of masking our interrupts -- it
> > allows us to short-circuit the ISR if it fires when we're not prepared
> > to handle more work.
> >
> > We can resolve this all by:
> > (a) disabling our IRQs after requesting them
> > (b) call {enable,disable}_irq() in the {enable,disable}_int()
> > callbacks
> > (c) remove the racy '->surprise_removed' hack from
> >     mwifiex_pcie_interrupt()
> > (d) document the effect (or lack thereof) of PCIE_HOST_INT_MASK, to
> >     clarify and possibly prevent future misuse
> >
> > Along the way, I decided to use underscores to prefix the
> > driver-private forms of "disabling interrupts" (instead of the awkward
> > "_noerr" suffix used already), partly to discourage their use.
> >
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> >  drivers/net/wireless/marvell/mwifiex/pcie.c | 70
> > ++++++++++++++++++++---------
> >  1 file changed, 49 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index 394224d6c219..ea75315bf19d 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > @@ -505,12 +505,10 @@ static int
> > mwifiex_pm_wakeup_card_complete(struct mwifiex_adapter *adapter)  }
> >
> >  /*
> > - * This function disables the host interrupt.
> > - *
> > - * The host interrupt mask is read, the disable bit is reset and
> > - * written back to the card host interrupt mask register.
> > + * This function masks the host interrupt. Effective only for legacy
> > + PCI
> > + * interrupts.
> >   */
> > -static int mwifiex_pcie_disable_host_int(struct mwifiex_adapter
> > *adapter)
> > +static int __mwifiex_pcie_disable_host_int(struct mwifiex_adapter
> > +*adapter)
> >  {
> >  	if (mwifiex_pcie_ok_to_access_hw(adapter)) {
> >  		if (mwifiex_write_reg(adapter, PCIE_HOST_INT_MASK, @@ -525,18
> > +523,30 @@ static int mwifiex_pcie_disable_host_int(struct mwifiex_adapter
> *adapter)
> >  	return 0;
> >  }
> >
> > -static void mwifiex_pcie_disable_host_int_noerr(struct
> > mwifiex_adapter *adapter)
> > +/*
> > + * Disable interrupts, synchronizing with any outstanding interrupts.
> > + */
> > +static void mwifiex_pcie_disable_host_int(struct mwifiex_adapter
> > +*adapter)
> >  {
> > -	WARN_ON(mwifiex_pcie_disable_host_int(adapter));
> > +	struct pcie_service_card *card = adapter->card;
> > +	int i;
> > +
> > +	WARN_ON(__mwifiex_pcie_disable_host_int(adapter));
> > +
> > +	if (card->msix_enable) {
> > +		for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++) {
> > +			disable_irq(card->msix_entries[i].vector);
> > +		}
> > +	} else {
> > +		disable_irq(card->dev->irq);
> 
> This approach is not safe for the non-MSI-X case, since we actually requested
> this IRQ with IRQF_SHARED. That's likely mostly for the legacy PCI interrupt
> case (where we *have* to support shared interrupts) and could probably be
> modified, but at any rate, this is unsafe as written.
> 
> Also, I've fielded objections to using the host-level IRQ masking for disabling
> MSI interrupts here. I'm still not completely sure *why* the objection, but I'm
> investigating whether there's any device-level mechanism for disabling MSI
> interrupts on the Wif card. (Marvell folks, feel free to speak up here.)
> 

per our investigate:
the msi interrupt could be latch  in device function temporary(pci_msi_mask_irq), but it is not able to disable interrupt generating from device, these pending interrupts will arrive after pci_msi_unmask_irq.. 

this is expected , as implied by the spec “While a vector is masked, the function is prohibited from sending the associated message, and the function must set the associated Pending bit whenever the function would otherwise send the message. When software unmasks a vector whose associated Pending bit is set, the function must schedule sending the associated message, and clear the Pending bit as soon as the message has been sent.”

Apart from these two API, the only way we can find is pci_disable_msi/msix, obviously not suitable..(quite strange, there is no lightweight host API for disable MSI interrupt,, maybe something related with MSI spec..)

And from device side, in current design, there is no similar “Host Interrupt Status Mask” registers/logics to prevent PCIe MSI logic to latch the interrupt request it received.



Regards,
Simon

> > +	}
> >  }
> >
> >  /*
> > - * This function enables the host interrupt.
> > - *
> > - * The host interrupt enable mask is written to the card
> > - * host interrupt mask register.
> > + * This function unmasks the host interrupt. Effective only for
> > + legacy PCI
> > + * interrupts.
> >   */
> > -static int mwifiex_pcie_enable_host_int(struct mwifiex_adapter
> > *adapter)
> > +static int __mwifiex_pcie_enable_host_int(struct mwifiex_adapter
> > +*adapter)
> >  {
> >  	if (mwifiex_pcie_ok_to_access_hw(adapter)) {
> >  		/* Simply write the mask to the register */ @@ -551,6 +561,26 @@
> > static int mwifiex_pcie_enable_host_int(struct mwifiex_adapter *adapter)
> >  	return 0;
> >  }
> >
> > +static int mwifiex_pcie_enable_host_int(struct mwifiex_adapter
> > +*adapter) {
> > +	struct pcie_service_card *card = adapter->card;
> > +	int i, ret;
> > +
> > +	ret = __mwifiex_pcie_enable_host_int(adapter);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (card->msix_enable) {
> > +		for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++) {
> > +			enable_irq(card->msix_entries[i].vector);
> > +		}
> > +	} else {
> > +		enable_irq(card->dev->irq);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * This function initializes TX buffer ring descriptors
> >   */
> > @@ -1738,7 +1768,7 @@ static int
> mwifiex_pcie_process_cmd_complete(struct mwifiex_adapter *adapter)
> >  			while (reg->sleep_cookie && (count++ < 10) &&
> >  			       mwifiex_pcie_ok_to_access_hw(adapter))
> >  				usleep_range(50, 60);
> > -			mwifiex_pcie_enable_host_int(adapter);
> > +			__mwifiex_pcie_enable_host_int(adapter);
> >  			mwifiex_process_sleep_confirm_resp(adapter, skb->data,
> >  							   skb->len);
> >  		} else {
> > @@ -2081,7 +2111,7 @@ static int mwifiex_prog_fw_w_helper(struct
> mwifiex_adapter *adapter,
> >  		    "info: Downloading FW image (%d bytes)\n",
> >  		    firmware_len);
> >
> > -	if (mwifiex_pcie_disable_host_int(adapter)) {
> > +	if (__mwifiex_pcie_disable_host_int(adapter)) {
> >  		mwifiex_dbg(adapter, ERROR,
> >  			    "%s: Disabling interrupts failed.\n", __func__);
> >  		return -1;
> > @@ -2335,8 +2365,7 @@ static void mwifiex_interrupt_status(struct
> mwifiex_adapter *adapter,
> >  		if ((pcie_ireg == 0xFFFFFFFF) || !pcie_ireg)
> >  			return;
> >
> > -
> > -		mwifiex_pcie_disable_host_int(adapter);
> > +		__mwifiex_pcie_disable_host_int(adapter);
> >
> >  		/* Clear the pending interrupts */
> >  		if (mwifiex_write_reg(adapter, PCIE_HOST_INT_STATUS, @@ -2387,9
> > +2416,6 @@ static irqreturn_t mwifiex_pcie_interrupt(int irq, void *context)
> >  	}
> >  	adapter = card->adapter;
> >
> > -	if (adapter->surprise_removed)
> > -		goto exit;
> > -
> >  	if (card->msix_enable)
> >  		mwifiex_interrupt_status(adapter, ctx->msg_id);
> >  	else
> > @@ -2494,7 +2520,7 @@ static int mwifiex_process_pcie_int(struct
> mwifiex_adapter *adapter)
> >  		    "info: cmd_sent=%d data_sent=%d\n",
> >  		    adapter->cmd_sent, adapter->data_sent);
> >  	if (!card->msi_enable && adapter->ps_state != PS_STATE_SLEEP)
> > -		mwifiex_pcie_enable_host_int(adapter);
> > +		__mwifiex_pcie_enable_host_int(adapter);
> >
> >  	return 0;
> >  }
> > @@ -3055,6 +3081,7 @@ static int mwifiex_pcie_request_irq(struct
> mwifiex_adapter *adapter)
> >  						  &card->msix_ctx[i]);
> >  				if (ret)
> >  					break;
> > +				disable_irq(card->msix_entries[i].vector);
> 
> Also, if we're really dealing with spurious interrupts at init time, then this
> leaves a window of time in between request_irq() and this
> disable_irq() in which we could still receive a bad IRQ. So this should be
> reworked to do one of:
> (a) move the request_irq() later, until we're really able to handle interrupts
> (b) set the IRQ_NOAUTOEN flag (for the non-shared case), to avoid enabling
> IRQs initially
> (c) use some sort of (yet-unknown) device-level mask for MSI interrupts.
> 
> I'm looking to address these problems in a v2. Many of the other patches are
> likely independent. I'll plan to resubmit them in the next series (if they aren't
> applied before then), to avoid conflicts with those that aren't independent,
> and because I intentionally put bugfixes (like this
> patch) first in the series.
> 
> Brian
> 
> >  			}
> >
> >  			if (ret) {
> > @@ -3087,6 +3114,7 @@ static int mwifiex_pcie_request_irq(struct
> mwifiex_adapter *adapter)
> >  		pr_err("request_irq failed: ret=%d\n", ret);
> >  		return -1;
> >  	}
> > +	disable_irq(pdev->irq);
> >
> >  	return 0;
> >  }
> > @@ -3244,7 +3272,7 @@ static struct mwifiex_if_ops pcie_ops = {
> >  	.register_dev =			mwifiex_register_dev,
> >  	.unregister_dev =		mwifiex_unregister_dev,
> >  	.enable_int =			mwifiex_pcie_enable_host_int,
> > -	.disable_int =			mwifiex_pcie_disable_host_int_noerr,
> > +	.disable_int =			mwifiex_pcie_disable_host_int,
> >  	.process_int_status =		mwifiex_process_int_status,
> >  	.host_to_card =			mwifiex_pcie_host_to_card,
> >  	.wakeup =			mwifiex_pm_wakeup_card,
> > --
> > 2.13.0.219.gdb65acc882-goog
> >

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web