| Message ID | 20240826-mwifiex-cleanup-1-v1-12-56e6f8e056ec@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mwifiex: two fixes and cleanup | expand |
On Mon, Aug 26, 2024 at 01:01:33PM +0200, Sascha Hauer wrote: > Historically all commands sent to the mwifiex driver have been > asynchronous. For this reason there is code that waits for the > last initialization command to complete before going on. Nowadays the > commands can be sent synchronously, meaning that they are completed > when the command call returns. This makes all the waiting code > unnecessary. It is removed in this patch. I am not sure to understand this. Is the code to have asynchronous command gone or it is just not used anymore? In the code here you remove waiting for the firmware init to be complete, but from the patch is not clear why this is not needed anymore. Maybe a specific commit you can reference in which such support was removed?
On Mon, Sep 09, 2024 at 07:14:11PM +0200, Francesco Dolcini wrote: > On Mon, Aug 26, 2024 at 01:01:33PM +0200, Sascha Hauer wrote: > > Historically all commands sent to the mwifiex driver have been > > asynchronous. For this reason there is code that waits for the > > last initialization command to complete before going on. Nowadays the > > commands can be sent synchronously, meaning that they are completed > > when the command call returns. This makes all the waiting code > > unnecessary. It is removed in this patch. > > I am not sure to understand this. Is the code to have asynchronous command gone > or it is just not used anymore? In the code here you remove waiting for the > firmware init to be complete, but from the patch is not clear why this is not > needed anymore. Maybe a specific commit you can reference in which such > support was removed? Commands can still be sent asynchronously by passing sync=false to mwifiex_send_cmd(), but this is no longer done in the initialization phase as of: | commit 7bff9c974e1a70819c30c37d8ec0d84d456f8237 | Author: Stone Piao <piaoyun@marvell.com> | Date: Tue Sep 25 20:23:39 2012 -0700 | | mwifiex: send firmware initialization commands synchronously | | The driver will send some commands to firmware during the | initialization. Currently these commands are sent asynchronously, | which means that we firstly insert all of them to a pre-allocated | command queue, and then start to process them one by one. The | command queue will soon be exhausted if we keep adding new | initialization commands. | | This issue can be resolved by sending initialization commands | synchronously because each command is consumed and the buffer is | recycled before queuing next command. | | Signed-off-by: Stone Piao <piaoyun@marvell.com> | Signed-off-by: Bing Zhao <bzhao@marvell.com> | Signed-off-by: John W. Linville <linville@tuxdriver.com> I'll mention this commit in the commit message next round. Sascha
On Mon, Sep 09, 2024 at 10:14:56PM +0200, Sascha Hauer wrote: > On Mon, Sep 09, 2024 at 07:14:11PM +0200, Francesco Dolcini wrote: > > On Mon, Aug 26, 2024 at 01:01:33PM +0200, Sascha Hauer wrote: > > > Historically all commands sent to the mwifiex driver have been > > > asynchronous. For this reason there is code that waits for the > > > last initialization command to complete before going on. Nowadays the > > > commands can be sent synchronously, meaning that they are completed > > > when the command call returns. This makes all the waiting code > > > unnecessary. It is removed in this patch. > > > > I am not sure to understand this. Is the code to have asynchronous command gone > > or it is just not used anymore? In the code here you remove waiting for the > > firmware init to be complete, but from the patch is not clear why this is not > > needed anymore. Maybe a specific commit you can reference in which such > > support was removed? > > Commands can still be sent asynchronously by passing sync=false to > mwifiex_send_cmd(), but this is no longer done in the initialization Understood. So this is just not unused code since quite some time. To me the change looks ok, but I would appreciate if someone else can have another look. > | commit 7bff9c974e1a70819c30c37d8ec0d84d456f8237 > | Author: Stone Piao <piaoyun@marvell.com> > | Date: Tue Sep 25 20:23:39 2012 -0700 > | > | mwifiex: send firmware initialization commands synchronously > | > | The driver will send some commands to firmware during the > | initialization. Currently these commands are sent asynchronously, > | which means that we firstly insert all of them to a pre-allocated > | command queue, and then start to process them one by one. The > | command queue will soon be exhausted if we keep adding new > | initialization commands. > | > | This issue can be resolved by sending initialization commands > | synchronously because each command is consumed and the buffer is > | recycled before queuing next command. > | > | Signed-off-by: Stone Piao <piaoyun@marvell.com> > | Signed-off-by: Bing Zhao <bzhao@marvell.com> > | Signed-off-by: John W. Linville <linville@tuxdriver.com> > > I'll mention this commit in the commit message next round. Perfect, thanks.
diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c index 402531a03ece3..8a614dc993b2c 100644 --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c @@ -892,18 +892,6 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter) ret = mwifiex_process_sta_cmdresp(priv, cmdresp_no, resp); } - /* Check init command response */ - if (adapter->hw_status == MWIFIEX_HW_STATUS_INITIALIZING) { - if (ret) { - mwifiex_dbg(adapter, ERROR, - "%s: cmd %#x failed during\t" - "initialization\n", __func__, cmdresp_no); - mwifiex_init_fw_complete(adapter); - return -1; - } else if (adapter->last_init_cmd == cmdresp_no) - adapter->hw_status = MWIFIEX_HW_STATUS_INIT_DONE; - } - if (adapter->curr_cmd) { if (adapter->curr_cmd->wait_q_enabled) adapter->cmd_wait_q.status = ret; @@ -1022,10 +1010,6 @@ mwifiex_cmd_timeout_func(struct timer_list *t) mwifiex_cancel_pending_ioctl(adapter); } } - if (adapter->hw_status == MWIFIEX_HW_STATUS_INITIALIZING) { - mwifiex_init_fw_complete(adapter); - return; - } if (adapter->if_ops.device_dump) adapter->if_ops.device_dump(adapter); diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c index 0259c9f88486b..b27596c7c02cb 100644 --- a/drivers/net/wireless/marvell/mwifiex/init.c +++ b/drivers/net/wireless/marvell/mwifiex/init.c @@ -486,7 +486,6 @@ int mwifiex_init_fw(struct mwifiex_adapter *adapter) int ret; struct mwifiex_private *priv; u8 i, first_sta = true; - int is_cmd_pend_q_empty; adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING; @@ -508,7 +507,6 @@ int mwifiex_init_fw(struct mwifiex_adapter *adapter) } if (adapter->mfg_mode) { adapter->hw_status = MWIFIEX_HW_STATUS_READY; - ret = -EINPROGRESS; } else { for (i = 0; i < adapter->priv_num; i++) { ret = mwifiex_sta_init_cmd(adapter->priv[i], @@ -520,18 +518,12 @@ int mwifiex_init_fw(struct mwifiex_adapter *adapter) } } - spin_lock_bh(&adapter->cmd_pending_q_lock); - is_cmd_pend_q_empty = list_empty(&adapter->cmd_pending_q); - spin_unlock_bh(&adapter->cmd_pending_q_lock); - if (!is_cmd_pend_q_empty) { - /* Send the first command in queue and return */ - if (mwifiex_main_process(adapter) != -1) - ret = -EINPROGRESS; - } else { - adapter->hw_status = MWIFIEX_HW_STATUS_READY; - } + adapter->hw_status = MWIFIEX_HW_STATUS_READY; - return ret; + if (adapter->if_ops.init_fw_port) + adapter->if_ops.init_fw_port(adapter); + + return 0; } /* diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c index 588887aa29a79..63344e2e03656 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.c +++ b/drivers/net/wireless/marvell/mwifiex/main.c @@ -354,13 +354,6 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter) if (adapter->cmd_resp_received) { adapter->cmd_resp_received = false; mwifiex_process_cmdresp(adapter); - - /* call mwifiex back when init_fw is done */ - if (adapter->hw_status == MWIFIEX_HW_STATUS_INIT_DONE) { - adapter->hw_status = MWIFIEX_HW_STATUS_READY; - mwifiex_init_fw_complete(adapter); - maybe_quirk_fw_disable_ds(adapter); - } } /* Check if we need to confirm Sleep Request @@ -578,21 +571,11 @@ static int _mwifiex_fw_dpc(const struct firmware *firmware, void *context) goto err_dnld_fw; } - adapter->init_wait_q_woken = false; ret = mwifiex_init_fw(adapter); - if (ret == -1) { + if (ret < 0) goto err_init_fw; - } else if (!ret) { - adapter->hw_status = MWIFIEX_HW_STATUS_READY; - goto done; - } - /* Wait for mwifiex_init to complete */ - if (!adapter->mfg_mode) { - wait_event_interruptible(adapter->init_wait_q, - adapter->init_wait_q_woken); - if (adapter->hw_status != MWIFIEX_HW_STATUS_READY) - goto err_init_fw; - } + + maybe_quirk_fw_disable_ds(adapter); if (!adapter->wiphy) { if (mwifiex_register_cfg80211(adapter)) { @@ -1551,7 +1534,6 @@ mwifiex_reinit_sw(struct mwifiex_adapter *adapter) adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING; clear_bit(MWIFIEX_SURPRISE_REMOVED, &adapter->work_flags); - init_waitqueue_head(&adapter->init_wait_q); clear_bit(MWIFIEX_IS_SUSPENDED, &adapter->work_flags); adapter->hs_activated = false; clear_bit(MWIFIEX_IS_CMD_TIMEDOUT, &adapter->work_flags); @@ -1719,7 +1701,6 @@ mwifiex_add_card(void *card, struct completion *fw_done, adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING; clear_bit(MWIFIEX_SURPRISE_REMOVED, &adapter->work_flags); - init_waitqueue_head(&adapter->init_wait_q); clear_bit(MWIFIEX_IS_SUSPENDED, &adapter->work_flags); adapter->hs_activated = false; init_waitqueue_head(&adapter->hs_activate_wait_q); diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h index f026e6069be3f..1eb0b5f9cb4a5 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.h +++ b/drivers/net/wireless/marvell/mwifiex/main.h @@ -240,7 +240,6 @@ struct mwifiex_dbg { enum MWIFIEX_HARDWARE_STATUS { MWIFIEX_HW_STATUS_READY, MWIFIEX_HW_STATUS_INITIALIZING, - MWIFIEX_HW_STATUS_INIT_DONE, MWIFIEX_HW_STATUS_RESET, MWIFIEX_HW_STATUS_NOT_READY }; @@ -869,8 +868,6 @@ struct mwifiex_adapter { unsigned long work_flags; u32 fw_release_number; u8 intf_hdr_len; - u16 init_wait_q_woken; - wait_queue_head_t init_wait_q; void *card; struct mwifiex_if_ops if_ops; atomic_t bypass_tx_pending; @@ -923,7 +920,6 @@ struct mwifiex_adapter { struct cmd_ctrl_node *curr_cmd; /* spin lock for command */ spinlock_t mwifiex_cmd_lock; - u16 last_init_cmd; struct timer_list cmd_timer; struct list_head cmd_free_q; /* spin lock for cmd_free_q */ @@ -1064,8 +1060,6 @@ void mwifiex_free_priv(struct mwifiex_private *priv); int mwifiex_init_fw(struct mwifiex_adapter *adapter); -int mwifiex_init_fw_complete(struct mwifiex_adapter *adapter); - void mwifiex_shutdown_drv(struct mwifiex_adapter *adapter); int mwifiex_dnld_fw(struct mwifiex_adapter *, struct mwifiex_fw_image *); diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c index 30dd4e58e2b1d..da89e15e5fe76 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c @@ -2406,11 +2406,5 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init) ret = mwifiex_send_cmd(priv, HostCmd_CMD_11N_CFG, HostCmd_ACT_GEN_SET, 0, &tx_cfg, true); - if (init) { - /* set last_init_cmd before sending the command */ - priv->adapter->last_init_cmd = HostCmd_CMD_11N_CFG; - ret = -EINPROGRESS; - } - return ret; } diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c index ea28d604ee69c..4c5b1de0e936c 100644 --- a/drivers/net/wireless/marvell/mwifiex/util.c +++ b/drivers/net/wireless/marvell/mwifiex/util.c @@ -115,24 +115,6 @@ static struct mwifiex_debug_data items[] = { static int num_of_items = ARRAY_SIZE(items); -/* - * Firmware initialization complete callback handler. - * - * This function wakes up the function waiting on the init - * wait queue for the firmware initialization to complete. - */ -int mwifiex_init_fw_complete(struct mwifiex_adapter *adapter) -{ - - if (adapter->hw_status == MWIFIEX_HW_STATUS_READY) - if (adapter->if_ops.init_fw_port) - adapter->if_ops.init_fw_port(adapter); - - adapter->init_wait_q_woken = true; - wake_up_interruptible(&adapter->init_wait_q); - return 0; -} - /* * This function sends init/shutdown command * to firmware.
Historically all commands sent to the mwifiex driver have been asynchronous. For this reason there is code that waits for the last initialization command to complete before going on. Nowadays the commands can be sent synchronously, meaning that they are completed when the command call returns. This makes all the waiting code unnecessary. It is removed in this patch. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/net/wireless/marvell/mwifiex/cmdevt.c | 16 ---------------- drivers/net/wireless/marvell/mwifiex/init.c | 18 +++++------------- drivers/net/wireless/marvell/mwifiex/main.c | 25 +++---------------------- drivers/net/wireless/marvell/mwifiex/main.h | 6 ------ drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 6 ------ drivers/net/wireless/marvell/mwifiex/util.c | 18 ------------------ 6 files changed, 8 insertions(+), 81 deletions(-)