| Message ID | 1370241587-2609-1-git-send-email-ordex@autistici.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 03/06/2013 08:39, Antonio Quartulli wrote: > From: Antonio Quartulli <antonio@open-mesh.com> > > 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 <antonio@open-mesh.com> > --- > net/wireless/nl80211.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > Have you tested tracing or ath6kl ? -- 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
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 <antonio@open-mesh.com> > > > > 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 <antonio@open-mesh.com> > > --- > > 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,
On Mon, 2013-06-03 at 08:39 +0200, Antonio Quartulli wrote: > From: Antonio Quartulli <antonio@open-mesh.com> > > 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 -- 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
On 03/06/2013 16:59, Johannes Berg wrote: > On Mon, 2013-06-03 at 08:39 +0200, Antonio Quartulli wrote: >> From: Antonio Quartulli <antonio@open-mesh.com> >> >> 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. -- 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
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 <antonio@open-mesh.com> > >> > >> 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 <ordex@autistici.org> writes: > From: Antonio Quartulli <antonio@open-mesh.com> > > 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 <antonio@open-mesh.com> 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 :)
On Wed, Jun 05, 2013 at 02:47:01AM -0700, Kalle Valo wrote: > Antonio Quartulli <ordex@autistici.org> writes: > > > From: Antonio Quartulli <antonio@open-mesh.com> > > > > 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 <antonio@open-mesh.com> > > 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 <ordex@autistici.org> 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 <antonio@open-mesh.com> >> >> >> >> 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...
On Wed, Jun 05, 2013 at 02:57:07AM -0700, Kalle Valo wrote: > Antonio Quartulli <ordex@autistici.org> 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 <antonio@open-mesh.com> > >> >> > >> >> 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 <antonio@open-mesh.com> writes: > On Wed, Jun 05, 2013 at 02:47:01AM -0700, Kalle Valo wrote: >> Antonio Quartulli <ordex@autistici.org> writes: >> >> > From: Antonio Quartulli <antonio@open-mesh.com> >> > >> > 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 <antonio@open-mesh.com> >> >> 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.
On Wed, Jun 05, 2013 at 03:05:21AM -0700, Kalle Valo wrote: > Antonio Quartulli <antonio@open-mesh.com> writes: > > > On Wed, Jun 05, 2013 at 02:47:01AM -0700, Kalle Valo wrote: > >> Antonio Quartulli <ordex@autistici.org> writes: > >> > >> > From: Antonio Quartulli <antonio@open-mesh.com> > >> > > >> > 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 <antonio@open-mesh.com> > >> > >> 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 <antonio@open-mesh.com> writes: > On Wed, Jun 05, 2013 at 02:57:07AM -0700, Kalle Valo wrote: >> Antonio Quartulli <ordex@autistici.org> 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.
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,
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);