2013-06-03 07:04:32

by Antonio Quartulli

[permalink] [raw]
Subject: [PATCH 1/2] nl80211: allow sending CMD_FRAME without specifying any frequency

From: Antonio Quartulli <[email protected]>

Users may want to send a frame on the current channel
without specifying it.

Make mgmt_tx pass a NULL channel to mac80211 if none has
been specified by the user.

Signed-off-by: Antonio Quartulli <[email protected]>
---
net/wireless/nl80211.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 31d265f..c2376c7 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -7142,6 +7142,9 @@ static int nl80211_tx_mgmt(struct sk_buff *skb, struct genl_info *info)
return -EOPNOTSUPP;

switch (wdev->iftype) {
+ case NL80211_IFTYPE_P2P_DEVICE:
+ if (!info->attrs[NL80211_ATTR_WIPHY_FREQ])
+ return -EINVAL;
case NL80211_IFTYPE_STATION:
case NL80211_IFTYPE_ADHOC:
case NL80211_IFTYPE_P2P_CLIENT:
@@ -7149,7 +7152,6 @@ static int nl80211_tx_mgmt(struct sk_buff *skb, struct genl_info *info)
case NL80211_IFTYPE_AP_VLAN:
case NL80211_IFTYPE_MESH_POINT:
case NL80211_IFTYPE_P2P_GO:
- case NL80211_IFTYPE_P2P_DEVICE:
break;
default:
return -EOPNOTSUPP;
@@ -7177,9 +7179,16 @@ static int nl80211_tx_mgmt(struct sk_buff *skb, struct genl_info *info)

no_cck = nla_get_flag(info->attrs[NL80211_ATTR_TX_NO_CCK_RATE]);

- err = nl80211_parse_chandef(rdev, info, &chandef);
- if (err)
- return err;
+ /*
+ * get the channel if any has been specified, otherwise pass NULL to
+ * mac80211. The latter will use the current one
+ */
+ chandef.chan = NULL;
+ if (info->attrs[NL80211_ATTR_WIPHY_FREQ]) {
+ err = nl80211_parse_chandef(rdev, info, &chandef);
+ if (err)
+ return err;
+ }

if (!dont_wait_for_ack) {
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
--
1.8.1.5



2013-06-03 17:16:24

by Antonio Quartulli

[permalink] [raw]
Subject: ath6kl_mgmt_tx with NULL chan (was Re: [PATCH 1/2] nl80211: allow sending CMD_FRAME without specifying any frequency)

On Mon, Jun 03, 2013 at 05:04:08PM +0200, Nicolas Cavallari wrote:
> On 03/06/2013 16:59, Johannes Berg wrote:
> > On Mon, 2013-06-03 at 08:39 +0200, Antonio Quartulli wrote:
> >> From: Antonio Quartulli <[email protected]>
> >>
> >> Users may want to send a frame on the current channel
> >> without specifying it.
> >>
> >> Make mgmt_tx pass a NULL channel to mac80211 if none has
> >> been specified by the user.
> >
> > cfg80211 isn't just a mac80211 frontend ... ;-)
> >
> > Also, as Nicolas said, ath6kl_mgmt_tx() will crash after this patch if
> > it's called in AP mode w/o a channel, so you need to think about that.
>
> It will crash unconditionally. All ath6kl_mgmt_tx()'s code paths access
> chan->center_freq at some point.

Hello Nicolas,
I'm also CCing Kalle Valo since get_maintainer.pl told me he is the guy for
these kind of questions :-)

I'm looking at ath6kl_mgmt_tx() in ath6kl/cfg80211.c and I've seen that the
currently "configured" frequency can be obtained by reading the
ath6kl_vif->ch_hint field.

But, is this correct? I couldn't see any real relation between the ch_hint field
and the real frequency (probably because a lot of logic is hidden to the
driver).
I could only understand that the ch_hint field stores the frequency passed as
parameter during the connection, but I have found no guarantee that this is the
really used one.

Can someone please clarify on this?

Cheers,

--
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara


Attachments:
(No filename) (1.47 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-06-05 11:01:58

by Antonio Quartulli

[permalink] [raw]
Subject: Re: ath6kl_mgmt_tx with NULL chan

On Wed, Jun 05, 2013 at 03:15:26AM -0700, Kalle Valo wrote:
> But when modifying that code, please add a check to make sure that
> channel 0 is not used by accident.

Ok, maybe I'll send another patch for this. I don't want to mix a new behaviour
with a fix.


Thanks a lot!


Cheers,

--
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara


Attachments:
(No filename) (369.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-06-05 09:47:11

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/2] nl80211: allow sending CMD_FRAME without specifying any frequency

Antonio Quartulli <[email protected]> writes:

> From: Antonio Quartulli <[email protected]>
>
> Users may want to send a frame on the current channel
> without specifying it.
>
> Make mgmt_tx pass a NULL channel to mac80211 if none has
> been specified by the user.
>
> Signed-off-by: Antonio Quartulli <[email protected]>

Why? And what users are we talking about here? It would be nice if the
commit log would give some context here for use who nothing about this
patch. "Users may want" is not very informative :)

--
Kalle Valo

2013-06-05 10:00:16

by Antonio Quartulli

[permalink] [raw]
Subject: Re: [PATCH 1/2] nl80211: allow sending CMD_FRAME without specifying any frequency

On Wed, Jun 05, 2013 at 02:47:01AM -0700, Kalle Valo wrote:
> Antonio Quartulli <[email protected]> writes:
>
> > From: Antonio Quartulli <[email protected]>
> >
> > Users may want to send a frame on the current channel
> > without specifying it.
> >
> > Make mgmt_tx pass a NULL channel to mac80211 if none has
> > been specified by the user.
> >
> > Signed-off-by: Antonio Quartulli <[email protected]>
>
> Why? And what users are we talking about here? It would be nice if the
> commit log would give some context here for use who nothing about this
> patch. "Users may want" is not very informative :)

Hello Kalle,
thanks for your feedback.

Sure, I can change the commit log.

However, I already wrote a couple of (userspace) applications which wanted to
send a message on the current channel and the only way to do it was to first get
the current channel and then pass it when sending a CMD_FRAME via nl80211. Of
course this approach is just a bit racy :-)

Moreover, I'm currently working on improving IBSS/RSN support in wpa_supplicant
and sending frames on the current channel is needed.

Do you think it is worth mentioning userspace applications like wpa_s in the
kernel commit message?

Cheers,

--
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara


Attachments:
(No filename) (1.27 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-06-03 15:01:34

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 2/2] mac80211: in mgmt_tx use the current channel if none has been specified

On Mon, 2013-06-03 at 08:39 +0200, Antonio Quartulli wrote:

> @@ -2845,6 +2852,10 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> rcu_read_lock();
> chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
>
> + /* if no channel was specified, use the current one */
> + if (chanctx_conf && !chan)
> + chan = chanctx_conf->def.chan;
> +
> if (chanctx_conf)
> need_offchan = chan != chanctx_conf->def.chan;
> else
> @@ -2852,6 +2863,12 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> rcu_read_unlock();
> }
>
> + /* at this point a channel should have been chosen */
> + if (!chan) {
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> +

These two changes make no sense at all. If you look at the function
you'll see that "chan" isn't used at all after the check, and modifying
the "check if ..." part to use the channel also doesn't make sense.

johannes


2013-06-03 15:04:12

by Nicolas Cavallari

[permalink] [raw]
Subject: Re: [PATCH 1/2] nl80211: allow sending CMD_FRAME without specifying any frequency

On 03/06/2013 16:59, Johannes Berg wrote:
> On Mon, 2013-06-03 at 08:39 +0200, Antonio Quartulli wrote:
>> From: Antonio Quartulli <[email protected]>
>>
>> Users may want to send a frame on the current channel
>> without specifying it.
>>
>> Make mgmt_tx pass a NULL channel to mac80211 if none has
>> been specified by the user.
>
> cfg80211 isn't just a mac80211 frontend ... ;-)
>
> Also, as Nicolas said, ath6kl_mgmt_tx() will crash after this patch if
> it's called in AP mode w/o a channel, so you need to think about that.

It will crash unconditionally. All ath6kl_mgmt_tx()'s code paths access
chan->center_freq at some point.

2013-06-03 14:51:54

by Antonio Quartulli

[permalink] [raw]
Subject: Re: [PATCH 1/2] nl80211: allow sending CMD_FRAME without specifying any frequency

On Mon, Jun 03, 2013 at 04:07:05PM +0200, Nicolas Cavallari wrote:
> On 03/06/2013 08:39, Antonio Quartulli wrote:
> > From: Antonio Quartulli <[email protected]>
> >
> > Users may want to send a frame on the current channel
> > without specifying it.
> >
> > Make mgmt_tx pass a NULL channel to mac80211 if none has
> > been specified by the user.
> >
> > Signed-off-by: Antonio Quartulli <[email protected]>
> > ---
> > net/wireless/nl80211.c | 17 +++++++++++++----
> > 1 file changed, 13 insertions(+), 4 deletions(-)
> >
>
> Have you tested tracing or ath6kl ?

no.
For tracing I think I totally forgot to update the "prototype". For ath6kl I
have no possibility since I have no hw using that driver :/

The concern about ath6kl comes from the fact that it is a fullMAC driver?


Cheers,

--
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara


Attachments:
(No filename) (891.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-06-03 18:05:15

by Antonio Quartulli

[permalink] [raw]
Subject: Re: [PATCH 2/2] mac80211: in mgmt_tx use the current channel if none has been specified

On Mon, Jun 03, 2013 at 05:01:30PM +0200, Johannes Berg wrote:
> On Mon, 2013-06-03 at 08:39 +0200, Antonio Quartulli wrote:
>
> > @@ -2845,6 +2852,10 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> > rcu_read_lock();
> > chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> >
> > + /* if no channel was specified, use the current one */
> > + if (chanctx_conf && !chan)
> > + chan = chanctx_conf->def.chan;
> > +
> > if (chanctx_conf)
> > need_offchan = chan != chanctx_conf->def.chan;
> > else
> > @@ -2852,6 +2863,12 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> > rcu_read_unlock();
> > }
> >
> > + /* at this point a channel should have been chosen */
> > + if (!chan) {
> > + ret = -EINVAL;
> > + goto out_unlock;
> > + }
> > +
>
> These two changes make no sense at all. If you look at the function
> you'll see that "chan" isn't used at all after the check,

uhm? it is passed to ieee80211_start_roc_work() right after (this part has not
been changed).

2904 ret = ieee80211_start_roc_work(local, sdata, chan,


> and modifying
> the "check if ..." part to use the channel also doesn't make sense.
>

to which part do you exactly refer? I'm just ensuring that the chan variable
gets assigned before something accesses it.

Cheers,

--
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara


Attachments:
(No filename) (1.41 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-06-03 18:16:08

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 2/2] mac80211: in mgmt_tx use the current channel if none has been specified

On Mon, 2013-06-03 at 20:03 +0200, Antonio Quartulli wrote:
> On Mon, Jun 03, 2013 at 05:01:30PM +0200, Johannes Berg wrote:
> > On Mon, 2013-06-03 at 08:39 +0200, Antonio Quartulli wrote:
> >
> > > @@ -2845,6 +2852,10 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> > > rcu_read_lock();
> > > chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> > >
> > > + /* if no channel was specified, use the current one */
> > > + if (chanctx_conf && !chan)
> > > + chan = chanctx_conf->def.chan;
> > > +
> > > if (chanctx_conf)
> > > need_offchan = chan != chanctx_conf->def.chan;
> > > else
> > > @@ -2852,6 +2863,12 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> > > rcu_read_unlock();
> > > }
> > >
> > > + /* at this point a channel should have been chosen */
> > > + if (!chan) {
> > > + ret = -EINVAL;
> > > + goto out_unlock;
> > > + }
> > > +
> >
> > These two changes make no sense at all. If you look at the function
> > you'll see that "chan" isn't used at all after the check,
>
> uhm? it is passed to ieee80211_start_roc_work() right after (this part has not
> been changed).

> 2904 ret = ieee80211_start_roc_work(local, sdata, chan,

No, you can't get there without need_offchan.

johannes


2013-06-03 07:04:32

by Antonio Quartulli

[permalink] [raw]
Subject: [PATCH 2/2] mac80211: in mgmt_tx use the current channel if none has been specified

From: Antonio Quartulli <[email protected]>

In mgmt_tx, if no channel has been specified via cfg80211,
use the current one.

If the interface requires offchan (because disconnected or
for other reasons) then fail.

Signed-off-by: Antonio Quartulli <[email protected]>
---
net/mac80211/cfg.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 9034da1..e6e41c7 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2836,6 +2836,13 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
return -EOPNOTSUPP;
}

+ /*
+ * configurations requiring offchan cannot work if no channel has been
+ * specified
+ */
+ if (need_offchan && !chan)
+ return -EINVAL;
+
mutex_lock(&local->mtx);

/* Check if the operating channel is the requested channel */
@@ -2845,6 +2852,10 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
rcu_read_lock();
chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);

+ /* if no channel was specified, use the current one */
+ if (chanctx_conf && !chan)
+ chan = chanctx_conf->def.chan;
+
if (chanctx_conf)
need_offchan = chan != chanctx_conf->def.chan;
else
@@ -2852,6 +2863,12 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
rcu_read_unlock();
}

+ /* at this point a channel should have been chosen */
+ if (!chan) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
if (need_offchan && !offchan) {
ret = -EBUSY;
goto out_unlock;
--
1.8.1.5


2013-06-03 14:14:13

by Nicolas Cavallari

[permalink] [raw]
Subject: Re: [PATCH 1/2] nl80211: allow sending CMD_FRAME without specifying any frequency

On 03/06/2013 08:39, Antonio Quartulli wrote:
> From: Antonio Quartulli <[email protected]>
>
> Users may want to send a frame on the current channel
> without specifying it.
>
> Make mgmt_tx pass a NULL channel to mac80211 if none has
> been specified by the user.
>
> Signed-off-by: Antonio Quartulli <[email protected]>
> ---
> net/wireless/nl80211.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>

Have you tested tracing or ath6kl ?

2013-06-05 09:57:13

by Kalle Valo

[permalink] [raw]
Subject: Re: ath6kl_mgmt_tx with NULL chan

Antonio Quartulli <[email protected]> writes:

> On Mon, Jun 03, 2013 at 05:04:08PM +0200, Nicolas Cavallari wrote:
>> On 03/06/2013 16:59, Johannes Berg wrote:
>> > On Mon, 2013-06-03 at 08:39 +0200, Antonio Quartulli wrote:
>> >> From: Antonio Quartulli <[email protected]>
>> >>
>> >> Users may want to send a frame on the current channel
>> >> without specifying it.
>> >>
>> >> Make mgmt_tx pass a NULL channel to mac80211 if none has
>> >> been specified by the user.
>> >
>> > cfg80211 isn't just a mac80211 frontend ... ;-)
>> >
>> > Also, as Nicolas said, ath6kl_mgmt_tx() will crash after this patch if
>> > it's called in AP mode w/o a channel, so you need to think about that.
>>
>> It will crash unconditionally. All ath6kl_mgmt_tx()'s code paths access
>> chan->center_freq at some point.
>
> Hello Nicolas,
> I'm also CCing Kalle Valo since get_maintainer.pl told me he is the guy for
> these kind of questions :-)
>
> I'm looking at ath6kl_mgmt_tx() in ath6kl/cfg80211.c and I've seen that the
> currently "configured" frequency can be obtained by reading the
> ath6kl_vif->ch_hint field.
>
> But, is this correct?

I did a quick look. To me using ch_hint looks correct.

> I couldn't see any real relation between the ch_hint field and the
> real frequency (probably because a lot of logic is hidden to the
> driver). I could only understand that the ch_hint field stores the
> frequency passed as parameter during the connection, but I have found
> no guarantee that this is the really used one.

Can you be more specific, please?

To me it looks that ch_hint is used both with ath6kl_wmi_reconnect_cmd()
and ath6kl_wmi_connect_cmd() commands, which both are used to connect to
a network. I don't see any other variables used for specifying the
frequency to the firmware. But I could just be blind...

--
Kalle Valo

2013-06-03 14:59:13

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 1/2] nl80211: allow sending CMD_FRAME without specifying any frequency

On Mon, 2013-06-03 at 08:39 +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli <[email protected]>
>
> Users may want to send a frame on the current channel
> without specifying it.
>
> Make mgmt_tx pass a NULL channel to mac80211 if none has
> been specified by the user.

cfg80211 isn't just a mac80211 frontend ... ;-)

Also, as Nicolas said, ath6kl_mgmt_tx() will crash after this patch if
it's called in AP mode w/o a channel, so you need to think about that.

Tracing looks fine though.

johannes


2013-06-05 10:15:32

by Kalle Valo

[permalink] [raw]
Subject: Re: ath6kl_mgmt_tx with NULL chan

Antonio Quartulli <[email protected]> writes:

> On Wed, Jun 05, 2013 at 02:57:07AM -0700, Kalle Valo wrote:
>> Antonio Quartulli <[email protected]> writes:
>>
>> > I'm looking at ath6kl_mgmt_tx() in ath6kl/cfg80211.c and I've seen that the
>> > currently "configured" frequency can be obtained by reading the
>> > ath6kl_vif->ch_hint field.
>> >
>> > But, is this correct?
>>
>> I did a quick look. To me using ch_hint looks correct.
>>
>> > I couldn't see any real relation between the ch_hint field and the
>> > real frequency (probably because a lot of logic is hidden to the
>> > driver). I could only understand that the ch_hint field stores the
>> > frequency passed as parameter during the connection, but I have found
>> > no guarantee that this is the really used one.
>>
>> Can you be more specific, please?
>>
>> To me it looks that ch_hint is used both with ath6kl_wmi_reconnect_cmd()
>> and ath6kl_wmi_connect_cmd() commands, which both are used to connect to
>> a network. I don't see any other variables used for specifying the
>> frequency to the firmware. But I could just be blind...
>
> I agree with your analysis. My doubt came from the fact that I don't know what
> the firmware does and I was wondering whether it could ignore the channel passed
> as argument on connect for some reason.

It might do that, I'm not involved with the firmware development.

> Actually the doubt was raised due to the variable name "ch_HINT".

Yeah, the name is really misleading. But that's still legacy from the
pre-cleanup driver, so I wouldn't worry about that too much.

> But you are the ath6k expert :-) Therefore I guess this can work.

It should but you never know :)

But when modifying that code, please add a check to make sure that
channel 0 is not used by accident.

--
Kalle Valo

2013-06-05 10:05:26

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/2] nl80211: allow sending CMD_FRAME without specifying any frequency

Antonio Quartulli <[email protected]> writes:

> On Wed, Jun 05, 2013 at 02:47:01AM -0700, Kalle Valo wrote:
>> Antonio Quartulli <[email protected]> writes:
>>
>> > From: Antonio Quartulli <[email protected]>
>> >
>> > Users may want to send a frame on the current channel
>> > without specifying it.
>> >
>> > Make mgmt_tx pass a NULL channel to mac80211 if none has
>> > been specified by the user.
>> >
>> > Signed-off-by: Antonio Quartulli <[email protected]>
>>
>> Why? And what users are we talking about here? It would be nice if the
>> commit log would give some context here for use who nothing about this
>> patch. "Users may want" is not very informative :)
>
> Hello Kalle,
> thanks for your feedback.
>
> Sure, I can change the commit log.
>
> However, I already wrote a couple of (userspace) applications which wanted to
> send a message on the current channel and the only way to do it was to first get
> the current channel and then pass it when sending a CMD_FRAME via nl80211. Of
> course this approach is just a bit racy :-)
>
> Moreover, I'm currently working on improving IBSS/RSN support in wpa_supplicant
> and sending frames on the current channel is needed.
>
> Do you think it is worth mentioning userspace applications like wpa_s in the
> kernel commit message?

I don't know what others think, but to me it is. It makes it easier to
understand why the change is made. And also do we really need the change
or not.

--
Kalle Valo

2013-06-05 10:05:28

by Antonio Quartulli

[permalink] [raw]
Subject: Re: ath6kl_mgmt_tx with NULL chan

On Wed, Jun 05, 2013 at 02:57:07AM -0700, Kalle Valo wrote:
> Antonio Quartulli <[email protected]> writes:
>
> > On Mon, Jun 03, 2013 at 05:04:08PM +0200, Nicolas Cavallari wrote:
> >> On 03/06/2013 16:59, Johannes Berg wrote:
> >> > On Mon, 2013-06-03 at 08:39 +0200, Antonio Quartulli wrote:
> >> >> From: Antonio Quartulli <[email protected]>
> >> >>
> >> >> Users may want to send a frame on the current channel
> >> >> without specifying it.
> >> >>
> >> >> Make mgmt_tx pass a NULL channel to mac80211 if none has
> >> >> been specified by the user.
> >> >
> >> > cfg80211 isn't just a mac80211 frontend ... ;-)
> >> >
> >> > Also, as Nicolas said, ath6kl_mgmt_tx() will crash after this patch if
> >> > it's called in AP mode w/o a channel, so you need to think about that.
> >>
> >> It will crash unconditionally. All ath6kl_mgmt_tx()'s code paths access
> >> chan->center_freq at some point.
> >
> > Hello Nicolas,
> > I'm also CCing Kalle Valo since get_maintainer.pl told me he is the guy for
> > these kind of questions :-)
> >
> > I'm looking at ath6kl_mgmt_tx() in ath6kl/cfg80211.c and I've seen that the
> > currently "configured" frequency can be obtained by reading the
> > ath6kl_vif->ch_hint field.
> >
> > But, is this correct?
>
> I did a quick look. To me using ch_hint looks correct.
>
> > I couldn't see any real relation between the ch_hint field and the
> > real frequency (probably because a lot of logic is hidden to the
> > driver). I could only understand that the ch_hint field stores the
> > frequency passed as parameter during the connection, but I have found
> > no guarantee that this is the really used one.
>
> Can you be more specific, please?
>
> To me it looks that ch_hint is used both with ath6kl_wmi_reconnect_cmd()
> and ath6kl_wmi_connect_cmd() commands, which both are used to connect to
> a network. I don't see any other variables used for specifying the
> frequency to the firmware. But I could just be blind...

I agree with your analysis. My doubt came from the fact that I don't know what
the firmware does and I was wondering whether it could ignore the channel passed
as argument on connect for some reason.

Actually the doubt was raised due to the variable name "ch_HINT".
But you are the ath6k expert :-) Therefore I guess this can work.

Thanks a lot!

Cheers,

--
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara


Attachments:
(No filename) (2.36 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-06-03 18:28:17

by Antonio Quartulli

[permalink] [raw]
Subject: Re: [PATCH 2/2] mac80211: in mgmt_tx use the current channel if none has been specified

On Mon, Jun 03, 2013 at 08:16:03PM +0200, Johannes Berg wrote:
> On Mon, 2013-06-03 at 20:03 +0200, Antonio Quartulli wrote:
> > On Mon, Jun 03, 2013 at 05:01:30PM +0200, Johannes Berg wrote:
> > > On Mon, 2013-06-03 at 08:39 +0200, Antonio Quartulli wrote:
> > >
> > > > @@ -2845,6 +2852,10 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> > > > rcu_read_lock();
> > > > chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> > > >
> > > > + /* if no channel was specified, use the current one */
> > > > + if (chanctx_conf && !chan)
> > > > + chan = chanctx_conf->def.chan;
> > > > +
> > > > if (chanctx_conf)
> > > > need_offchan = chan != chanctx_conf->def.chan;
> > > > else
> > > > @@ -2852,6 +2863,12 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> > > > rcu_read_unlock();
> > > > }
> > > >
> > > > + /* at this point a channel should have been chosen */
> > > > + if (!chan) {
> > > > + ret = -EINVAL;
> > > > + goto out_unlock;
> > > > + }
> > > > +
> > >
> > > These two changes make no sense at all. If you look at the function
> > > you'll see that "chan" isn't used at all after the check,
> >
> > uhm? it is passed to ieee80211_start_roc_work() right after (this part has not
> > been changed).
>
> > 2904 ret = ieee80211_start_roc_work(local, sdata, chan,
>
> No, you can't get there without need_offchan.

ah, you are right. Now I see why these two changes do not make sense at all :-)

Thanks!

--
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara


Attachments:
(No filename) (1.57 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-06-05 10:10:39

by Antonio Quartulli

[permalink] [raw]
Subject: Re: [PATCH 1/2] nl80211: allow sending CMD_FRAME without specifying any frequency

On Wed, Jun 05, 2013 at 03:05:21AM -0700, Kalle Valo wrote:
> Antonio Quartulli <[email protected]> writes:
>
> > On Wed, Jun 05, 2013 at 02:47:01AM -0700, Kalle Valo wrote:
> >> Antonio Quartulli <[email protected]> writes:
> >>
> >> > From: Antonio Quartulli <[email protected]>
> >> >
> >> > Users may want to send a frame on the current channel
> >> > without specifying it.
> >> >
> >> > Make mgmt_tx pass a NULL channel to mac80211 if none has
> >> > been specified by the user.
> >> >
> >> > Signed-off-by: Antonio Quartulli <[email protected]>
> >>
> >> Why? And what users are we talking about here? It would be nice if the
> >> commit log would give some context here for use who nothing about this
> >> patch. "Users may want" is not very informative :)
> >
> > Hello Kalle,
> > thanks for your feedback.
> >
> > Sure, I can change the commit log.
> >
> > However, I already wrote a couple of (userspace) applications which wanted to
> > send a message on the current channel and the only way to do it was to first get
> > the current channel and then pass it when sending a CMD_FRAME via nl80211. Of
> > course this approach is just a bit racy :-)
> >
> > Moreover, I'm currently working on improving IBSS/RSN support in wpa_supplicant
> > and sending frames on the current channel is needed.
> >
> > Do you think it is worth mentioning userspace applications like wpa_s in the
> > kernel commit message?
>
> I don't know what others think, but to me it is. It makes it easier to
> understand why the change is made. And also do we really need the change
> or not.

Oky. Then I'll change the commit message and add these details in the next
version.

Thanks!


--
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara


Attachments:
(No filename) (1.73 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments