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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6bf6412d-8f9e-461d-913c-9718b5f0b8d3@ti.com>
Date: Thu, 14 Nov 2024 19:44:27 +0200
From: "Nemanov, Michael" <michael.nemanov@...com>
To: Johannes Berg <johannes@...solutions.net>, Kalle Valo <kvalo@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Rob
 Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor
 Dooley <conor+dt@...nel.org>, <linux-wireless@...r.kernel.org>,
        <netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
CC: Sabeeh Khan <sabeeh-khan@...com>
Subject: Re: [PATCH v5 09/17] wifi: cc33xx: Add main.c

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.