by Andreas Fenkart

[permalink] [raw]

Subject: [PATCH 7/7] mwifiex: make mwifiex_insert_cmd_to_free_q local static

after factoring out mwifiex_cancel_pending_scan_cmd
the function is noc called outside of cmdevt file

Signed-off-by: Andreas Fenkart <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/cmdevt.c | 6 +++++-
drivers/net/wireless/marvell/mwifiex/main.h | 2 --
2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 61426b3..4b0f44d 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -26,6 +26,10 @@
#include "11n.h"
#include "11ac.h"

+static void
+mwifiex_insert_cmd_to_free_q(struct mwifiex_adapter *adapter,
+ struct cmd_ctrl_node *cmd_node);
+
/*
* This function initializes a command node.
*
@@ -619,7 +623,7 @@ int mwifiex_send_cmd(struct mwifiex_private *priv, u16 cmd_no,
* The function also calls the completion callback if required, before
* cleaning the command node and re-inserting it into the free queue.
*/
-void
+static void
mwifiex_insert_cmd_to_free_q(struct mwifiex_adapter *adapter,
struct cmd_ctrl_node *cmd_node)
{
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index f71f894..840e5d4 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1045,8 +1045,6 @@ void mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter *adapter);
void mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter);
void mwifiex_cancel_pending_scan_cmd(struct mwifiex_adapter *adapter);

-void mwifiex_insert_cmd_to_free_q(struct mwifiex_adapter *adapter,
- struct cmd_ctrl_node *cmd_node);
void mwifiex_recycle_cmd_node(struct mwifiex_adapter *adapter,
struct cmd_ctrl_node *cmd_node);

--
2.7.0

2016-03-10 08:36:22

by Andreas Fenkart

[permalink] [raw]

Subject: Re: [PATCH 6/7] mwifiex: factor out mwifiex_cancel_pending_scan_cmd

Hi Julian,

thanks for your time!

2016-02-25 2:14 GMT+01:00 Julian Calaby <[email protected]>:
> Hi Andreas,
>
> On Thu, Feb 25, 2016 at 11:08 AM, Andreas Fenkart <[email protected]> wrote:
>> releasing the scan_pending lock in mwifiex_check_next_scan_command
>> is valid, since the lock is taken again, and all nodes removed
>> from the scan_pending queue.
>>
>> Signed-off-by: Andreas Fenkart <[email protected]>
>> ---
>> drivers/net/wireless/marvell/mwifiex/cmdevt.c | 43 ++++++++++------------
>> drivers/net/wireless/marvell/mwifiex/main.h | 1 +
>> drivers/net/wireless/marvell/mwifiex/scan.c | 23 +++---------
>> drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 13 +------
>> 4 files changed, 27 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
>> index 6ddc98b..490d0d1 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/scan.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
>> @@ -1920,13 +1910,10 @@ static void mwifiex_check_next_scan_command(struct mwifiex_private *priv)
>> }
>> } else if ((priv->scan_aborting && !priv->scan_request) ||
>> priv->scan_block) {
>> - list_for_each_entry_safe(cmd_node, tmp_node,
>> - &adapter->scan_pending_q, list) {
>> - list_del(&cmd_node->list);
>> - mwifiex_insert_cmd_to_free_q(adapter, cmd_node);
>> - }
>> spin_unlock_irqrestore(&adapter->scan_pending_q_lock, flags);
>>
>> + mwifiex_cancel_pending_scan_cmd(adapter);
>> +
>
> This is creating a "short" window where &adapter->scan_pending_q_lock
> is unlocked here. Is that safe?
>
> You might want to write mwifiex_cancel_pending_scan_cmd() as two
> functions, one which takes the spinlock and calls the other and one
> which does all the work so you can call the latter here without that
> window where ..._q_lock is unlocked.

I added this comment to the description of the updated patch, that I
will send out shortly:

Releasing the scan_pending lock in mwifiex_check_next_scan_command
introduces a short window where pending scan commands can be removed
or added before removing them all in mwifiex_cancel_pending_scan_cmd.
I think this is safe, since the worst thing to happen is that a
pending scan cmd is removed by the command handler. Adding new scan
commands is not possible while one is pending, see scan_processing.
Since all commands are removed from the queue anyway, we don't care if
some commands are removed by a different code path earlier, the final
state remains the same.
I assume, that the critical section needed for the check has been
extended over clearing the pending scan queue, out of convenience. The
lock was already held and releasing it first just to grab it again was
more work. It doesn't seem to be necessary because of concurrency.

Thanks,
Andi