diff mbox series

[5/8] wifi: mac80211: disallow drivers with HT wider than HE

Message ID 20240111181514.da15fe3214d2.I4df51ad2f4c844615c168bf9bdb498925b3c77d4@changeid (mailing list archive)
State Accepted
Delegated to: Johannes Berg
Headers show
Series cfg80211/mac80211 patches from our internal tree 2024-01-11 | expand

Commit Message

Miri Korenblit Jan. 11, 2024, 4:17 p.m. UTC 
From: Johannes Berg <johannes.berg@intel.com>

To simplify the code in the next patch, disallow drivers
supporting 40 MHz in HT but not HE, since we'd otherwise
have to track local maximum bandwidth per mode there.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Reviewed-by: Gregory Greenman <gregory.greenman@intel.com>
Signed-off-by: Miri Korenblit <miriam.rachel.korenblit@intel.com>
---
 net/mac80211/main.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Jonathan Bither Jan. 11, 2024, 8:39 p.m. UTC | #1 
On 1/11/24 11:17, Miri Korenblit wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> To simplify the code in the next patch, disallow drivers
> supporting 40 MHz in HT but not HE, since we'd otherwise
> have to track local maximum bandwidth per mode there.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Reviewed-by: Gregory Greenman <gregory.greenman@intel.com>
> Signed-off-by: Miri Korenblit <miriam.rachel.korenblit@intel.com>
> ---
>   net/mac80211/main.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index d48fa1147c14..e05bcc35bc1e 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -1119,8 +1119,26 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>   		supp_vht = supp_vht || sband->vht_cap.vht_supported;
>   
>   		for_each_sband_iftype_data(sband, i, iftd) {
> +			u8 he_40_mhz_cap;
> +
>   			supp_he = supp_he || iftd->he_cap.has_he;
>   			supp_eht = supp_eht || iftd->eht_cap.has_eht;
> +
> +			if (sband->band == NL80211_BAND_2GHZ)
> +				he_40_mhz_cap =
> +					IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_IN_2G;
> +			else
> +				he_40_mhz_cap =
> +					IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G;
> +
> +			/* currently no support for HE client where HT has 40 MHz but not HT */
where HT has 40 MHz but not HE?
> +			if (iftd->he_cap.has_he &&
> +			    iftd->types_mask & (BIT(NL80211_IFTYPE_STATION) |
> +						BIT(NL80211_IFTYPE_P2P_CLIENT)) &&
> +			    sband->ht_cap.ht_supported &&
> +			    sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 &&
> +			    !(iftd->he_cap.he_cap_elem.phy_cap_info[0] & he_40_mhz_cap))
> +				return -EINVAL;
>   		}
>   
>   		/* HT, VHT, HE require QoS, thus >= 4 queues */
Johannes Berg Jan. 12, 2024, 10:21 a.m. UTC | #2 
On Thu, 2024-01-11 at 15:39 -0500, Jonathan Bither wrote:
> 
> > +			/* currently no support for HE client where HT has 40 MHz but not HT */
> where HT has 40 MHz but not HE?
> 

Yep, typo, thanks. I'll just fix it when I apply it.

(Btw, it'd help to trim quotes - no need to quote _all_ the context
before and after.)

johannes
Kalle Valo Jan. 12, 2024, 1:10 p.m. UTC | #3 
Miri Korenblit <miriam.rachel.korenblit@intel.com> writes:

> +			/* currently no support for HE client where HT has 40 MHz but not HT */
> +			if (iftd->he_cap.has_he &&
> +			    iftd->types_mask & (BIT(NL80211_IFTYPE_STATION) |
> +						BIT(NL80211_IFTYPE_P2P_CLIENT)) &&
> +			    sband->ht_cap.ht_supported &&
> +			    sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 &&
> +			    !(iftd->he_cap.he_cap_elem.phy_cap_info[0] & he_40_mhz_cap))
> +				return -EINVAL;

Should there be a warning message so that this is noticed if it ever
happens? I don't know.
Jonathan Bither Jan. 12, 2024, 5:27 p.m. UTC | #4 
On 1/12/24 05:21, Johannes Berg wrote:
> On Thu, 2024-01-11 at 15:39 -0500, Jonathan Bither wrote:
>>> +			/* currently no support for HE client where HT has 40 MHz but not HT */
>> where HT has 40 MHz but not HE?
>>
> Yep, typo, thanks. I'll just fix it when I apply it.
No problem.
>
> (Btw, it'd help to trim quotes - no need to quote _all_ the context
> before and after.)
Sure thing, I'll try to be more mindful of this in the future.
>
> johannes
Johannes Berg Jan. 12, 2024, 6:42 p.m. UTC | #5 
On Fri, 2024-01-12 at 15:10 +0200, Kalle Valo wrote:
> Miri Korenblit <miriam.rachel.korenblit@intel.com> writes:
> 
> > +			/* currently no support for HE client where HT has 40 MHz but not HT */
> > +			if (iftd->he_cap.has_he &&
> > +			    iftd->types_mask & (BIT(NL80211_IFTYPE_STATION) |
> > +						BIT(NL80211_IFTYPE_P2P_CLIENT)) &&
> > +			    sband->ht_cap.ht_supported &&
> > +			    sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 &&
> > +			    !(iftd->he_cap.he_cap_elem.phy_cap_info[0] & he_40_mhz_cap))
> > +				return -EINVAL;
> 
> Should there be a warning message so that this is noticed if it ever
> happens? I don't know.

Yeah I don't really know either. I've done that a lot in the past, but
these days I'm kind of thinking that people who develop their drivers
should have some debug story and be able to figure it out? You know
better perhaps ...

Though it'd kind of suck to indent this further with WARN_ON ;-)

johannes
Ben Greear Jan. 12, 2024, 7:58 p.m. UTC | #6 
On 1/12/24 10:42, Johannes Berg wrote:
> On Fri, 2024-01-12 at 15:10 +0200, Kalle Valo wrote:
>> Miri Korenblit <miriam.rachel.korenblit@intel.com> writes:
>>
>>> +			/* currently no support for HE client where HT has 40 MHz but not HT */
>>> +			if (iftd->he_cap.has_he &&
>>> +			    iftd->types_mask & (BIT(NL80211_IFTYPE_STATION) |
>>> +						BIT(NL80211_IFTYPE_P2P_CLIENT)) &&
>>> +			    sband->ht_cap.ht_supported &&
>>> +			    sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 &&
>>> +			    !(iftd->he_cap.he_cap_elem.phy_cap_info[0] & he_40_mhz_cap))
>>> +				return -EINVAL;
>>
>> Should there be a warning message so that this is noticed if it ever
>> happens? I don't know.
> 
> Yeah I don't really know either. I've done that a lot in the past, but
> these days I'm kind of thinking that people who develop their drivers
> should have some debug story and be able to figure it out? You know
> better perhaps ...
> 
> Though it'd kind of suck to indent this further with WARN_ON ;-)
> 
> johannes

I tried backporting this patch into my 6.7 tree.  An mtk7915 radio system blows up badly
in this case.  Likely this is mt76 bug, but also...it used to work and the crash doesn't
make it very obvious that the above code is to blame.

So, I suggest making this a WARN_ON with appropriate debugging output, let that get some
testing, and then when drivers are fixed, maybe add in the 'return -EINVAL'.


mt7915e 0000:06:00.0: mt7915_register_device failed, ret: -22
mt7915e 0000:06:00.0: mt7915_pci_probe had error on try 3/3, ret: -22
stack segment: 0000 [#1] PREEMPT SMP
CPU: 3 PID: 35 Comm: ksoftirqd/3 Tainted: G        WC         6.7.0+ #31
Hardware name: Broachlink NOAH V2 E3845/Aptio CRB, BIOS 5.6.10 08/19/2021
RIP: 0010:tasklet_action_common.constprop.0+0x80/0x220
Code: 00 00 00 00 49 8b 44 24 08 48 89 18 49 89 5c 24 08 66 90 65 66 44 09 35 9e a6 ea 7e fb 0f 1f 44 00 00 48 85 ed 74 53 48 89 eb <48> 8b 6d 000
RSP: 0018:ffffc90000153e50 EFLAGS: 00010202
RAX: ffff8881283271a8 RBX: 005fff800002012c RCX: 0000000000000006
RDX: ffffffff82606ad0 RSI: 0000000000000006 RDI: ffffea0004435488
RBP: 005fff800002012c R08: 0000000000000002 R09: 0000000080000100
R10: ffffffff826060c0 R11: 000000000002e000 R12: ffff88813bd9c230
R13: ffffea0004435488 R14: 0000000000000040 R15: 0000000000000006
FS:  0000000000000000(0000) GS:ffff88813bd80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000565149684ff4 CR3: 000000012176b000 CR4: 00000000001006f0
Call Trace:
  <TASK>
  ? die+0x2d/0x80
  ? do_trap+0xcd/0xf0
  ? do_error_trap+0x65/0x80
  ? exc_stack_segment+0x33/0x50
  ? asm_exc_stack_segment+0x22/0x30
  ? tasklet_action_common.constprop.0+0x80/0x220
  __do_softirq+0xb4/0x293
  ? sort_range+0x20/0x20
  run_ksoftirqd+0x1f/0x30
  smpboot_thread_fn+0xc2/0x1a0
  kthread+0xdc/0x110
  ? kthread_complete_and_exit+0x20/0x20
  ret_from_fork+0x28/0x40
  ? kthread_complete_and_exit+0x20/0x20
  ret_from_fork_asm+0x11/0x20
  </TASK>
Modules linked in: qrtr ofpart spi_nor mtd intel_rapl_msr at24 spi_intel_platform regmap_i2c spi_intel iTCO_wdt intel_pmc_bxt iTCO_vendor_supports
---[ end trace 0000000000000000 ]---

Thanks,
Ben
Kalle Valo Jan. 13, 2024, 6:54 a.m. UTC | #7 
Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2024-01-12 at 15:10 +0200, Kalle Valo wrote:
>> Miri Korenblit <miriam.rachel.korenblit@intel.com> writes:
>> 
>> > +			/* currently no support for HE client where HT has 40 MHz but not HT */
>> > +			if (iftd->he_cap.has_he &&
>> > +			    iftd->types_mask & (BIT(NL80211_IFTYPE_STATION) |
>> > +						BIT(NL80211_IFTYPE_P2P_CLIENT)) &&
>> > +			    sband->ht_cap.ht_supported &&
>> > +			    sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 &&
>> > +			    !(iftd->he_cap.he_cap_elem.phy_cap_info[0] & he_40_mhz_cap))
>> > +				return -EINVAL;
>> 
>> Should there be a warning message so that this is noticed if it ever
>> happens? I don't know.
>
> Yeah I don't really know either. I've done that a lot in the past, but
> these days I'm kind of thinking that people who develop their drivers
> should have some debug story and be able to figure it out? You know
> better perhaps ...

My worry here is the time needed to figure this all out. Especially if
we have more of these kind silent checks it could get complex quite
quick.

> Though it'd kind of suck to indent this further with WARN_ON ;-)

Yeah, WARN_ON() feels a bit too much. I was more thinking about a small
pr_err() or something like that which give at least some hint what's
happening.
Johannes Berg Jan. 15, 2024, 10 p.m. UTC | #8 
On Fri, 2024-01-12 at 11:58 -0800, Ben Greear wrote:
> 
> I tried backporting this patch into my 6.7 tree.  An mtk7915 radio system blows up badly
> in this case.  Likely this is mt76 bug, but also...it used to work and the crash doesn't
> make it very obvious that the above code is to blame.

Yeah, hence my comment about kernel developers hopefully being able to
figure it out :-)

> mt7915e 0000:06:00.0: mt7915_register_device failed, ret: -22
> mt7915e 0000:06:00.0: mt7915_pci_probe had error on try 3/3, ret: -22

Felix says this kind of retry logic doesn't exist upstream, maybe you
have some delta in your tree that's making it crash?

Also, from what he says and looking at the code, it should register with
HE 40 MHz capability set whenever has_he==true, so also here, do you
have any non-upstream changes that could affect it?

johannes
Ben Greear Jan. 15, 2024, 11:14 p.m. UTC | #9 
On 1/15/24 14:00, Johannes Berg wrote:
> On Fri, 2024-01-12 at 11:58 -0800, Ben Greear wrote:
>>
>> I tried backporting this patch into my 6.7 tree.  An mtk7915 radio system blows up badly
>> in this case.  Likely this is mt76 bug, but also...it used to work and the crash doesn't
>> make it very obvious that the above code is to blame.
> 
> Yeah, hence my comment about kernel developers hopefully being able to
> figure it out :-)
> 
>> mt7915e 0000:06:00.0: mt7915_register_device failed, ret: -22
>> mt7915e 0000:06:00.0: mt7915_pci_probe had error on try 3/3, ret: -22
> 
> Felix says this kind of retry logic doesn't exist upstream, maybe you
> have some delta in your tree that's making it crash?
> 
> Also, from what he says and looking at the code, it should register with
> HE 40 MHz capability set whenever has_he==true, so also here, do you
> have any non-upstream changes that could affect it?

Yes, I do.  Don't think I have anything that adjusts the 40Mhz, but possible.

Anyway, I know how to fix/work around it now, so if it works for Felix
then fine with me.

I still think you should have a pr_err or warn_on_once though, since if that clause
hits, at best case the radio suddenly became un-available.

Thanks,
Ben
diff mbox series

Patch

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index d48fa1147c14..e05bcc35bc1e 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1119,8 +1119,26 @@  int ieee80211_register_hw(struct ieee80211_hw *hw)
 		supp_vht = supp_vht || sband->vht_cap.vht_supported;
 
 		for_each_sband_iftype_data(sband, i, iftd) {
+			u8 he_40_mhz_cap;
+
 			supp_he = supp_he || iftd->he_cap.has_he;
 			supp_eht = supp_eht || iftd->eht_cap.has_eht;
+
+			if (sband->band == NL80211_BAND_2GHZ)
+				he_40_mhz_cap =
+					IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_IN_2G;
+			else
+				he_40_mhz_cap =
+					IEEE80211_HE_PHY_CAP0_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G;
+
+			/* currently no support for HE client where HT has 40 MHz but not HT */
+			if (iftd->he_cap.has_he &&
+			    iftd->types_mask & (BIT(NL80211_IFTYPE_STATION) |
+						BIT(NL80211_IFTYPE_P2P_CLIENT)) &&
+			    sband->ht_cap.ht_supported &&
+			    sband->ht_cap.cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40 &&
+			    !(iftd->he_cap.he_cap_elem.phy_cap_info[0] & he_40_mhz_cap))
+				return -EINVAL;
 		}
 
 		/* HT, VHT, HE require QoS, thus >= 4 queues */