Hi Brian, > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > Sent: Thursday, September 01, 2016 1:05 AM > To: Amitkumar Karwar > Cc: linux-wireless@xxxxxxxxxxxxxxx; Cathy Luo; Nishant Sarmukadam > Subject: Re: [v4,1/2] mwifiex: add manufacturing mode support > > Hi, > > On Tue, Jul 26, 2016 at 03:09:19PM +0530, Amitkumar Karwar wrote: > > By default normal mode is chosen when driver is loaded. This patch > > adds a provision to choose manufacturing mode via module parameters. > > > > Below command loads driver in manufacturing mode insmod mwifiex.ko > > mfg_mode=1. > > > > Tested-by: chunfan chen <jeffc@xxxxxxxxxxx> > > Signed-off-by: Amitkumar Karwar <akarwar@xxxxxxxxxxx> > > --- > > v4: Removed mfg_firmware module parameter and hardcoded the firmware > name > > in driver(Kalle Valo). > > --- > > drivers/net/wireless/marvell/mwifiex/cmdevt.c | 8 ++++++++ > > drivers/net/wireless/marvell/mwifiex/init.c | 22 +++++++++++++++--- > ---- > > drivers/net/wireless/marvell/mwifiex/main.c | 25 > +++++++++++++++++++++---- > > drivers/net/wireless/marvell/mwifiex/main.h | 2 ++ > > drivers/net/wireless/marvell/mwifiex/pcie.c | 2 +- > > drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +- > > drivers/net/wireless/marvell/mwifiex/usb.c | 2 +- > > 7 files changed, 49 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c > > b/drivers/net/wireless/marvell/mwifiex/cmdevt.c > > index d433aa0..636cfa0 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c > > +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c > > @@ -595,6 +595,14 @@ int mwifiex_send_cmd(struct mwifiex_private > *priv, u16 cmd_no, > > return -1; > > } > > } > > + /* We don't expect commands in manufacturing mode. They are cooked > > + * in application and ready to download buffer is passed to the > driver > > + */ > > You have the alignment wrong here. Needs an extra space. > > > + if (adapter->mfg_mode && cmd_no) { > > + dev_dbg(adapter->dev, "Ignoring commands in manufacturing > mode\n"); > > + return -1; > > + } > > + > > > > /* Get a new command node */ > > cmd_node = mwifiex_get_cmd_node(adapter); diff --git > > a/drivers/net/wireless/marvell/mwifiex/init.c > > b/drivers/net/wireless/marvell/mwifiex/init.c > > index 1489c90..82839d9 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/init.c > > +++ b/drivers/net/wireless/marvell/mwifiex/init.c > > @@ -298,6 +298,7 @@ static void mwifiex_init_adapter(struct > mwifiex_adapter *adapter) > > memset(&adapter->arp_filter, 0, sizeof(adapter->arp_filter)); > > adapter->arp_filter_size = 0; > > adapter->max_mgmt_ie_index = MAX_MGMT_IE_INDEX; > > + adapter->mfg_mode = mfg_mode; > > adapter->key_api_major_ver = 0; > > adapter->key_api_minor_ver = 0; > > eth_broadcast_addr(adapter->perm_addr); > > @@ -553,15 +554,22 @@ int mwifiex_init_fw(struct mwifiex_adapter > *adapter) > > return -1; > > } > > } > > + if (adapter->mfg_mode) { > > + adapter->hw_status = MWIFIEX_HW_STATUS_READY; > > + ret = -EINPROGRESS; > > + } else { > > + for (i = 0; i < adapter->priv_num; i++) { > > + if (adapter->priv[i]) { > > + ret = mwifiex_sta_init_cmd(adapter->priv[i], > > + first_sta, true); > > + if (ret == -1) > > + return -1; > > + > > + first_sta = false; > > + } > > + > > > > - for (i = 0; i < adapter->priv_num; i++) { > > - if (adapter->priv[i]) { > > - ret = mwifiex_sta_init_cmd(adapter->priv[i], > first_sta, > > - true); > > - if (ret == -1) > > - return -1; > > > > - first_sta = false; > > } > > } > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c > > b/drivers/net/wireless/marvell/mwifiex/main.c > > index db4925d..7fbf74b 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/main.c > > +++ b/drivers/net/wireless/marvell/mwifiex/main.c > > @@ -23,6 +23,7 @@ > > #include "11n.h" > > > > #define VERSION "1.0" > > +#define MFG_FIRMWARE "mwifiex_mfg.bin" > > > > static unsigned int debug_mask = MWIFIEX_DEFAULT_DEBUG_MASK; > > module_param(debug_mask, uint, 0); @@ -36,6 +37,9 @@ static unsigned > > short driver_mode; module_param(driver_mode, ushort, 0); > > MODULE_PARM_DESC(driver_mode, > > "station=0x1(default), ap-sta=0x3, station-p2p=0x5, > > ap-sta-p2p=0x7"); > > +bool mfg_mode; > > Does this really need to be global? It's only actually used in init.c, > so could it help to move it to init.c and make it static? We also use mfg_mode in main.c(mwifiex_init_hw_fw), so it can't be moved to init.s > > > +module_param(mfg_mode, bool, 0); > > +MODULE_PARM_DESC(mfg_mode, "0:disable 1:enable (bool)"); > > That's not a very helpful description. Perhaps you could mention in a > word or two what this mode means? > > Brian Thanks. I just submitted v5 patches addressing your comments. Regards, Amitkumar Karwar > > > > > /* > > * This function registers the device and performs all the necessary > > @@ -559,10 +563,12 @@ static void mwifiex_fw_dpc(const struct firmware > *firmware, void *context) > > goto done; > > } > > /* Wait for mwifiex_init to complete */ > > - wait_event_interruptible(adapter->init_wait_q, > > - adapter->init_wait_q_woken); > > - if (adapter->hw_status != MWIFIEX_HW_STATUS_READY) > > - goto err_init_fw; > > + 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; > > + } > > > > priv = adapter->priv[MWIFIEX_BSS_ROLE_STA]; > > if (mwifiex_register_cfg80211(adapter)) { @@ -666,6 +672,17 @@ > > static int mwifiex_init_hw_fw(struct mwifiex_adapter *adapter) { > > int ret; > > > > + /* Override default firmware with manufacturing one if > > + * manufacturing mode is enabled > > + */ > > + if (mfg_mode) { > > + if (strlcpy(adapter->fw_name, MFG_FIRMWARE, > > + sizeof(adapter->fw_name)) >= > > + sizeof(adapter->fw_name)) { > > + pr_err("%s: fw_name too long!\n", __func__); > > + return -1; > > + } > > + } > > ret = request_firmware_nowait(THIS_MODULE, 1, adapter->fw_name, > > adapter->dev, GFP_KERNEL, adapter, > > mwifiex_fw_dpc); > > diff --git a/drivers/net/wireless/marvell/mwifiex/main.h > > b/drivers/net/wireless/marvell/mwifiex/main.h > > index 5902600..fcc2af35 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/main.h > > +++ b/drivers/net/wireless/marvell/mwifiex/main.h > > @@ -58,6 +58,7 @@ > > #include "sdio.h" > > > > extern const char driver_version[]; > > +extern bool mfg_mode; > > > > struct mwifiex_adapter; > > struct mwifiex_private; > > @@ -990,6 +991,7 @@ struct mwifiex_adapter { > > u32 drv_info_size; > > bool scan_chan_gap_enabled; > > struct sk_buff_head rx_data_q; > > + bool mfg_mode; > > struct mwifiex_chan_stats *chan_stats; > > u32 num_in_chan_stats; > > int survey_idx; > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c > > b/drivers/net/wireless/marvell/mwifiex/pcie.c > > index 453ab6a..a6af85d 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c > > @@ -225,7 +225,7 @@ static void mwifiex_pcie_remove(struct pci_dev > *pdev) > > if (!adapter || !adapter->priv_num) > > return; > > > > - if (user_rmmod) { > > + if (user_rmmod && !adapter->mfg_mode) { > > #ifdef CONFIG_PM_SLEEP > > if (adapter->is_suspended) > > mwifiex_pcie_resume(&pdev->dev); > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c > > b/drivers/net/wireless/marvell/mwifiex/sdio.c > > index d3e1561..6dba409 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c > > @@ -289,7 +289,7 @@ mwifiex_sdio_remove(struct sdio_func *func) > > > > mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num); > > > > - if (user_rmmod) { > > + if (user_rmmod && !adapter->mfg_mode) { > > if (adapter->is_suspended) > > mwifiex_sdio_resume(adapter->dev); > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c > > b/drivers/net/wireless/marvell/mwifiex/usb.c > > index 0857575..ba616ec 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/usb.c > > +++ b/drivers/net/wireless/marvell/mwifiex/usb.c > > @@ -611,7 +611,7 @@ static void mwifiex_usb_disconnect(struct > usb_interface *intf) > > if (!adapter->priv_num) > > return; > > > > - if (user_rmmod) { > > + if (user_rmmod && !adapter->mfg_mode) { > > #ifdef CONFIG_PM > > if (adapter->is_suspended) > > mwifiex_usb_resume(intf);
