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 : > Hi Andreas, > > On Thu, Feb 25, 2016 at 11:08 AM, Andreas Fenkart 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 >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] mwifiex: factor out mwifiex_cancel_pending_scan_cmd
Hi Andreas, On Thu, Feb 25, 2016 at 11:08 AM, Andreas Fenkart 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 > --- > 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. Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/7] mwifiex: factor out mwifiex_cancel_pending_scan_cmd
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 --- 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/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c index cb25aa7..61426b3 100644 --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c @@ -991,6 +991,23 @@ mwifiex_cmd_timeout_func(unsigned long function_context) adapter->if_ops.card_reset(adapter); } +void +mwifiex_cancel_pending_scan_cmd(struct mwifiex_adapter *adapter) +{ + struct cmd_ctrl_node *cmd_node = NULL, *tmp_node; + unsigned long flags; + + /* Cancel all pending scan command */ + spin_lock_irqsave(&adapter->scan_pending_q_lock, flags); + list_for_each_entry_safe(cmd_node, tmp_node, +&adapter->scan_pending_q, list) { + list_del(&cmd_node->list); + cmd_node->wait_q_enabled = false; + mwifiex_insert_cmd_to_free_q(adapter, cmd_node); + } + spin_unlock_irqrestore(&adapter->scan_pending_q_lock, flags); +} + /* * This function cancels all the pending commands. * @@ -1029,16 +1046,7 @@ mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter *adapter) spin_unlock_irqrestore(&adapter->cmd_pending_q_lock, flags); spin_unlock_irqrestore(&adapter->mwifiex_cmd_lock, cmd_flags); - /* Cancel all pending scan command */ - spin_lock_irqsave(&adapter->scan_pending_q_lock, flags); - list_for_each_entry_safe(cmd_node, tmp_node, -&adapter->scan_pending_q, list) { - list_del(&cmd_node->list); - - cmd_node->wait_q_enabled = false; - mwifiex_insert_cmd_to_free_q(adapter, cmd_node); - } - spin_unlock_irqrestore(&adapter->scan_pending_q_lock, flags); + mwifiex_cancel_pending_scan_cmd(adapter); if (adapter->scan_processing) { spin_lock_irqsave(&adapter->mwifiex_cmd_lock, cmd_flags); @@ -1070,9 +1078,8 @@ mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter *adapter) void mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter) { - struct cmd_ctrl_node *cmd_node = NULL, *tmp_node = NULL; + struct cmd_ctrl_node *cmd_node = NULL; unsigned long cmd_flags; - unsigned long scan_pending_q_flags; struct mwifiex_private *priv; int i; @@ -1094,17 +1101,7 @@ mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter) mwifiex_recycle_cmd_node(adapter, cmd_node); } - /* Cancel all pending scan command */ - spin_lock_irqsave(&adapter->scan_pending_q_lock, - scan_pending_q_flags); - list_for_each_entry_safe(cmd_node, tmp_node, -&adapter->scan_pending_q, list) { - list_del(&cmd_node->list); - cmd_node->wait_q_enabled = false; - mwifiex_insert_cmd_to_free_q(adapter, cmd_node); - } - spin_unlock_irqrestore(&adapter->scan_pending_q_lock, - scan_pending_q_flags); + mwifiex_cancel_pending_scan_cmd(adapter); if (adapter->scan_processing) { spin_lock_irqsave(&adapter->mwifiex_cmd_lock, cmd_flags); diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h index 2f7f478..f71f894 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.h +++ b/drivers/net/wireless/marvell/mwifiex/main.h @@ -1043,6 +1043,7 @@ int mwifiex_alloc_cmd_buffer(struct mwifiex_adapter *adapter); int mwifiex_free_cmd_buffer(struct mwifiex_adapter *adapter); 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); 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 @@ -563,8 +563,6 @@ mwifiex_scan_channel_list(struct mwifiex_private *priv, int ret = 0; struct mwifiex_chan_scan_param_set *tmp_chan_list; struct mwifiex_chan_scan_param_set *start_chan; - struct cmd_ctrl_node *cmd_node, *tmp_node; - unsigned lon