[PATCH 1/2] mwifiex: sdio: create global list of adapters
we need to verify that a an adapter pointer, still points to a valid adapter. this is not possible through other global structures, such as net_namespace_list. the problem with the net_namespace_list is that an adapter can have multiple interfaces, hence we had to de-reference the maybe invalid pointer to get the interface struct. Signed-off-by: Andreas Fenkart --- drivers/net/wireless/mwifiex/sdio.c | 14 ++ drivers/net/wireless/mwifiex/sdio.h | 1 + 2 files changed, 15 insertions(+) diff --git a/drivers/net/wireless/mwifiex/sdio.c b/drivers/net/wireless/mwifiex/sdio.c index 91e36cd..a70e114 100644 --- a/drivers/net/wireless/mwifiex/sdio.c +++ b/drivers/net/wireless/mwifiex/sdio.c @@ -69,6 +69,12 @@ static struct memory_type_mapping mem_type_mapping_tbl[] = { }; /* + * list of active cards + */ +static LIST_HEAD(cards); +static DEFINE_MUTEX(cards_mutex); + +/* * SDIO probe. * * This function probes an mwifiex device and registers it. It allocates @@ -130,6 +136,10 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) ret = -1; } + mutex_lock(&cards_mutex); + list_add(&card->next, &cards); + mutex_unlock(&cards_mutex); + return ret; } @@ -196,6 +206,10 @@ mwifiex_sdio_remove(struct sdio_func *func) if (!card) return; + mutex_lock(&cards_mutex); + list_del(&card->next); + mutex_unlock(&cards_mutex); + adapter = card->adapter; if (!adapter || !adapter->priv_num) return; diff --git a/drivers/net/wireless/mwifiex/sdio.h b/drivers/net/wireless/mwifiex/sdio.h index 957cca2..9be5650 100644 --- a/drivers/net/wireless/mwifiex/sdio.h +++ b/drivers/net/wireless/mwifiex/sdio.h @@ -260,6 +260,7 @@ struct sdio_mmc_card { struct mwifiex_sdio_mpa_tx mpa_tx; struct mwifiex_sdio_mpa_rx mpa_rx; + struct list_head next; }; struct mwifiex_sdio_device { -- 2.1.4 -- 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 2/2] mwifiex: sdio: bug: dead-lock in card reset
Card reset is implemented by removing/re-adding the adapter instance. This is implemented by removing the mmc host, which will then trigger a cascade of removal callbacks including mwifiex_sdio_remove. The dead-lock results in the latter function, trying to cancel the work item executing the mmc host removal. This patch adds a driver level, not adapter level, work struct to break the dead-lock Signed-off-by: Andreas Fenkart --- drivers/net/wireless/mwifiex/main.h | 1 - drivers/net/wireless/mwifiex/sdio.c | 63 + 2 files changed, 50 insertions(+), 14 deletions(-) diff --git a/drivers/net/wireless/mwifiex/main.h b/drivers/net/wireless/mwifiex/main.h index f0a6af1..702f348 100644 --- a/drivers/net/wireless/mwifiex/main.h +++ b/drivers/net/wireless/mwifiex/main.h @@ -437,7 +437,6 @@ enum rdwr_status { enum mwifiex_iface_work_flags { MWIFIEX_IFACE_WORK_FW_DUMP, - MWIFIEX_IFACE_WORK_CARD_RESET, }; struct mwifiex_adapter; diff --git a/drivers/net/wireless/mwifiex/sdio.c b/drivers/net/wireless/mwifiex/sdio.c index a70e114..0996b0e 100644 --- a/drivers/net/wireless/mwifiex/sdio.c +++ b/drivers/net/wireless/mwifiex/sdio.c @@ -75,6 +75,19 @@ static LIST_HEAD(cards); static DEFINE_MUTEX(cards_mutex); /* + * Card removal will remove the interface then re-add it + * Hence the work_struct can't be part of the adapter + * otherwise there will be deadlock when we remove the + * adapter, and cancel the work struct executing the reset + */ +static struct +{ + struct work_struct work; + struct sdio_mmc_card *card; +} s_card_reset; +static DEFINE_SEMAPHORE(s_card_reset_sem); + +/* * SDIO probe. * * This function probes an mwifiex device and registers it. It allocates @@ -1964,10 +1977,36 @@ mwifiex_update_mp_end_port(struct mwifiex_adapter *adapter, u16 port) port, card->mp_data_port_mask); } -static void mwifiex_sdio_card_reset_work(struct mwifiex_adapter *adapter) +static void mwifiex_sdio_card_reset_work(struct work_struct *work) { - struct sdio_mmc_card *card = adapter->card; - struct mmc_host *target = card->func->card->host; + struct sdio_mmc_card *card = NULL; + struct mmc_host *target; + struct list_head *cur; + + WARN_ON(work != &s_card_reset.work); + + /* adapter pointer might have been invalidated +* e.g. card removed from slot +*/ + mutex_lock(&cards_mutex); + list_for_each_prev(cur, &cards) { + card = list_entry(cur, struct sdio_mmc_card, next); + if (card == s_card_reset.card) + break; + } + if (!card || card != s_card_reset.card) { + pr_err("card was removed, cancel card reset\n"); + mutex_unlock(&cards_mutex); + return; + } + + /* card pointer still valid inc ref in host, it's all we need */ + target = card->func->card->host; + mmc_claim_host(target); + mutex_unlock(&cards_mutex); + + /* release the work struct, we are done with it */ + up(&s_card_reset_sem); /* The actual reset operation must be run outside of driver thread. * This is because mmc_remove_host() will cause the device to be @@ -1976,13 +2015,14 @@ static void mwifiex_sdio_card_reset_work(struct mwifiex_adapter *adapter) * * We run it in a totally independent workqueue. */ - pr_err("Resetting card...\n"); mmc_remove_host(target); /* 200ms delay is based on experiment with sdhci controller */ mdelay(200); target->rescan_entered = 0; /* rescan non-removable cards */ mmc_add_host(target); + + mmc_release_host(target); } /* This function read/write firmware */ @@ -2160,9 +2200,6 @@ static void mwifiex_sdio_work(struct work_struct *work) struct mwifiex_adapter *adapter = container_of(work, struct mwifiex_adapter, iface_work); - if (test_and_clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, - &adapter->iface_work_flags)) - mwifiex_sdio_card_reset_work(adapter); if (test_and_clear_bit(MWIFIEX_IFACE_WORK_FW_DUMP, &adapter->iface_work_flags)) mwifiex_sdio_fw_dump_work(work); @@ -2171,12 +2208,9 @@ static void mwifiex_sdio_work(struct work_struct *work) /* This function resets the card */ static void mwifiex_sdio_card_reset(struct mwifiex_adapter *adapter) { - if (test_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &adapter->iface_work_flags)) - return; - - set_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &adapter->iface_work_flags); - - schedule_work(&adapter->iface_work); + down(&s_card_reset_sem); + s_card_reset.card = adapter->card; + schedule_work(&am
Re: [PATCH 2/2] mwifiex: sdio: bug: dead-lock in card reset
Hi Amit, 2015-04-13 12:27 GMT+02:00 Amitkumar Karwar : > > We had recently submitted a patch to address this issue. So these two patches > won't be needed now. > http://www.spinics.net/lists/linux-wireless/msg135146.html > > Could you please check and let us know if you have any suggestions for > improvement? They work for my use case: 1 adapter / non-removable Problems not solved: - the use of global static variable is racy in case there two adapters, and both need reset simultaneously - since work struct is independent of the interface (sdio_func), it will also not be notified if the card is removed physically for illustrating the latter: - cmd is hung - card timeout triggers, and issues card reset, card reset stores adapter pointer in global variable - user removes card - mmc layer triggers removal of card, global pointer becomes invalid - card reset work struct de-references invalid pointer regards, Andreas -- 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 5/7] mwifiex: scan: replace pointer arithmetic with array access
improves readability Signed-off-by: Andreas Fenkart --- drivers/net/wireless/marvell/mwifiex/scan.c | 17 +++-- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c index 5588750..6ddc98b 100644 --- a/drivers/net/wireless/marvell/mwifiex/scan.c +++ b/drivers/net/wireless/marvell/mwifiex/scan.c @@ -999,27 +999,24 @@ mwifiex_config_scan(struct mwifiex_private *priv, chan_idx++) { channel = user_scan_in->chan_list[chan_idx].chan_number; - (scan_chan_list + chan_idx)->chan_number = channel; + scan_chan_list[chan_idx].chan_number = channel; radio_type = user_scan_in->chan_list[chan_idx].radio_type; - (scan_chan_list + chan_idx)->radio_type = radio_type; + scan_chan_list[chan_idx].radio_type = radio_type; scan_type = user_scan_in->chan_list[chan_idx].scan_type; if (scan_type == MWIFIEX_SCAN_TYPE_PASSIVE) - (scan_chan_list + -chan_idx)->chan_scan_mode_bitmap + scan_chan_list[chan_idx].chan_scan_mode_bitmap |= (MWIFIEX_PASSIVE_SCAN | MWIFIEX_HIDDEN_SSID_REPORT); else - (scan_chan_list + -chan_idx)->chan_scan_mode_bitmap + scan_chan_list[chan_idx].chan_scan_mode_bitmap &= ~MWIFIEX_PASSIVE_SCAN; if (*filtered_scan) - (scan_chan_list + -chan_idx)->chan_scan_mode_bitmap + scan_chan_list[chan_idx].chan_scan_mode_bitmap |= MWIFIEX_DISABLE_CHAN_FILT; if (user_scan_in->chan_list[chan_idx].scan_time) { @@ -1034,9 +1031,9 @@ mwifiex_config_scan(struct mwifiex_private *priv, scan_dur = adapter->active_scan_time; } - (scan_chan_list + chan_idx)->min_scan_time = + scan_chan_list[chan_idx].min_scan_time = cpu_to_le16(scan_dur); - (scan_chan_list + chan_idx)->max_scan_time = + scan_chan_list[chan_idx].max_scan_time = cpu_to_le16(scan_dur); } -- 2.7.0 -- 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 0/7] mwifiex: cleanups
Andreas Fenkart (7): mwifiex: scan: simplify dereference of bss_desc fields mwifiex: scan: factor out has_ieee_hdr/has_vendor_hdr mwifiex: scan: simplify ternary operators using gnu extension mwifiex: scan: factor out dbg_security_flags mwifiex: scan: replace pointer arithmetic with array access mwifiex: factor out mwifiex_cancel_pending_scan_cmd mwifiex: make mwifiex_insert_cmd_to_free_q local static drivers/net/wireless/marvell/mwifiex/cmdevt.c | 49 +++--- drivers/net/wireless/marvell/mwifiex/main.h| 3 +- drivers/net/wireless/marvell/mwifiex/scan.c| 184 +++-- drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 13 +- 4 files changed, 91 insertions(+), 158 deletions(-) -- 2.7.0 -- 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 2/7] mwifiex: scan: factor out has_ieee_hdr/has_vendor_hdr
Signed-off-by: Andreas Fenkart --- drivers/net/wireless/marvell/mwifiex/scan.c | 52 + 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c index 43d7a92..276382e 100644 --- a/drivers/net/wireless/marvell/mwifiex/scan.c +++ b/drivers/net/wireless/marvell/mwifiex/scan.c @@ -76,6 +76,18 @@ static u8 mwifiex_rsn_oui[CIPHER_SUITE_MAX][4] = { { 0x00, 0x0f, 0xac, 0x04 }, /* AES */ }; +static bool +has_ieee_hdr(struct ieee_types_generic *ie, u8 key) +{ + return (ie && (ie->ieee_hdr.element_id == key)); +} + +static bool +has_vendor_hdr(struct ieee_types_vendor_specific *ie, u8 key) +{ + return (ie && (ie->vend_hdr.element_id == key)); +} + /* * This function parses a given IE for a given OUI. * @@ -121,8 +133,7 @@ mwifiex_is_rsn_oui_present(struct mwifiex_bssdescriptor *bss_desc, u32 cipher) struct ie_body *iebody; u8 ret = MWIFIEX_OUI_NOT_PRESENT; - if (bss_desc->bcn_rsn_ie && - (bss_desc->bcn_rsn_ie->ieee_hdr.element_id == WLAN_EID_RSN)) { + if (has_ieee_hdr(bss_desc->bcn_rsn_ie, WLAN_EID_RSN)) { iebody = (struct ie_body *) (((u8 *) bss_desc->bcn_rsn_ie->data) + RSN_GTK_OUI_OFFSET); @@ -148,9 +159,7 @@ mwifiex_is_wpa_oui_present(struct mwifiex_bssdescriptor *bss_desc, u32 cipher) struct ie_body *iebody; u8 ret = MWIFIEX_OUI_NOT_PRESENT; - if (bss_desc->bcn_wpa_ie && - (bss_desc->bcn_wpa_ie->vend_hdr.element_id == -WLAN_EID_VENDOR_SPECIFIC)) { + if (has_vendor_hdr(bss_desc->bcn_wpa_ie, WLAN_EID_VENDOR_SPECIFIC)) { iebody = (struct ie_body *) bss_desc->bcn_wpa_ie->data; oui = &mwifiex_wpa_oui[cipher][0]; ret = mwifiex_search_oui_in_ie(iebody, oui); @@ -180,11 +189,8 @@ mwifiex_is_bss_wapi(struct mwifiex_private *priv, struct mwifiex_bssdescriptor *bss_desc) { if (priv->sec_info.wapi_enabled && - (bss_desc->bcn_wapi_ie && -(bss_desc->bcn_wapi_ie->ieee_hdr.element_id == - WLAN_EID_BSS_AC_ACCESS_DELAY))) { + has_ieee_hdr(bss_desc->bcn_wapi_ie, WLAN_EID_BSS_AC_ACCESS_DELAY)) return true; - } return false; } @@ -198,11 +204,8 @@ mwifiex_is_bss_no_sec(struct mwifiex_private *priv, { if (!priv->sec_info.wep_enabled && !priv->sec_info.wpa_enabled && !priv->sec_info.wpa2_enabled && - (!bss_desc->bcn_wpa_ie || - (bss_desc->bcn_wpa_ie->vend_hdr.element_id != -WLAN_EID_VENDOR_SPECIFIC)) && - (!bss_desc->bcn_rsn_ie || - (bss_desc->bcn_rsn_ie->ieee_hdr.element_id != WLAN_EID_RSN)) && + !has_vendor_hdr(bss_desc->bcn_wpa_ie, WLAN_EID_VENDOR_SPECIFIC) && + !has_ieee_hdr(bss_desc->bcn_rsn_ie, WLAN_EID_RSN) && !priv->sec_info.encryption_mode && !bss_desc->privacy) { return true; } @@ -234,9 +237,7 @@ mwifiex_is_bss_wpa(struct mwifiex_private *priv, { if (!priv->sec_info.wep_enabled && priv->sec_info.wpa_enabled && !priv->sec_info.wpa2_enabled && - (bss_desc->bcn_wpa_ie && -(bss_desc->bcn_wpa_ie->vend_hdr.element_id == - WLAN_EID_VENDOR_SPECIFIC)) + has_vendor_hdr(bss_desc->bcn_wpa_ie, WLAN_EID_VENDOR_SPECIFIC) /* * Privacy bit may NOT be set in some APs like * LinkSys WRT54G && bss_desc->privacy @@ -270,8 +271,7 @@ mwifiex_is_bss_wpa2(struct mwifiex_private *priv, { if (!priv->sec_info.wep_enabled && !priv->sec_info.wpa_enabled && priv->sec_info.wpa2_enabled && - (bss_desc->bcn_rsn_ie && -(bss_desc->bcn_rsn_ie->ieee_hdr.element_id == WLAN_EID_RSN))) { + has_ieee_hdr(bss_desc->bcn_rsn_ie, WLAN_EID_RSN)) { /* * Privacy bit may NOT be set in some APs like * LinkSys WRT54G && bss_desc->privacy @@ -304,11 +304,8 @@ mwifiex_is_bss_adhoc_aes(struct mwifiex_private *priv, { if (!priv->sec_info.wep_enabled && !priv->sec_info.wpa_enabled && !priv->sec_info.wpa2_enabled && - (!bss_desc->bcn_wpa_ie || -(bss_desc->bcn_wpa_ie->vend_hdr.element_id != - WLAN_EID_VENDOR_SPECIFIC)) && - (!bss_desc->bcn_rsn_ie || -(bss_desc->bcn_rsn_ie->ieee_hdr.element_id !=
[PATCH 1/7] mwifiex: scan: simplify dereference of bss_desc fields
given this structure: struct foo { struct bar { int baz; } } these accesses are equivalent: (*(foo->bar)).baz foo->bar->baz Signed-off-by: Andreas Fenkart --- drivers/net/wireless/marvell/mwifiex/scan.c | 96 ++--- 1 file changed, 45 insertions(+), 51 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c index c20017c..43d7a92 100644 --- a/drivers/net/wireless/marvell/mwifiex/scan.c +++ b/drivers/net/wireless/marvell/mwifiex/scan.c @@ -121,8 +121,8 @@ mwifiex_is_rsn_oui_present(struct mwifiex_bssdescriptor *bss_desc, u32 cipher) struct ie_body *iebody; u8 ret = MWIFIEX_OUI_NOT_PRESENT; - if (((bss_desc->bcn_rsn_ie) && ((*(bss_desc->bcn_rsn_ie)). - ieee_hdr.element_id == WLAN_EID_RSN))) { + if (bss_desc->bcn_rsn_ie && + (bss_desc->bcn_rsn_ie->ieee_hdr.element_id == WLAN_EID_RSN)) { iebody = (struct ie_body *) (((u8 *) bss_desc->bcn_rsn_ie->data) + RSN_GTK_OUI_OFFSET); @@ -148,9 +148,9 @@ mwifiex_is_wpa_oui_present(struct mwifiex_bssdescriptor *bss_desc, u32 cipher) struct ie_body *iebody; u8 ret = MWIFIEX_OUI_NOT_PRESENT; - if (((bss_desc->bcn_wpa_ie) && -((*(bss_desc->bcn_wpa_ie)).vend_hdr.element_id == - WLAN_EID_VENDOR_SPECIFIC))) { + if (bss_desc->bcn_wpa_ie && + (bss_desc->bcn_wpa_ie->vend_hdr.element_id == +WLAN_EID_VENDOR_SPECIFIC)) { iebody = (struct ie_body *) bss_desc->bcn_wpa_ie->data; oui = &mwifiex_wpa_oui[cipher][0]; ret = mwifiex_search_oui_in_ie(iebody, oui); @@ -181,8 +181,8 @@ mwifiex_is_bss_wapi(struct mwifiex_private *priv, { if (priv->sec_info.wapi_enabled && (bss_desc->bcn_wapi_ie && -((*(bss_desc->bcn_wapi_ie)).ieee_hdr.element_id == - WLAN_EID_BSS_AC_ACCESS_DELAY))) { +(bss_desc->bcn_wapi_ie->ieee_hdr.element_id == + WLAN_EID_BSS_AC_ACCESS_DELAY))) { return true; } return false; @@ -197,12 +197,12 @@ mwifiex_is_bss_no_sec(struct mwifiex_private *priv, struct mwifiex_bssdescriptor *bss_desc) { if (!priv->sec_info.wep_enabled && !priv->sec_info.wpa_enabled && - !priv->sec_info.wpa2_enabled && ((!bss_desc->bcn_wpa_ie) || - ((*(bss_desc->bcn_wpa_ie)).vend_hdr.element_id != + !priv->sec_info.wpa2_enabled && + (!bss_desc->bcn_wpa_ie || + (bss_desc->bcn_wpa_ie->vend_hdr.element_id != WLAN_EID_VENDOR_SPECIFIC)) && - ((!bss_desc->bcn_rsn_ie) || - ((*(bss_desc->bcn_rsn_ie)).ieee_hdr.element_id != -WLAN_EID_RSN)) && + (!bss_desc->bcn_rsn_ie || + (bss_desc->bcn_rsn_ie->ieee_hdr.element_id != WLAN_EID_RSN)) && !priv->sec_info.encryption_mode && !bss_desc->privacy) { return true; } @@ -233,9 +233,10 @@ mwifiex_is_bss_wpa(struct mwifiex_private *priv, struct mwifiex_bssdescriptor *bss_desc) { if (!priv->sec_info.wep_enabled && priv->sec_info.wpa_enabled && - !priv->sec_info.wpa2_enabled && ((bss_desc->bcn_wpa_ie) && - ((*(bss_desc->bcn_wpa_ie)). -vend_hdr.element_id == WLAN_EID_VENDOR_SPECIFIC)) + !priv->sec_info.wpa2_enabled && + (bss_desc->bcn_wpa_ie && +(bss_desc->bcn_wpa_ie->vend_hdr.element_id == + WLAN_EID_VENDOR_SPECIFIC)) /* * Privacy bit may NOT be set in some APs like * LinkSys WRT54G && bss_desc->privacy @@ -245,12 +246,10 @@ mwifiex_is_bss_wpa(struct mwifiex_private *priv, "info: %s: WPA:\t" "wpa_ie=%#x wpa2_ie=%#x WEP=%s WPA=%s WPA2=%s\t" "EncMode=%#x privacy=%#x\n", __func__, - (bss_desc->bcn_wpa_ie) ? - (*bss_desc->bcn_wpa_ie). - vend_hdr.element_id : 0, - (bss_desc->bcn_rsn_ie) ? - (*bss_desc->bcn_rsn_ie). - ieee_hdr.element_id : 0, + bss_desc->bcn_wpa_ie ? + bss_desc->bcn_wpa_ie->vend_hdr.element_id : 0, + bss_desc->bcn_rsn_ie ? + bss_desc->bcn_rsn_ie->iee
[PATCH 4/7] mwifiex: scan: factor out dbg_security_flags
merge copy/paste code Signed-off-by: Andreas Fenkart --- drivers/net/wireless/marvell/mwifiex/scan.c | 73 ++--- 1 file changed, 24 insertions(+), 49 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c index f612c1b..5588750 100644 --- a/drivers/net/wireless/marvell/mwifiex/scan.c +++ b/drivers/net/wireless/marvell/mwifiex/scan.c @@ -76,6 +76,26 @@ static u8 mwifiex_rsn_oui[CIPHER_SUITE_MAX][4] = { { 0x00, 0x0f, 0xac, 0x04 }, /* AES */ }; +static void +_dbg_security_flags(int log_level, struct mwifiex_private *priv, + struct mwifiex_bssdescriptor *bss_desc) +{ + _mwifiex_dbg(priv->adapter, log_level, +"info: %s: WPA:\twpa_ie=%#x wpa2_ie=%#x WEP=%s WPA=%s WPA2=%s\tEncMode=%#x privacy=%#x\n", +__func__, +bss_desc->bcn_wpa_ie ? +bss_desc->bcn_wpa_ie->vend_hdr.element_id : 0, +bss_desc->bcn_rsn_ie ? +bss_desc->bcn_rsn_ie->ieee_hdr.element_id : 0, +priv->sec_info.wep_enabled ? "e" : "d", +priv->sec_info.wpa_enabled ? "e" : "d", +priv->sec_info.wpa2_enabled ? "e" : "d", +priv->sec_info.encryption_mode, +bss_desc->privacy); +} +#define dbg_security_flags(mask, priv, bss_desc) \ + _dbg_security_flags(MWIFIEX_DBG_##mask, priv, bss_desc) + static bool has_ieee_hdr(struct ieee_types_generic *ie, u8 key) { @@ -243,19 +263,7 @@ mwifiex_is_bss_wpa(struct mwifiex_private *priv, * LinkSys WRT54G && bss_desc->privacy */ ) { - mwifiex_dbg(priv->adapter, INFO, - "info: %s: WPA:\t" - "wpa_ie=%#x wpa2_ie=%#x WEP=%s WPA=%s WPA2=%s\t" - "EncMode=%#x privacy=%#x\n", __func__, - bss_desc->bcn_wpa_ie ? - bss_desc->bcn_wpa_ie->vend_hdr.element_id : 0, - bss_desc->bcn_rsn_ie ? - bss_desc->bcn_rsn_ie->ieee_hdr.element_id : 0, - (priv->sec_info.wep_enabled) ? "e" : "d", - (priv->sec_info.wpa_enabled) ? "e" : "d", - (priv->sec_info.wpa2_enabled) ? "e" : "d", - priv->sec_info.encryption_mode, - bss_desc->privacy); + dbg_security_flags(INFO, priv, bss_desc); return true; } return false; @@ -276,19 +284,7 @@ mwifiex_is_bss_wpa2(struct mwifiex_private *priv, * Privacy bit may NOT be set in some APs like * LinkSys WRT54G && bss_desc->privacy */ - mwifiex_dbg(priv->adapter, INFO, - "info: %s: WPA2:\t" - "wpa_ie=%#x wpa2_ie=%#x WEP=%s WPA=%s WPA2=%s\t" - "EncMode=%#x privacy=%#x\n", __func__, - bss_desc->bcn_wpa_ie ? - bss_desc->bcn_wpa_ie->vend_hdr.element_id : 0, - bss_desc->bcn_rsn_ie ? - bss_desc->bcn_rsn_ie->ieee_hdr.element_id : 0, - (priv->sec_info.wep_enabled) ? "e" : "d", - (priv->sec_info.wpa_enabled) ? "e" : "d", - (priv->sec_info.wpa2_enabled) ? "e" : "d", - priv->sec_info.encryption_mode, - bss_desc->privacy); + dbg_security_flags(INFO, priv, bss_desc); return true; } return false; @@ -325,17 +321,7 @@ mwifiex_is_bss_dynamic_wep(struct mwifiex_private *priv, !has_vendor_hdr(bss_desc->bcn_wpa_ie, WLAN_EID_VENDOR_SPECIFIC) && !has_ieee_hdr(bss_desc->bcn_rsn_ie, WLAN_EID_RSN) && priv->sec_info.encryption_mode && bss_desc->privacy) { - mwifiex_dbg(priv->adapter, INFO, - "info: %s: dynamic\t" - "WEP: wpa_ie=%#x wpa2_ie=%#x\t" - "EncMode=%#x privacy=%#x\n", - __func__, - bss_desc->bcn_wpa_ie ? - bss_desc->bcn_wpa_ie->vend_hdr.element_id : 0, - bss_desc->bcn_rsn_ie ? -
[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 --- 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 -- 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
[PATCH 3/7] mwifiex: scan: simplify ternary operators using gnu extension
"x ? x : y" can be simplified as "x ? : y" https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html#Conditionals Signed-off-by: Andreas Fenkart --- drivers/net/wireless/marvell/mwifiex/scan.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c index 276382e..f612c1b 100644 --- a/drivers/net/wireless/marvell/mwifiex/scan.c +++ b/drivers/net/wireless/marvell/mwifiex/scan.c @@ -845,14 +845,11 @@ mwifiex_config_scan(struct mwifiex_private *priv, /* Set the BSS type scan filter, use Adapter setting if unset */ scan_cfg_out->bss_mode = - (user_scan_in->bss_mode ? (u8) user_scan_in-> -bss_mode : (u8) adapter->scan_mode); + (u8)(user_scan_in->bss_mode ?: adapter->scan_mode); /* Set the number of probes to send, use Adapter setting if unset */ - num_probes = - (user_scan_in->num_probes ? user_scan_in-> -num_probes : adapter->scan_probes); + num_probes = user_scan_in->num_probes ?: adapter->scan_probes; /* * Set the BSSID filter to the incoming configuration, -- 2.7.0 -- 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 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
[PATCH v2 3/7] mwifiex: scan: simplify ternary operators using gnu extension
"x ? x : y" can be simplified as "x ? : y" https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html#Conditionals Reviewed-by: Julian Calaby Signed-off-by: Andreas Fenkart --- drivers/net/wireless/marvell/mwifiex/scan.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c index ed886af..f623834 100644 --- a/drivers/net/wireless/marvell/mwifiex/scan.c +++ b/drivers/net/wireless/marvell/mwifiex/scan.c @@ -845,14 +845,11 @@ mwifiex_config_scan(struct mwifiex_private *priv, /* Set the BSS type scan filter, use Adapter setting if unset */ scan_cfg_out->bss_mode = - (user_scan_in->bss_mode ? (u8) user_scan_in-> -bss_mode : (u8) adapter->scan_mode); + (u8)(user_scan_in->bss_mode ?: adapter->scan_mode); /* Set the number of probes to send, use Adapter setting if unset */ - num_probes = - (user_scan_in->num_probes ? user_scan_in-> -num_probes : adapter->scan_probes); + num_probes = user_scan_in->num_probes ?: adapter->scan_probes; /* * Set the BSSID filter to the incoming configuration, -- 2.7.0 -- 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 v2 0/7] mwifiex: cleanups
v2: - addressed issues from review - race-condition issue discussed in patch description Andreas Fenkart (7): mwifiex: scan: simplify dereference of bss_desc fields mwifiex: scan: factor out has_ieee_hdr/has_vendor_hdr mwifiex: scan: simplify ternary operators using gnu extension mwifiex: scan: factor out dbg_security_flags mwifiex: scan: replace pointer arithmetic with array access mwifiex: factor out mwifiex_cancel_pending_scan_cmd mwifiex: make mwifiex_insert_cmd_to_free_q local static drivers/net/wireless/marvell/mwifiex/cmdevt.c | 125 +++--- drivers/net/wireless/marvell/mwifiex/main.h| 3 +- drivers/net/wireless/marvell/mwifiex/scan.c| 185 - drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 13 +- 4 files changed, 128 insertions(+), 198 deletions(-) -- 2.7.0 -- 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 v2 6/7] mwifiex: factor out mwifiex_cancel_pending_scan_cmd
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 flag. 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 and grab it again was just more work. It doesn't seem to be necessary because of concurrency. 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(
[PATCH v2 5/7] mwifiex: scan: replace pointer arithmetic with array access
improves readability Reviewed-by: Julian Calaby Signed-off-by: Andreas Fenkart --- drivers/net/wireless/marvell/mwifiex/scan.c | 17 +++-- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c index c137fd8..327b4d8 100644 --- a/drivers/net/wireless/marvell/mwifiex/scan.c +++ b/drivers/net/wireless/marvell/mwifiex/scan.c @@ -1000,27 +1000,24 @@ mwifiex_config_scan(struct mwifiex_private *priv, chan_idx++) { channel = user_scan_in->chan_list[chan_idx].chan_number; - (scan_chan_list + chan_idx)->chan_number = channel; + scan_chan_list[chan_idx].chan_number = channel; radio_type = user_scan_in->chan_list[chan_idx].radio_type; - (scan_chan_list + chan_idx)->radio_type = radio_type; + scan_chan_list[chan_idx].radio_type = radio_type; scan_type = user_scan_in->chan_list[chan_idx].scan_type; if (scan_type == MWIFIEX_SCAN_TYPE_PASSIVE) - (scan_chan_list + -chan_idx)->chan_scan_mode_bitmap + scan_chan_list[chan_idx].chan_scan_mode_bitmap |= (MWIFIEX_PASSIVE_SCAN | MWIFIEX_HIDDEN_SSID_REPORT); else - (scan_chan_list + -chan_idx)->chan_scan_mode_bitmap + scan_chan_list[chan_idx].chan_scan_mode_bitmap &= ~MWIFIEX_PASSIVE_SCAN; if (*filtered_scan) - (scan_chan_list + -chan_idx)->chan_scan_mode_bitmap + scan_chan_list[chan_idx].chan_scan_mode_bitmap |= MWIFIEX_DISABLE_CHAN_FILT; if (user_scan_in->chan_list[chan_idx].scan_time) { @@ -1035,9 +1032,9 @@ mwifiex_config_scan(struct mwifiex_private *priv, scan_dur = adapter->active_scan_time; } - (scan_chan_list + chan_idx)->min_scan_time = + scan_chan_list[chan_idx].min_scan_time = cpu_to_le16(scan_dur); - (scan_chan_list + chan_idx)->max_scan_time = + scan_chan_list[chan_idx].max_scan_time = cpu_to_le16(scan_dur); } -- 2.7.0 -- 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 v2 2/7] mwifiex: scan: factor out has_ieee_hdr/has_vendor_hdr
Signed-off-by: Andreas Fenkart --- drivers/net/wireless/marvell/mwifiex/scan.c | 52 + 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c index a7bdde5..ed886af 100644 --- a/drivers/net/wireless/marvell/mwifiex/scan.c +++ b/drivers/net/wireless/marvell/mwifiex/scan.c @@ -76,6 +76,18 @@ static u8 mwifiex_rsn_oui[CIPHER_SUITE_MAX][4] = { { 0x00, 0x0f, 0xac, 0x04 }, /* AES */ }; +static bool +has_ieee_hdr(struct ieee_types_generic *ie, u8 key) +{ + return (ie && ie->ieee_hdr.element_id == key); +} + +static bool +has_vendor_hdr(struct ieee_types_vendor_specific *ie, u8 key) +{ + return (ie && ie->vend_hdr.element_id == key); +} + /* * This function parses a given IE for a given OUI. * @@ -121,8 +133,7 @@ mwifiex_is_rsn_oui_present(struct mwifiex_bssdescriptor *bss_desc, u32 cipher) struct ie_body *iebody; u8 ret = MWIFIEX_OUI_NOT_PRESENT; - if (bss_desc->bcn_rsn_ie && - bss_desc->bcn_rsn_ie->ieee_hdr.element_id == WLAN_EID_RSN) { + if (has_ieee_hdr(bss_desc->bcn_rsn_ie, WLAN_EID_RSN)) { iebody = (struct ie_body *) (((u8 *) bss_desc->bcn_rsn_ie->data) + RSN_GTK_OUI_OFFSET); @@ -148,9 +159,7 @@ mwifiex_is_wpa_oui_present(struct mwifiex_bssdescriptor *bss_desc, u32 cipher) struct ie_body *iebody; u8 ret = MWIFIEX_OUI_NOT_PRESENT; - if (bss_desc->bcn_wpa_ie && - bss_desc->bcn_wpa_ie->vend_hdr.element_id == - WLAN_EID_VENDOR_SPECIFIC) { + if (has_vendor_hdr(bss_desc->bcn_wpa_ie, WLAN_EID_VENDOR_SPECIFIC)) { iebody = (struct ie_body *) bss_desc->bcn_wpa_ie->data; oui = &mwifiex_wpa_oui[cipher][0]; ret = mwifiex_search_oui_in_ie(iebody, oui); @@ -180,11 +189,8 @@ mwifiex_is_bss_wapi(struct mwifiex_private *priv, struct mwifiex_bssdescriptor *bss_desc) { if (priv->sec_info.wapi_enabled && - (bss_desc->bcn_wapi_ie && -bss_desc->bcn_wapi_ie->ieee_hdr.element_id == -WLAN_EID_BSS_AC_ACCESS_DELAY)) { + has_ieee_hdr(bss_desc->bcn_wapi_ie, WLAN_EID_BSS_AC_ACCESS_DELAY)) return true; - } return false; } @@ -198,11 +204,8 @@ mwifiex_is_bss_no_sec(struct mwifiex_private *priv, { if (!priv->sec_info.wep_enabled && !priv->sec_info.wpa_enabled && !priv->sec_info.wpa2_enabled && - (!bss_desc->bcn_wpa_ie || -bss_desc->bcn_wpa_ie->vend_hdr.element_id != -WLAN_EID_VENDOR_SPECIFIC) && - (!bss_desc->bcn_rsn_ie || -bss_desc->bcn_rsn_ie->ieee_hdr.element_id != WLAN_EID_RSN) && + !has_vendor_hdr(bss_desc->bcn_wpa_ie, WLAN_EID_VENDOR_SPECIFIC) && + !has_ieee_hdr(bss_desc->bcn_rsn_ie, WLAN_EID_RSN) && !priv->sec_info.encryption_mode && !bss_desc->privacy) { return true; } @@ -234,9 +237,7 @@ mwifiex_is_bss_wpa(struct mwifiex_private *priv, { if (!priv->sec_info.wep_enabled && priv->sec_info.wpa_enabled && !priv->sec_info.wpa2_enabled && - (bss_desc->bcn_wpa_ie && -bss_desc->bcn_wpa_ie->vend_hdr.element_id == -WLAN_EID_VENDOR_SPECIFIC) + has_vendor_hdr(bss_desc->bcn_wpa_ie, WLAN_EID_VENDOR_SPECIFIC) /* * Privacy bit may NOT be set in some APs like * LinkSys WRT54G && bss_desc->privacy @@ -270,8 +271,7 @@ mwifiex_is_bss_wpa2(struct mwifiex_private *priv, { if (!priv->sec_info.wep_enabled && !priv->sec_info.wpa_enabled && priv->sec_info.wpa2_enabled && - (bss_desc->bcn_rsn_ie && -bss_desc->bcn_rsn_ie->ieee_hdr.element_id == WLAN_EID_RSN)) { + has_ieee_hdr(bss_desc->bcn_rsn_ie, WLAN_EID_RSN)) { /* * Privacy bit may NOT be set in some APs like * LinkSys WRT54G && bss_desc->privacy @@ -304,11 +304,8 @@ mwifiex_is_bss_adhoc_aes(struct mwifiex_private *priv, { if (!priv->sec_info.wep_enabled && !priv->sec_info.wpa_enabled && !priv->sec_info.wpa2_enabled && - (!bss_desc->bcn_wpa_ie || -bss_desc->bcn_wpa_ie->vend_hdr.element_id != -WLAN_EID_VENDOR_SPECIFIC) && - (!bss_desc->bcn_rsn_ie || -bss_desc->bcn_rsn_ie->ieee_hdr.element_id != WLAN_EID_RSN) && + !has
[PATCH v2 7/7] mwifiex: make mwifiex_insert_cmd_to_free_q local static
after factoring out mwifiex_cancel_pending_scan_cmd the function is not called outside of cmdevt file moved function to head of file to avoid forward declaration, also moved mwifiex_recycle_cmd_node since they are very similar Signed-off-by: Andreas Fenkart --- drivers/net/wireless/marvell/mwifiex/cmdevt.c | 82 +-- drivers/net/wireless/marvell/mwifiex/main.h | 2 - 2 files changed, 41 insertions(+), 43 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c index 61426b3..3d7de4a 100644 --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c @@ -105,6 +105,47 @@ mwifiex_clean_cmd_node(struct mwifiex_adapter *adapter, } /* + * This function returns a command to the command free queue. + * + * The function also calls the completion callback if required, before + * cleaning the command node and re-inserting it into the free queue. + */ +static void +mwifiex_insert_cmd_to_free_q(struct mwifiex_adapter *adapter, +struct cmd_ctrl_node *cmd_node) +{ + unsigned long flags; + + if (!cmd_node) + return; + + if (cmd_node->wait_q_enabled) + mwifiex_complete_cmd(adapter, cmd_node); + /* Clean the node */ + mwifiex_clean_cmd_node(adapter, cmd_node); + + /* Insert node into cmd_free_q */ + spin_lock_irqsave(&adapter->cmd_free_q_lock, flags); + list_add_tail(&cmd_node->list, &adapter->cmd_free_q); + spin_unlock_irqrestore(&adapter->cmd_free_q_lock, flags); +} + +/* This function reuses a command node. */ +void mwifiex_recycle_cmd_node(struct mwifiex_adapter *adapter, + struct cmd_ctrl_node *cmd_node) +{ + struct host_cmd_ds_command *host_cmd = (void *)cmd_node->cmd_skb->data; + + mwifiex_insert_cmd_to_free_q(adapter, cmd_node); + + atomic_dec(&adapter->cmd_pending); + mwifiex_dbg(adapter, CMD, + "cmd: FREE_CMD: cmd=%#x, cmd_pending=%d\n", + le16_to_cpu(host_cmd->command), + atomic_read(&adapter->cmd_pending)); +} + +/* * This function sends a host command to the firmware. * * The function copies the host command into the driver command @@ -614,47 +655,6 @@ int mwifiex_send_cmd(struct mwifiex_private *priv, u16 cmd_no, } /* - * This function returns a command to the command free queue. - * - * The function also calls the completion callback if required, before - * cleaning the command node and re-inserting it into the free queue. - */ -void -mwifiex_insert_cmd_to_free_q(struct mwifiex_adapter *adapter, -struct cmd_ctrl_node *cmd_node) -{ - unsigned long flags; - - if (!cmd_node) - return; - - if (cmd_node->wait_q_enabled) - mwifiex_complete_cmd(adapter, cmd_node); - /* Clean the node */ - mwifiex_clean_cmd_node(adapter, cmd_node); - - /* Insert node into cmd_free_q */ - spin_lock_irqsave(&adapter->cmd_free_q_lock, flags); - list_add_tail(&cmd_node->list, &adapter->cmd_free_q); - spin_unlock_irqrestore(&adapter->cmd_free_q_lock, flags); -} - -/* This function reuses a command node. */ -void mwifiex_recycle_cmd_node(struct mwifiex_adapter *adapter, - struct cmd_ctrl_node *cmd_node) -{ - struct host_cmd_ds_command *host_cmd = (void *)cmd_node->cmd_skb->data; - - mwifiex_insert_cmd_to_free_q(adapter, cmd_node); - - atomic_dec(&adapter->cmd_pending); - mwifiex_dbg(adapter, CMD, - "cmd: FREE_CMD: cmd=%#x, cmd_pending=%d\n", - le16_to_cpu(host_cmd->command), - atomic_read(&adapter->cmd_pending)); -} - -/* * This function queues a command to the command pending queue. * * This in effect adds the command to the command list to be executed. 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 -- 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 v2 4/7] mwifiex: scan: factor out dbg_security_flags
merge copy/paste code Signed-off-by: Andreas Fenkart --- drivers/net/wireless/marvell/mwifiex/scan.c | 74 ++--- 1 file changed, 25 insertions(+), 49 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c index f623834..c137fd8 100644 --- a/drivers/net/wireless/marvell/mwifiex/scan.c +++ b/drivers/net/wireless/marvell/mwifiex/scan.c @@ -76,6 +76,27 @@ static u8 mwifiex_rsn_oui[CIPHER_SUITE_MAX][4] = { { 0x00, 0x0f, 0xac, 0x04 }, /* AES */ }; +static void +_dbg_security_flags(int log_level, const char *func, const char *desc, + struct mwifiex_private *priv, + struct mwifiex_bssdescriptor *bss_desc) +{ + _mwifiex_dbg(priv->adapter, log_level, +"info: %s: %s:\twpa_ie=%#x wpa2_ie=%#x WEP=%s WPA=%s WPA2=%s\tEncMode=%#x privacy=%#x\n", +func, desc, +bss_desc->bcn_wpa_ie ? +bss_desc->bcn_wpa_ie->vend_hdr.element_id : 0, +bss_desc->bcn_rsn_ie ? +bss_desc->bcn_rsn_ie->ieee_hdr.element_id : 0, +priv->sec_info.wep_enabled ? "e" : "d", +priv->sec_info.wpa_enabled ? "e" : "d", +priv->sec_info.wpa2_enabled ? "e" : "d", +priv->sec_info.encryption_mode, +bss_desc->privacy); +} +#define dbg_security_flags(mask, desc, priv, bss_desc) \ + _dbg_security_flags(MWIFIEX_DBG_##mask, desc, __func__, priv, bss_desc) + static bool has_ieee_hdr(struct ieee_types_generic *ie, u8 key) { @@ -243,19 +264,7 @@ mwifiex_is_bss_wpa(struct mwifiex_private *priv, * LinkSys WRT54G && bss_desc->privacy */ ) { - mwifiex_dbg(priv->adapter, INFO, - "info: %s: WPA:\t" - "wpa_ie=%#x wpa2_ie=%#x WEP=%s WPA=%s WPA2=%s\t" - "EncMode=%#x privacy=%#x\n", __func__, - bss_desc->bcn_wpa_ie ? - bss_desc->bcn_wpa_ie->vend_hdr.element_id : 0, - bss_desc->bcn_rsn_ie ? - bss_desc->bcn_rsn_ie->ieee_hdr.element_id : 0, - (priv->sec_info.wep_enabled) ? "e" : "d", - (priv->sec_info.wpa_enabled) ? "e" : "d", - (priv->sec_info.wpa2_enabled) ? "e" : "d", - priv->sec_info.encryption_mode, - bss_desc->privacy); + dbg_security_flags(INFO, "WPA", priv, bss_desc); return true; } return false; @@ -276,19 +285,7 @@ mwifiex_is_bss_wpa2(struct mwifiex_private *priv, * Privacy bit may NOT be set in some APs like * LinkSys WRT54G && bss_desc->privacy */ - mwifiex_dbg(priv->adapter, INFO, - "info: %s: WPA2:\t" - "wpa_ie=%#x wpa2_ie=%#x WEP=%s WPA=%s WPA2=%s\t" - "EncMode=%#x privacy=%#x\n", __func__, - bss_desc->bcn_wpa_ie ? - bss_desc->bcn_wpa_ie->vend_hdr.element_id : 0, - bss_desc->bcn_rsn_ie ? - bss_desc->bcn_rsn_ie->ieee_hdr.element_id : 0, - (priv->sec_info.wep_enabled) ? "e" : "d", - (priv->sec_info.wpa_enabled) ? "e" : "d", - (priv->sec_info.wpa2_enabled) ? "e" : "d", - priv->sec_info.encryption_mode, - bss_desc->privacy); + dbg_security_flags(INFO, "WAP2", priv, bss_desc); return true; } return false; @@ -325,17 +322,7 @@ mwifiex_is_bss_dynamic_wep(struct mwifiex_private *priv, !has_vendor_hdr(bss_desc->bcn_wpa_ie, WLAN_EID_VENDOR_SPECIFIC) && !has_ieee_hdr(bss_desc->bcn_rsn_ie, WLAN_EID_RSN) && priv->sec_info.encryption_mode && bss_desc->privacy) { - mwifiex_dbg(priv->adapter, INFO, - "info: %s: dynamic\t" - "WEP: wpa_ie=%#x wpa2_ie=%#x\t" - "EncMode=%#x privacy=%#x\n", - __func__, - bss_desc->bcn_wpa_ie ? - bss_desc->
[PATCH v2 1/7] mwifiex: scan: simplify dereference of bss_desc fields
given this structure: struct foo { struct bar { int baz; } } these accesses are equivalent: (*(foo->bar)).baz foo->bar->baz Signed-off-by: Andreas Fenkart --- drivers/net/wireless/marvell/mwifiex/scan.c | 98 ++--- 1 file changed, 46 insertions(+), 52 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c index c20017c..a7bdde5 100644 --- a/drivers/net/wireless/marvell/mwifiex/scan.c +++ b/drivers/net/wireless/marvell/mwifiex/scan.c @@ -121,8 +121,8 @@ mwifiex_is_rsn_oui_present(struct mwifiex_bssdescriptor *bss_desc, u32 cipher) struct ie_body *iebody; u8 ret = MWIFIEX_OUI_NOT_PRESENT; - if (((bss_desc->bcn_rsn_ie) && ((*(bss_desc->bcn_rsn_ie)). - ieee_hdr.element_id == WLAN_EID_RSN))) { + if (bss_desc->bcn_rsn_ie && + bss_desc->bcn_rsn_ie->ieee_hdr.element_id == WLAN_EID_RSN) { iebody = (struct ie_body *) (((u8 *) bss_desc->bcn_rsn_ie->data) + RSN_GTK_OUI_OFFSET); @@ -148,9 +148,9 @@ mwifiex_is_wpa_oui_present(struct mwifiex_bssdescriptor *bss_desc, u32 cipher) struct ie_body *iebody; u8 ret = MWIFIEX_OUI_NOT_PRESENT; - if (((bss_desc->bcn_wpa_ie) && -((*(bss_desc->bcn_wpa_ie)).vend_hdr.element_id == - WLAN_EID_VENDOR_SPECIFIC))) { + if (bss_desc->bcn_wpa_ie && + bss_desc->bcn_wpa_ie->vend_hdr.element_id == + WLAN_EID_VENDOR_SPECIFIC) { iebody = (struct ie_body *) bss_desc->bcn_wpa_ie->data; oui = &mwifiex_wpa_oui[cipher][0]; ret = mwifiex_search_oui_in_ie(iebody, oui); @@ -181,8 +181,8 @@ mwifiex_is_bss_wapi(struct mwifiex_private *priv, { if (priv->sec_info.wapi_enabled && (bss_desc->bcn_wapi_ie && -((*(bss_desc->bcn_wapi_ie)).ieee_hdr.element_id == - WLAN_EID_BSS_AC_ACCESS_DELAY))) { +bss_desc->bcn_wapi_ie->ieee_hdr.element_id == +WLAN_EID_BSS_AC_ACCESS_DELAY)) { return true; } return false; @@ -197,12 +197,12 @@ mwifiex_is_bss_no_sec(struct mwifiex_private *priv, struct mwifiex_bssdescriptor *bss_desc) { if (!priv->sec_info.wep_enabled && !priv->sec_info.wpa_enabled && - !priv->sec_info.wpa2_enabled && ((!bss_desc->bcn_wpa_ie) || - ((*(bss_desc->bcn_wpa_ie)).vend_hdr.element_id != -WLAN_EID_VENDOR_SPECIFIC)) && - ((!bss_desc->bcn_rsn_ie) || - ((*(bss_desc->bcn_rsn_ie)).ieee_hdr.element_id != -WLAN_EID_RSN)) && + !priv->sec_info.wpa2_enabled && + (!bss_desc->bcn_wpa_ie || +bss_desc->bcn_wpa_ie->vend_hdr.element_id != +WLAN_EID_VENDOR_SPECIFIC) && + (!bss_desc->bcn_rsn_ie || +bss_desc->bcn_rsn_ie->ieee_hdr.element_id != WLAN_EID_RSN) && !priv->sec_info.encryption_mode && !bss_desc->privacy) { return true; } @@ -233,9 +233,10 @@ mwifiex_is_bss_wpa(struct mwifiex_private *priv, struct mwifiex_bssdescriptor *bss_desc) { if (!priv->sec_info.wep_enabled && priv->sec_info.wpa_enabled && - !priv->sec_info.wpa2_enabled && ((bss_desc->bcn_wpa_ie) && - ((*(bss_desc->bcn_wpa_ie)). -vend_hdr.element_id == WLAN_EID_VENDOR_SPECIFIC)) + !priv->sec_info.wpa2_enabled && + (bss_desc->bcn_wpa_ie && +bss_desc->bcn_wpa_ie->vend_hdr.element_id == +WLAN_EID_VENDOR_SPECIFIC) /* * Privacy bit may NOT be set in some APs like * LinkSys WRT54G && bss_desc->privacy @@ -245,12 +246,10 @@ mwifiex_is_bss_wpa(struct mwifiex_private *priv, "info: %s: WPA:\t" "wpa_ie=%#x wpa2_ie=%#x WEP=%s WPA=%s WPA2=%s\t" "EncMode=%#x privacy=%#x\n", __func__, - (bss_desc->bcn_wpa_ie) ? - (*bss_desc->bcn_wpa_ie). - vend_hdr.element_id : 0, - (bss_desc->bcn_rsn_ie) ? - (*bss_desc->bcn_rsn_ie). - ieee_hdr.element_id : 0, + bss_desc->bcn_wpa_ie ? + bss_desc->bcn_wpa_ie->vend_hdr.element_id : 0, + bss_desc->bcn_rsn_ie ? +
Re: mwifiex: kernel WARN_ON during system suspend
Hi Vishal, 2016-04-07 15:02 GMT+02:00 Vishal Thanki : > Hi All, > > I noticed that sometime I see the kernel trace while system enters into > suspend. The trace log is attached with this email. > > From the kernel trace it appears that mwifiex_cancel_all_pending_cmd() > function tries to cancel the current command by invoking > mwifiex_complete_cmd(). Just before calling this routine, the > adapter->curr_cmd->wait_q_enabled is set to false. And there is a > WARN_ON() in the mwifiex_complete_cmd() routine on this variable being > false, because this condition will always evaluate to true. thanks for the analysis. tested-by: afenk...@gmail.com > > I think by setting the wait_q_enabled flag to false after calling the > mwifiex_complete_cmd() should fix this problem. I have attached the > patch for the same, however I am not sure if that is the right way to > fix this issue. If there is any better way to fix it, please suggest. > > Thanks, > Vishal -- 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 2/2] mwifiex: sdio: bug: dead-lock in card reset
Hi Kalle, thanks for reviewing this 2015-04-28 18:04 GMT+02:00 Kalle Valo : > Amitkumar Karwar writes: > >>> Card reset is implemented by removing/re-adding the adapter instance. >>> This is implemented by removing the mmc host, which will then trigger a >>> cascade of removal callbacks including mwifiex_sdio_remove. >>> The dead-lock results in the latter function, trying to cancel the work >>> item executing the mmc host removal. This patch adds a driver level, not >>> adapter level, work struct to break the dead-lock >>> > > Ok, so we are talking about this commit which apparently fell through > the cracks: > > https://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?id=2f5872b60146 > > Like I said as a reply to patch 1, using static variables for this is > ugly. Isn't there really any better way to handle the problem? The root of the evil is calling internal mmc-host API to solve a problem in mwifiex: target = adapter->card->sdio_func->host; mmc_remove_host(target); mdelay(200); mmc_add_host(target); This code needs to run on a workqueue that is independent of the adapter, since all workqueues of the adapter are destroyed. We can hide the adapter pointer through sdio_set_drvdata, Then from the workqueue, we can iterate over all sdio_func in the system. Must be possible through the device model somehow, then use, a *global* variable to filter out the sdio_func that is ours. That saves us from de-referencing the weak adapter pointer (card could be removed by mmc-core workqueue running in parallel). But it doesn't safe us from a weak mmc host pointer: That particular card has a BT function as well, and if cmd handler is hosed for one function, it's hosed for the other one as well. If they both issue card reset simultaneously we have a race condition. (we can't claim the host we are going to destroy), - unless they both run on the same workqueue, and the workqueue only executes 1 task simultaneously . It's also quite unpolite of one function to reset card before consulting the other functions. Something like request reset / and each function has a veto. (There is also an FM tuner, that can send it's data via I2S, not affected by the sdio cmd handler) The current solution is pragmatic, 99.99% of the cards are non-removable / 1 adapter per system / no-BT? But it might be a bit labor intensive to maintain, since changes in the mmc subsystem might break it by accident. https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=81df6cafe28b358739d121205e1ddaeec2ed5b15 /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
mwifiex: cmd timeout during suspend
Observed FW reset during suspend. This is harmful since during suspend, driver shall not reset and reload the FW. Problem has been observed on kernel 4.0 / 39a88044. I've already seen this upstream patch: 1ebd221f88397 mwifiex: stop command path in suspend handle Since the issue is a bit hard to reproduce, I'm wondering, if that patch already fixes the problem or if it's a new issue. [ 164.668498] PM: Syncing filesystems ... done. [ 164.691406] Freezing user space processes ... (elapsed 0.003 seconds) done. [ 164.702717] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done. [ 174.587750] mwifiex_sdio mmc0:0001:1: mwifiex_cmd_timeout_func: Timeout cmd id = 0x6, act = 0x3 [ 174.596872] mwifiex_sdio mmc0:0001:1: num_data_h2c_failure = 0 [ 174.602970] mwifiex_sdio mmc0:0001:1: num_cmd_h2c_failure = 0 [ 174.608975] mwifiex_sdio mmc0:0001:1: is_cmd_timedout = 1 [ 174.614620] mwifiex_sdio mmc0:0001:1: num_tx_timeout = 0 [ 174.620171] mwifiex_sdio mmc0:0001:1: last_cmd_index = 3 [ 174.625725] mwifiex_sdio mmc0:0001:1: last_cmd_id: 06 00 06 00 06 00 06 00 06 00 [ 174.633453] mwifiex_sdio mmc0:0001:1: last_cmd_act: 03 00 03 00 03 00 03 00 03 00 [ 174.641271] mwifiex_sdio mmc0:0001:1: last_cmd_resp_index = 2 [ 174.647276] mwifiex_sdio mmc0:0001:1: last_cmd_resp_id: 06 80 06 80 06 80 06 80 06 80 [ 174.655456] mwifiex_sdio mmc0:0001:1: last_event_index = 0 [ 174.661189] mwifiex_sdio mmc0:0001:1: last_event: 0a 00 17 00 2b 00 0a 00 0b 00 [ 174.668826] mwifiex_sdio mmc0:0001:1: data_sent=0 cmd_sent=0 [ 174.674743] mwifiex_sdio mmc0:0001:1: ps_mode=1 ps_state=0 [ 174.681867] mwifiex_sdio mmc0:0001:1: cmd timeout [ 174.689400] mwifiex_sdio mmc0:0001:1: === DRIVER INFO DUMP START=== [ 174.696670] mwifiex_sdio mmc0:0001:1: SDIO register DUMP START [ 174.717782] mwifiex_sdio mmc0:0001:1: None of the WOWLAN triggers enabled [ 174.726453] mwifiex_sdio: Resetting card... [ 184.727059] mwifiex_sdio mmc0:0001:1: mwifiex_cmd_timeout_func: Timeout cmd id = 0x28, act = 0x13 [ 184.736357] mwifiex_sdio mmc0:0001:1: num_data_h2c_failure = 0 [ 184.742456] mwifiex_sdio mmc0:0001:1: num_cmd_h2c_failure = 0 [ 184.748461] mwifiex_sdio mmc0:0001:1: is_cmd_timedout = 1 [ 184.754103] mwifiex_sdio mmc0:0001:1: num_tx_timeout = 0 [ 184.759654] mwifiex_sdio mmc0:0001:1: last_cmd_index = 1 [ 184.765209] mwifiex_sdio mmc0:0001:1: last_cmd_id: 28 00 28 00 06 00 06 00 24 00 [ 184.772937] mwifiex_sdio mmc0:0001:1: last_cmd_act: 13 00 13 00 03 00 03 00 24 65 [ 184.780754] mwifiex_sdio mmc0:0001:1: last_cmd_resp_index = 4 [ 184.786760] mwifiex_sdio mmc0:0001:1: last_cmd_resp_id: 06 80 06 80 06 80 24 80 28 80 [ 184.794940] mwifiex_sdio mmc0:0001:1: last_event_index = 0 [ 184.800672] mwifiex_sdio mmc0:0001:1: last_event: 0a 00 17 00 2b 00 0a 00 0b 00 [ 184.808310] mwifiex_sdio mmc0:0001:1: data_sent=0 cmd_sent=1 [ 184.814225] mwifiex_sdio mmc0:0001:1: ps_mode=1 ps_state=0 [ 184.819988] mwifiex_sdio mmc0:0001:1: cmd timeout [ 184.825005] ieee80211 phy0: deleting the crypto keys -- 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] mwifiex: sdio: reset adapter using mmc_hw_reset
Since 1fb654fd97ff("mmc: sdio: add reset callback to bus operations"), sdio cards can be power cycled using mmc_hw_reset. The use mmc_remove_host/mmc_add_host is discouraged, because these are internal functions to the mmc core and should only be used by mmc hosts Signed-off-by: Andreas Fenkart --- drivers/net/wireless/mwifiex/sdio.c | 51 ++--- drivers/net/wireless/mwifiex/sdio.h | 3 +++ 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/drivers/net/wireless/mwifiex/sdio.c b/drivers/net/wireless/mwifiex/sdio.c index a0b121f..e4c35ee 100644 --- a/drivers/net/wireless/mwifiex/sdio.c +++ b/drivers/net/wireless/mwifiex/sdio.c @@ -91,6 +91,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) return -ENOMEM; card->func = func; + card->device_id = id; func->card->quirks |= MMC_QUIRK_BLKSZ_FOR_BYTE_MODE; @@ -2107,26 +2108,46 @@ mwifiex_update_mp_end_port(struct mwifiex_adapter *adapter, u16 port) port, card->mp_data_port_mask); } +static void mwifiex_recreate_adapter(struct sdio_mmc_card *card) +{ + struct sdio_func *func = card->func; + const struct sdio_device_id *device_id = card->device_id; + + /* TODO mmc_hw_reset does not require destroying and re-probing the +* whole adapter. Hence there was no need to for this rube-goldberg +* design to reload the fw from an external workqueue. If we don't +* destroy the adapter we could reload the fw from +* mwifiex_main_work_queue directly. +* The real difficulty with fw reset is to restore all the user +* settings applied through ioctl. By destroying and recreating the +* adapter, we take the easy way out, since we rely on user space to +* restore them. We assume that user space will treat the new +* incarnation of the adapter(interfaces) as if they had been just +* discovered and initializes them from scratch. +*/ + + mwifiex_sdio_remove(func); + + /* power cycle the adapter */ + sdio_claim_host(func); + mmc_hw_reset(func->card->host); + sdio_release_host(func); + + mwifiex_sdio_probe(func, device_id); +} + static struct mwifiex_adapter *save_adapter; static void mwifiex_sdio_card_reset_work(struct mwifiex_adapter *adapter) { struct sdio_mmc_card *card = adapter->card; - struct mmc_host *target = card->func->card->host; - - /* The actual reset operation must be run outside of driver thread. -* This is because mmc_remove_host() will cause the device to be -* instantly destroyed, and the driver then needs to end its thread, -* leading to a deadlock. -* -* We run it in a totally independent workqueue. -*/ - mwifiex_dbg(adapter, WARN, "Resetting card...\n"); - mmc_remove_host(target); - /* 200ms delay is based on experiment with sdhci controller */ - mdelay(200); - target->rescan_entered = 0; /* rescan non-removable cards */ - mmc_add_host(target); + /* TODO card pointer is unprotected. If the adapter is removed +* physically, sdio core might trigger mwifiex_sdio_remove, before this +* workqueue is run, which will destroy the adapter struct. When this +* workqueue eventually exceutes it will dereference an invalid adapter +* pointer +*/ + mwifiex_recreate_adapter(card); } /* This function read/write firmware */ diff --git a/drivers/net/wireless/mwifiex/sdio.h b/drivers/net/wireless/mwifiex/sdio.h index 6f645cf..c44da61 100644 --- a/drivers/net/wireless/mwifiex/sdio.h +++ b/drivers/net/wireless/mwifiex/sdio.h @@ -262,6 +262,9 @@ struct sdio_mmc_card { struct mwifiex_sdio_mpa_tx mpa_tx; struct mwifiex_sdio_mpa_rx mpa_rx; + + /* needed for card reset */ + const struct sdio_device_id *device_id; }; struct mwifiex_sdio_device { -- 2.1.4 -- 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 3/4] mwifiex: remove CMD_F_CANCELED flag
CMD_F_CANCELED was used to abort mwifiex_process_cmdresp in case it already started or starts processing the cmd. But this was probably not working the way intended: - it is racy: mwifiex_process_cmdresp might already have passed that test and is continuing to use the cmd node being recycled - mwifiex_process_cmdresp repeatedly uses adapter->curr_cmd which we just set to NULL - mwifiex_recycle_cmd_node will clear the flag The reason why it probably works is that mwifiex_cancel_pending_ioctl is only called from mwifiex_cmd_timeout_func, where the there is little chance of a command response still arriving Signed-off-by: Andreas Fenkart --- drivers/net/wireless/mwifiex/cmdevt.c | 23 ++- drivers/net/wireless/mwifiex/fw.h | 1 - 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c index 87b6dee..6458e17 100644 --- a/drivers/net/wireless/mwifiex/cmdevt.c +++ b/drivers/net/wireless/mwifiex/cmdevt.c @@ -807,17 +807,6 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter) adapter->is_cmd_timedout = 0; resp = (struct host_cmd_ds_command *) adapter->curr_cmd->resp_skb->data; - if (adapter->curr_cmd->cmd_flag & CMD_F_CANCELED) { - mwifiex_dbg(adapter, ERROR, - "CMD_RESP: %#x been canceled\n", - le16_to_cpu(resp->command)); - mwifiex_recycle_cmd_node(adapter, adapter->curr_cmd); - spin_lock_irqsave(&adapter->mwifiex_cmd_lock, flags); - adapter->curr_cmd = NULL; - spin_unlock_irqrestore(&adapter->mwifiex_cmd_lock, flags); - return -1; - } - if (adapter->curr_cmd->cmd_flag & CMD_F_HOSTCMD) { /* Copy original response back to response buffer */ struct mwifiex_ds_misc_cmd *hostcmd; @@ -1090,10 +1079,18 @@ mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter) (adapter->curr_cmd->wait_q_enabled)) { spin_lock_irqsave(&adapter->mwifiex_cmd_lock, cmd_flags); cmd_node = adapter->curr_cmd; - cmd_node->cmd_flag |= CMD_F_CANCELED; - mwifiex_recycle_cmd_node(adapter, cmd_node); + /* setting curr_cmd to NULL is quite dangerous, because +* mwifiex_process_cmdresp checks curr_cmd to be != NULL +* at the beginning then relies on it and dereferences +* it at will +* this probably works since mwifiex_cmd_timeout_func +* is the only caller of this function and responses +* at that point +*/ adapter->curr_cmd = NULL; spin_unlock_irqrestore(&adapter->mwifiex_cmd_lock, cmd_flags); + + mwifiex_recycle_cmd_node(adapter, cmd_node); } /* Cancel all pending scan command */ diff --git a/drivers/net/wireless/mwifiex/fw.h b/drivers/net/wireless/mwifiex/fw.h index cd09051..c71e6b2 100644 --- a/drivers/net/wireless/mwifiex/fw.h +++ b/drivers/net/wireless/mwifiex/fw.h @@ -432,7 +432,6 @@ enum P2P_MODES { #define CMD_F_HOSTCMD (1 << 0) -#define CMD_F_CANCELED (1 << 1) #define HostCmd_CMD_ID_MASK 0x0fff -- 2.1.4 -- 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 2/4] mwifiex: remove redundant reset of cmd_wait_q status
mwifiex_cancel_pending_ioctl is called only from mwifiex_cmd_timeout_func. There the wait_q status is set to -ETIMEDWAIT before calling this function. Whether we reset the status to -1 or leave it at -ETIMEDWAIT at end doesn't matter since both are != 0 hence mean failure Signed-off-by: Andreas Fenkart --- drivers/net/wireless/mwifiex/cmdevt.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c index 2f4715e..87b6dee 100644 --- a/drivers/net/wireless/mwifiex/cmdevt.c +++ b/drivers/net/wireless/mwifiex/cmdevt.c @@ -1123,7 +1123,6 @@ mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter) } } } - adapter->cmd_wait_q.status = -1; } /* -- 2.1.4 -- 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 1/4] mwifiex: remove explicit mwifiex_complete_cmd calls
standard call chain when releasing a cmd node: mwifiex_recycle_cmd_node -> mwifiex_insert_cmd_to_free_q -> mwifiex_complete_cmd, if wait_q_enabled calling mwifiex_complete_cmd explicitly and setting wait_q_enabled = false is redundant Signed-off-by: Andreas Fenkart --- drivers/net/wireless/mwifiex/cmdevt.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c index 207da40..2f4715e 100644 --- a/drivers/net/wireless/mwifiex/cmdevt.c +++ b/drivers/net/wireless/mwifiex/cmdevt.c @@ -167,8 +167,6 @@ static int mwifiex_dnld_cmd_to_fw(struct mwifiex_private *priv, mwifiex_dbg(adapter, ERROR, "DNLD_CMD: FW in reset state, ignore cmd %#x\n", cmd_code); - if (cmd_node->wait_q_enabled) - mwifiex_complete_cmd(adapter, cmd_node); mwifiex_recycle_cmd_node(adapter, cmd_node); queue_work(adapter->workqueue, &adapter->main_work); return -1; @@ -1024,6 +1022,7 @@ mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter *adapter) adapter->curr_cmd->wait_q_enabled = false; adapter->cmd_wait_q.status = -1; mwifiex_complete_cmd(adapter, adapter->curr_cmd); + /* no recycle probably wait for response */ } /* Cancel all pending command */ spin_lock_irqsave(&adapter->cmd_pending_q_lock, flags); @@ -1032,11 +1031,8 @@ mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter *adapter) list_del(&cmd_node->list); spin_unlock_irqrestore(&adapter->cmd_pending_q_lock, flags); - if (cmd_node->wait_q_enabled) { + if (cmd_node->wait_q_enabled) adapter->cmd_wait_q.status = -1; - mwifiex_complete_cmd(adapter, cmd_node); - cmd_node->wait_q_enabled = false; - } mwifiex_recycle_cmd_node(adapter, cmd_node); spin_lock_irqsave(&adapter->cmd_pending_q_lock, flags); } @@ -1094,10 +1090,8 @@ mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter) (adapter->curr_cmd->wait_q_enabled)) { spin_lock_irqsave(&adapter->mwifiex_cmd_lock, cmd_flags); cmd_node = adapter->curr_cmd; - cmd_node->wait_q_enabled = false; cmd_node->cmd_flag |= CMD_F_CANCELED; mwifiex_recycle_cmd_node(adapter, cmd_node); - mwifiex_complete_cmd(adapter, adapter->curr_cmd); adapter->curr_cmd = NULL; spin_unlock_irqrestore(&adapter->mwifiex_cmd_lock, cmd_flags); } -- 2.1.4 -- 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 0/4] refactor cmd_node recycling
Following patches are trying to simplify the code paths used to recycle processed commands Andreas Fenkart (4): mwifiex: remove explicit mwifiex_complete_cmd calls mwifiex: remove redundant reset of cmd_wait_q status mwifiex: remove CMD_F_CANCELED flag mwifiex: simplify mwifiex_complete_cmd drivers/net/wireless/mwifiex/cmdevt.c| 35 +++- drivers/net/wireless/mwifiex/fw.h| 1 - drivers/net/wireless/mwifiex/sta_ioctl.c | 4 ++-- drivers/net/wireless/mwifiex/util.c | 12 --- 4 files changed, 18 insertions(+), 34 deletions(-) -- 2.1.4 -- 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 4/4] mwifiex: simplify mwifiex_complete_cmd
600f5d909a54("mwifiex: cleanup ioctl wait queue and abstraction layer") introduced the wakeup_interruptible suppression in mwifiex_complete_cmd b1a47aa5e1e1("mwifiex: fix system hang issue in cmd timeout error case") then added wakup_interruptible to mwifiex_cmd_timeout_func the single place setting a status of ETIMEDOUT. Instead of doing extra work, using the standard call-chain will have the same effect: mwifiex_cancel_pending_ioctl -> mwifiex_recycle_cmd_node -> mwifiex_insert_cmd_to_free_q -> mwifiex_complete_cmd -> wake_up_interruptible The difference is that previously the condition was not set to true, but that's probably just an oversight in b1a47aa5e1e1 and shouldn't have any consequence Signed-off-by: Andreas Fenkart --- drivers/net/wireless/mwifiex/cmdevt.c| 1 - drivers/net/wireless/mwifiex/sta_ioctl.c | 4 ++-- drivers/net/wireless/mwifiex/util.c | 12 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c index 6458e17..27b778d 100644 --- a/drivers/net/wireless/mwifiex/cmdevt.c +++ b/drivers/net/wireless/mwifiex/cmdevt.c @@ -976,7 +976,6 @@ mwifiex_cmd_timeout_func(unsigned long function_context) if (cmd_node->wait_q_enabled) { adapter->cmd_wait_q.status = -ETIMEDOUT; - wake_up_interruptible(&adapter->cmd_wait_q.wait); mwifiex_cancel_pending_ioctl(adapter); } } diff --git a/drivers/net/wireless/mwifiex/sta_ioctl.c b/drivers/net/wireless/mwifiex/sta_ioctl.c index d8b7d9c..a6c8a4f 100644 --- a/drivers/net/wireless/mwifiex/sta_ioctl.c +++ b/drivers/net/wireless/mwifiex/sta_ioctl.c @@ -66,8 +66,8 @@ int mwifiex_wait_queue_complete(struct mwifiex_adapter *adapter, if (status <= 0) { if (status == 0) status = -ETIMEDOUT; - mwifiex_dbg(adapter, ERROR, - "cmd_wait_q terminated: %d\n", status); + mwifiex_dbg(adapter, ERROR, "cmd_wait_q terminated: %d\n", + status); mwifiex_cancel_all_pending_cmd(adapter); return status; } diff --git a/drivers/net/wireless/mwifiex/util.c b/drivers/net/wireless/mwifiex/util.c index 790e619..b65ef5b 100644 --- a/drivers/net/wireless/mwifiex/util.c +++ b/drivers/net/wireless/mwifiex/util.c @@ -496,16 +496,12 @@ int mwifiex_recv_packet(struct mwifiex_private *priv, struct sk_buff *skb) int mwifiex_complete_cmd(struct mwifiex_adapter *adapter, struct cmd_ctrl_node *cmd_node) { - mwifiex_dbg(adapter, CMD, - "cmd completed: status=%d\n", + WARN_ON(!cmd_node->wait_q_enabled); + mwifiex_dbg(adapter, CMD, "cmd completed: status=%d\n", adapter->cmd_wait_q.status); - *(cmd_node->condition) = true; - - if (adapter->cmd_wait_q.status == -ETIMEDOUT) - mwifiex_dbg(adapter, ERROR, "cmd timeout\n"); - else - wake_up_interruptible(&adapter->cmd_wait_q.wait); + *cmd_node->condition = true; + wake_up_interruptible(&adapter->cmd_wait_q.wait); return 0; } -- 2.1.4 -- 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