diff mbox

[iw] add "channels" PHY command listing frequencies with more details

Message ID 1463739223-19205-1-git-send-email-zajec5@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show

Commit Message

Rafał Miłecki May 20, 2016, 10:13 a.m. UTC
Channels (frequencies) are getting more details that users may want to
know about. E.g. it's important to know which frequencies allow using
40/80/160 MHz channels to setup AP properly.

We list channels in "info" command output but it's already quite big and
it was agreed to introduce new command rather than expand the old one.

This patch adds "channels" command printing what was already available
in the "info" plus details about supported channel widths. It also
removes DFS info from the "info" output.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 info.c |  30 --------------
 phy.c  | 143 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+), 30 deletions(-)

Comments

Johannes Berg May 31, 2016, 10:16 a.m. UTC | #1
On Fri, 2016-05-20 at 12:13 +0200, Rafał Miłecki wrote:
> Channels (frequencies) are getting more details that users may want
> to
> know about. E.g. it's important to know which frequencies allow using
> 40/80/160 MHz channels to setup AP properly.
> 
> We list channels in "info" command output but it's already quite big
> and
> it was agreed to introduce new command rather than expand the old
> one.
> 
> This patch adds "channels" command printing what was already
> available
> in the "info" plus details about supported channel widths. It also
> removes DFS info from the "info" output.

Very nice.

Unfortunately, I get compiler warnings about width_* being possibly
used uninitialized. Can you address that please?

> +TOPLEVEL(channels, NULL, NL80211_CMD_GET_WIPHY, 0, CIB_PHY,
> handle_channels, "Show available channels.");

Maybe that should be allowed for a CIB_DEV too?

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafał Miłecki May 31, 2016, 1:10 p.m. UTC | #2
On 31 May 2016 at 12:16, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2016-05-20 at 12:13 +0200, Rafał Miłecki wrote:
>> Channels (frequencies) are getting more details that users may want
>> to
>> know about. E.g. it's important to know which frequencies allow using
>> 40/80/160 MHz channels to setup AP properly.
>>
>> We list channels in "info" command output but it's already quite big
>> and
>> it was agreed to introduce new command rather than expand the old
>> one.
>>
>> This patch adds "channels" command printing what was already
>> available
>> in the "info" plus details about supported channel widths. It also
>> removes DFS info from the "info" output.
>
> Very nice.
>
> Unfortunately, I get compiler warnings about width_* being possibly
> used uninitialized. Can you address that please?

It's nice your compiled got this mistake, my didn't. There were
actually meant to be static. I'll fix that.


>> +TOPLEVEL(channels, NULL, NL80211_CMD_GET_WIPHY, 0, CIB_PHY,
>> handle_channels, "Show available channels.");
>
> Maybe that should be allowed for a CIB_DEV too?

Since this is PHY specific, I was thinking it should be CIB_PHY. I
didn't see reason to allow querying devices. Similarly we don't
support "iw dev wlan0 reg get".

Anyway, I can change that if you think it's better for some reason.
Any hint how to make command usable with both: phy and dev?
Johannes Berg May 31, 2016, 1:14 p.m. UTC | #3
On Tue, 2016-05-31 at 15:10 +0200, Rafał Miłecki wrote:

> > Unfortunately, I get compiler warnings about width_* being possibly
> > used uninitialized. Can you address that please?
> It's nice your compiled got this mistake, my didn't. There were
> actually meant to be static. I'll fix that.

I actually think the compiler is wrong, and there's no reason for them
to be static, is there?

The first iteration of the loop should always initialize since -1 is
used for initialization?

In any case, I'd prefer to avoid static, if necessary pass some kind of
context structure in to the function?

> > > +TOPLEVEL(channels, NULL, NL80211_CMD_GET_WIPHY, 0, CIB_PHY,
> > > handle_channels, "Show available channels.");
> > Maybe that should be allowed for a CIB_DEV too?
> Since this is PHY specific, I was thinking it should be CIB_PHY. I
> didn't see reason to allow querying devices. Similarly we don't
> support "iw dev wlan0 reg get".
> 
> Anyway, I can change that if you think it's better for some reason.

I was just thinking that supporting it with wlan0 would in certain
cases make it easier for the user (not having to look up the phy
number)

> Any hint how to make command usable with both: phy and dev?

Just add:

TOPLEVEL(channels, NULL, NL80211_CMD_GET_WIPHY, 0, CIB_NETDEV, ...);

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafał Miłecki May 31, 2016, 3:58 p.m. UTC | #4
On 31 May 2016 at 15:14, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2016-05-31 at 15:10 +0200, Rafał Miłecki wrote:
>
>> > Unfortunately, I get compiler warnings about width_* being possibly
>> > used uninitialized. Can you address that please?
>> It's nice your compiled got this mistake, my didn't. There were
>> actually meant to be static. I'll fix that.
>
> I actually think the compiler is wrong, and there's no reason for them
> to be static, is there?
>
> The first iteration of the loop should always initialize since -1 is
> used for initialization?
>
> In any case, I'd prefer to avoid static, if necessary pass some kind of
> context structure in to the function?

I don't know if we can assume getting all three attributes:
NL80211_BAND_ATTR_HT_CAPA
NL80211_BAND_ATTR_VHT_CAPA
NL80211_BAND_ATTR_FREQS
at the same single handler call. I guess not. That's why I believe we
need all width_* to be static, so they keep value across separated
handler calls.


>> > > +TOPLEVEL(channels, NULL, NL80211_CMD_GET_WIPHY, 0, CIB_PHY,
>> > > handle_channels, "Show available channels.");
>> > Maybe that should be allowed for a CIB_DEV too?
>> Since this is PHY specific, I was thinking it should be CIB_PHY. I
>> didn't see reason to allow querying devices. Similarly we don't
>> support "iw dev wlan0 reg get".
>>
>> Anyway, I can change that if you think it's better for some reason.
>
> I was just thinking that supporting it with wlan0 would in certain
> cases make it easier for the user (not having to look up the phy
> number)
>
>> Any hint how to make command usable with both: phy and dev?
>
> Just add:
>
> TOPLEVEL(channels, NULL, NL80211_CMD_GET_WIPHY, 0, CIB_NETDEV, ...);

But this will disallow using "iw phy phy0 channels". I'd prefer to
have this one working in the first place and having "iw dev wlan0
channels" as an alternative. Is there a way to achieve that easily?
Rafał Miłecki May 31, 2016, 4:20 p.m. UTC | #5
On 31 May 2016 at 17:58, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 31 May 2016 at 15:14, Johannes Berg <johannes@sipsolutions.net> wrote:
>> On Tue, 2016-05-31 at 15:10 +0200, Rafał Miłecki wrote:
>>
>>> > Unfortunately, I get compiler warnings about width_* being possibly
>>> > used uninitialized. Can you address that please?
>>> It's nice your compiled got this mistake, my didn't. There were
>>> actually meant to be static. I'll fix that.
>>
>> I actually think the compiler is wrong, and there's no reason for them
>> to be static, is there?
>>
>> The first iteration of the loop should always initialize since -1 is
>> used for initialization?
>>
>> In any case, I'd prefer to avoid static, if necessary pass some kind of
>> context structure in to the function?
>
> I don't know if we can assume getting all three attributes:
> NL80211_BAND_ATTR_HT_CAPA
> NL80211_BAND_ATTR_VHT_CAPA
> NL80211_BAND_ATTR_FREQS
> at the same single handler call. I guess not. That's why I believe we
> need all width_* to be static, so they keep value across separated
> handler calls.

Yes, this is definitely the case. I did some debugging from user space
and printed one line per a single handler call:
[i:0] WIPHY_BANDS:0
[i:1] WIPHY_BANDS:0
[i:2] WIPHY_BANDS:0
[i:3] WIPHY_BANDS:1 ATTR_HT_CAPA:1 ATTR_VHT_CAPA:0 ATTR_FREQS:0
[i:4] WIPHY_BANDS:1 ATTR_HT_CAPA:1 ATTR_VHT_CAPA:1 ATTR_FREQS:0
[i:5] WIPHY_BANDS:1 ATTR_HT_CAPA:0 ATTR_VHT_CAPA:0 ATTR_FREQS:1
[i:6] WIPHY_BANDS:1 ATTR_HT_CAPA:0 ATTR_VHT_CAPA:0 ATTR_FREQS:1
[i:7] WIPHY_BANDS:1 ATTR_HT_CAPA:0 ATTR_VHT_CAPA:0 ATTR_FREQS:1
[i:8] WIPHY_BANDS:1 ATTR_HT_CAPA:0 ATTR_VHT_CAPA:0 ATTR_FREQS:1
[i:9] WIPHY_BANDS:1 ATTR_HT_CAPA:0 ATTR_VHT_CAPA:0 ATTR_FREQS:1
[i:10] WIPHY_BANDS:1 ATTR_HT_CAPA:0 ATTR_VHT_CAPA:0 ATTR_FREQS:1
[i:11] WIPHY_BANDS:1 ATTR_HT_CAPA:0 ATTR_VHT_CAPA:0 ATTR_FREQS:1
[i:12] WIPHY_BANDS:1 ATTR_HT_CAPA:0 ATTR_VHT_CAPA:0 ATTR_FREQS:1
[i:13] WIPHY_BANDS:1 ATTR_HT_CAPA:0 ATTR_VHT_CAPA:0 ATTR_FREQS:1
(...)

As you can see we have ATTR_HT_CAPA and ATTR_VHT_CAPA set only in few
early calls. It means we need to extract info from them and store it
in static variables.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/info.c b/info.c
index 32de0af..b375123 100644
--- a/info.c
+++ b/info.c
@@ -47,20 +47,6 @@  static char *cipher_name(__u32 c)
 	}
 }
 
-static char *dfs_state_name(enum nl80211_dfs_state state)
-{
-	switch (state) {
-	case NL80211_DFS_USABLE:
-		return "usable";
-	case NL80211_DFS_AVAILABLE:
-		return "available";
-	case NL80211_DFS_UNAVAILABLE:
-		return "unavailable";
-	default:
-		return "unknown";
-	}
-}
-
 static int ext_feature_isset(const unsigned char *ext_features, int ext_features_len,
 			     enum nl80211_ext_feature_index ftidx)
 {
@@ -198,22 +184,6 @@  next:
 					if (open)
 						printf(")");
 					printf("\n");
-
-					if (!tb_freq[NL80211_FREQUENCY_ATTR_DISABLED] && tb_freq[NL80211_FREQUENCY_ATTR_DFS_STATE]) {
-						enum nl80211_dfs_state state = nla_get_u32(tb_freq[NL80211_FREQUENCY_ATTR_DFS_STATE]);
-						unsigned long time;
-
-						printf("\t\t\t  DFS state: %s", dfs_state_name(state));
-						if (tb_freq[NL80211_FREQUENCY_ATTR_DFS_TIME]) {
-							time = nla_get_u32(tb_freq[NL80211_FREQUENCY_ATTR_DFS_TIME]);
-							printf(" (for %lu sec)", time/1000);
-						}
-						printf("\n");
-						if (tb_freq[NL80211_FREQUENCY_ATTR_DFS_CAC_TIME])
-							printf("\t\t\t  DFS CAC time: %u ms\n",
-							       nla_get_u32(tb_freq[NL80211_FREQUENCY_ATTR_DFS_CAC_TIME]));
-					}
-
 				}
 			}
 
diff --git a/phy.c b/phy.c
index 13e8260..0639f25 100644
--- a/phy.c
+++ b/phy.c
@@ -14,6 +14,149 @@ 
 #include "nl80211.h"
 #include "iw.h"
 
+static char *dfs_state_name(enum nl80211_dfs_state state)
+{
+	switch (state) {
+	case NL80211_DFS_USABLE:
+		return "usable";
+	case NL80211_DFS_AVAILABLE:
+		return "available";
+	case NL80211_DFS_UNAVAILABLE:
+		return "unavailable";
+	default:
+		return "unknown";
+	}
+}
+
+static int print_channels_handler(struct nl_msg *msg, void *arg)
+{
+	struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg));
+
+	struct nlattr *tb_msg[NL80211_ATTR_MAX + 1];
+	struct nlattr *tb_band[NL80211_BAND_ATTR_MAX + 1];
+	struct nlattr *tb_freq[NL80211_FREQUENCY_ATTR_MAX + 1];
+	struct nlattr *nl_band;
+	struct nlattr *nl_freq;
+	int rem_band, rem_freq;
+	static int last_band = -1;
+	bool width_40, width_80, width_160;
+
+	nla_parse(tb_msg, NL80211_ATTR_MAX, genlmsg_attrdata(gnlh, 0), genlmsg_attrlen(gnlh, 0), NULL);
+
+	if (tb_msg[NL80211_ATTR_WIPHY_BANDS]) {
+		nla_for_each_nested(nl_band, tb_msg[NL80211_ATTR_WIPHY_BANDS], rem_band) {
+			if (last_band != nl_band->nla_type) {
+				printf("Band %d:\n", nl_band->nla_type + 1);
+				width_40 = false;
+				width_80 = false;
+				width_160 = false;
+				last_band = nl_band->nla_type;
+			}
+
+			nla_parse(tb_band, NL80211_BAND_ATTR_MAX, nla_data(nl_band), nla_len(nl_band), NULL);
+
+			if (tb_band[NL80211_BAND_ATTR_HT_CAPA]) {
+				__u16 cap = nla_get_u16(tb_band[NL80211_BAND_ATTR_HT_CAPA]);
+
+				if (cap & BIT(1))
+					width_40 = true;
+			}
+
+			if (tb_band[NL80211_BAND_ATTR_VHT_CAPA]) {
+				__u32 capa;
+
+				width_80 = true;
+
+				capa = nla_get_u32(tb_band[NL80211_BAND_ATTR_VHT_CAPA]);
+				switch ((capa >> 2) & 3) {
+				case 2:
+					/* width_80p80 = true; */
+					/* fall through */
+				case 1:
+					width_160 = true;
+				break;
+				}
+			}
+
+			if (tb_band[NL80211_BAND_ATTR_FREQS]) {
+				nla_for_each_nested(nl_freq, tb_band[NL80211_BAND_ATTR_FREQS], rem_freq) {
+					uint32_t freq;
+
+					nla_parse(tb_freq, NL80211_FREQUENCY_ATTR_MAX, nla_data(nl_freq), nla_len(nl_freq), NULL);
+
+					if (!tb_freq[NL80211_FREQUENCY_ATTR_FREQ])
+						continue;
+					freq = nla_get_u32(tb_freq[NL80211_FREQUENCY_ATTR_FREQ]);
+					printf("\t* %d MHz [%d] ", freq, ieee80211_frequency_to_channel(freq));
+
+					if (tb_freq[NL80211_FREQUENCY_ATTR_DISABLED]) {
+						printf("(disabled)\n");
+						continue;
+					}
+					printf("\n");
+
+					if (tb_freq[NL80211_FREQUENCY_ATTR_MAX_TX_POWER])
+						printf("\t  Maximum TX power: %.1f dBm\n", 0.01 * nla_get_u32(tb_freq[NL80211_FREQUENCY_ATTR_MAX_TX_POWER]));
+
+					/* If both flags are set assume an new kernel */
+					if (tb_freq[NL80211_FREQUENCY_ATTR_NO_IR] && tb_freq[__NL80211_FREQUENCY_ATTR_NO_IBSS]) {
+						printf("\t  No IR\n");
+					} else if (tb_freq[NL80211_FREQUENCY_ATTR_PASSIVE_SCAN]) {
+						printf("\t  Passive scan\n");
+					} else if (tb_freq[__NL80211_FREQUENCY_ATTR_NO_IBSS]){
+						printf("\t  No IBSS\n");
+					}
+
+					if (tb_freq[NL80211_FREQUENCY_ATTR_RADAR])
+						printf("\t  Radar detection\n");
+
+					printf("\t  Channel widths:");
+					if (!tb_freq[NL80211_FREQUENCY_ATTR_NO_20MHZ])
+						printf(" 20MHz");
+					if (width_40 && !tb_freq[NL80211_FREQUENCY_ATTR_NO_HT40_MINUS])
+						printf(" HT40-");
+					if (width_40 && !tb_freq[NL80211_FREQUENCY_ATTR_NO_HT40_PLUS])
+						printf(" HT40+");
+					if (width_80 && !tb_freq[NL80211_FREQUENCY_ATTR_NO_80MHZ])
+						printf(" VHT80");
+					if (width_160 && !tb_freq[NL80211_FREQUENCY_ATTR_NO_160MHZ])
+						printf(" VHT160");
+					printf("\n");
+
+					if (!tb_freq[NL80211_FREQUENCY_ATTR_DISABLED] && tb_freq[NL80211_FREQUENCY_ATTR_DFS_STATE]) {
+						enum nl80211_dfs_state state = nla_get_u32(tb_freq[NL80211_FREQUENCY_ATTR_DFS_STATE]);
+						unsigned long time;
+
+						printf("\t  DFS state: %s", dfs_state_name(state));
+						if (tb_freq[NL80211_FREQUENCY_ATTR_DFS_TIME]) {
+							time = nla_get_u32(tb_freq[NL80211_FREQUENCY_ATTR_DFS_TIME]);
+							printf(" (for %lu sec)", time / 1000);
+						}
+						printf("\n");
+						if (tb_freq[NL80211_FREQUENCY_ATTR_DFS_CAC_TIME])
+							printf("\t  DFS CAC time: %u ms\n",
+							       nla_get_u32(tb_freq[NL80211_FREQUENCY_ATTR_DFS_CAC_TIME]));
+					}
+				}
+			}
+		}
+	}
+
+	return NL_SKIP;
+}
+
+static int handle_channels(struct nl80211_state *state, struct nl_msg *msg,
+			   int argc, char **argv, enum id_input id)
+{
+	nla_put_flag(msg, NL80211_ATTR_SPLIT_WIPHY_DUMP);
+	nlmsg_hdr(msg)->nlmsg_flags |= NLM_F_DUMP;
+
+	register_handler(print_channels_handler, NULL);
+
+	return 0;
+}
+TOPLEVEL(channels, NULL, NL80211_CMD_GET_WIPHY, 0, CIB_PHY, handle_channels, "Show available channels.");
+
 static int handle_name(struct nl80211_state *state,
 		       struct nl_msg *msg,
 		       int argc, char **argv,