* [PATCH v2] staging: wilc1000: renames struct tstrRSSI and its members u8Index, u8Full
From: Tahia Khan @ 2017-02-22 17:14 UTC (permalink / raw)
To: outreachy-kernel, aditya.shankar, ganesh.krishna, gregkh,
linux-wireless, devel, linux-kernel
Fixes multiple camel case checks on struct tstrRSSI from checkpatch.pl:
Avoid CamelCase: <tstrRSSI>
Avoid CamelCase: <u8Full>
Avoid CamelCase: <u8Index>
Signed-off-by: Tahia Khan <tahia.khan@gmail.com>
---
drivers/staging/wilc1000/coreconfigurator.h | 8 ++++----
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 10 +++++-----
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/wilc1000/coreconfigurator.h b/drivers/staging/wilc1000/coreconfigurator.h
index cff1698..5b65c4f 100644
--- a/drivers/staging/wilc1000/coreconfigurator.h
+++ b/drivers/staging/wilc1000/coreconfigurator.h
@@ -70,9 +70,9 @@ enum connect_status {
CONNECT_STS_FORCE_16_BIT = 0xFFFF
};
-struct tstrRSSI {
- u8 u8Full;
- u8 u8Index;
+struct tstr_RSSI {
+ u8 full;
+ u8 index;
s8 as8RSSI[NUM_RSSI];
};
@@ -93,7 +93,7 @@ struct network_info {
u8 *ies;
u16 ies_len;
void *join_params;
- struct tstrRSSI str_rssi;
+ struct tstr_RSSI str_rssi;
u64 tsf_hi;
};
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index f7ce47c..56f133e 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -205,8 +205,8 @@ static u32 get_rssi_avg(struct network_info *network_info)
{
u8 i;
int rssi_v = 0;
- u8 num_rssi = (network_info->str_rssi.u8Full) ?
- NUM_RSSI : (network_info->str_rssi.u8Index);
+ u8 num_rssi = (network_info->str_rssi.full) ?
+ NUM_RSSI : (network_info->str_rssi.index);
for (i = 0; i < num_rssi; i++)
rssi_v += network_info->str_rssi.as8RSSI[i];
@@ -346,13 +346,13 @@ static void add_network_to_shadow(struct network_info *pstrNetworkInfo,
} else {
ap_index = ap_found;
}
- rssi_index = last_scanned_shadow[ap_index].str_rssi.u8Index;
+ rssi_index = last_scanned_shadow[ap_index].str_rssi.index;
last_scanned_shadow[ap_index].str_rssi.as8RSSI[rssi_index++] = pstrNetworkInfo->rssi;
if (rssi_index == NUM_RSSI) {
rssi_index = 0;
- last_scanned_shadow[ap_index].str_rssi.u8Full = 1;
+ last_scanned_shadow[ap_index].str_rssi.full = 1;
}
- last_scanned_shadow[ap_index].str_rssi.u8Index = rssi_index;
+ last_scanned_shadow[ap_index].str_rssi.index = rssi_index;
last_scanned_shadow[ap_index].rssi = pstrNetworkInfo->rssi;
last_scanned_shadow[ap_index].cap_info = pstrNetworkInfo->cap_info;
last_scanned_shadow[ap_index].ssid_len = pstrNetworkInfo->ssid_len;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2] staging: wilc1000: renames struct tstrRSSI and its members u8Index, u8Full
From: Arend Van Spriel @ 2017-02-22 19:50 UTC (permalink / raw)
To: Tahia Khan, outreachy-kernel, aditya.shankar, ganesh.krishna,
gregkh, linux-wireless, devel, linux-kernel
In-Reply-To: <20170222171403.GA20626@coolbox>
On 22-2-2017 18:14, Tahia Khan wrote:
> Fixes multiple camel case checks on struct tstrRSSI from checkpatch.pl:
>
> Avoid CamelCase: <tstrRSSI>
> Avoid CamelCase: <u8Full>
> Avoid CamelCase: <u8Index>
Just a generic remark that may help you with other changes you will be
making in the linux kernel. Warnings from checkpatch.pl and other tools
are useful, but try to look further than just fixing a warning.
Understand what the code is doing is just as important.
> Signed-off-by: Tahia Khan <tahia.khan@gmail.com>
> ---
> drivers/staging/wilc1000/coreconfigurator.h | 8 ++++----
> drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 10 +++++-----
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/coreconfigurator.h b/drivers/staging/wilc1000/coreconfigurator.h
> index cff1698..5b65c4f 100644
> --- a/drivers/staging/wilc1000/coreconfigurator.h
> +++ b/drivers/staging/wilc1000/coreconfigurator.h
> @@ -70,9 +70,9 @@ enum connect_status {
> CONNECT_STS_FORCE_16_BIT = 0xFFFF
> };
>
> -struct tstrRSSI {
> - u8 u8Full;
> - u8 u8Index;
> +struct tstr_RSSI {
> + u8 full;
> + u8 index;
> s8 as8RSSI[NUM_RSSI];
So you have a struct here with three members and you choose to only
change the first two. What do you think about the third one just by
looking at it. And what about the name of the struct. What does 'tstr' mean?
> };
>
> @@ -93,7 +93,7 @@ struct network_info {
> u8 *ies;
> u16 ies_len;
> void *join_params;
> - struct tstrRSSI str_rssi;
> + struct tstr_RSSI str_rssi;
> u64 tsf_hi;
> };
>
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index f7ce47c..56f133e 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -205,8 +205,8 @@ static u32 get_rssi_avg(struct network_info *network_info)
> {
> u8 i;
> int rssi_v = 0;
> - u8 num_rssi = (network_info->str_rssi.u8Full) ?
> - NUM_RSSI : (network_info->str_rssi.u8Index);
> + u8 num_rssi = (network_info->str_rssi.full) ?
> + NUM_RSSI : (network_info->str_rssi.index);
so struct tstr_RSSI is really a rssi history buffer so maybe naming it
as such makes it clearer, ie. struct rssi_history_buffer.
> for (i = 0; i < num_rssi; i++)
> rssi_v += network_info->str_rssi.as8RSSI[i];
> @@ -346,13 +346,13 @@ static void add_network_to_shadow(struct network_info *pstrNetworkInfo,
> } else {
> ap_index = ap_found;
> }
> - rssi_index = last_scanned_shadow[ap_index].str_rssi.u8Index;
> + rssi_index = last_scanned_shadow[ap_index].str_rssi.index;
> last_scanned_shadow[ap_index].str_rssi.as8RSSI[rssi_index++] = pstrNetworkInfo->rssi;
> if (rssi_index == NUM_RSSI) {
> rssi_index = 0;
> - last_scanned_shadow[ap_index].str_rssi.u8Full = 1;
> + last_scanned_shadow[ap_index].str_rssi.full = 1;
So the 'full' member is actually a bool and you might type it as such.
Hope this helps.
Regards,
Arend
> }
> - last_scanned_shadow[ap_index].str_rssi.u8Index = rssi_index;
> + last_scanned_shadow[ap_index].str_rssi.index = rssi_index;
> last_scanned_shadow[ap_index].rssi = pstrNetworkInfo->rssi;
> last_scanned_shadow[ap_index].cap_info = pstrNetworkInfo->cap_info;
> last_scanned_shadow[ap_index].ssid_len = pstrNetworkInfo->ssid_len;
>
^ permalink raw reply
* Re: [PATCH v2] ath10k: Remove return statement from a void function
From: Arend Van Spriel @ 2017-02-22 19:54 UTC (permalink / raw)
To: Marcin Rokicki, ath10k; +Cc: linux-wireless
In-Reply-To: <1487672891-8584-1-git-send-email-marcin.rokicki@tieto.com>
On 21-2-2017 11:28, Marcin Rokicki wrote:
> The empty 'return;' statement in a void function should be
> used to return from somewhere else than the end.
>
> Signed-off-by: Marcin Rokicki <marcin.rokicki@tieto.com>
> ---
>
> Changes for v2
> -remove only return statement instead of empty err label
> which can be used in the future
>
> ---
> drivers/net/wireless/ath/ath10k/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> index 59729aa..a22d3c9 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -2311,7 +2311,7 @@ static void ath10k_core_register_work(struct work_struct *work)
> /* TODO: It's probably a good idea to release device from the driver
> * but calling device_release_driver() here will cause a deadlock.
> */
> - return;
> + ;
Not exactly what I meant. Just drop the whole line include semicolon.
Regards,
Arend
> }
>
> int ath10k_core_register(struct ath10k *ar, u32 chip_id)
>
^ permalink raw reply
* Re: [PATCH] wireless-regdb: Update rules for Australia (AU) and add 60GHz rules
From: Seth Forshee @ 2017-02-22 21:30 UTC (permalink / raw)
To: Ryan Mounce; +Cc: wireless-regdb, linux-wireless
In-Reply-To: <20170218063515.16071-1-ryan@mounce.com.au>
On Sat, Feb 18, 2017 at 05:05:15PM +1030, Ryan Mounce wrote:
> Sourced from the latest legislation at
> https://www.legislation.gov.au/Details/F2016C0043
This link isn't working for me.
> The current rules exceed legal limits between 5250-5330MHz, and permit
> illegal operation in 5600-5650MHz (reserved for weather radar).
The changes also increases the limits for most other ranges, by quite a
bit in some cases. Seems that this should also be mentioned in the
changelog, and I'd like to be able to check those changes against the
documentation.
Thanks,
Seth
>
> Signed-off-by: Ryan Mounce <ryan@mounce.com.au>
> ---
> db.txt | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/db.txt b/db.txt
> index 05108e0..b951e0c 100644
> --- a/db.txt
> +++ b/db.txt
> @@ -85,12 +85,17 @@ country AT: DFS-ETSI
> # 60 GHz band channels 1-4, ref: Etsi En 302 567
> (57000 - 66000 @ 2160), (40)
>
> +# Source:
> +# https://www.legislation.gov.au/Details/F2016C00432
> +# Both DFS-ETSI and DFS-FCC are acceptable per AS/NZS 4268 Appendix B
> country AU: DFS-ETSI
> - (2402 - 2482 @ 40), (20)
> - (5170 - 5250 @ 80), (17), AUTO-BW
> - (5250 - 5330 @ 80), (24), DFS, AUTO-BW
> - (5490 - 5710 @ 160), (24), DFS
> - (5735 - 5835 @ 80), (30)
> + (2402 - 2482 @ 40), (36)
> + (5170 - 5250 @ 80), (23), NO-OUTDOOR, AUTO-BW
> + (5250 - 5330 @ 80), (23), NO-OUTDOOR, AUTO-BW, DFS
> + (5490 - 5590 @ 80), (30), DFS
> + (5650 - 5730 @ 80), (30), DFS
> + (5735 - 5835 @ 80), (36)
> + (57000 - 66000 @ 2160), (43), NO-OUTDOOR
>
> country AW: DFS-ETSI
> (2402 - 2482 @ 40), (20)
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH] wireless-regdb: Update rules for Australia (AU) and add 60GHz rules
From: Ryan Mounce @ 2017-02-22 23:05 UTC (permalink / raw)
To: Seth Forshee; +Cc: wireless-regdb, linux-wireless
In-Reply-To: <20170222213057.GA33925@ubuntu-hedt>
On 23 February 2017 at 08:00, Seth Forshee <seth.forshee@canonical.com> wrote:
> On Sat, Feb 18, 2017 at 05:05:15PM +1030, Ryan Mounce wrote:
>> Sourced from the latest legislation at
>> https://www.legislation.gov.au/Details/F2016C0043
>
> This link isn't working for me.
Oops, I truncated that URL. Working link (also in the patch itself):
https://www.legislation.gov.au/Details/F2016C00432
>> The current rules exceed legal limits between 5250-5330MHz, and permit
>> illegal operation in 5600-5650MHz (reserved for weather radar).
>
> The changes also increases the limits for most other ranges, by quite a
> bit in some cases. Seems that this should also be mentioned in the
> changelog, and I'd like to be able to check those changes against the
> documentation.
All of the updated power limits are converted to dBm but otherwise
straight out of the document, so they shouldn't be a problem. I'll
update the changelog to note this.
The frequency ranges have been "rounded down" to conform with the
apparent convention of only including ranges used by standard 802.11
channels, rather than using the actual frequency ranges as has been
done for some countries (and most 60GHz rules). My preference is the
latter so that there is a clear correlation between the regdb and
relevant documentation, and so that the regdb may become useful beyond
802.11. With your blessing as maintainer I will also make this change
to the patch.
Regards,
Ryan Mounce
> Thanks,
> Seth
>
>>
>> Signed-off-by: Ryan Mounce <ryan@mounce.com.au>
>> ---
>> db.txt | 15 ++++++++++-----
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/db.txt b/db.txt
>> index 05108e0..b951e0c 100644
>> --- a/db.txt
>> +++ b/db.txt
>> @@ -85,12 +85,17 @@ country AT: DFS-ETSI
>> # 60 GHz band channels 1-4, ref: Etsi En 302 567
>> (57000 - 66000 @ 2160), (40)
>>
>> +# Source:
>> +# https://www.legislation.gov.au/Details/F2016C00432
>> +# Both DFS-ETSI and DFS-FCC are acceptable per AS/NZS 4268 Appendix B
>> country AU: DFS-ETSI
>> - (2402 - 2482 @ 40), (20)
>> - (5170 - 5250 @ 80), (17), AUTO-BW
>> - (5250 - 5330 @ 80), (24), DFS, AUTO-BW
>> - (5490 - 5710 @ 160), (24), DFS
>> - (5735 - 5835 @ 80), (30)
>> + (2402 - 2482 @ 40), (36)
>> + (5170 - 5250 @ 80), (23), NO-OUTDOOR, AUTO-BW
>> + (5250 - 5330 @ 80), (23), NO-OUTDOOR, AUTO-BW, DFS
>> + (5490 - 5590 @ 80), (30), DFS
>> + (5650 - 5730 @ 80), (30), DFS
>> + (5735 - 5835 @ 80), (36)
>> + (57000 - 66000 @ 2160), (43), NO-OUTDOOR
>>
>> country AW: DFS-ETSI
>> (2402 - 2482 @ 40), (20)
>> --
>> 2.11.0
>>
^ permalink raw reply
* [PATCH] rtlwifi: fix spelling mistake: "conuntry" -> "country"
From: Colin King @ 2017-02-22 23:35 UTC (permalink / raw)
To: Larry Finger, Chaoming Li, Kalle Valo, linux-wireless, netdev
Cc: linux-kernel
From: Colin Ian King <colin.king@canonical.com>
trivial fix to spelling mistake in RT_TRACE message
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/realtek/rtlwifi/regd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/regd.c b/drivers/net/wireless/realtek/rtlwifi/regd.c
index 558c31b..1bf3eb2 100644
--- a/drivers/net/wireless/realtek/rtlwifi/regd.c
+++ b/drivers/net/wireless/realtek/rtlwifi/regd.c
@@ -435,7 +435,7 @@ int rtl_regd_init(struct ieee80211_hw *hw,
channel_plan_to_country_code(rtlpriv->efuse.channel_plan);
RT_TRACE(rtlpriv, COMP_REGD, DBG_DMESG,
- "rtl: EEPROM regdomain: 0x%0x conuntry code: %d\n",
+ "rtl: EEPROM regdomain: 0x%0x country code: %d\n",
rtlpriv->efuse.channel_plan, rtlpriv->regd.country_code);
if (rtlpriv->regd.country_code >= COUNTRY_CODE_MAX) {
--
2.10.2
^ permalink raw reply related
* Re: [PATCH v2] staging: wilc1000: renames struct tstrRSSI and its members u8Index, u8Full
From: Tahia Khan @ 2017-02-23 3:54 UTC (permalink / raw)
To: arend.vanspriel, outreachy-kernel, aditya.shankar, ganesh.krishna,
gregkh, linux-wireless, devel, linux-kernel
In-Reply-To: <838d3e67-646d-b2d3-ef7d-5812675db6db@broadcom.com>
On Wed, Feb 22, 2017 at 08:50:31PM +0100, Arend Van Spriel wrote:
> On 22-2-2017 18:14, Tahia Khan wrote:
> > Fixes multiple camel case checks on struct tstrRSSI from checkpatch.pl:
> >
> > Avoid CamelCase: <tstrRSSI>
> > Avoid CamelCase: <u8Full>
> > Avoid CamelCase: <u8Index>
>
> Just a generic remark that may help you with other changes you will be
> making in the linux kernel. Warnings from checkpatch.pl and other tools
> are useful, but try to look further than just fixing a warning.
> Understand what the code is doing is just as important.
>
> > Signed-off-by: Tahia Khan <tahia.khan@gmail.com>
> > ---
> > drivers/staging/wilc1000/coreconfigurator.h | 8 ++++----
> > drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 10 +++++-----
> > 2 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/wilc1000/coreconfigurator.h b/drivers/staging/wilc1000/coreconfigurator.h
> > index cff1698..5b65c4f 100644
> > --- a/drivers/staging/wilc1000/coreconfigurator.h
> > +++ b/drivers/staging/wilc1000/coreconfigurator.h
> > @@ -70,9 +70,9 @@ enum connect_status {
> > CONNECT_STS_FORCE_16_BIT = 0xFFFF
> > };
> >
> > -struct tstrRSSI {
> > - u8 u8Full;
> > - u8 u8Index;
> > +struct tstr_RSSI {
> > + u8 full;
> > + u8 index;
> > s8 as8RSSI[NUM_RSSI];
>
> So you have a struct here with three members and you choose to only
> change the first two. What do you think about the third one just by
> looking at it. And what about the name of the struct. What does 'tstr' mean?
>
> > };
> >
> > @@ -93,7 +93,7 @@ struct network_info {
> > u8 *ies;
> > u16 ies_len;
> > void *join_params;
> > - struct tstrRSSI str_rssi;
> > + struct tstr_RSSI str_rssi;
> > u64 tsf_hi;
> > };
> >
> > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > index f7ce47c..56f133e 100644
> > --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > @@ -205,8 +205,8 @@ static u32 get_rssi_avg(struct network_info *network_info)
> > {
> > u8 i;
> > int rssi_v = 0;
> > - u8 num_rssi = (network_info->str_rssi.u8Full) ?
> > - NUM_RSSI : (network_info->str_rssi.u8Index);
> > + u8 num_rssi = (network_info->str_rssi.full) ?
> > + NUM_RSSI : (network_info->str_rssi.index);
>
> so struct tstr_RSSI is really a rssi history buffer so maybe naming it
> as such makes it clearer, ie. struct rssi_history_buffer.
>
> > for (i = 0; i < num_rssi; i++)
> > rssi_v += network_info->str_rssi.as8RSSI[i];
> > @@ -346,13 +346,13 @@ static void add_network_to_shadow(struct network_info *pstrNetworkInfo,
> > } else {
> > ap_index = ap_found;
> > }
> > - rssi_index = last_scanned_shadow[ap_index].str_rssi.u8Index;
> > + rssi_index = last_scanned_shadow[ap_index].str_rssi.index;
> > last_scanned_shadow[ap_index].str_rssi.as8RSSI[rssi_index++] = pstrNetworkInfo->rssi;
> > if (rssi_index == NUM_RSSI) {
> > rssi_index = 0;
> > - last_scanned_shadow[ap_index].str_rssi.u8Full = 1;
> > + last_scanned_shadow[ap_index].str_rssi.full = 1;
>
> So the 'full' member is actually a bool and you might type it as such.
>
> Hope this helps.
>
> Regards,
> Arend
>
> > }
> > - last_scanned_shadow[ap_index].str_rssi.u8Index = rssi_index;
> > + last_scanned_shadow[ap_index].str_rssi.index = rssi_index;
> > last_scanned_shadow[ap_index].rssi = pstrNetworkInfo->rssi;
> > last_scanned_shadow[ap_index].cap_info = pstrNetworkInfo->cap_info;
> > last_scanned_shadow[ap_index].ssid_len = pstrNetworkInfo->ssid_len;
> >
Thanks for the feedback Arend, I really appreciate it. I've decided to go with
these changes in my follow-up patch request:
- rename tstrRSSI to 'rssi_history_buffer' as Aren suggested since it makes the
purpose of the struct clear
- remove Hungarian notation from all tstrRSSI members' names
- change type of u8Full to bool since it's only ever 1 or 0
- change name of as8RSSI to 'samples' since this buffer is only ever used to
compute an average, and the "rssi" prefix is implied by the struct's name
- rename str_rssi to rssi_history in the network_info struct for clarity
Since my reasoning for these changes deviates from just "renaming to
avoid camel casing" (as in the original checkpatch.pl warning), would it still
make sense to submit all this in a single patch? I know my commit message
needs to change but I wonder if this is too much detail.
Tahia
^ permalink raw reply
* Re: [PATCH] wireless-regdb: Update rules for Australia (AU) and add 60GHz rules
From: Johannes Berg @ 2017-02-23 5:58 UTC (permalink / raw)
To: Ryan Mounce, Seth Forshee; +Cc: wireless-regdb, linux-wireless
In-Reply-To: <CAN+fvRb8R_3QyrnFFp0gd8PCcFXwbitNyh8K=ONqbmsGTrBsHQ@mail.gmail.com>
> The frequency ranges have been "rounded down" to conform with the
> apparent convention of only including ranges used by standard 802.11
> channels, rather than using the actual frequency ranges as has been
> done for some countries (and most 60GHz rules). My preference is the
> latter so that there is a clear correlation between the regdb and
> relevant documentation, and so that the regdb may become useful
> beyond 802.11. With your blessing as maintainer I will also make this
> change to the patch.
I think that's just an artifact of the original database source and
would also prefer the documented ranges to be used.
johannes
^ permalink raw reply
* Re: [PATCH v2] staging: wilc1000: renames struct tstrRSSI and its members u8Index, u8Full
From: Joe Perches @ 2017-02-23 6:24 UTC (permalink / raw)
To: Arend Van Spriel, Tahia Khan, outreachy-kernel, aditya.shankar,
ganesh.krishna, gregkh, linux-wireless, devel, linux-kernel
In-Reply-To: <838d3e67-646d-b2d3-ef7d-5812675db6db@broadcom.com>
On Wed, 2017-02-22 at 20:50 +0100, Arend Van Spriel wrote:
> On 22-2-2017 18:14, Tahia Khan wrote:
> > Fixes multiple camel case checks on struct tstrRSSI from checkpatch.pl:
[]
> Just a generic remark that may help you with other changes you will be
> making in the linux kernel. Warnings from checkpatch.pl and other tools
> are useful, but try to look further than just fixing a warning.
> Understand what the code is doing is just as important.
I'd assert understanding what the code is doing is
_more_ important. Style consistency simply helps
improve the speed of a new reader's understanding.
^ permalink raw reply
* Re: [Outreachy kernel] Re: [PATCH v2] staging: wilc1000: renames struct tstrRSSI and its members u8Index, u8Full
From: Julia Lawall @ 2017-02-23 7:08 UTC (permalink / raw)
To: Tahia Khan
Cc: arend.vanspriel, outreachy-kernel, aditya.shankar, ganesh.krishna,
gregkh, linux-wireless, devel, linux-kernel
In-Reply-To: <20170223035400.GA9378@coolbox>
> Thanks for the feedback Arend, I really appreciate it. I've decided to go with
> these changes in my follow-up patch request:
>
> - rename tstrRSSI to 'rssi_history_buffer' as Aren suggested since it makes the
> purpose of the struct clear
> - remove Hungarian notation from all tstrRSSI members' names
> - change type of u8Full to bool since it's only ever 1 or 0
> - change name of as8RSSI to 'samples' since this buffer is only ever used to
> compute an average, and the "rssi" prefix is implied by the struct's name
> - rename str_rssi to rssi_history in the network_info struct for clarity
>
> Since my reasoning for these changes deviates from just "renaming to
> avoid camel casing" (as in the original checkpatch.pl warning), would it still
> make sense to submit all this in a single patch? I know my commit message
> needs to change but I wonder if this is too much detail.
I would strongly suggest not to do it all in a single patch. Even if these
changes are not very complicated conceptually, there is always a chance of
doing things wrong. Taking the problems one by one will improve the chance
that the result is correct. Also, the results will be easier for you and
others to review if each patch only does one thing. And easier to revert
if needed later if something goes wrong.
julia
^ permalink raw reply
* Re: [Outreachy kernel] Re: [PATCH v2] staging: wilc1000: renames struct tstrRSSI and its members u8Index, u8Full
From: Arend Van Spriel @ 2017-02-23 8:56 UTC (permalink / raw)
To: Julia Lawall, Tahia Khan
Cc: outreachy-kernel, aditya.shankar, ganesh.krishna, gregkh,
linux-wireless, devel, linux-kernel
In-Reply-To: <alpine.DEB.2.20.1702230804410.2203@hadrien>
On 23-2-2017 8:08, Julia Lawall wrote:
>> Thanks for the feedback Arend, I really appreciate it. I've decided to go with
>> these changes in my follow-up patch request:
>>
>> - rename tstrRSSI to 'rssi_history_buffer' as Aren suggested since it makes the
>> purpose of the struct clear
>> - remove Hungarian notation from all tstrRSSI members' names
>> - change type of u8Full to bool since it's only ever 1 or 0
>> - change name of as8RSSI to 'samples' since this buffer is only ever used to
>> compute an average, and the "rssi" prefix is implied by the struct's name
>> - rename str_rssi to rssi_history in the network_info struct for clarity
>>
>> Since my reasoning for these changes deviates from just "renaming to
>> avoid camel casing" (as in the original checkpatch.pl warning), would it still
>> make sense to submit all this in a single patch? I know my commit message
>> needs to change but I wonder if this is too much detail.
>
> I would strongly suggest not to do it all in a single patch. Even if these
> changes are not very complicated conceptually, there is always a chance of
> doing things wrong. Taking the problems one by one will improve the chance
> that the result is correct. Also, the results will be easier for you and
> others to review if each patch only does one thing. And easier to revert
> if needed later if something goes wrong.
It is all related to cleaning up stuff in a single struct which I
consider "one thing" here. To me it looks a bit silly if you rename one
struct member when it is obvious that the other two need to be renamed
as well. The only somewhat sensible split I see here is: 1) rename the
struct itself, 2) rename the struct members, and 3) rename str_rssi
member in struct network_info.
Regards,
Arend
^ permalink raw reply
* Re: [RFC v2 1/2] cfg80211: support 4-way handshake offloading for WPA/WPA2-PSK
From: Arend Van Spriel @ 2017-02-23 9:56 UTC (permalink / raw)
To: Johannes Berg, Jouni Malinen; +Cc: linux-wireless, Eliad Peller
In-Reply-To: <1487688447.15350.4.camel@sipsolutions.net>
On 21-2-2017 15:47, Johannes Berg wrote:
>
>> The .len verifies that it's at least that long. We're thus ignoring
>> additional bytes in the PSK case if they're present, which I suppose
>> we should fix by checking the exact length in the code separately.
>>
libnl seems to have the notion of min_len and max_len in its policy
definition, but the kernel does not so .len is actually min_len indeed.
> IOW, I'll add this:
>
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -8044,6 +8044,8 @@ static int nl80211_crypto_settings(struct cfg80211_registered_device *rdev,
> if (!wiphy_ext_feature_isset(&rdev->wiphy,
> NL80211_EXT_FEATURE_4WAY_HANDSHAKE_STA_PSK))
> return -EINVAL;
> + if (nla_len(info->attrs[NL80211_ATTR_PMK]) != WLAN_PMK_LEN)
> + return -EINVAL;
Makes sense.
Regards,
Arend
> settings->psk = nla_data(info->attrs[NL80211_ATTR_PMK]);
> }
>
>
> johannes
>
^ permalink raw reply
* Re: [PATCH] cfg80211: support 4-way handshake offloading for WPA/WPA2-PSK
From: Arend Van Spriel @ 2017-02-23 10:40 UTC (permalink / raw)
To: Jithu Jance; +Cc: Johannes Berg, linux-wireless, Eliad Peller, Jouni Malinen
In-Reply-To: <9b6de09f-d872-fd67-971a-6da369fefe1a@broadcom.com>
On 23-2-2017 11:36, Arend Van Spriel wrote:
> On 22-2-2017 13:24, Jithu Jance wrote:
>> Hi Jouni, Johannes et al.,
>>
>> On Wed, Feb 22, 2017 at 1:27 AM, Arend Van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>> On 21-2-2017 13:37, Johannes Berg wrote:
>>>>
>>>>> What I am also missing is how to deal with NL80211_CMD_CONNECTED
>>>>> event signalling. Should that be given after (un)successful
>>>>> completion of the 4-way handshake or should we have a separate event
>>>>> for that?
>>>>
>>>> That's a good point. I don't think a new event really works well, but
>>>> perhaps we should extend the NL80211_TIMEOUT_* values with this? It's
>>>> not strictly a timeout, I guess, but it's not a direct rejection of the
>>>> association either.
>>>>
>>>> Any thoughts?
>>>
>>> I discussed about the event mechanism with Jithu which is who asked me
>>> to look at their patches for this feature. His suggestion was to have a
>>> "port authorized" event after successful handshake as it will help in
>>> key mgmt offload cases. He gave the following example: Suppose you are
>>> doing a fresh 8021x followed by 4way handshake offload. Right now there
>>> is no event to indicate to the upper layer that the 4way handshake have
>>> been completed successfully. So the only thing to do in wpa_supplicant
>>> is to consider it completed and if not be hit with deauth.
>>
>> In wpa_supplicant terms, I was looking for an event notification to move the
>> wpa_state from WPA_ASSOCIATED to WPA_COMPLETED. :)
>
> Currently in wpa_supplicant_event_assoc() the 4-way handshake offload is
> handled as below:
>
> if ((wpa_s->drv_flags & WPA_DRIVER_FLAGS_4WAY_HANDSHAKE) &&
> wpa_key_mgmt_wpa_psk(wpa_s->key_mgmt)) {
> /*
> * We are done; the driver will take care of RSN 4-way
> * handshake.
> */
> wpa_supplicant_cancel_auth_timeout(wpa_s);
> wpa_supplicant_set_state(wpa_s, WPA_COMPLETED);
> eapol_sm_notify_portValid(wpa_s->eapol, TRUE);
> eapol_sm_notify_eap_success(wpa_s->eapol, TRUE);
> } else if ((wpa_s->drv_flags & WPA_DRIVER_FLAGS_4WAY_HANDSHAKE) &&
> wpa_key_mgmt_wpa_ieee8021x(wpa_s->key_mgmt)) {
> /*
> * The driver will take care of RSN 4-way handshake, so
> we need
> * to allow EAPOL supplicant to complete its work without
> * waiting for WPA supplicant.
> */
> eapol_sm_notify_portValid(wpa_s->eapol, TRUE);
> } else if (ft_completed) {
>
> So for 4-way hs offload for wpa/wpa2-psk it already calls
> eapol_sm_notify_eap_success() and reading the documentation of that
> function:
>
> /**
> * eapol_sm_notify_eap_success - Notification of external EAP success
> trigger
> * @sm: Pointer to EAPOL state machine allocated with eapol_sm_init()
> * @success: %TRUE = set success, %FALSE = clear success
> *
> * Notify the EAPOL state machine that external event has forced EAP
> state to
> * success (success = %TRUE). This can be cleared by setting success =
> %FALSE.
> *
> * This function is called to update EAP state when WPA-PSK key
> handshake has
> * been completed successfully since WPA-PSK does not use EAP state machine.
> */
> void eapol_sm_notify_eap_success(struct eapol_sm *sm, Boolean success)
>
> So the code in wpa_supplicant_event_assoc() is either lying or the assoc
> event should come after driver has completed successfully. To me it
> seems worth it to have NL80211_CMD_EAP_COMPLETED event carrying the
> result or indeed give the CONNECTED event later in the driver.
Ehm. Looking at the code in wpa_supplicant_event_assoc() it would be
better to use NL80211_CMD_EAPOL_PORT_VALID event to cover both
WPA/WPA2-PSK and 8021X.
Regards,
Arend
> Regards,
> Arend
>
^ permalink raw reply
* Re: [PATCH] cfg80211: support 4-way handshake offloading for WPA/WPA2-PSK
From: Arend Van Spriel @ 2017-02-23 10:36 UTC (permalink / raw)
To: Jithu Jance; +Cc: Johannes Berg, linux-wireless, Eliad Peller, Jouni Malinen
In-Reply-To: <CA+gyVQ11yZjkJXjiVoMVijs=_K8itZMQrces9XQha_bT4=H-DA@mail.gmail.com>
On 22-2-2017 13:24, Jithu Jance wrote:
> Hi Jouni, Johannes et al.,
>
> On Wed, Feb 22, 2017 at 1:27 AM, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 21-2-2017 13:37, Johannes Berg wrote:
>>>
>>>> What I am also missing is how to deal with NL80211_CMD_CONNECTED
>>>> event signalling. Should that be given after (un)successful
>>>> completion of the 4-way handshake or should we have a separate event
>>>> for that?
>>>
>>> That's a good point. I don't think a new event really works well, but
>>> perhaps we should extend the NL80211_TIMEOUT_* values with this? It's
>>> not strictly a timeout, I guess, but it's not a direct rejection of the
>>> association either.
>>>
>>> Any thoughts?
>>
>> I discussed about the event mechanism with Jithu which is who asked me
>> to look at their patches for this feature. His suggestion was to have a
>> "port authorized" event after successful handshake as it will help in
>> key mgmt offload cases. He gave the following example: Suppose you are
>> doing a fresh 8021x followed by 4way handshake offload. Right now there
>> is no event to indicate to the upper layer that the 4way handshake have
>> been completed successfully. So the only thing to do in wpa_supplicant
>> is to consider it completed and if not be hit with deauth.
>
> In wpa_supplicant terms, I was looking for an event notification to move the
> wpa_state from WPA_ASSOCIATED to WPA_COMPLETED. :)
Currently in wpa_supplicant_event_assoc() the 4-way handshake offload is
handled as below:
if ((wpa_s->drv_flags & WPA_DRIVER_FLAGS_4WAY_HANDSHAKE) &&
wpa_key_mgmt_wpa_psk(wpa_s->key_mgmt)) {
/*
* We are done; the driver will take care of RSN 4-way
* handshake.
*/
wpa_supplicant_cancel_auth_timeout(wpa_s);
wpa_supplicant_set_state(wpa_s, WPA_COMPLETED);
eapol_sm_notify_portValid(wpa_s->eapol, TRUE);
eapol_sm_notify_eap_success(wpa_s->eapol, TRUE);
} else if ((wpa_s->drv_flags & WPA_DRIVER_FLAGS_4WAY_HANDSHAKE) &&
wpa_key_mgmt_wpa_ieee8021x(wpa_s->key_mgmt)) {
/*
* The driver will take care of RSN 4-way handshake, so
we need
* to allow EAPOL supplicant to complete its work without
* waiting for WPA supplicant.
*/
eapol_sm_notify_portValid(wpa_s->eapol, TRUE);
} else if (ft_completed) {
So for 4-way hs offload for wpa/wpa2-psk it already calls
eapol_sm_notify_eap_success() and reading the documentation of that
function:
/**
* eapol_sm_notify_eap_success - Notification of external EAP success
trigger
* @sm: Pointer to EAPOL state machine allocated with eapol_sm_init()
* @success: %TRUE = set success, %FALSE = clear success
*
* Notify the EAPOL state machine that external event has forced EAP
state to
* success (success = %TRUE). This can be cleared by setting success =
%FALSE.
*
* This function is called to update EAP state when WPA-PSK key
handshake has
* been completed successfully since WPA-PSK does not use EAP state machine.
*/
void eapol_sm_notify_eap_success(struct eapol_sm *sm, Boolean success)
So the code in wpa_supplicant_event_assoc() is either lying or the assoc
event should come after driver has completed successfully. To me it
seems worth it to have NL80211_CMD_EAP_COMPLETED event carrying the
result or indeed give the CONNECTED event later in the driver.
Regards,
Arend
^ permalink raw reply
* [bug report] NFC Digital: Add target NFC-DEP support
From: Dan Carpenter @ 2017-02-23 10:45 UTC (permalink / raw)
To: thierry.escande; +Cc: linux-wireless
Hello Thierry Escande,
The patch 1c7a4c24fbfd: "NFC Digital: Add target NFC-DEP support"
from Sep 19, 2013, leads to the following static checker warning:
net/nfc/digital_dep.c:1303 digital_tg_recv_dep_req()
error: double free of 'resp'
net/nfc/digital_dep.c
1287
1288 goto free_resp;
1289 }
1290
1291 rc = nfc_tm_data_received(ddev->nfc_dev, resp);
This function does a kfree_skb() on the error path. I don't know about
the success path. Other code seems to assume it frees on success so
maybe?
1292
1293 exit:
1294 kfree_skb(ddev->chaining_skb);
1295 ddev->chaining_skb = NULL;
1296
1297 ddev->atn_count = 0;
1298
1299 kfree_skb(ddev->saved_skb);
1300 ddev->saved_skb = NULL;
1301
1302 if (rc)
1303 kfree_skb(resp);
Of course kfree_skb() is refcounted but I think this has to be a bug.
1304
1305 return;
1306
1307 free_resp:
1308 dev_kfree_skb(resp);
But then we do dev_kfree_skb() here. It's not clear to me why sometimes
we use regular kfree_skb() but not here.
1309 }
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] cfg80211: support 4-way handshake offloading for WPA/WPA2-PSK
From: Johannes Berg @ 2017-02-23 10:37 UTC (permalink / raw)
To: Arend Van Spriel, Jithu Jance; +Cc: linux-wireless, Eliad Peller, Jouni Malinen
In-Reply-To: <9b6de09f-d872-fd67-971a-6da369fefe1a@broadcom.com>
> So the code in wpa_supplicant_event_assoc() is either lying or the
> assoc event should come after driver has completed successfully. To
> me it seems worth it to have NL80211_CMD_EAP_COMPLETED event carrying
> the result or indeed give the CONNECTED event later in the driver.
Sorry - need to look at this in more detail later.
But yes, we do give CONNECTED event later in the driver right now.
johannes
^ permalink raw reply
* Re: [PATCH] cfg80211: support 4-way handshake offloading for WPA/WPA2-PSK
From: Jithu Jance @ 2017-02-23 10:56 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Johannes Berg, linux-wireless, Eliad Peller, Jouni Malinen
In-Reply-To: <1248c04b-a643-9254-383c-707ab68c0815@broadcom.com>
On Thu, Feb 23, 2017 at 4:10 PM, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> Ehm. Looking at the code in wpa_supplicant_event_assoc() it would be
> better to use NL80211_CMD_EAPOL_PORT_VALID event to cover both
> WPA/WPA2-PSK and 8021X.
Yes. IMHO, the assoc/reassoc ind should move the state to WPA_ASSOCIATED
and a separate event like NL80211_CMD_EAPOL_PORT_VALID/AUTHORIZED should
move the connection state to WPA_COMPLETED.
Thanks,
--
Jithu Jance
^ permalink raw reply
* [PATCH 2/2][RFC] mac80211_hwsim: Report radio addresses in NEW_RADIO/GET_RADIO
From: Andrew Zaborowski @ 2017-02-23 12:02 UTC (permalink / raw)
To: linux-wireless
In-Reply-To: <20170223120211.22358-1-andrew.zaborowski@intel.com>
Add a HWSIM_ATTR_RADIO_ADDR attribute to those to events/response so
that a userspace medium can query the list of addresses in the
simulator.
When a userspace medium needs to handle a broadcast frame it can't
easily implement the equivalent of what the default kernel medium does
because it doesn't know who to forward the frame to. The
HWSIM_CMD_GET_RADIO command is a little useless in this respect.
Userspace needs to use HWSIM_CMD_GET_RADIO, then cross-reference the
radio names with wiphy names using NL80211_CMD_GET_WIPHY dump, map the
wiphy names to wiphy indexes (those are more useful than wiphy names
because they don't change -- a wiphy name change doesn't generate an
event so this can't be easily tracked), and use the
NL80211_CMD_GET_INTERFACES dump to obtain the wiphy_idx to mac address
mapping.
Additionally there are two addresses used by hwsim radios
(data->addresses[0] and [1]), first used at the network level and the
second used at hwsim radio level and in HWSIM_ATTR_ADDR_TRANSMITTER /
_RECEIVER. By default they differ by 0x40 in the first byte but I'm not
sure userspace can make this assumption to do the mapping from the
address inside the frame headers to the address of the hwsim receiving
radio. I suspect the network layers can change address 0 and they will
be out of sync.
Signed-off-by: Andrew Zaborowski <andrew.zaborowski@intel.com>
---
Additionally I tried to add a HWSIM_ATTR_WIPHY to report the wiphy index
directly without users going through wiphy name to index mapping, but
get_wiphy_idx() is internal to cfg80211. The index is exposed to
userspace and is more useful than the name so I wonder if this function
should be exported from cfg80211.
---
drivers/net/wireless/mac80211_hwsim.c | 16 ++++++++++++++++
drivers/net/wireless/mac80211_hwsim.h | 2 ++
2 files changed, 18 insertions(+)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index ba4978d..dcc1df0 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -2305,6 +2305,7 @@ struct hwsim_new_radio_params {
bool destroy_on_close;
const char *hwname;
bool no_vif;
+ const struct mac_address *addrs;
};
static void hwsim_mcast_config_msg(struct sk_buff *mcast_skb,
@@ -2318,10 +2319,15 @@ static void hwsim_mcast_config_msg(struct sk_buff *mcast_skb,
HWSIM_MCGRP_CONFIG, GFP_KERNEL);
}
+#define HWSIM_ADDR_COUNT \
+ ARRAY_SIZE(((struct mac80211_hwsim_data *) NULL)->addresses)
+
static int append_radio_msg(struct sk_buff *skb, int id,
struct hwsim_new_radio_params *param)
{
int ret;
+ int i;
+ uint8_t addr_buf[HWSIM_ADDR_COUNT * ETH_ALEN];
ret = nla_put_u32(skb, HWSIM_ATTR_RADIO_ID, id);
if (ret < 0)
@@ -2379,6 +2385,13 @@ static int append_radio_msg(struct sk_buff *skb, int id,
return ret;
}
+ for (i = 0; i < HWSIM_ADDR_COUNT; i++)
+ memcpy(addr_buf + i * ETH_ALEN, param->addrs[i].addr, ETH_ALEN);
+
+ ret = nla_put(skb, HWSIM_ATTR_RADIO_ADDR, sizeof(addr_buf), addr_buf);
+ if (ret < 0)
+ return ret;
+
return 0;
}
@@ -2682,6 +2695,8 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
list_add_tail(&data->list, &hwsim_radios);
spin_unlock_bh(&hwsim_radio_lock);
+ param->addrs = data->addresses;
+
if (idx > 0)
hwsim_mcast_new_radio(idx, info, param);
@@ -2772,6 +2787,7 @@ static int mac80211_hwsim_get_radio(struct sk_buff *skb,
param.regd = data->regd;
param.channels = data->channels;
param.hwname = wiphy_name(data->hw->wiphy);
+ param.addrs = data->addresses;
res = append_radio_msg(skb, data->idx, ¶m);
if (res < 0)
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index 39f2246..53b4cd7 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -123,6 +123,7 @@ enum {
* @HWSIM_ATTR_RADIO_NAME: Name of radio, e.g. phy666
* @HWSIM_ATTR_NO_VIF: Do not create vif (wlanX) when creating radio.
* @HWSIM_ATTR_FREQ: Frequency at which packet is transmitted or received.
+ * @HWSIM_ATTR_RADIO_ADDR: Radio hardware addresses as an array
* @__HWSIM_ATTR_MAX: enum limit
*/
@@ -149,6 +150,7 @@ enum {
HWSIM_ATTR_NO_VIF,
HWSIM_ATTR_FREQ,
HWSIM_ATTR_PAD,
+ HWSIM_ATTR_RADIO_ADDR,
__HWSIM_ATTR_MAX,
};
#define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
--
2.9.3
^ permalink raw reply related
* [PATCH 1/2] mac80211_hwsim: Make sure NEW_RADIO contains final name
From: Andrew Zaborowski @ 2017-02-23 12:02 UTC (permalink / raw)
To: linux-wireless
ieee80211_alloc_hw_nm will validate the requested name (if any) before
creating the new device and may use a name different from the one
requested rather than fail. Make sure the HWSIM_CMD_NEW_RADIO
event/response generated has the final name or userspace will receive
the wrong name. Note that mac80211_hwsim_new_radio may now modify
params.
A check for duplicate radio name could be added separately.
Signed-off-by: Andrew Zaborowski <andrew.zaborowski@intel.com>
---
drivers/net/wireless/mac80211_hwsim.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 0150747..ba4978d 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -2438,6 +2438,9 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
goto failed;
}
+ /* ieee80211_alloc_hw_nm may have used a default name */
+ param->hwname = wiphy_name(hw->wiphy);
+
if (info)
net = genl_info_net(info);
else
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v2] ath10k: Remove return statement from a void function
From: Joe Perches @ 2017-02-23 12:54 UTC (permalink / raw)
To: Arend Van Spriel, Marcin Rokicki, ath10k; +Cc: linux-wireless
In-Reply-To: <b7ac6892-4cbd-bd60-382f-5c51e9ae588a@broadcom.com>
On Wed, 2017-02-22 at 20:54 +0100, Arend Van Spriel wrote:
>
> On 21-2-2017 11:28, Marcin Rokicki wrote:
> > The empty 'return;' statement in a void function should be
> > used to return from somewhere else than the end.
> >
> > Signed-off-by: Marcin Rokicki <marcin.rokicki@tieto.com>
> > ---
> >
> > Changes for v2
> > -remove only return statement instead of empty err label
> > which can be used in the future
> >
> > ---
> > drivers/net/wireless/ath/ath10k/core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> > index 59729aa..a22d3c9 100644
> > --- a/drivers/net/wireless/ath/ath10k/core.c
> > +++ b/drivers/net/wireless/ath/ath10k/core.c
> > @@ -2311,7 +2311,7 @@ static void ath10k_core_register_work(struct work_struct *work)
> > /* TODO: It's probably a good idea to release device from the driver
> > * but calling device_release_driver() here will cause a deadlock.
> > */
> > - return;
> > + ;
>
> Not exactly what I meant. Just drop the whole line include semicolon.
gcc doesn't support a goto label without a
statement immediately before the end-of-function.
ie:
void foo(void)
{
goto err;
err:
}
fails to compile.
^ permalink raw reply
* [PATCHv2] ath10k: Update available channel list for 5G radio
From: c_traja @ 2017-02-23 13:18 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Tamizh chelvam
From: Tamizh chelvam <c_traja@qti.qualcomm.com>
If a 5 GHz radio is calibrated for operation in both
the low band (channels 36 to 64) and high band(channels 100 to 169),
hardware allows operations in all the listed channels. However,
if the chip has been calibrated only for the low/high band and
a high/low band channel is configured, due to lack of calibration
there will be potentially invalid signal on those non calibrated channels.
To avoid this problem this patch sets IEEE80211_CHAN_DISABLED flag for
those non calibrated channels by using low_5ghz_chan and high_5ghz_chan
values which we get from target through wmi service ready event.
Driver initialized flags are getting re initialized in handle_channel
in cfg80211. So calling the function to disable the non supported channel
from reg_notifier().
Signed-off-by: Tamizh chelvam <c_traja@qti.qualcomm.com>
---
v2:
* Removed two separate arrays for channel list, and changed logic
to disable non calibrated channels.
drivers/net/wireless/ath/ath10k/core.h | 2 ++
drivers/net/wireless/ath/ath10k/mac.c | 19 +++++++++++++++++++
drivers/net/wireless/ath/ath10k/wmi.c | 6 ++++++
drivers/net/wireless/ath/ath10k/wmi.h | 2 ++
4 files changed, 29 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 88d14be..d4b9a0e 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -775,6 +775,8 @@ struct ath10k {
u32 num_rf_chains;
u32 max_spatial_stream;
/* protected by conf_mutex */
+ u32 low_5ghz_chan;
+ u32 high_5ghz_chan;
bool ani_enabled;
bool p2p;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index abc291d..c5b6c2a 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3126,6 +3126,21 @@ static void ath10k_regd_update(struct ath10k *ar)
ath10k_warn(ar, "failed to set pdev regdomain: %d\n", ret);
}
+void ath10k_mac_update_channel_list(struct ath10k *ar,
+ struct ieee80211_supported_band *band)
+{
+ int i;
+
+ if (ar->low_5ghz_chan && ar->high_5ghz_chan) {
+ for (i = 0; i < band->n_channels; i++) {
+ if (band->channels[i].center_freq < ar->low_5ghz_chan ||
+ band->channels[i].center_freq > ar->high_5ghz_chan)
+ band->channels[i].flags |=
+ IEEE80211_CHAN_DISABLED;
+ }
+ }
+}
+
static void ath10k_reg_notifier(struct wiphy *wiphy,
struct regulatory_request *request)
{
@@ -3149,6 +3164,10 @@ static void ath10k_reg_notifier(struct wiphy *wiphy,
if (ar->state == ATH10K_STATE_ON)
ath10k_regd_update(ar);
mutex_unlock(&ar->conf_mutex);
+
+ if (ar->phy_capability & WHAL_WLAN_11A_CAPABILITY)
+ ath10k_mac_update_channel_list(ar,
+ ar->hw->wiphy->bands[NL80211_BAND_5GHZ]);
}
/***************/
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 2f1743e..b31c4f7 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -4593,6 +4593,8 @@ static int ath10k_wmi_alloc_host_mem(struct ath10k *ar, u32 req_id,
arg->phy_capab = ev->phy_capability;
arg->num_rf_chains = ev->num_rf_chains;
arg->eeprom_rd = ev->hal_reg_capabilities.eeprom_rd;
+ arg->low_5ghz_chan = ev->hal_reg_capabilities.low_5ghz_chan;
+ arg->high_5ghz_chan = ev->hal_reg_capabilities.high_5ghz_chan;
arg->num_mem_reqs = ev->num_mem_reqs;
arg->service_map = ev->wmi_service_bitmap;
arg->service_map_len = sizeof(ev->wmi_service_bitmap);
@@ -4629,6 +4631,8 @@ static int ath10k_wmi_alloc_host_mem(struct ath10k *ar, u32 req_id,
arg->phy_capab = ev->phy_capability;
arg->num_rf_chains = ev->num_rf_chains;
arg->eeprom_rd = ev->hal_reg_capabilities.eeprom_rd;
+ arg->low_5ghz_chan = ev->hal_reg_capabilities.low_5ghz_chan;
+ arg->high_5ghz_chan = ev->hal_reg_capabilities.high_5ghz_chan;
arg->num_mem_reqs = ev->num_mem_reqs;
arg->service_map = ev->wmi_service_bitmap;
arg->service_map_len = sizeof(ev->wmi_service_bitmap);
@@ -4682,6 +4686,8 @@ static void ath10k_wmi_event_service_ready_work(struct work_struct *work)
ar->phy_capability = __le32_to_cpu(arg.phy_capab);
ar->num_rf_chains = __le32_to_cpu(arg.num_rf_chains);
ar->hw_eeprom_rd = __le32_to_cpu(arg.eeprom_rd);
+ ar->low_5ghz_chan = __le32_to_cpu(arg.low_5ghz_chan);
+ ar->high_5ghz_chan = __le32_to_cpu(arg.high_5ghz_chan);
ath10k_dbg_dump(ar, ATH10K_DBG_WMI, NULL, "wmi svc: ",
arg.service_map, arg.service_map_len);
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 386aa51..78350d4 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -6335,6 +6335,8 @@ struct wmi_svc_rdy_ev_arg {
__le32 num_rf_chains;
__le32 eeprom_rd;
__le32 num_mem_reqs;
+ __le32 low_5ghz_chan;
+ __le32 high_5ghz_chan;
const __le32 *service_map;
size_t service_map_len;
const struct wlan_host_mem_req *mem_reqs[WMI_MAX_MEM_REQS];
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH] wireless-regdb: Update rules for Australia (AU) and add 60GHz rules
From: Seth Forshee @ 2017-02-23 13:01 UTC (permalink / raw)
To: Johannes Berg; +Cc: Ryan Mounce, wireless-regdb, linux-wireless
In-Reply-To: <1487829525.1956.2.camel@sipsolutions.net>
On Thu, Feb 23, 2017 at 06:58:45AM +0100, Johannes Berg wrote:
>
> > The frequency ranges have been "rounded down" to conform with the
> > apparent convention of only including ranges used by standard 802.11
> > channels, rather than using the actual frequency ranges as has been
> > done for some countries (and most 60GHz rules). My preference is the
> > latter so that there is a clear correlation between the regdb and
> > relevant documentation, and so that the regdb may become useful
> > beyond 802.11. With your blessing as maintainer I will also make this
> > change to the patch.
>
> I think that's just an artifact of the original database source and
> would also prefer the documented ranges to be used.
Yes, I prefer the documented ranges as well.
Thanks,
Seth
^ permalink raw reply
* Re: [Outreachy kernel] Re: [PATCH v2] staging: wilc1000: renames struct tstrRSSI and its members u8Index, u8Full
From: Julia Lawall @ 2017-02-23 13:38 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Julia Lawall, Tahia Khan, outreachy-kernel, aditya.shankar,
ganesh.krishna, gregkh, linux-wireless, devel, linux-kernel
In-Reply-To: <1ffdd756-aaa4-5e1e-b72f-3e5d1f2daeb2@broadcom.com>
On Thu, 23 Feb 2017, 'Arend Van Spriel' via outreachy-kernel wrote:
> On 23-2-2017 8:08, Julia Lawall wrote:
> >> Thanks for the feedback Arend, I really appreciate it. I've decided to go with
> >> these changes in my follow-up patch request:
> >>
> >> - rename tstrRSSI to 'rssi_history_buffer' as Aren suggested since it makes the
> >> purpose of the struct clear
> >> - remove Hungarian notation from all tstrRSSI members' names
> >> - change type of u8Full to bool since it's only ever 1 or 0
> >> - change name of as8RSSI to 'samples' since this buffer is only ever used to
> >> compute an average, and the "rssi" prefix is implied by the struct's name
> >> - rename str_rssi to rssi_history in the network_info struct for clarity
> >>
> >> Since my reasoning for these changes deviates from just "renaming to
> >> avoid camel casing" (as in the original checkpatch.pl warning), would it still
> >> make sense to submit all this in a single patch? I know my commit message
> >> needs to change but I wonder if this is too much detail.
> >
> > I would strongly suggest not to do it all in a single patch. Even if these
> > changes are not very complicated conceptually, there is always a chance of
> > doing things wrong. Taking the problems one by one will improve the chance
> > that the result is correct. Also, the results will be easier for you and
> > others to review if each patch only does one thing. And easier to revert
> > if needed later if something goes wrong.
>
> It is all related to cleaning up stuff in a single struct which I
> consider "one thing" here. To me it looks a bit silly if you rename one
> struct member when it is obvious that the other two need to be renamed
> as well. The only somewhat sensible split I see here is: 1) rename the
> struct itself, 2) rename the struct members, and 3) rename str_rssi
> member in struct network_info.
OK. I guess I mainly saw the change of type as being different.
julia
>
> Regards,
> Arend
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1ffdd756-aaa4-5e1e-b72f-3e5d1f2daeb2%40broadcom.com.
> For more options, visit https://groups.google.com/d/optout.
>
^ permalink raw reply
* [PATCH v2] wireless-regdb: Update rules for Australia (AU) and add 60GHz rules
From: Ryan Mounce @ 2017-02-23 13:52 UTC (permalink / raw)
To: seth.forshee; +Cc: wireless-regdb, linux-wireless
In-Reply-To: <20170223130142.GA90803@ubuntu-hedt>
Sourced from the current legislation at
https://www.legislation.gov.au/Details/F2016C00432
The current rules exceed legal limits between 5250-5330MHz, and permit
illegal operation in 5600-5650MHz (disallowed regardless of DFS).
Frequency ranges and EIRP limits for all ranges have been updated to
match items 59-63, 65 in the linked document. As the rules for AU have
never previously mirrored local regulations, changes include a
significant increase in the allowable 2.4GHz EIRP and smaller increases
in most other bands.
In order to allow 80MHz operation between 5650-5730MHz (bordering two
bands) the lower, more restrictive band has been rounded up by 5MHz.
Signed-off-by: Ryan Mounce <ryan@mounce.com.au>
---
db.txt | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/db.txt b/db.txt
index 05108e0..00e81b6 100644
--- a/db.txt
+++ b/db.txt
@@ -85,12 +85,17 @@ country AT: DFS-ETSI
# 60 GHz band channels 1-4, ref: Etsi En 302 567
(57000 - 66000 @ 2160), (40)
+# Source:
+# https://www.legislation.gov.au/Details/F2016C00432
+# Both DFS-ETSI and DFS-FCC are acceptable per AS/NZS 4268 Appendix B
country AU: DFS-ETSI
- (2402 - 2482 @ 40), (20)
- (5170 - 5250 @ 80), (17), AUTO-BW
- (5250 - 5330 @ 80), (24), DFS, AUTO-BW
- (5490 - 5710 @ 160), (24), DFS
- (5735 - 5835 @ 80), (30)
+ (2400 - 2483.5 @ 40), (36)
+ (5150 - 5250 @ 80), (23), NO-OUTDOOR, AUTO-BW
+ (5250 - 5350 @ 80), (23), NO-OUTDOOR, AUTO-BW, DFS
+ (5470 - 5600 @ 80), (30), DFS
+ (5650 - 5730 @ 80), (30), DFS
+ (5730 - 5850 @ 80), (36)
+ (57000 - 66000 @ 2160), (43), NO-OUTDOOR
country AW: DFS-ETSI
(2402 - 2482 @ 40), (20)
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v2] wireless-regdb: Update rules for Australia (AU) and add 60GHz rules
From: Seth Forshee @ 2017-02-23 15:35 UTC (permalink / raw)
To: Ryan Mounce; +Cc: wireless-regdb, linux-wireless, Johannes Berg
In-Reply-To: <20170223135253.1681-1-ryan@mounce.com.au>
On Fri, Feb 24, 2017 at 12:22:53AM +1030, Ryan Mounce wrote:
> Sourced from the current legislation at
> https://www.legislation.gov.au/Details/F2016C00432
>
> The current rules exceed legal limits between 5250-5330MHz, and permit
> illegal operation in 5600-5650MHz (disallowed regardless of DFS).
>
> Frequency ranges and EIRP limits for all ranges have been updated to
> match items 59-63, 65 in the linked document. As the rules for AU have
> never previously mirrored local regulations, changes include a
> significant increase in the allowable 2.4GHz EIRP and smaller increases
> in most other bands.
>
> In order to allow 80MHz operation between 5650-5730MHz (bordering two
> bands) the lower, more restrictive band has been rounded up by 5MHz.
>
> Signed-off-by: Ryan Mounce <ryan@mounce.com.au>
> ---
> db.txt | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/db.txt b/db.txt
> index 05108e0..00e81b6 100644
> --- a/db.txt
> +++ b/db.txt
> @@ -85,12 +85,17 @@ country AT: DFS-ETSI
> # 60 GHz band channels 1-4, ref: Etsi En 302 567
> (57000 - 66000 @ 2160), (40)
>
> +# Source:
> +# https://www.legislation.gov.au/Details/F2016C00432
> +# Both DFS-ETSI and DFS-FCC are acceptable per AS/NZS 4268 Appendix B
> country AU: DFS-ETSI
> - (2402 - 2482 @ 40), (20)
> - (5170 - 5250 @ 80), (17), AUTO-BW
> - (5250 - 5330 @ 80), (24), DFS, AUTO-BW
> - (5490 - 5710 @ 160), (24), DFS
> - (5735 - 5835 @ 80), (30)
> + (2400 - 2483.5 @ 40), (36)
The mention of ETSI EN 300 328 in item 55 (a) leads me to believe that
this is the limit we should be using, i.e. 500 mW. It is a bit confusing
though since it seems like such devices would also fall under "digital
modulation transmitters."
> + (5150 - 5250 @ 80), (23), NO-OUTDOOR, AUTO-BW
This looks correct.
> + (5250 - 5350 @ 80), (23), NO-OUTDOOR, AUTO-BW, DFS
Since this range requires TPC we need to set the power limit at 100 mW.
> + (5470 - 5600 @ 80), (30), DFS
> + (5650 - 5730 @ 80), (30), DFS
These ranges also require TPC so we need to set the limit at 500 mW.
> + (5730 - 5850 @ 80), (36)
In the document the ranges are 5650-5725 MHz and 5725-5850 MHz. I
suspect that the existing rules fudge that to line up with the wifi
channels, which technically is okay because the rules in the former
range are more restrictive. I wonder if we shouldn't be recording them
here as per the document and adding AUTO-BW. Johannes, any thoughts?
> + (57000 - 66000 @ 2160), (43), NO-OUTDOOR
This looks correct.
Thanks,
Seth
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox