LinuxLists.cc - [PATCHv2 1/4] nl80211: allow sending CMD_FRAME without specifying any frequency

2013-06-05 11:55:10

Subject: [PATCHv2 1/4] 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.

This is particularly useful for the correct implementation
of the IBSS/RSN support in wpa_supplicant which requires to
receive and send AUTH frames.

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

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

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 88e820b..06af395 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -7139,6 +7139,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:
@@ -7146,7 +7149,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;
@@ -7174,9 +7176,15 @@ 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
+ * the driver. 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-05 11:55:15

by Antonio Quartulli

[permalink] [raw]

Subject: [PATCHv2 4/4] ath6kl: make mgmt_tx accept a NULL channel

From: Antonio Quartulli <[email protected]>

cfg80211 passes a NULL channel to mgmt_tx if the frame has
to be sent on the one currently in use by the device.
Make the implementation of mgmt_tx correctly handle this
case

Cc: Kalle Valo <[email protected]>
Cc: Nicolas Cavallari <[email protected]>
Signed-off-by: Antonio Quartulli <[email protected]>
---
drivers/net/wireless/ath/ath6kl/cfg80211.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c
index f7995b2..b542203 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -3175,10 +3175,17 @@ static int ath6kl_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
{
struct ath6kl_vif *vif = ath6kl_vif_from_wdev(wdev);
struct ath6kl *ar = ath6kl_priv(vif->ndev);
- u32 id;
+ u32 id, freq;
const struct ieee80211_mgmt *mgmt;
bool more_data, queued;

+ /* default to the current channel, but use the one specified as argument
+ * if any
+ */
+ freq = vif->ch_hint;
+ if (chan)
+ freq = chan->center_freq;
+
mgmt = (const struct ieee80211_mgmt *) buf;
if (vif->nw_type == AP_NETWORK && test_bit(CONNECTED, &vif->flags) &&
ieee80211_is_probe_resp(mgmt->frame_control) &&
@@ -3188,8 +3195,7 @@ static int ath6kl_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
* command to allow the target to fill in the generic IEs.
*/
*cookie = 0; /* TX status not supported */
- return ath6kl_send_go_probe_resp(vif, buf, len,
- chan->center_freq);
+ return ath6kl_send_go_probe_resp(vif, buf, len, freq);
}

id = vif->send_action_id++;
@@ -3205,17 +3211,14 @@ static int ath6kl_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,

/* AP mode Power saving processing */
if (vif->nw_type == AP_NETWORK) {
- queued = ath6kl_mgmt_powersave_ap(vif,
- id, chan->center_freq,
- wait, buf,
- len, &more_data, no_cck);
+ queued = ath6kl_mgmt_powersave_ap(vif, id, freq, wait, buf, len,
+ &more_data, no_cck);
if (queued)
return 0;
}

- return ath6kl_wmi_send_mgmt_cmd(ar->wmi, vif->fw_vif_idx, id,
- chan->center_freq, wait,
- buf, len, no_cck);
+ return ath6kl_wmi_send_mgmt_cmd(ar->wmi, vif->fw_vif_idx, id, freq,
+ wait, buf, len, no_cck);
}

static void ath6kl_mgmt_frame_register(struct wiphy *wiphy,
--
1.8.1.5

2013-06-05 14:19:16

by Antonio Quartulli

[permalink] [raw]

Subject: Re: [PATCHv2 3/4] brcm80211: make mgmt_tx in brcmfmac accept a NULL channel

On Wed, Jun 05, 2013 at 07:14:07AM -0700, Arend van Spriel wrote:
> On 06/05/2013 03:57 PM, Antonio Quartulli wrote:
> > On Wed, Jun 05, 2013 at 06:53:32AM -0700, Arend van Spriel wrote:
> >
> > [...]
> >
> >>> + freq = cfg->channel;
> >>> + if (chan)
> >>> + freq = chan->center_freq;
> >>> + chan_nr = ieee80211_frequency_to_channel(freq);
> >>
> >> Could you get rid of 'freq' variable and use
> >> ieee80211_frequency_to_channel() on cfg->channel or chan->center_freq.
> >>
> >
> > I tried that, but the line indented below the if would be longer than 80 chars
> > and breaking it is quite ugly.
> >
> >> Another thing is that cfg->channel can be zero resulting in chan_nr
> >> being zero. I had a quick look whether the device firmware can handle
> >> this. I suspect not, but I will need to ask to be sure.
> >>
> >
> > ok. But when cfg->channel is zero, where is the current freq stored?
>
> It is not. The rf on the device is set to a certain channel so it can be
> retrieved from the device:
>
> brcmf_fil_cmd_int_get(vif->ifp, BRCMF_C_GET_CHANNEL, &freq);
>
> The vif pointer can be obtained using container_of() on the wdev. That
> is done in mgmt_tx() already some lines above. May you should move
> determining the vif pointer and do it unconditional.
>

Yeah, I like this approach.

I'll send v3 of this patchset with this change in it.
@johannes: I'll also revert the order of the patches.

Thanks a lot Arend.

Regards,

--
Antonio Quartulli

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

Attachments:

(No filename) (1.50 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-06-05 14:14:16

by Arend van Spriel

[permalink] [raw]

Subject: Re: [PATCHv2 3/4] brcm80211: make mgmt_tx in brcmfmac accept a NULL channel

On 06/05/2013 03:57 PM, Antonio Quartulli wrote:
> On Wed, Jun 05, 2013 at 06:53:32AM -0700, Arend van Spriel wrote:
>
> [...]
>
>>> + freq = cfg->channel;
>>> + if (chan)
>>> + freq = chan->center_freq;
>>> + chan_nr = ieee80211_frequency_to_channel(freq);
>>
>> Could you get rid of 'freq' variable and use
>> ieee80211_frequency_to_channel() on cfg->channel or chan->center_freq.
>>
>
> I tried that, but the line indented below the if would be longer than 80 chars
> and breaking it is quite ugly.
>
>> Another thing is that cfg->channel can be zero resulting in chan_nr
>> being zero. I had a quick look whether the device firmware can handle
>> this. I suspect not, but I will need to ask to be sure.
>>
>
> ok. But when cfg->channel is zero, where is the current freq stored?

It is not. The rf on the device is set to a certain channel so it can be
retrieved from the device:

brcmf_fil_cmd_int_get(vif->ifp, BRCMF_C_GET_CHANNEL, &freq);

The vif pointer can be obtained using container_of() on the wdev. That
is done in mgmt_tx() already some lines above. May you should move
determining the vif pointer and do it unconditional.

Regards,
Arend

2013-06-05 13:53:46

by Arend van Spriel

[permalink] [raw]

Subject: Re: [PATCHv2 3/4] brcm80211: make mgmt_tx in brcmfmac accept a NULL channel

On 06/05/2013 01:53 PM, Antonio Quartulli wrote:
> From: Antonio Quartulli <[email protected]>
>
> cfg80211 passes a NULL channel to mgmt_tx if the frame has
> to be sent on the one currently in use by the device.
> Make the implementation of mgmt_tx correctly handle this
> case
>
> Cc: Arend van Spriel <[email protected]>
> Cc: [email protected]
> Signed-off-by: Antonio Quartulli <[email protected]>
> ---
> drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> index 6d758f2..dcb0c00 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
> @@ -3917,6 +3917,7 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> struct brcmf_fil_af_params_le *af_params;
> bool ack;
> s32 chan_nr;
> + u32 freq;
>
> brcmf_dbg(TRACE, "Enter\n");
>
> @@ -3968,8 +3969,13 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
> memcpy(&af_params->bssid[0], &mgmt->bssid[0], ETH_ALEN);
> /* Add the length exepted for 802.11 header */
> action_frame->len = cpu_to_le16(len - DOT11_MGMT_HDR_LEN);
> - /* Add the channel */
> - chan_nr = ieee80211_frequency_to_channel(chan->center_freq);
> + /* Add the channel. Default to the current one, but use the one
> + * specified as parameter if any
> + */
> + freq = cfg->channel;
> + if (chan)
> + freq = chan->center_freq;
> + chan_nr = ieee80211_frequency_to_channel(freq);

Could you get rid of 'freq' variable and use
ieee80211_frequency_to_channel() on cfg->channel or chan->center_freq.

Another thing is that cfg->channel can be zero resulting in chan_nr
being zero. I had a quick look whether the device firmware can handle
this. I suspect not, but I will need to ask to be sure.

Regards,
Arend

2013-06-05 11:55:10

by Antonio Quartulli

[permalink] [raw]

Subject: [PATCHv2 2/4] mac80211: make mgmt_tx accept a NULL channel

From: Antonio Quartulli <[email protected]>

cfg80211 passes a NULL channel to mgmt_tx if the frame has
to be sent on the one currently in use by the device.
Make the implementation of mgmt_tx correctly handle this
case. Fail if offchan is required.

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

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 3062210..617c5c8 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2838,6 +2838,12 @@ 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 */
@@ -2847,10 +2853,14 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
rcu_read_lock();
chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);

- if (chanctx_conf)
- need_offchan = chan != chanctx_conf->def.chan;
- else
+ if (chanctx_conf) {
+ need_offchan = chan && (chan != chanctx_conf->def.chan);
+ } else if (!chan) {
+ ret = -EINVAL;
+ goto out_unlock;
+ } else {
need_offchan = true;
+ }
rcu_read_unlock();
}

--
1.8.1.5

2013-06-05 11:55:13

by Antonio Quartulli

[permalink] [raw]

Subject: [PATCHv2 3/4] brcm80211: make mgmt_tx in brcmfmac accept a NULL channel

2013-06-05 13:59:55

by Antonio Quartulli

[permalink] [raw]

Subject: Re: [PATCHv2 3/4] brcm80211: make mgmt_tx in brcmfmac accept a NULL channel

On Wed, Jun 05, 2013 at 06:53:32AM -0700, Arend van Spriel wrote:

[...]

> > + freq = cfg->channel;
> > + if (chan)
> > + freq = chan->center_freq;
> > + chan_nr = ieee80211_frequency_to_channel(freq);
>
> Could you get rid of 'freq' variable and use
> ieee80211_frequency_to_channel() on cfg->channel or chan->center_freq.
>

I tried that, but the line indented below the if would be longer than 80 chars
and breaking it is quite ugly.

> Another thing is that cfg->channel can be zero resulting in chan_nr
> being zero. I had a quick look whether the device firmware can handle
> this. I suspect not, but I will need to ask to be sure.
>

ok. But when cfg->channel is zero, where is the current freq stored?

Regards,

--
Antonio Quartulli

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

Attachments:

(No filename) (816.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments