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

[Nemanov, Michael]

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.