diff mbox series

[FIX] brcmfmac: fix reporting support for 160 MHz channels

Message ID 20181108150829.24842-1-zajec5@gmail.com (mailing list archive)
State Accepted
Commit d1fe6ad6f6bd61c84788d3a7b11e459a439c6169
Delegated to: Kalle Valo
Headers show
Series [FIX] brcmfmac: fix reporting support for 160 MHz channels | expand

Commit Message

Rafał Miłecki Nov. 8, 2018, 3:08 p.m. UTC
From: Rafał Miłecki <rafal@milecki.pl>

Driver can report IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ so it's
important to provide valid & complete info about supported bands for
each channel. By default no support for 160 MHz should be assumed unless
firmware reports it for a given channel later.

This fixes info passed to the userspace. Without that change userspace
could try to use invalid channel and fail to start an interface.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Cc: stable@vger.kernel.org
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Kalle Valo Nov. 9, 2018, 2:05 p.m. UTC | #1
Rafał Miłecki <zajec5@gmail.com> writes:

> From: Rafał Miłecki <rafal@milecki.pl>
>
> Driver can report IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ so it's
> important to provide valid & complete info about supported bands for
> each channel. By default no support for 160 MHz should be assumed unless
> firmware reports it for a given channel later.
>
> This fixes info passed to the userspace. Without that change userspace
> could try to use invalid channel and fail to start an interface.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Cc: stable@vger.kernel.org

Should this be queued to 4.20?
Rafał Miłecki Nov. 9, 2018, 8:10 p.m. UTC | #2
On 2018-11-09 15:05, Kalle Valo wrote:
> Rafał Miłecki <zajec5@gmail.com> writes:
> 
>> From: Rafał Miłecki <rafal@milecki.pl>
>> 
>> Driver can report IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ so it's
>> important to provide valid & complete info about supported bands for
>> each channel. By default no support for 160 MHz should be assumed 
>> unless
>> firmware reports it for a given channel later.
>> 
>> This fixes info passed to the userspace. Without that change userspace
>> could try to use invalid channel and fail to start an interface.
>> 
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> Cc: stable@vger.kernel.org
> 
> Should this be queued to 4.20?

That's my suggestion.

I try to mark fixes (patches for currently developed release) with an
extra FIX tag in a subject. Do you see those when using your patchwork
tool? Do you have any other method in mind that would be preferred by
you?
Kalle Valo Nov. 16, 2018, 8:40 a.m. UTC | #3
Rafał Miłecki <rafal@milecki.pl> writes:

> On 2018-11-09 15:05, Kalle Valo wrote:
>> Rafał Miłecki <zajec5@gmail.com> writes:
>>
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Driver can report IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ so it's
>>> important to provide valid & complete info about supported bands for
>>> each channel. By default no support for 160 MHz should be assumed
>>> unless
>>> firmware reports it for a given channel later.
>>>
>>> This fixes info passed to the userspace. Without that change userspace
>>> could try to use invalid channel and fail to start an interface.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> Cc: stable@vger.kernel.org
>>
>> Should this be queued to 4.20?
>
> That's my suggestion.

Ok, I'll queue this for 4.20.
Kalle Valo Nov. 16, 2018, 8:46 a.m. UTC | #4
(changing subject for better visibility and trimming Cc)

Rafał Miłecki <rafal@milecki.pl> writes:

> On 2018-11-09 15:05, Kalle Valo wrote:
>> Rafał Miłecki <zajec5@gmail.com> writes:
>>
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Driver can report IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ so it's
>>> important to provide valid & complete info about supported bands for
>>> each channel. By default no support for 160 MHz should be assumed
>>> unless
>>> firmware reports it for a given channel later.
>>>
>>> This fixes info passed to the userspace. Without that change userspace
>>> could try to use invalid channel and fail to start an interface.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> Cc: stable@vger.kernel.org
>>
>> Should this be queued to 4.20?
>
> That's my suggestion.
>
> I try to mark fixes (patches for currently developed release) with an
> extra FIX tag in a subject. Do you have any other method in mind that
> would be preferred by you?

Yes, I do see your FIX tag in patchwork:

 [ 31] [FIX] brcmfmac: fix reporting support for 160 MHz channels   2018-11-08 

But "FIX" is a bit ambigous as not all fixes not go to wireless-drivers,
they can also go to wireless-drivers-next. So I prefer using the release
number (or name of the tree) like this:

[PATCH 4.20] brcmfmac: fix reporting support for 160 MHz channels

After seeing your question I added something about this to the wiki
which hopefully helps others:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#tree_labels
Johannes Berg Nov. 16, 2018, 8:49 a.m. UTC | #5
On Fri, 2018-11-16 at 10:46 +0200, Kalle Valo wrote:

> Yes, I do see your FIX tag in patchwork:
> 
>  [ 31] [FIX] brcmfmac: fix reporting support for 160 MHz channels   2018-11-08 
> 
> But "FIX" is a bit ambigous as not all fixes not go to wireless-drivers,
> they can also go to wireless-drivers-next. So I prefer using the release
> number (or name of the tree) like this:
> 
> [PATCH 4.20] brcmfmac: fix reporting support for 160 MHz channels

FWIW, davem/networking just use 

[PATCH net]
[PATCH net-next]

which puts a bit more effort on the submitter but is a bit easier on the
maintainer I suppose. Also, not really a problem here, but it would help
disambiguate different trees on the same mailing list. I don't really
mind either way.

johannes
Kalle Valo Nov. 16, 2018, 8:54 a.m. UTC | #6
Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2018-11-16 at 10:46 +0200, Kalle Valo wrote:
>
>> Yes, I do see your FIX tag in patchwork:
>> 
>>  [ 31] [FIX] brcmfmac: fix reporting support for 160 MHz channels   2018-11-08 
>> 
>> But "FIX" is a bit ambigous as not all fixes not go to wireless-drivers,
>> they can also go to wireless-drivers-next. So I prefer using the release
>> number (or name of the tree) like this:
>> 
>> [PATCH 4.20] brcmfmac: fix reporting support for 160 MHz channels
>
> FWIW, davem/networking just use 
>
> [PATCH net]
> [PATCH net-next]
>
> which puts a bit more effort on the submitter but is a bit easier on the
> maintainer I suppose. Also, not really a problem here, but it would help
> disambiguate different trees on the same mailing list. I don't really
> mind either way.

Actually I already added that to the wiki[1] but made it optional just
bacause it's harder for patch submitters who are not so familiar with
our tree structure. But yes, I also like using the full tree name as the
label, even more so as it would help me to automatically assign patches
to correct maintainers in patchwork.

[1] https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#tree_labels
Rafał Miłecki Nov. 16, 2018, 8:55 a.m. UTC | #7
On 2018-11-16 09:46, Kalle Valo wrote:
> (changing subject for better visibility and trimming Cc)
> 
> Rafał Miłecki <rafal@milecki.pl> writes:
> 
>> On 2018-11-09 15:05, Kalle Valo wrote:
>>> Rafał Miłecki <zajec5@gmail.com> writes:
>>> 
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>> 
>>>> Driver can report IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ so it's
>>>> important to provide valid & complete info about supported bands for
>>>> each channel. By default no support for 160 MHz should be assumed
>>>> unless
>>>> firmware reports it for a given channel later.
>>>> 
>>>> This fixes info passed to the userspace. Without that change 
>>>> userspace
>>>> could try to use invalid channel and fail to start an interface.
>>>> 
>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>> Cc: stable@vger.kernel.org
>>> 
>>> Should this be queued to 4.20?
>> 
>> That's my suggestion.
>> 
>> I try to mark fixes (patches for currently developed release) with an
>> extra FIX tag in a subject. Do you have any other method in mind that
>> would be preferred by you?
> 
> Yes, I do see your FIX tag in patchwork:
> 
>  [ 31] [FIX] brcmfmac: fix reporting support for 160 MHz channels   
> 2018-11-08
> 
> But "FIX" is a bit ambigous as not all fixes not go to 
> wireless-drivers,
> they can also go to wireless-drivers-next. So I prefer using the 
> release
> number (or name of the tree) like this:
> 
> [PATCH 4.20] brcmfmac: fix reporting support for 160 MHz channels
> 
> After seeing your question I added something about this to the wiki
> which hopefully helps others:
> 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#tree_labels

Got it, thanks!
Kalle Valo Nov. 16, 2018, 1:13 p.m. UTC | #8
Rafał Miłecki wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Driver can report IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ so it's
> important to provide valid & complete info about supported bands for
> each channel. By default no support for 160 MHz should be assumed unless
> firmware reports it for a given channel later.
> 
> This fixes info passed to the userspace. Without that change userspace
> could try to use invalid channel and fail to start an interface.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Cc: stable@vger.kernel.org

Patch applied to wireless-drivers.git, thanks.

d1fe6ad6f6bd brcmfmac: fix reporting support for 160 MHz channels
diff mbox series

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 230a378c26fc..7f0a5bade70a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -6005,7 +6005,8 @@  static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
 			 * for subsequent chanspecs.
 			 */
 			channel->flags = IEEE80211_CHAN_NO_HT40 |
-					 IEEE80211_CHAN_NO_80MHZ;
+					 IEEE80211_CHAN_NO_80MHZ |
+					 IEEE80211_CHAN_NO_160MHZ;
 			ch.bw = BRCMU_CHAN_BW_20;
 			cfg->d11inf.encchspec(&ch);
 			chaninfo = ch.chspec;