2014-05-20 20:36:58

by Paul Stewart

[permalink] [raw]
Subject: [PATCH] nl80211: Provide TDLS link state

Provide a method to query TDLS state in drivers that use the
NL80211_CMD_TDLS_OPER method for link setup.

Signed-off-by: Paul Stewart <[email protected]>
---
include/uapi/linux/nl80211.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 9922b9b..4f163f9 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -3866,6 +3866,7 @@ enum nl80211_pmksa_candidate_attr {
* @NL80211_TDLS_TEARDOWN: Teardown a TDLS link which is already established
* @NL80211_TDLS_ENABLE_LINK: Enable TDLS link
* @NL80211_TDLS_DISABLE_LINK: Disable TDLS link
+ * @NL80211_TDLS_QUERY_LINK: Query TDLS link status
*/
enum nl80211_tdls_operation {
NL80211_TDLS_DISCOVERY_REQ,
@@ -3873,6 +3874,19 @@ enum nl80211_tdls_operation {
NL80211_TDLS_TEARDOWN,
NL80211_TDLS_ENABLE_LINK,
NL80211_TDLS_DISABLE_LINK,
+ NL80211_TDLS_QUERY_LINK,
+};
+
+/**
+ * enum nl80211_tdls_link_state - values returned fo %NL80211_TDLS_QUERY_LINK
+ * @NL80211_TDLS_LINK_STATE_UNKNOWN: Nothing is known about this peer
+ * @NL80211_TDLS_LINK_STATE_UNCONNECTED: TDLS link is not setup to peer
+ * @NL80211_TDLS_LINK_STATE_CONNECTED: TDLS link is setup to peer
+ */
+enum nl80211_tdls_link_state {
+ NL80211_TDLS_LINK_STATE_UNKNOWN,
+ NL80211_TDLS_LINK_STATE_UNCONNECTED,
+ NL80211_TDLS_LINK_STATE_CONNECTED,
};

/*
--
1.9.1.423.g4596e3a



2014-05-20 22:33:18

by Paul Stewart

[permalink] [raw]
Subject: [PATCHv2] nl80211: Provide TDLS link state

Provide a method to query TDLS state in drivers that use the
NL80211_CMD_TDLS_OPER method for link setup.

Signed-off-by: Paul Stewart <[email protected]>
---
include/uapi/linux/nl80211.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 9922b9b..44aca23 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -3866,6 +3866,7 @@ enum nl80211_pmksa_candidate_attr {
* @NL80211_TDLS_TEARDOWN: Teardown a TDLS link which is already established
* @NL80211_TDLS_ENABLE_LINK: Enable TDLS link
* @NL80211_TDLS_DISABLE_LINK: Disable TDLS link
+ * @NL80211_TDLS_QUERY_LINK: Query TDLS link status
*/
enum nl80211_tdls_operation {
NL80211_TDLS_DISCOVERY_REQ,
@@ -3873,6 +3874,19 @@ enum nl80211_tdls_operation {
NL80211_TDLS_TEARDOWN,
NL80211_TDLS_ENABLE_LINK,
NL80211_TDLS_DISABLE_LINK,
+ NL80211_TDLS_QUERY_LINK,
+};
+
+/**
+ * enum nl80211_tdls_link_state - values returned for %NL80211_TDLS_QUERY_LINK
+ * @NL80211_TDLS_LINK_STATE_UNKNOWN: Nothing is known about this peer
+ * @NL80211_TDLS_LINK_STATE_UNCONNECTED: TDLS link is not setup to peer
+ * @NL80211_TDLS_LINK_STATE_CONNECTED: TDLS link is setup to peer
+ */
+enum nl80211_tdls_link_state {
+ NL80211_TDLS_LINK_STATE_UNKNOWN,
+ NL80211_TDLS_LINK_STATE_UNCONNECTED,
+ NL80211_TDLS_LINK_STATE_CONNECTED,
};

/*
--
1.9.1.423.g4596e3a


2014-05-21 20:00:00

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCHv2] nl80211: Provide TDLS link state

On Wed, 2014-05-21 at 09:17 -0700, Paul Stewart wrote:

> I still think this might be too heavyweight in situations where the
> firmware handles TDLS links. Think about a system where the firmware
> is completely responsible for terminating incoming TDLS requests and
> offers no facility to notify the kernel as the TDLS link (initiated
> remotely) was established.

Is it really that bad to demand the firmware notify the host? It's not
like this is a very frequent event, and if querying is supported then
notification can't be all that much more difficult.

We're not in a great position to demand that upstream, I guess, it
should be easier for you :-)

If it's really such a big problem, in theory one could support
get_station() (which gives you a MAC address) and not dump_station(). I
wouldn't necessarily recommend it, but it does seem possible.


> Unless the driver polls the firmware for
> ALL MAC addresses, there's no way for a station entry to magically
> appear. I wan this method so we can directly ask the firmware "hey,
> is there a TDLS link to 02:00:00:00:01:00?" This pre-supposes that
> user-space has some knowledge about this remote host (suppose we're
> streaming media to it) and we want to know whether in this case, for
> diagnostic reasons, whether TDLS was established between them at this
> time.

But then the next diagnostic thing will be ... oh, let's see what
* the signal quality is
* bitrate is used
* etc.

all of which is covered by station information (and can optionally be
made available for the station entry.) Right now you're only asking "is
there a link" but I see no reason that (in particular for
diagnostics/debug!) you wouldn't want more :-)

johannes


2014-05-20 21:54:36

by Luca Coelho

[permalink] [raw]
Subject: Re: [PATCH] nl80211: Provide TDLS link state

Hi Paul,

On Tue, 2014-05-20 at 13:27 -0700, Paul Stewart wrote:
> Provide a method to query TDLS state in drivers that use the
> NL80211_CMD_TDLS_OPER method for link setup.
>
> Signed-off-by: Paul Stewart <[email protected]>
> ---
> include/uapi/linux/nl80211.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index 9922b9b..4f163f9 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -3866,6 +3866,7 @@ enum nl80211_pmksa_candidate_attr {
> * @NL80211_TDLS_TEARDOWN: Teardown a TDLS link which is already established
> * @NL80211_TDLS_ENABLE_LINK: Enable TDLS link
> * @NL80211_TDLS_DISABLE_LINK: Disable TDLS link
> + * @NL80211_TDLS_QUERY_LINK: Query TDLS link status
> */
> enum nl80211_tdls_operation {
> NL80211_TDLS_DISCOVERY_REQ,
> @@ -3873,6 +3874,19 @@ enum nl80211_tdls_operation {
> NL80211_TDLS_TEARDOWN,
> NL80211_TDLS_ENABLE_LINK,
> NL80211_TDLS_DISABLE_LINK,
> + NL80211_TDLS_QUERY_LINK,
> +};
> +
> +/**
> + * enum nl80211_tdls_link_state - values returned fo %NL80211_TDLS_QUERY_LINK

Small typo here. s/fo/for/ ?

--
Luca.


2014-05-21 10:41:45

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCHv2] nl80211: Provide TDLS link state

On Tue, 2014-05-20 at 13:27 -0700, Paul Stewart wrote:
> Provide a method to query TDLS state in drivers that use the
> NL80211_CMD_TDLS_OPER method for link setup.

Apart from the fact that your patch isn't very clear on how this was
intended to be used (some return value?) I don't really see why even
such a driver couldn't implement station operations
(get_station/dump_station) and get the same API for this that mac80211
already implements.

iw wlan0 station dump gives me
Station 02:00:00:00:01:00 (on wlan0)
inactive time: 450 ms
rx bytes: 3068
rx packets: 2
tx bytes: 3036
tx packets: 2
tx retries: 0
tx failed: 0
signal: -30 dBm
signal avg: -30 dBm
tx bitrate: 24.0 MBit/s
rx bitrate: 12.0 MBit/s
authorized: yes
authenticated: yes
preamble: long
WMM/WME: no
MFP: no
TDLS peer: yes
[...]


johannes


2014-05-22 06:56:52

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCHv2] nl80211: Provide TDLS link state

On Wed, 2014-05-21 at 13:19 -0700, Paul Stewart wrote:

> > Is it really that bad to demand the firmware notify the host? It's not
> > like this is a very frequent event, and if querying is supported then
> > notification can't be all that much more difficult.
>
> I think firmware vendors are coming to the point of view that they
> must minimize host processor wakeups. They may argue from that
> perspective that the firmware should divulge such information only on
> request.

That's reasonable, but the request could also be "list all TDLS peers
please", right? Then you could still implement it very easily. It
doesn't have to be an event.

johannes


2014-05-21 16:17:39

by Paul Stewart

[permalink] [raw]
Subject: Re: [PATCHv2] nl80211: Provide TDLS link state

On Wed, May 21, 2014 at 3:41 AM, Johannes Berg
<[email protected]> wrote:
>
> On Tue, 2014-05-20 at 13:27 -0700, Paul Stewart wrote:
> > Provide a method to query TDLS state in drivers that use the
> > NL80211_CMD_TDLS_OPER method for link setup.
>
> Apart from the fact that your patch isn't very clear on how this was
> intended to be used (some return value?) I don't really see why even
> such a driver couldn't implement station operations
> (get_station/dump_station) and get the same API for this that mac80211
> already implements.


I still think this might be too heavyweight in situations where the
firmware handles TDLS links. Think about a system where the firmware
is completely responsible for terminating incoming TDLS requests and
offers no facility to notify the kernel as the TDLS link (initiated
remotely) was established. Unless the driver polls the firmware for
ALL MAC addresses, there's no way for a station entry to magically
appear. I wan this method so we can directly ask the firmware "hey,
is there a TDLS link to 02:00:00:00:01:00?" This pre-supposes that
user-space has some knowledge about this remote host (suppose we're
streaming media to it) and we want to know whether in this case, for
diagnostic reasons, whether TDLS was established between them at this
time.
>
>
> iw wlan0 station dump gives me
> Station 02:00:00:00:01:00 (on wlan0)
> inactive time: 450 ms
> rx bytes: 3068
> rx packets: 2
> tx bytes: 3036
> tx packets: 2
> tx retries: 0
> tx failed: 0
> signal: -30 dBm
> signal avg: -30 dBm
> tx bitrate: 24.0 MBit/s
> rx bitrate: 12.0 MBit/s
> authorized: yes
> authenticated: yes
> preamble: long
> WMM/WME: no
> MFP: no
> TDLS peer: yes
> [...]
>
>
> johannes
>

2014-05-21 20:19:06

by Paul Stewart

[permalink] [raw]
Subject: Re: [PATCHv2] nl80211: Provide TDLS link state

On Wed, May 21, 2014 at 12:59 PM, Johannes Berg
<[email protected]> wrote:
> On Wed, 2014-05-21 at 09:17 -0700, Paul Stewart wrote:
>
>> I still think this might be too heavyweight in situations where the
>> firmware handles TDLS links. Think about a system where the firmware
>> is completely responsible for terminating incoming TDLS requests and
>> offers no facility to notify the kernel as the TDLS link (initiated
>> remotely) was established.
>
> Is it really that bad to demand the firmware notify the host? It's not
> like this is a very frequent event, and if querying is supported then
> notification can't be all that much more difficult.

I think firmware vendors are coming to the point of view that they
must minimize host processor wakeups. They may argue from that
perspective that the firmware should divulge such information only on
request.

> We're not in a great position to demand that upstream, I guess, it
> should be easier for you :-)
>
> If it's really such a big problem, in theory one could support
> get_station() (which gives you a MAC address) and not dump_station(). I
> wouldn't necessarily recommend it, but it does seem possible.
>

Okay. I suppose I can shelve this until I can ask for firmware
changes to support this pattern. I'm not sure I'll get it, though,
and I'm doubtful whether that's a tenable path for others.