diff mbox series

Revert "mac80211: add parse regulatory info in 6 GHz operation information"

Message ID 20230821105903.7482379cde47.Ib72645d02fadc24b520db118abd82e861c87316e@changeid (mailing list archive)
State Rejected
Delegated to: Johannes Berg
Headers show
Series Revert "mac80211: add parse regulatory info in 6 GHz operation information" | expand

Commit Message

Johannes Berg Aug. 21, 2023, 8:59 a.m. UTC 
From: Johannes Berg <johannes.berg@intel.com>

This reverts commit cb751b7a57e5 ("mac80211: add parse regulatory info
in 6 GHz operation information") which added a station type bss_conf
assignment in a parsing helper function, which will corrupt mesh data.

Fixes: cb751b7a57e5 ("mac80211: add parse regulatory info in 6 GHz operation information")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/mac80211.h |  2 --
 net/mac80211/util.c    | 14 --------------
 2 files changed, 16 deletions(-)

Comments

Johannes Berg Aug. 21, 2023, 9:06 a.m. UTC | #1 
On Mon, 2023-08-21 at 10:59 +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> This reverts commit cb751b7a57e5 ("mac80211: add parse regulatory info
> in 6 GHz operation information") which added a station type bss_conf
> assignment in a parsing helper function, which will corrupt mesh data.
> 

Ah crap this won't work, rtw89 already uses this.

Wen please send a fix for this ASAP.

johannes
Wen Gong Aug. 21, 2023, 10:36 a.m. UTC | #2 
On 8/21/2023 5:06 PM, Johannes Berg wrote:
> On Mon, 2023-08-21 at 10:59 +0200, Johannes Berg wrote:
>> From: Johannes Berg <johannes.berg@intel.com>
>>
>> This reverts commit cb751b7a57e5 ("mac80211: add parse regulatory info
>> in 6 GHz operation information") which added a station type bss_conf
>> assignment in a parsing helper function, which will corrupt mesh data.
>>
> Ah crap this won't work, rtw89 already uses this.
>
> Wen please send a fix for this ASAP.
>
> johannes

Hi Johannes,

I looked the patch some times, but I do not know how it corrupt mesh data,

Is there any clue for me?
Johannes Berg Aug. 21, 2023, 10:57 a.m. UTC | #3 
On Mon, 2023-08-21 at 18:36 +0800, Wen Gong wrote:
> On 8/21/2023 5:06 PM, Johannes Berg wrote:
> > On Mon, 2023-08-21 at 10:59 +0200, Johannes Berg wrote:
> > > From: Johannes Berg <johannes.berg@intel.com>
> > > 
> > > This reverts commit cb751b7a57e5 ("mac80211: add parse regulatory info
> > > in 6 GHz operation information") which added a station type bss_conf
> > > assignment in a parsing helper function, which will corrupt mesh data.
> > > 
> > Ah crap this won't work, rtw89 already uses this.
> > 
> > Wen please send a fix for this ASAP.
> > 
> > johannes
> 
> Hi Johannes,
> 
> I looked the patch some times, but I do not know how it corrupt mesh data,
> 
> Is there any clue for me?

Hah, no, I'm wrong ... I looked at it and for some reason thought of
u.mgd instead of vif.bss_conf. Sorry!

Still it's not correct though to write to vif.bss_conf in this function
because it's called from mesh_matches_local() to see if it's even
compatible, for example, so the mere calling a "give me the 6 GHz
chandef" function doesn't indicate we actually are going to use it now.

Also it's wrong for multi-link anyway, so maybe it should have an
optional pointer to the bss_conf to fill it in, or just do it outside of
the function.

johannes
Wen Gong Aug. 21, 2023, 11:11 a.m. UTC | #4 
On 8/21/2023 6:57 PM, Johannes Berg wrote:
> On Mon, 2023-08-21 at 18:36 +0800, Wen Gong wrote:
>> On 8/21/2023 5:06 PM, Johannes Berg wrote:
>>> On Mon, 2023-08-21 at 10:59 +0200, Johannes Berg wrote:
>>>> From: Johannes Berg <johannes.berg@intel.com>
>>>>
>>>> This reverts commit cb751b7a57e5 ("mac80211: add parse regulatory info
>>>> in 6 GHz operation information") which added a station type bss_conf
>>>> assignment in a parsing helper function, which will corrupt mesh data.
>>>>
>>> Ah crap this won't work, rtw89 already uses this.
>>>
>>> Wen please send a fix for this ASAP.
>>>
>>> johannes
>> Hi Johannes,
>>
>> I looked the patch some times, but I do not know how it corrupt mesh data,
>>
>> Is there any clue for me?
> Hah, no, I'm wrong ... I looked at it and for some reason thought of
> u.mgd instead of vif.bss_conf. Sorry!
>
> Still it's not correct though to write to vif.bss_conf in this function
> because it's called from mesh_matches_local() to see if it's even
> compatible, for example, so the mere calling a "give me the 6 GHz
> chandef" function doesn't indicate we actually are going to use it now.

Do you mean mesh_matches_local() is only a try to call 
ieee80211_chandef_he_6ghz_oper(),

NOT real use the 6 GHz chandef?

>
> Also it's wrong for multi-link anyway, so maybe it should have an
> optional pointer to the bss_conf to fill it in, or just do it outside of
> the function.
Yes, I also found it just now.
>
> johannes
Johannes Berg Aug. 21, 2023, 11:34 a.m. UTC | #5 
On Mon, 2023-08-21 at 19:11 +0800, Wen Gong wrote:
> On 8/21/2023 6:57 PM, Johannes Berg wrote:
> > On Mon, 2023-08-21 at 18:36 +0800, Wen Gong wrote:
> > > On 8/21/2023 5:06 PM, Johannes Berg wrote:
> > > > On Mon, 2023-08-21 at 10:59 +0200, Johannes Berg wrote:
> > > > > From: Johannes Berg <johannes.berg@intel.com>
> > > > > 
> > > > > This reverts commit cb751b7a57e5 ("mac80211: add parse regulatory info
> > > > > in 6 GHz operation information") which added a station type bss_conf
> > > > > assignment in a parsing helper function, which will corrupt mesh data.
> > > > > 
> > > > Ah crap this won't work, rtw89 already uses this.
> > > > 
> > > > Wen please send a fix for this ASAP.
> > > > 
> > > > johannes
> > > Hi Johannes,
> > > 
> > > I looked the patch some times, but I do not know how it corrupt mesh data,
> > > 
> > > Is there any clue for me?
> > Hah, no, I'm wrong ... I looked at it and for some reason thought of
> > u.mgd instead of vif.bss_conf. Sorry!
> > 
> > Still it's not correct though to write to vif.bss_conf in this function
> > because it's called from mesh_matches_local() to see if it's even
> > compatible, for example, so the mere calling a "give me the 6 GHz
> > chandef" function doesn't indicate we actually are going to use it now.
> 
> Do you mean mesh_matches_local() is only a try to call 
> ieee80211_chandef_he_6ghz_oper(),
> 
> NOT real use the 6 GHz chandef?

Yes, I believe so.

Anyway it would seem better for a utility function to have clearer
defined output, I think?

johannes
Wen Gong Aug. 22, 2023, 6:17 a.m. UTC | #6 
On 8/21/2023 7:34 PM, Johannes Berg wrote:
> On Mon, 2023-08-21 at 19:11 +0800, Wen Gong wrote:
>> On 8/21/2023 6:57 PM, Johannes Berg wrote:
>>> On Mon, 2023-08-21 at 18:36 +0800, Wen Gong wrote:
>>>> On 8/21/2023 5:06 PM, Johannes Berg wrote:
>>>>> On Mon, 2023-08-21 at 10:59 +0200, Johannes Berg wrote:
>>>>>> From: Johannes Berg <johannes.berg@intel.com>
>>>>>>
>>>>>> This reverts commit cb751b7a57e5 ("mac80211: add parse regulatory info
>>>>>> in 6 GHz operation information") which added a station type bss_conf
>>>>>> assignment in a parsing helper function, which will corrupt mesh data.
>>>>>>
>>>>> Ah crap this won't work, rtw89 already uses this.
>>>>>
>>>>> Wen please send a fix for this ASAP.
>>>>>
>>>>> johannes
>>>> Hi Johannes,
>>>>
>>>> I looked the patch some times, but I do not know how it corrupt mesh data,
>>>>
>>>> Is there any clue for me?
>>> Hah, no, I'm wrong ... I looked at it and for some reason thought of
>>> u.mgd instead of vif.bss_conf. Sorry!
>>>
>>> Still it's not correct though to write to vif.bss_conf in this function
>>> because it's called from mesh_matches_local() to see if it's even
>>> compatible, for example, so the mere calling a "give me the 6 GHz
>>> chandef" function doesn't indicate we actually are going to use it now.
>> Do you mean mesh_matches_local() is only a try to call
>> ieee80211_chandef_he_6ghz_oper(),
>>
>> NOT real use the 6 GHz chandef?
> Yes, I believe so.
>
> Anyway it would seem better for a utility function to have clearer
> defined output, I think?
>
> johannes

Do you mean move the logic of set bss_conf->power_type to another new 
function from

ieee80211_chandef_he_6ghz_oper()?
Johannes Berg Aug. 22, 2023, 7:09 a.m. UTC | #7 
On Tue, 2023-08-22 at 14:17 +0800, Wen Gong wrote:
> > 
> > Anyway it would seem better for a utility function to have clearer
> > defined output, I think?
> > 
> > johannes
> 
> Do you mean move the logic of set bss_conf->power_type to another new 
> function from
> 
> ieee80211_chandef_he_6ghz_oper()?
> 

No, not really. But we could have a pointer to the bss_conf or even the
particular value, as an obvious output from the function.

johannes
kernel test robot Aug. 23, 2023, 9:12 p.m. UTC | #8 
Hi Johannes,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.5-rc7 next-20230823]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Johannes-Berg/Revert-mac80211-add-parse-regulatory-info-in-6-GHz-operation-information/20230821-170019
base:   linus/master
patch link:    https://lore.kernel.org/r/20230821105903.7482379cde47.Ib72645d02fadc24b520db118abd82e861c87316e%40changeid
patch subject: [PATCH] Revert "mac80211: add parse regulatory info in 6 GHz operation information"
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230824/202308240547.O8ZxrrXI-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230824/202308240547.O8ZxrrXI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308240547.O8ZxrrXI-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/net/wireless/realtek/rtw89/regd.c: In function 'rtw89_reg_6ghz_power_recalc':
>> drivers/net/wireless/realtek/rtw89/regd.c:514:39: error: 'struct ieee80211_bss_conf' has no member named 'power_type'; did you mean 'txpower_type'?
     514 |                 switch (vif->bss_conf.power_type) {
         |                                       ^~~~~~~~~~
         |                                       txpower_type


vim +514 drivers/net/wireless/realtek/rtw89/regd.c

f6baa1d3d5703f Zong-Zhe Yang 2023-06-02  505  
f6baa1d3d5703f Zong-Zhe Yang 2023-06-02  506  void rtw89_reg_6ghz_power_recalc(struct rtw89_dev *rtwdev,
f6baa1d3d5703f Zong-Zhe Yang 2023-06-02  507  				 struct rtw89_vif *rtwvif, bool active)
f6baa1d3d5703f Zong-Zhe Yang 2023-06-02  508  {
f6baa1d3d5703f Zong-Zhe Yang 2023-06-02  509  	struct ieee80211_vif *vif = rtwvif_to_vif(rtwvif);
f6baa1d3d5703f Zong-Zhe Yang 2023-06-02  510  
f6baa1d3d5703f Zong-Zhe Yang 2023-06-02  511  	lockdep_assert_held(&rtwdev->mutex);
f6baa1d3d5703f Zong-Zhe Yang 2023-06-02  512  
f6baa1d3d5703f Zong-Zhe Yang 2023-06-02  513  	if (active) {
f6baa1d3d5703f Zong-Zhe Yang 2023-06-02 @514  		switch (vif->bss_conf.power_type) {
diff mbox series

Patch

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 3a8a2d2c58c3..813d4a654470 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -637,7 +637,6 @@  struct ieee80211_fils_discovery {
  *	interval.
  * @beacon_tx_rate: The configured beacon transmit rate that needs to be passed
  *	to driver when rate control is offloaded to firmware.
- * @power_type: power type of BSS for 6 GHz
  * @tx_pwr_env: transmit power envelope array of BSS.
  * @tx_pwr_env_num: number of @tx_pwr_env.
  * @pwr_reduction: power constraint of BSS.
@@ -746,7 +745,6 @@  struct ieee80211_bss_conf {
 	struct ieee80211_fils_discovery fils_discovery;
 	u32 unsol_bcast_probe_resp_interval;
 	struct cfg80211_bitrate_mask beacon_tx_rate;
-	enum ieee80211_ap_reg_power power_type;
 	struct ieee80211_tx_pwr_env tx_pwr_env[IEEE80211_TPE_MAX_IE_COUNT];
 	u8 tx_pwr_env_num;
 	u8 pwr_reduction;
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 8a6917cf63cf..7bd3b64ddac6 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -3841,7 +3841,6 @@  bool ieee80211_chandef_he_6ghz_oper(struct ieee80211_sub_if_data *sdata,
 	const struct ieee80211_sta_eht_cap *eht_cap;
 	struct cfg80211_chan_def he_chandef = *chandef;
 	const struct ieee80211_he_6ghz_oper *he_6ghz_oper;
-	struct ieee80211_bss_conf *bss_conf = &sdata->vif.bss_conf;
 	bool support_80_80, support_160, support_320;
 	u8 he_phy_cap, eht_phy_cap;
 	u32 freq;
@@ -3894,19 +3893,6 @@  bool ieee80211_chandef_he_6ghz_oper(struct ieee80211_sub_if_data *sdata,
 					      NL80211_BAND_6GHZ);
 	he_chandef.chan = ieee80211_get_channel(sdata->local->hw.wiphy, freq);
 
-	switch (u8_get_bits(he_6ghz_oper->control,
-			    IEEE80211_HE_6GHZ_OPER_CTRL_REG_INFO)) {
-	case IEEE80211_6GHZ_CTRL_REG_LPI_AP:
-		bss_conf->power_type = IEEE80211_REG_LPI_AP;
-		break;
-	case IEEE80211_6GHZ_CTRL_REG_SP_AP:
-		bss_conf->power_type = IEEE80211_REG_SP_AP;
-		break;
-	default:
-		bss_conf->power_type = IEEE80211_REG_UNSET_AP;
-		break;
-	}
-
 	if (!eht_oper ||
 	    !(eht_oper->params & IEEE80211_EHT_OPER_INFO_PRESENT)) {
 		switch (u8_get_bits(he_6ghz_oper->control,