[PATCH 0/4] refactor cmd_node recycling

[PATCH 0/4] refactor cmd_node recycling

[Andreas Fenkart]

Following patches are trying to simplify the code paths used
to recycle processed commands

Andreas Fenkart (4):
  mwifiex: remove explicit mwifiex_complete_cmd calls
  mwifiex: remove redundant reset of cmd_wait_q status
  mwifiex: remove CMD_F_CANCELED flag
  mwifiex: simplify mwifiex_complete_cmd

 drivers/net/wireless/mwifiex/cmdevt.c    | 35 +++++++++++---------------------
 drivers/net/wireless/mwifiex/fw.h        |  1 -
 drivers/net/wireless/mwifiex/sta_ioctl.c |  4 ++--
 drivers/net/wireless/mwifiex/util.c      | 12 ++++-------
 4 files changed, 18 insertions(+), 34 deletions(-)

-- 
2.1.4

[PATCH 1/4] mwifiex: remove explicit mwifiex_complete_cmd calls

[Andreas Fenkart]

standard call chain when releasing a cmd node:
mwifiex_recycle_cmd_node
-> mwifiex_insert_cmd_to_free_q
-> mwifiex_complete_cmd, if wait_q_enabled

calling mwifiex_complete_cmd explicitly and setting
wait_q_enabled = false is redundant

Signed-off-by: Andreas Fenkart <afenkart@gmail.com>
---
 drivers/net/wireless/mwifiex/cmdevt.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c
index 207da40..2f4715e 100644
--- a/drivers/net/wireless/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/mwifiex/cmdevt.c
@@ -167,8 +167,6 @@ static int mwifiex_dnld_cmd_to_fw(struct mwifiex_private *priv,
 		mwifiex_dbg(adapter, ERROR,
 			    "DNLD_CMD: FW in reset state, ignore cmd %#x\n",
 			cmd_code);
-		if (cmd_node->wait_q_enabled)
-			mwifiex_complete_cmd(adapter, cmd_node);
 		mwifiex_recycle_cmd_node(adapter, cmd_node);
 		queue_work(adapter->workqueue, &adapter->main_work);
 		return -1;
@@ -1024,6 +1022,7 @@ mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter *adapter)
 		adapter->curr_cmd->wait_q_enabled = false;
 		adapter->cmd_wait_q.status = -1;
 		mwifiex_complete_cmd(adapter, adapter->curr_cmd);
+		/* no recycle probably wait for response */
 	}
 	/* Cancel all pending command */
 	spin_lock_irqsave(&adapter->cmd_pending_q_lock, flags);
@@ -1032,11 +1031,8 @@ mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter *adapter)
 		list_del(&cmd_node->list);
 		spin_unlock_irqrestore(&adapter->cmd_pending_q_lock, flags);
 
-		if (cmd_node->wait_q_enabled) {
+		if (cmd_node->wait_q_enabled)
 			adapter->cmd_wait_q.status = -1;
-			mwifiex_complete_cmd(adapter, cmd_node);
-			cmd_node->wait_q_enabled = false;
-		}
 		mwifiex_recycle_cmd_node(adapter, cmd_node);
 		spin_lock_irqsave(&adapter->cmd_pending_q_lock, flags);
 	}
@@ -1094,10 +1090,8 @@ mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter)
 	    (adapter->curr_cmd->wait_q_enabled)) {
 		spin_lock_irqsave(&adapter->mwifiex_cmd_lock, cmd_flags);
 		cmd_node = adapter->curr_cmd;
-		cmd_node->wait_q_enabled = false;
 		cmd_node->cmd_flag |= CMD_F_CANCELED;
 		mwifiex_recycle_cmd_node(adapter, cmd_node);
-		mwifiex_complete_cmd(adapter, adapter->curr_cmd);
 		adapter->curr_cmd = NULL;
 		spin_unlock_irqrestore(&adapter->mwifiex_cmd_lock, cmd_flags);
 	}
-- 
2.1.4

[PATCH 2/4] mwifiex: remove redundant reset of cmd_wait_q status

[Andreas Fenkart]

mwifiex_cancel_pending_ioctl is called only from
mwifiex_cmd_timeout_func. There the wait_q status is set to
-ETIMEDWAIT before calling this function. Whether we reset the status
to -1 or leave it at -ETIMEDWAIT at end doesn't matter since both
are != 0 hence mean failure

Signed-off-by: Andreas Fenkart <afenkart@gmail.com>
---
 drivers/net/wireless/mwifiex/cmdevt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c
index 2f4715e..87b6dee 100644
--- a/drivers/net/wireless/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/mwifiex/cmdevt.c
@@ -1123,7 +1123,6 @@ mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter)
 			}
 		}
 	}
-	adapter->cmd_wait_q.status = -1;
 }
 
 /*
-- 
2.1.4

[PATCH 3/4] mwifiex: remove CMD_F_CANCELED flag

[Andreas Fenkart]

CMD_F_CANCELED was used to abort mwifiex_process_cmdresp in
case it already started or starts processing the cmd.
But this was probably not working the way intended:
- it is racy: mwifiex_process_cmdresp might already have passed that
  test and is continuing to use the cmd node being recycled
- mwifiex_process_cmdresp repeatedly uses adapter->curr_cmd which
  we just set to NULL
- mwifiex_recycle_cmd_node will clear the flag

The reason why it probably works is that mwifiex_cancel_pending_ioctl
is only called from mwifiex_cmd_timeout_func, where the there is little
chance of a command response still arriving

Signed-off-by: Andreas Fenkart <afenkart@gmail.com>
---
 drivers/net/wireless/mwifiex/cmdevt.c | 23 ++++++++++-------------
 drivers/net/wireless/mwifiex/fw.h     |  1 -
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c
index 87b6dee..6458e17 100644
--- a/drivers/net/wireless/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/mwifiex/cmdevt.c
@@ -807,17 +807,6 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter)
 	adapter->is_cmd_timedout = 0;
 
 	resp = (struct host_cmd_ds_command *) adapter->curr_cmd->resp_skb->data;
-	if (adapter->curr_cmd->cmd_flag & CMD_F_CANCELED) {
-		mwifiex_dbg(adapter, ERROR,
-			    "CMD_RESP: %#x been canceled\n",
-			    le16_to_cpu(resp->command));
-		mwifiex_recycle_cmd_node(adapter, adapter->curr_cmd);
-		spin_lock_irqsave(&adapter->mwifiex_cmd_lock, flags);
-		adapter->curr_cmd = NULL;
-		spin_unlock_irqrestore(&adapter->mwifiex_cmd_lock, flags);
-		return -1;
-	}
-
 	if (adapter->curr_cmd->cmd_flag & CMD_F_HOSTCMD) {
 		/* Copy original response back to response buffer */
 		struct mwifiex_ds_misc_cmd *hostcmd;
@@ -1090,10 +1079,18 @@ mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter)
 	    (adapter->curr_cmd->wait_q_enabled)) {
 		spin_lock_irqsave(&adapter->mwifiex_cmd_lock, cmd_flags);
 		cmd_node = adapter->curr_cmd;
-		cmd_node->cmd_flag |= CMD_F_CANCELED;
-		mwifiex_recycle_cmd_node(adapter, cmd_node);
+		/* setting curr_cmd to NULL is quite dangerous, because
+		 * mwifiex_process_cmdresp checks curr_cmd to be != NULL
+		 * at the beginning then relies on it and dereferences
+		 * it at will
+		 * this probably works since mwifiex_cmd_timeout_func
+		 * is the only caller of this function and responses
+		 * at that point
+		 */
 		adapter->curr_cmd = NULL;
 		spin_unlock_irqrestore(&adapter->mwifiex_cmd_lock, cmd_flags);
+
+		mwifiex_recycle_cmd_node(adapter, cmd_node);
 	}
 
 	/* Cancel all pending scan command */
diff --git a/drivers/net/wireless/mwifiex/fw.h b/drivers/net/wireless/mwifiex/fw.h
index cd09051..c71e6b2 100644
--- a/drivers/net/wireless/mwifiex/fw.h
+++ b/drivers/net/wireless/mwifiex/fw.h
@@ -432,7 +432,6 @@ enum P2P_MODES {
 
 
 #define CMD_F_HOSTCMD           (1 << 0)
-#define CMD_F_CANCELED          (1 << 1)
 
 #define HostCmd_CMD_ID_MASK             0x0fff
 
-- 
2.1.4

[PATCH 4/4] mwifiex: simplify mwifiex_complete_cmd

[Andreas Fenkart]

600f5d909a54("mwifiex: cleanup ioctl wait queue and abstraction layer")
introduced the wakeup_interruptible suppression in mwifiex_complete_cmd
b1a47aa5e1e1("mwifiex: fix system hang issue in cmd timeout error case")
then added wakup_interruptible to mwifiex_cmd_timeout_func the single
place setting a status of ETIMEDOUT.
Instead of doing extra work, using the standard call-chain will have the
same effect:
mwifiex_cancel_pending_ioctl
-> mwifiex_recycle_cmd_node
-> mwifiex_insert_cmd_to_free_q
-> mwifiex_complete_cmd
-> wake_up_interruptible

The difference is that previously the condition was not set to true,
but that's probably just an oversight in b1a47aa5e1e1 and shouldn't
have any consequence

Signed-off-by: Andreas Fenkart <afenkart@gmail.com>
---
 drivers/net/wireless/mwifiex/cmdevt.c    |  1 -
 drivers/net/wireless/mwifiex/sta_ioctl.c |  4 ++--
 drivers/net/wireless/mwifiex/util.c      | 12 ++++--------
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c
index 6458e17..27b778d 100644
--- a/drivers/net/wireless/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/mwifiex/cmdevt.c
@@ -976,7 +976,6 @@ mwifiex_cmd_timeout_func(unsigned long function_context)
 
 		if (cmd_node->wait_q_enabled) {
 			adapter->cmd_wait_q.status = -ETIMEDOUT;
-			wake_up_interruptible(&adapter->cmd_wait_q.wait);
 			mwifiex_cancel_pending_ioctl(adapter);
 		}
 	}
diff --git a/drivers/net/wireless/mwifiex/sta_ioctl.c b/drivers/net/wireless/mwifiex/sta_ioctl.c
index d8b7d9c..a6c8a4f 100644
--- a/drivers/net/wireless/mwifiex/sta_ioctl.c
+++ b/drivers/net/wireless/mwifiex/sta_ioctl.c
@@ -66,8 +66,8 @@ int mwifiex_wait_queue_complete(struct mwifiex_adapter *adapter,
 	if (status <= 0) {
 		if (status == 0)
 			status = -ETIMEDOUT;
-		mwifiex_dbg(adapter, ERROR,
-			    "cmd_wait_q terminated: %d\n", status);
+		mwifiex_dbg(adapter, ERROR, "cmd_wait_q terminated: %d\n",
+			    status);
 		mwifiex_cancel_all_pending_cmd(adapter);
 		return status;
 	}
diff --git a/drivers/net/wireless/mwifiex/util.c b/drivers/net/wireless/mwifiex/util.c
index 790e619..b65ef5b 100644
--- a/drivers/net/wireless/mwifiex/util.c
+++ b/drivers/net/wireless/mwifiex/util.c
@@ -496,16 +496,12 @@ int mwifiex_recv_packet(struct mwifiex_private *priv, struct sk_buff *skb)
 int mwifiex_complete_cmd(struct mwifiex_adapter *adapter,
 			 struct cmd_ctrl_node *cmd_node)
 {
-	mwifiex_dbg(adapter, CMD,
-		    "cmd completed: status=%d\n",
+	WARN_ON(!cmd_node->wait_q_enabled);
+	mwifiex_dbg(adapter, CMD, "cmd completed: status=%d\n",
 		    adapter->cmd_wait_q.status);
 
-	*(cmd_node->condition) = true;
-
-	if (adapter->cmd_wait_q.status == -ETIMEDOUT)
-		mwifiex_dbg(adapter, ERROR, "cmd timeout\n");
-	else
-		wake_up_interruptible(&adapter->cmd_wait_q.wait);
+	*cmd_node->condition = true;
+	wake_up_interruptible(&adapter->cmd_wait_q.wait);
 
 	return 0;
 }
-- 
2.1.4

RE: [PATCH 0/4] refactor cmd_node recycling

[Amitkumar Karwar]

Hi Andreas,

> From: Andreas Fenkart [mailto:afenkart@gmail.com]
> Sent: Friday, July 17, 2015 12:43 PM
> To: linux-wireless@vger.kernel.org
> Cc: Amitkumar Karwar; Kalle Valo; Andreas Fenkart
> Subject: [PATCH 0/4] refactor cmd_node recycling
> 
> Following patches are trying to simplify the code paths used to recycle
> processed commands
> 
> Andreas Fenkart (4):
>   mwifiex: remove explicit mwifiex_complete_cmd calls
>   mwifiex: remove redundant reset of cmd_wait_q status
>   mwifiex: remove CMD_F_CANCELED flag
>   mwifiex: simplify mwifiex_complete_cmd
> 
>  drivers/net/wireless/mwifiex/cmdevt.c    | 35 +++++++++++--------------
> -------
>  drivers/net/wireless/mwifiex/fw.h        |  1 -
>  drivers/net/wireless/mwifiex/sta_ioctl.c |  4 ++--
>  drivers/net/wireless/mwifiex/util.c      | 12 ++++-------
>  4 files changed, 18 insertions(+), 34 deletions(-)
> 

Looks fine to me. Thanks for the cleanup work and simplifying the command path.

Regards,
Amitkumar

RE: [PATCH 3/4] mwifiex: remove CMD_F_CANCELED flag

[Amitkumar Karwar]

Hi Andreas,

> From: Andreas Fenkart [mailto:afenkart@gmail.com]
> Sent: Friday, July 17, 2015 12:43 PM
> To: linux-wireless@vger.kernel.org
> Cc: Amitkumar Karwar; Kalle Valo; Andreas Fenkart
> Subject: [PATCH 3/4] mwifiex: remove CMD_F_CANCELED flag
> 
> CMD_F_CANCELED was used to abort mwifiex_process_cmdresp in case it
> already started or starts processing the cmd.
> But this was probably not working the way intended:
> - it is racy: mwifiex_process_cmdresp might already have passed that
>   test and is continuing to use the cmd node being recycled
> - mwifiex_process_cmdresp repeatedly uses adapter->curr_cmd which
>   we just set to NULL
> - mwifiex_recycle_cmd_node will clear the flag
> 
> The reason why it probably works is that mwifiex_cancel_pending_ioctl is
> only called from mwifiex_cmd_timeout_func, where the there is little
> chance of a command response still arriving
> 

You are right. Command timeout handler is called when there is no response from firmware for 10 seconds. If firmware is alive and working, we would have received command response within a  millisecond. So there is very little chance of command response arriving while executing command timeout handler.

Regards,
Amit

RE: [PATCH 1/4] mwifiex: remove explicit mwifiex_complete_cmd calls

[Amitkumar Karwar]

Hi Andreas,

> From: Andreas Fenkart [mailto:afenkart@gmail.com]
> Sent: Friday, July 17, 2015 12:43 PM
> To: linux-wireless@vger.kernel.org
> Cc: Amitkumar Karwar; Kalle Valo; Andreas Fenkart
> Subject: [PATCH 1/4] mwifiex: remove explicit mwifiex_complete_cmd calls
> 
> standard call chain when releasing a cmd node:
> mwifiex_recycle_cmd_node
> -> mwifiex_insert_cmd_to_free_q
> -> mwifiex_complete_cmd, if wait_q_enabled
> 
> calling mwifiex_complete_cmd explicitly and setting wait_q_enabled =
> false is redundant
> 
> Signed-off-by: Andreas Fenkart <afenkart@gmail.com>
> ---
>  drivers/net/wireless/mwifiex/cmdevt.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/mwifiex/cmdevt.c
> b/drivers/net/wireless/mwifiex/cmdevt.c
> index 207da40..2f4715e 100644
> --- a/drivers/net/wireless/mwifiex/cmdevt.c
> +++ b/drivers/net/wireless/mwifiex/cmdevt.c
> @@ -167,8 +167,6 @@ static int mwifiex_dnld_cmd_to_fw(struct
> mwifiex_private *priv,
>  		mwifiex_dbg(adapter, ERROR,
>  			    "DNLD_CMD: FW in reset state, ignore cmd %#x\n",
>  			cmd_code);
> -		if (cmd_node->wait_q_enabled)
> -			mwifiex_complete_cmd(adapter, cmd_node);
>  		mwifiex_recycle_cmd_node(adapter, cmd_node);
>  		queue_work(adapter->workqueue, &adapter->main_work);
>  		return -1;
> @@ -1024,6 +1022,7 @@ mwifiex_cancel_all_pending_cmd(struct
> mwifiex_adapter *adapter)
>  		adapter->curr_cmd->wait_q_enabled = false;
>  		adapter->cmd_wait_q.status = -1;
>  		mwifiex_complete_cmd(adapter, adapter->curr_cmd);
> +		/* no recycle probably wait for response */
>  	}
>  	/* Cancel all pending command */
>  	spin_lock_irqsave(&adapter->cmd_pending_q_lock, flags); @@ -
> 1032,11 +1031,8 @@ mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter
> *adapter)
>  		list_del(&cmd_node->list);
>  		spin_unlock_irqrestore(&adapter->cmd_pending_q_lock, flags);
> 
> -		if (cmd_node->wait_q_enabled) {
> +		if (cmd_node->wait_q_enabled)
>  			adapter->cmd_wait_q.status = -1;
> -			mwifiex_complete_cmd(adapter, cmd_node);
> -			cmd_node->wait_q_enabled = false;
> -		}
>  		mwifiex_recycle_cmd_node(adapter, cmd_node);
>  		spin_lock_irqsave(&adapter->cmd_pending_q_lock, flags);
>  	}
> @@ -1094,10 +1090,8 @@ mwifiex_cancel_pending_ioctl(struct
> mwifiex_adapter *adapter)
>  	    (adapter->curr_cmd->wait_q_enabled)) {
>  		spin_lock_irqsave(&adapter->mwifiex_cmd_lock, cmd_flags);
>  		cmd_node = adapter->curr_cmd;
> -		cmd_node->wait_q_enabled = false;
>  		cmd_node->cmd_flag |= CMD_F_CANCELED;
>  		mwifiex_recycle_cmd_node(adapter, cmd_node);
> -		mwifiex_complete_cmd(adapter, adapter->curr_cmd);
>  		adapter->curr_cmd = NULL;
>  		spin_unlock_irqrestore(&adapter->mwifiex_cmd_lock,
> cmd_flags);
>  	}
> --
> 2.1.4

Acked-by: Amitkumar Karwar <akarwar@marvell.com>

Regards,
Amitkumar

RE: [PATCH 2/4] mwifiex: remove redundant reset of cmd_wait_q status

[Amitkumar Karwar]

Hi Andreas,

> From: Andreas Fenkart [mailto:afenkart@gmail.com]
> Sent: Friday, July 17, 2015 12:43 PM
> To: linux-wireless@vger.kernel.org
> Cc: Amitkumar Karwar; Kalle Valo; Andreas Fenkart
> Subject: [PATCH 2/4] mwifiex: remove redundant reset of cmd_wait_q
> status
> 
> mwifiex_cancel_pending_ioctl is called only from
> mwifiex_cmd_timeout_func. There the wait_q status is set to -ETIMEDWAIT
> before calling this function. Whether we reset the status to -1 or leave
> it at -ETIMEDWAIT at end doesn't matter since both are != 0 hence mean
> failure
> 
> Signed-off-by: Andreas Fenkart <afenkart@gmail.com>
> ---
>  drivers/net/wireless/mwifiex/cmdevt.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/mwifiex/cmdevt.c
> b/drivers/net/wireless/mwifiex/cmdevt.c
> index 2f4715e..87b6dee 100644
> --- a/drivers/net/wireless/mwifiex/cmdevt.c
> +++ b/drivers/net/wireless/mwifiex/cmdevt.c
> @@ -1123,7 +1123,6 @@ mwifiex_cancel_pending_ioctl(struct
> mwifiex_adapter *adapter)
>  			}
>  		}
>  	}
> -	adapter->cmd_wait_q.status = -1;
>  }
> 
>  /*
> --
> 2.1.4

Acked-by: Amitkumar Karwar <akarwar@marvell.com>

Regards,
Amitkumar

RE: [PATCH 3/4] mwifiex: remove CMD_F_CANCELED flag

[Amitkumar Karwar]

Hi Andreas,

> From: Andreas Fenkart [mailto:afenkart@gmail.com]
> Sent: Friday, July 17, 2015 12:43 PM
> To: linux-wireless@vger.kernel.org
> Cc: Amitkumar Karwar; Kalle Valo; Andreas Fenkart
> Subject: [PATCH 3/4] mwifiex: remove CMD_F_CANCELED flag
> 
> CMD_F_CANCELED was used to abort mwifiex_process_cmdresp in case it
> already started or starts processing the cmd.
> But this was probably not working the way intended:
> - it is racy: mwifiex_process_cmdresp might already have passed that
>   test and is continuing to use the cmd node being recycled
> - mwifiex_process_cmdresp repeatedly uses adapter->curr_cmd which
>   we just set to NULL
> - mwifiex_recycle_cmd_node will clear the flag
> 
> The reason why it probably works is that mwifiex_cancel_pending_ioctl is
> only called from mwifiex_cmd_timeout_func, where the there is little
> chance of a command response still arriving
> 
> Signed-off-by: Andreas Fenkart <afenkart@gmail.com>
> ---
>  drivers/net/wireless/mwifiex/cmdevt.c | 23 ++++++++++-------------
>  drivers/net/wireless/mwifiex/fw.h     |  1 -
>  2 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/wireless/mwifiex/cmdevt.c
> b/drivers/net/wireless/mwifiex/cmdevt.c
> index 87b6dee..6458e17 100644
> --- a/drivers/net/wireless/mwifiex/cmdevt.c
> +++ b/drivers/net/wireless/mwifiex/cmdevt.c
> @@ -807,17 +807,6 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter
> *adapter)
>  	adapter->is_cmd_timedout = 0;
> 
>  	resp = (struct host_cmd_ds_command *) adapter->curr_cmd->resp_skb-
> >data;
> -	if (adapter->curr_cmd->cmd_flag & CMD_F_CANCELED) {
> -		mwifiex_dbg(adapter, ERROR,
> -			    "CMD_RESP: %#x been canceled\n",
> -			    le16_to_cpu(resp->command));
> -		mwifiex_recycle_cmd_node(adapter, adapter->curr_cmd);
> -		spin_lock_irqsave(&adapter->mwifiex_cmd_lock, flags);
> -		adapter->curr_cmd = NULL;
> -		spin_unlock_irqrestore(&adapter->mwifiex_cmd_lock, flags);
> -		return -1;
> -	}
> -
>  	if (adapter->curr_cmd->cmd_flag & CMD_F_HOSTCMD) {
>  		/* Copy original response back to response buffer */
>  		struct mwifiex_ds_misc_cmd *hostcmd;
> @@ -1090,10 +1079,18 @@ mwifiex_cancel_pending_ioctl(struct
> mwifiex_adapter *adapter)
>  	    (adapter->curr_cmd->wait_q_enabled)) {
>  		spin_lock_irqsave(&adapter->mwifiex_cmd_lock, cmd_flags);
>  		cmd_node = adapter->curr_cmd;
> -		cmd_node->cmd_flag |= CMD_F_CANCELED;
> -		mwifiex_recycle_cmd_node(adapter, cmd_node);
> +		/* setting curr_cmd to NULL is quite dangerous, because
> +		 * mwifiex_process_cmdresp checks curr_cmd to be != NULL
> +		 * at the beginning then relies on it and dereferences
> +		 * it at will
> +		 * this probably works since mwifiex_cmd_timeout_func
> +		 * is the only caller of this function and responses
> +		 * at that point
> +		 */
>  		adapter->curr_cmd = NULL;
>  		spin_unlock_irqrestore(&adapter->mwifiex_cmd_lock,
> cmd_flags);
> +
> +		mwifiex_recycle_cmd_node(adapter, cmd_node);
>  	}
> 
>  	/* Cancel all pending scan command */
> diff --git a/drivers/net/wireless/mwifiex/fw.h
> b/drivers/net/wireless/mwifiex/fw.h
> index cd09051..c71e6b2 100644
> --- a/drivers/net/wireless/mwifiex/fw.h
> +++ b/drivers/net/wireless/mwifiex/fw.h
> @@ -432,7 +432,6 @@ enum P2P_MODES {
> 
> 
>  #define CMD_F_HOSTCMD           (1 << 0)
> -#define CMD_F_CANCELED          (1 << 1)
> 
>  #define HostCmd_CMD_ID_MASK             0x0fff
> 
> --
> 2.1.4

Acked-by: Amitkumar Karwar <akarwar@marvell.com>

Regards,
Amitkumar

RE: [PATCH 4/4] mwifiex: simplify mwifiex_complete_cmd

[Amitkumar Karwar]

Hi Andreas,

> From: Andreas Fenkart [mailto:afenkart@gmail.com]
> Sent: Friday, July 17, 2015 12:43 PM
> To: linux-wireless@vger.kernel.org
> Cc: Amitkumar Karwar; Kalle Valo; Andreas Fenkart
> Subject: [PATCH 4/4] mwifiex: simplify mwifiex_complete_cmd
> 
> 600f5d909a54("mwifiex: cleanup ioctl wait queue and abstraction layer")
> introduced the wakeup_interruptible suppression in mwifiex_complete_cmd
> b1a47aa5e1e1("mwifiex: fix system hang issue in cmd timeout error case")
> then added wakup_interruptible to mwifiex_cmd_timeout_func the single
> place setting a status of ETIMEDOUT.
> Instead of doing extra work, using the standard call-chain will have the
> same effect:
> mwifiex_cancel_pending_ioctl
> -> mwifiex_recycle_cmd_node
> -> mwifiex_insert_cmd_to_free_q
> -> mwifiex_complete_cmd
> -> wake_up_interruptible
> 
> The difference is that previously the condition was not set to true, but
> that's probably just an oversight in b1a47aa5e1e1 and shouldn't have any
> consequence
> 
> Signed-off-by: Andreas Fenkart <afenkart@gmail.com>
> ---
>  drivers/net/wireless/mwifiex/cmdevt.c    |  1 -
>  drivers/net/wireless/mwifiex/sta_ioctl.c |  4 ++--
>  drivers/net/wireless/mwifiex/util.c      | 12 ++++--------
>  3 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/wireless/mwifiex/cmdevt.c
> b/drivers/net/wireless/mwifiex/cmdevt.c
> index 6458e17..27b778d 100644
> --- a/drivers/net/wireless/mwifiex/cmdevt.c
> +++ b/drivers/net/wireless/mwifiex/cmdevt.c
> @@ -976,7 +976,6 @@ mwifiex_cmd_timeout_func(unsigned long
> function_context)
> 
>  		if (cmd_node->wait_q_enabled) {
>  			adapter->cmd_wait_q.status = -ETIMEDOUT;
> -			wake_up_interruptible(&adapter->cmd_wait_q.wait);
>  			mwifiex_cancel_pending_ioctl(adapter);
>  		}
>  	}
> diff --git a/drivers/net/wireless/mwifiex/sta_ioctl.c
> b/drivers/net/wireless/mwifiex/sta_ioctl.c
> index d8b7d9c..a6c8a4f 100644
> --- a/drivers/net/wireless/mwifiex/sta_ioctl.c
> +++ b/drivers/net/wireless/mwifiex/sta_ioctl.c
> @@ -66,8 +66,8 @@ int mwifiex_wait_queue_complete(struct mwifiex_adapter
> *adapter,
>  	if (status <= 0) {
>  		if (status == 0)
>  			status = -ETIMEDOUT;
> -		mwifiex_dbg(adapter, ERROR,
> -			    "cmd_wait_q terminated: %d\n", status);
> +		mwifiex_dbg(adapter, ERROR, "cmd_wait_q terminated: %d\n",
> +			    status);
>  		mwifiex_cancel_all_pending_cmd(adapter);
>  		return status;
>  	}
> diff --git a/drivers/net/wireless/mwifiex/util.c
> b/drivers/net/wireless/mwifiex/util.c
> index 790e619..b65ef5b 100644
> --- a/drivers/net/wireless/mwifiex/util.c
> +++ b/drivers/net/wireless/mwifiex/util.c
> @@ -496,16 +496,12 @@ int mwifiex_recv_packet(struct mwifiex_private
> *priv, struct sk_buff *skb)  int mwifiex_complete_cmd(struct
> mwifiex_adapter *adapter,
>  			 struct cmd_ctrl_node *cmd_node)
>  {
> -	mwifiex_dbg(adapter, CMD,
> -		    "cmd completed: status=%d\n",
> +	WARN_ON(!cmd_node->wait_q_enabled);
> +	mwifiex_dbg(adapter, CMD, "cmd completed: status=%d\n",
>  		    adapter->cmd_wait_q.status);
> 
> -	*(cmd_node->condition) = true;
> -
> -	if (adapter->cmd_wait_q.status == -ETIMEDOUT)
> -		mwifiex_dbg(adapter, ERROR, "cmd timeout\n");
> -	else
> -		wake_up_interruptible(&adapter->cmd_wait_q.wait);
> +	*cmd_node->condition = true;
> +	wake_up_interruptible(&adapter->cmd_wait_q.wait);
> 
>  	return 0;
>  }
> --
> 2.1.4

Acked-by: Amitkumar Karwar <akarwar@marvell.com>

Regards,
Amitkumar

Re: [1/4] mwifiex: remove explicit mwifiex_complete_cmd calls

[Kalle Valo]

> standard call chain when releasing a cmd node:
> mwifiex_recycle_cmd_node
> -> mwifiex_insert_cmd_to_free_q
> -> mwifiex_complete_cmd, if wait_q_enabled
> 
> calling mwifiex_complete_cmd explicitly and setting
> wait_q_enabled = false is redundant
> 
> Signed-off-by: Andreas Fenkart <afenkart@gmail.com>
> Acked-by: Amitkumar Karwar <akarwar@marvell.com>

Thanks, 4 patches applied to wireless-drivers-next.git:

e3a3ef25b8cb mwifiex: remove explicit mwifiex_complete_cmd calls
aeb03000837e mwifiex: remove redundant reset of cmd_wait_q status
e9f21d403699 mwifiex: remove CMD_F_CANCELED flag
c5bc15fce6aa mwifiex: simplify mwifiex_complete_cmd

Kalle Valo