Re: [PATCH v5 09/17] wifi: cc33xx: Add main.c

["Nemanov, Michael" <michael.nemanov@ti.com>]

On 11/8/2024 1:42 PM, Johannes Berg wrote:

>> +	if (sta_rate_set) {
>> +		wlvif->rate_set = cc33xx_tx_enabled_rates_get(cc, sta_rate_set,
>> +							      wlvif->band);
>> +	}
> 
> you have a thing for extra braces ;-)
> (also in many other places)
> 

Yeah most of those head debug traces that were dropped as part of the 
feedback. Will fix the style too.

>> +static int cc33xx_init_ieee80211(struct cc33xx *cc)
>> +{
>> +	unsigned int i;
>> +
>> +	if (cc->conf.core.mixed_mode_support) {
>> +		static const u32 cipher_suites[] = {
>> +			WLAN_CIPHER_SUITE_CCMP,
>> +			WLAN_CIPHER_SUITE_AES_CMAC,
>> +			WLAN_CIPHER_SUITE_TKIP,
>> +			WLAN_CIPHER_SUITE_GCMP,
>> +			WLAN_CIPHER_SUITE_GCMP_256,
>> +			WLAN_CIPHER_SUITE_BIP_GMAC_128,
>> +			WLAN_CIPHER_SUITE_BIP_GMAC_256,
>> +		};
>> +		cc->hw->wiphy->cipher_suites = cipher_suites;
>> +		cc->hw->wiphy->n_cipher_suites = ARRAY_SIZE(cipher_suites);
>> +
>> +	} else {
>> +		static const u32 cipher_suites[] = {
>> +			WLAN_CIPHER_SUITE_CCMP,
>> +			WLAN_CIPHER_SUITE_AES_CMAC,
>> +			WLAN_CIPHER_SUITE_GCMP,
>> +			WLAN_CIPHER_SUITE_GCMP_256,
>> +			WLAN_CIPHER_SUITE_BIP_GMAC_128,
>> +			WLAN_CIPHER_SUITE_BIP_GMAC_256,
>> +		};
> 
> I don't see you have GEM here, yet you handle it in other places above,
> that seems odd. Also I'm not sure it can work at all now that we removed
> the whole extended IV mess, unless you offloaded that?
> 

This cipher is unsupported. Will remove this and any related code.

>> +	/* clear channel flags from the previous usage
>> +	 * and restore max_power & max_antenna_gain values.
>> +	 */
>> +	for (i = 0; i < ARRAY_SIZE(cc33xx_channels); i++) {
>> +		cc33xx_band_2ghz.channels[i].flags = 0;
>> +		cc33xx_band_2ghz.channels[i].max_power = CC33XX_MAX_TXPWR;
>> +		cc33xx_band_2ghz.channels[i].max_antenna_gain = 0;
>> +	}
>> +
>> +	for (i = 0; i < ARRAY_SIZE(cc33xx_channels_5ghz); i++) {
>> +		cc33xx_band_5ghz.channels[i].flags = 0;
>> +		cc33xx_band_5ghz.channels[i].max_power = CC33XX_MAX_TXPWR;
>> +		cc33xx_band_5ghz.channels[i].max_antenna_gain = 0;
>> +	}
>> +
>> +	/* Enable/Disable He based on conf file params */
>> +	if (!cc->conf.mac.he_enable) {
>> +		cc33xx_band_2ghz.iftype_data = NULL;
>> +		cc33xx_band_2ghz.n_iftype_data = 0;
>> +
>> +		cc33xx_band_5ghz.iftype_data = NULL;
>> +		cc33xx_band_5ghz.n_iftype_data = 0;
>> +	}
> 
> it seems wrong to modify the global data here
> 
>> +	/* We keep local copies of the band structs because we need to
>> +	 * modify them on a per-device basis.
>> +	 */
>> +	memcpy(&cc->bands[NL80211_BAND_2GHZ], &cc33xx_band_2ghz,
>> +	       sizeof(cc33xx_band_2ghz));
>> +	memcpy(&cc->bands[NL80211_BAND_2GHZ].ht_cap,
>> +	       &cc->ht_cap[NL80211_BAND_2GHZ],
>> +	       sizeof(*cc->ht_cap));
> 
> and in particular if you *then* do that??
> 

I see your point. I'll drop the init loops and use C initializers 
instead as this data does not change. Any dynamic modification will done 
to cc->bands.

Thanks and regards,
Michael.