| Message ID | 20250324-mwifiex-cleanup-1-v5-10-1128a2be02af@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | mwifiex: cleanups | expand |
Hi Sascha, On Mon, Mar 24, 2025 at 02:24:11PM +0100, Sascha Hauer wrote: > Historically all commands sent to the mwifiex driver have been > asynchronous. The different commands sent during driver initialization > have been queued at once and only the final command has been waited > for being ready before finally starting the driver. > > This has been changed in 7bff9c974e1a ("mwifiex: send firmware > initialization commands synchronously"). With this the initialization > is finished once the last mwifiex_send_cmd_sync() (now > mwifiex_send_cmd()) has returned. This makes all the code used to > wait for the last initialization command to be finished unnecessary, > so it's removed in this patch. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> There are a few things that confuse me in here. See below. > --- > 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(-) > ... > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c > index 8b61e45cd6678..fc58ca1a60ca8 100644 > --- a/drivers/net/wireless/marvell/mwifiex/init.c > +++ b/drivers/net/wireless/marvell/mwifiex/init.c > @@ -487,7 +487,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; > > @@ -509,7 +508,6 @@ int mwifiex_init_fw(struct mwifiex_adapter *adapter) > } > if (adapter->mfg_mode) { > adapter->hw_status = MWIFIEX_HW_STATUS_READY; > - ret = -EINPROGRESS; Why are you dropping this line? To be fair, I'm not sure I understand all the manufacturing-mode support anyway, but I equally don't understand why you're dropping this. > } else { > for (i = 0; i < adapter->priv_num; i++) { > ret = mwifiex_sta_init_cmd(adapter->priv[i], > @@ -521,18 +519,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); I believe your reasoning around the synchronous command logic, but would it help to include any sort of fail-safe here for the future? Something like: WARN_ON(!list_empty(&adapter->cmd_pending_q)); ? Or am I being overly cautious? > - 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; You need to update the function comments now that you're dropping this. Brian > - } 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/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > index f2e9f582ae818..199a8e52e5b16 100644 > --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > @@ -2418,11 +2418,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) { The 'init' function parameter is no longer used. Can you drop it from the function signature? Brian > - /* set last_init_cmd before sending the command */ > - priv->adapter->last_init_cmd = HostCmd_CMD_11N_CFG; > - ret = -EINPROGRESS; > - } > - > return ret; > } ...
Hi Brian, On Tue, Mar 25, 2025 at 03:25:26PM -0700, Brian Norris wrote: > Hi Sascha, > > On Mon, Mar 24, 2025 at 02:24:11PM +0100, Sascha Hauer wrote: > > Historically all commands sent to the mwifiex driver have been > > asynchronous. The different commands sent during driver initialization > > have been queued at once and only the final command has been waited > > for being ready before finally starting the driver. > > > > This has been changed in 7bff9c974e1a ("mwifiex: send firmware > > initialization commands synchronously"). With this the initialization > > is finished once the last mwifiex_send_cmd_sync() (now > > mwifiex_send_cmd()) has returned. This makes all the code used to > > wait for the last initialization command to be finished unnecessary, > > so it's removed in this patch. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > There are a few things that confuse me in here. See below. > > > --- > > 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(-) > > > ... > > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c > > index 8b61e45cd6678..fc58ca1a60ca8 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/init.c > > +++ b/drivers/net/wireless/marvell/mwifiex/init.c > > @@ -487,7 +487,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; > > > > @@ -509,7 +508,6 @@ int mwifiex_init_fw(struct mwifiex_adapter *adapter) > > } > > if (adapter->mfg_mode) { > > adapter->hw_status = MWIFIEX_HW_STATUS_READY; > > - ret = -EINPROGRESS; > > Why are you dropping this line? To be fair, I'm not sure I understand > all the manufacturing-mode support anyway, but I equally don't > understand why you're dropping this. Short version: This change is correct. I started a lengthy explanation why I did this, but realized that it's better to split this patch further up to make it more clear. I'll create a new series from this patch. > > > } else { > > for (i = 0; i < adapter->priv_num; i++) { > > ret = mwifiex_sta_init_cmd(adapter->priv[i], > > @@ -521,18 +519,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); > > I believe your reasoning around the synchronous command logic, but would > it help to include any sort of fail-safe here for the future? Something > like: > > WARN_ON(!list_empty(&adapter->cmd_pending_q)); > > ? Or am I being overly cautious? mwifiex_init_fw() might be called from some obscure firmware command time out path, so let's add it just to be sure. Better safe than sorry. > > > - 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; > > You need to update the function comments now that you're dropping this. Will do. > > diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > > index f2e9f582ae818..199a8e52e5b16 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > > +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c > > @@ -2418,11 +2418,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) { > > The 'init' function parameter is no longer used. Can you drop it from > the function signature? Good point. I'll make an extra patch from this though to not further obfuscate this patch. Sascha
diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c index 90cb469c897eb..fa7641f09719b 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 8b61e45cd6678..fc58ca1a60ca8 100644 --- a/drivers/net/wireless/marvell/mwifiex/init.c +++ b/drivers/net/wireless/marvell/mwifiex/init.c @@ -487,7 +487,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; @@ -509,7 +508,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], @@ -521,18 +519,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 3cb3db7a089b8..8d7384cfe80a1 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)) { @@ -1549,7 +1532,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); @@ -1717,7 +1699,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 e566470226d8f..7fe268761b074 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.h +++ b/drivers/net/wireless/marvell/mwifiex/main.h @@ -239,7 +239,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 }; @@ -865,8 +864,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; @@ -919,7 +916,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 */ @@ -1060,8 +1056,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 f2e9f582ae818..199a8e52e5b16 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c @@ -2418,11 +2418,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. The different commands sent during driver initialization have been queued at once and only the final command has been waited for being ready before finally starting the driver. This has been changed in 7bff9c974e1a ("mwifiex: send firmware initialization commands synchronously"). With this the initialization is finished once the last mwifiex_send_cmd_sync() (now mwifiex_send_cmd()) has returned. This makes all the code used to wait for the last initialization command to be finished unnecessary, so it's 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(-)