Hi,
I came across an issue reported by an internal static analysis tool when
tested on the code for brcm80211 driver.
The commit which introduced is -
5b435de0d786869c95d1962121af0d7df2542009
("net: wireless: add brcm80211 drivers")
In the file -
drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c
within the function wlc_lcnphy_tx_iqlo_cal()
we assign value to band_idx as below
band_idx = (CHSPEC_IS5G(pi->radio_chanspec) ? 1 : 0);
From this band_idx could be either 1 or 0.
But when we look at the array iqcal_gainparams_numgains_lcnphy[] at
drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c
We can notice that it is initialized with only one value in it.
static const u16 iqcal_gainparams_numgains_lcnphy[1] = {
ARRAY_SIZE(tbl_iqcal_gainparams_lcnphy_2G),
};
So, when we try to access iqcal_gainparams_numgains_lcnphy[band_idx]
within the for loop in the same function we could be reading a wrong
value of iqcal_gainparams_numgains_lcnphy[1]
Do you have any suggestion on how we can resolve this?
Is it possible to keep band_idx to just 0?
Thanks,
Samasth.
- moved John to Cc: as he passed the wireless maintainer role to Kalle
long ago.
On 5/6/2024 9:03 PM, [email protected] wrote:
> Hi,
>
>
> I came across an issue reported by an internal static analysis tool when
> tested on the code for brcm80211 driver.
>
> The commit which introduced is -
> 5b435de0d786869c95d1962121af0d7df2542009
> ("net: wireless: add brcm80211 drivers")
>
>
> In the file -
> drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c
>
> within the function wlc_lcnphy_tx_iqlo_cal()
>
> we assign value to band_idx as below
> band_idx = (CHSPEC_IS5G(pi->radio_chanspec) ? 1 : 0);
> From this band_idx could be either 1 or 0.
>
> But when we look at the array iqcal_gainparams_numgains_lcnphy[] at
> drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c
>
> We can notice that it is initialized with only one value in it.
>
> static const u16 iqcal_gainparams_numgains_lcnphy[1] = {
> ARRAY_SIZE(tbl_iqcal_gainparams_lcnphy_2G),
> };
>
>
> So, when we try to access iqcal_gainparams_numgains_lcnphy[band_idx]
> within the for loop in the same function we could be reading a wrong
> value of iqcal_gainparams_numgains_lcnphy[1]
>
> Do you have any suggestion on how we can resolve this?
> Is it possible to keep band_idx to just 0?
Hi Samasth,
Did some digging through the code and the only device for which the LCN
phy code is used is the BCM4313 which is a 2G-only device. Hence the
band_idx in the code you pointed out will never be 1. My suggestion
would be to get rid on band_idx and always use 0 instead and add:
if (WARN_ON(CHSPEC_IS5G(pi->radio_chanspec)))
return;
before the kmalloc_array() call at the beginning of the function.
Regards,
Arend
On 5/7/24 12:09 PM, Arend van Spriel wrote:
> - moved John to Cc: as he passed the wireless maintainer role to Kalle
> long ago.
>
> On 5/6/2024 9:03 PM, [email protected] wrote:
>> Hi,
>>
>>
>> I came across an issue reported by an internal static analysis tool
>> when tested on the code for brcm80211 driver.
>>
>> The commit which introduced is -
>> 5b435de0d786869c95d1962121af0d7df2542009
>> ("net: wireless: add brcm80211 drivers")
>>
>>
>> In the file -
>> drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c
>>
>> within the function wlc_lcnphy_tx_iqlo_cal()
>>
>> we assign value to band_idx as below
>> band_idx = (CHSPEC_IS5G(pi->radio_chanspec) ? 1 : 0);
>> From this band_idx could be either 1 or 0.
>>
>> But when we look at the array iqcal_gainparams_numgains_lcnphy[] at
>> drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c
>>
>> We can notice that it is initialized with only one value in it.
>>
>> static const u16 iqcal_gainparams_numgains_lcnphy[1] = {
>> ARRAY_SIZE(tbl_iqcal_gainparams_lcnphy_2G),
>> };
>>
>>
>> So, when we try to access iqcal_gainparams_numgains_lcnphy[band_idx]
>> within the for loop in the same function we could be reading a wrong
>> value of iqcal_gainparams_numgains_lcnphy[1]
>>
>> Do you have any suggestion on how we can resolve this?
>> Is it possible to keep band_idx to just 0?
>
> Hi Samasth,
>
> Did some digging through the code and the only device for which the LCN
> phy code is used is the BCM4313 which is a 2G-only device. Hence the
> band_idx in the code you pointed out will never be 1. My suggestion
> would be to get rid on band_idx and always use 0 instead and add:
>
> if (WARN_ON(CHSPEC_IS5G(pi->radio_chanspec)))
> return;
>
> before the kmalloc_array() call at the beginning of the function.
Thanks for looking into it Arend.
I will work on a patch and make the changes as you suggested. Will send
it out for review soon.
Samasth.
>
> Regards,
> Arend