| Message ID | 20220914033620.12742-5-ian.lin@infineon.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix connect/p2p issue series | expand |
On 9/22/2022 12:44 PM, Kalle Valo wrote: > Ian Lin <ian.lin@infineon.com> writes: > >> From: Syed Rafiuddeen <syed.rafiuddeen@cypress.com> >> >> cfg80211 layer on DUT STA is disconnecting ongoing connection attempt after >> receiving association response, because cfg80211 layer does not have valid >> AP bss information. On association response event, brcmfmac communicates >> the AP bss information to cfg80211 layer, but SSID seem to be empty in AP >> bss information, and cfg80211 layer prints kernel warning and then >> disconnects the ongoing connection attempt. >> >> SSID is empty in SSID IE, but 'bi->SSID' contains a valid SSID, so >> updating the SSID for hidden AP while informing its bss information >> to cfg80211 layer. >> >> Signed-off-by: Syed Rafiuddeen <syed.rafiuddeen@infineon.com> > Syed's email address in From does not match with s-o-b. > >> @@ -3032,6 +3033,12 @@ static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg, >> notify_ielen = le32_to_cpu(bi->ie_length); >> bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100; >> >> + ssid = brcmf_parse_tlvs(notify_ie, notify_ielen, WLAN_EID_SSID); >> + if (ssid && ssid->data[0] == '\0' && ssid->len == bi->SSID_len) { >> + /* Update SSID for hidden AP */ >> + memcpy((u8 *)ssid->data, bi->SSID, bi->SSID_len); >> + } > memcpy() takes a void pointer so the cast is not needed. There should be a type casting since 'ssid' is a const pointer. As you saw there will be build warning in PATCH v2 (sorry I didn't notice that locally) I will send PATCH v3 to restore type casting, is that ok?
On 9/22/2022 6:52 PM, Lin Ian (CSSITB CSS ICW SW WFS / EE) wrote: > > > On 9/22/2022 12:44 PM, Kalle Valo wrote: >> Ian Lin <ian.lin@infineon.com> writes: >> >>> From: Syed Rafiuddeen <syed.rafiuddeen@cypress.com> >>> >>> cfg80211 layer on DUT STA is disconnecting ongoing connection attempt >>> after >>> receiving association response, because cfg80211 layer does not have >>> valid >>> AP bss information. On association response event, brcmfmac communicates >>> the AP bss information to cfg80211 layer, but SSID seem to be empty >>> in AP >>> bss information, and cfg80211 layer prints kernel warning and then >>> disconnects the ongoing connection attempt. >>> >>> SSID is empty in SSID IE, but 'bi->SSID' contains a valid SSID, so >>> updating the SSID for hidden AP while informing its bss information >>> to cfg80211 layer. >>> >>> Signed-off-by: Syed Rafiuddeen <syed.rafiuddeen@infineon.com> >> Syed's email address in From does not match with s-o-b. >> >>> @@ -3032,6 +3033,12 @@ static s32 brcmf_inform_single_bss(struct >>> brcmf_cfg80211_info *cfg, >>> notify_ielen = le32_to_cpu(bi->ie_length); >>> bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100; >>> >>> + ssid = brcmf_parse_tlvs(notify_ie, notify_ielen, WLAN_EID_SSID); >>> + if (ssid && ssid->data[0] == '\0' && ssid->len == bi->SSID_len) { >>> + /* Update SSID for hidden AP */ >>> + memcpy((u8 *)ssid->data, bi->SSID, bi->SSID_len); >>> + } >> memcpy() takes a void pointer so the cast is not needed. > There should be a type casting since 'ssid' is a const pointer. > As you saw there will be build warning in PATCH v2 (sorry I didn't > notice that locally) > I will send PATCH v3 to restore type casting, is that ok? > writing through a const pointer seems broken. Should you instead remove the const qualifier from the declaration? const struct brcmf_tlv *ssid = NULL; if the problem is that brcmf_parse_tlvs() itself returns a const pointer, then I'd typecast during the assignment instead of the memcpy() call. also note the NULL initializer is unnecessary since it is always overwritten by the call to brcmf_parse_tlvs()
Jeff Johnson <quic_jjohnson@quicinc.com> writes: >>>> @@ -3032,6 +3033,12 @@ static s32 brcmf_inform_single_bss(struct >>>> brcmf_cfg80211_info *cfg, >>>> notify_ielen = le32_to_cpu(bi->ie_length); >>>> bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100; >>>> >>>> + ssid = brcmf_parse_tlvs(notify_ie, notify_ielen, WLAN_EID_SSID); >>>> + if (ssid && ssid->data[0] == '\0' && ssid->len == bi->SSID_len) { >>>> + /* Update SSID for hidden AP */ >>>> + memcpy((u8 *)ssid->data, bi->SSID, bi->SSID_len); >>>> + } >>> memcpy() takes a void pointer so the cast is not needed. >> >> There should be a type casting since 'ssid' is a const pointer. >> As you saw there will be build warning in PATCH v2 (sorry I didn't >> notice that locally) >> I will send PATCH v3 to restore type casting, is that ok? >> > > writing through a const pointer seems broken. Yeah, this is exactly why I don't like casting. I see so often casts removing the const and that can lead to another problems.
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index eba03a994e95..ca5cbbb622d2 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c @@ -3003,6 +3003,7 @@ static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg, u8 *notify_ie; size_t notify_ielen; struct cfg80211_inform_bss bss_data = {}; + const struct brcmf_tlv *ssid = NULL; if (le32_to_cpu(bi->length) > WL_BSS_INFO_MAX) { bphy_err(drvr, "Bss info is larger than buffer. Discarding\n"); @@ -3032,6 +3033,12 @@ static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg, notify_ielen = le32_to_cpu(bi->ie_length); bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100; + ssid = brcmf_parse_tlvs(notify_ie, notify_ielen, WLAN_EID_SSID); + if (ssid && ssid->data[0] == '\0' && ssid->len == bi->SSID_len) { + /* Update SSID for hidden AP */ + memcpy((u8 *)ssid->data, bi->SSID, bi->SSID_len); + } + brcmf_dbg(CONN, "bssid: %pM\n", bi->BSSID); brcmf_dbg(CONN, "Channel: %d(%d)\n", channel, freq); brcmf_dbg(CONN, "Capability: %X\n", notify_capability);