Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ff7e72ae-0431-b568-32fc-7b1cbf250e20@quicinc.com>
Date:   Fri, 23 Sep 2022 08:29:34 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.0
Subject: Re: [PATCH 4/5] brcmfmac: Update SSID of hidden AP while informing
 its bss to cfg80211 layer
Content-Language: en-US
To:     "Lin Ian (CSSITB CSS ICW SW WFS / EE)" <ian.lin@infineon.com>,
        Kalle Valo <kvalo@kernel.org>
CC:     <linux-wireless@vger.kernel.org>,
        <brcm80211-dev-list@broadcom.com>,
        <brcm80211-dev-list@cypress.com>, <franky.lin@broadcom.com>,
        <hante.meuleman@broadcom.com>, <Double.Lo@infineon.com>
References: <20220914033620.12742-1-ian.lin@infineon.com>
 <20220914033620.12742-5-ian.lin@infineon.com> <87tu506mre.fsf@kernel.org>
 <d390462d-1cd2-fc3f-5b0c-bdcd332ed0bc@infineon.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <d390462d-1cd2-fc3f-5b0c-bdcd332ed0bc@infineon.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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()