[PATCHv4 1/4] mac80211: make mgmt_tx accept a NULL channel

[PATCHv4 1/4] mac80211: make mgmt_tx accept a NULL channel

[Antonio Quartulli]

From: Antonio Quartulli <antonio@open-mesh.com>

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 <antonio@open-mesh.com>
---
 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

[PATCHv4 2/4] brcm80211: make mgmt_tx in brcmfmac accept a NULL channel

[Antonio Quartulli]

From: Antonio Quartulli <antonio@open-mesh.com>

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 <arend@broadcom.com>
Cc: brcm80211-dev-list@broadcom.com
Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
---

v3:
- read current freq from the firmware via BRCMF_C_GET_CHANNEL

 drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c
index 6d758f2..8bd256b 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");
 
@@ -3929,6 +3930,8 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 		return -EPERM;
 	}
 
+	vif = container_of(wdev, struct brcmf_cfg80211_vif, wdev);
+
 	if (ieee80211_is_probe_resp(mgmt->frame_control)) {
 		/* Right now the only reason to get a probe response */
 		/* is for p2p listen response or for p2p GO from     */
@@ -3944,7 +3947,6 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 		ie_offset =  DOT11_MGMT_HDR_LEN +
 			     DOT11_BCN_PRB_FIXED_LEN;
 		ie_len = len - ie_offset;
-		vif = container_of(wdev, struct brcmf_cfg80211_vif, wdev);
 		if (vif == cfg->p2p.bss_idx[P2PAPI_BSSCFG_PRIMARY].vif)
 			vif = cfg->p2p.bss_idx[P2PAPI_BSSCFG_DEVICE].vif;
 		err = brcmf_vif_set_mgmt_ie(vif,
@@ -3968,8 +3970,15 @@ 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. Use the one specified as parameter if any or
+		 * the current one (got from the firmware) otherwise
+		 */
+		if (chan)
+			freq = chan->center_freq;
+		else
+			brcmf_fil_cmd_int_get(vif->ifp, BRCMF_C_GET_CHANNEL,
+					      &freq);
+		chan_nr = ieee80211_frequency_to_channel(freq);
 		af_params->channel = cpu_to_le32(chan_nr);
 
 		memcpy(action_frame->data, &buf[DOT11_MGMT_HDR_LEN],
-- 
1.8.1.5

Re: [PATCHv4 2/4] brcm80211: make mgmt_tx in brcmfmac accept a NULL channel

[Arend van Spriel]

On 06/05/2013 05:09 PM, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@open-mesh.com>
>
> 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 <arend@broadcom.com>
> Cc: brcm80211-dev-list@broadcom.com
> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> ---

For my name above you (or Johannes) can change 'Cc:' to 'Acked-by:'

Gr. AvS

> v3:
> - read current freq from the firmware via BRCMF_C_GET_CHANNEL
>
>   drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
>

Re: [PATCHv4 2/4] brcm80211: make mgmt_tx in brcmfmac accept a NULL channel

[Arend van Spriel]

On 06/05/2013 05:34 PM, Arend van Spriel wrote:
> On 06/05/2013 05:09 PM, Antonio Quartulli wrote:
>> From: Antonio Quartulli <antonio@open-mesh.com>
>>
>> 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 <arend@broadcom.com>
>> Cc: brcm80211-dev-list@broadcom.com
>> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
>> ---
>
> For my name above you (or Johannes) can change 'Cc:' to 'Acked-by:'

Or John?

> Gr. AvS
>
>> v3:
>> - read current freq from the firmware via BRCMF_C_GET_CHANNEL
>>
>>   drivers/net/wireless/brcm80211/brcmfmac/wl_cfg80211.c | 15
>> ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>
>

Re: [PATCHv4 2/4] brcm80211: make mgmt_tx in brcmfmac accept a NULL channel

[Johannes Berg]

On Wed, 2013-06-05 at 17:35 +0200, Arend van Spriel wrote:
> On 06/05/2013 05:34 PM, Arend van Spriel wrote:
> > On 06/05/2013 05:09 PM, Antonio Quartulli wrote:
> >> From: Antonio Quartulli <antonio@open-mesh.com>
> >>
> >> 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 <arend@broadcom.com>
> >> Cc: brcm80211-dev-list@broadcom.com
> >> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> >> ---
> >
> > For my name above you (or Johannes) can change 'Cc:' to 'Acked-by:'
> 
> Or John?

I think I'll take the whole series if that's ok with everyone, otherwise
we get synchronisation issues between John and myself :)

johannes

Re: [PATCHv4 2/4] brcm80211: make mgmt_tx in brcmfmac accept a NULL channel

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

iOn Wed, Jun 05, 2013 at 08:40:10AM -0700, Johannes Berg wrote:
> On Wed, 2013-06-05 at 17:35 +0200, Arend van Spriel wrote:
> > On 06/05/2013 05:34 PM, Arend van Spriel wrote:
> > > On 06/05/2013 05:09 PM, Antonio Quartulli wrote:
> > >> From: Antonio Quartulli <antonio@open-mesh.com>
> > >>
> > >> 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 <arend@broadcom.com>
> > >> Cc: brcm80211-dev-list@broadcom.com
> > >> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> > >> ---
> > >
> > > For my name above you (or Johannes) can change 'Cc:' to 'Acked-by:'
> > 
> > Or John?
> 
> I think I'll take the whole series if that's ok with everyone, otherwise
> we get synchronisation issues between John and myself :)
>

Yeah, I think that's the best option.

Thanks everybody!

-- 
Antonio Quartulli

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv4 2/4] brcm80211: make mgmt_tx in brcmfmac accept a NULL channel

[John W. Linville]

On Wed, Jun 05, 2013 at 05:40:10PM +0200, Johannes Berg wrote:
> On Wed, 2013-06-05 at 17:35 +0200, Arend van Spriel wrote:
> > On 06/05/2013 05:34 PM, Arend van Spriel wrote:
> > > On 06/05/2013 05:09 PM, Antonio Quartulli wrote:
> > >> From: Antonio Quartulli <antonio@open-mesh.com>
> > >>
> > >> 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 <arend@broadcom.com>
> > >> Cc: brcm80211-dev-list@broadcom.com
> > >> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> > >> ---
> > >
> > > For my name above you (or Johannes) can change 'Cc:' to 'Acked-by:'
> > 
> > Or John?
> 
> I think I'll take the whole series if that's ok with everyone, otherwise
> we get synchronisation issues between John and myself :)

Yes, please.

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[PATCHv4 3/4] ath6kl: make mgmt_tx accept a NULL channel

[Antonio Quartulli]

From: Antonio Quartulli <antonio@open-mesh.com>

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 <kvalo@qca.qualcomm.com>
Cc: Nicolas Cavallari <Nicolas.Cavallari@lri.fr>
Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
---

v4:
- ensure to never send freq zero to the firmware


 drivers/net/wireless/ath/ath6kl/cfg80211.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c
index f7995b2..2437ad2 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -3175,10 +3175,21 @@ 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;
+
+	/* never send freq zero to the firmware */
+	if (WARN_ON(freq == 0))
+		return -EINVAL;
+
 	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 +3199,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 +3215,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

Re: [PATCHv4 3/4] ath6kl: make mgmt_tx accept a NULL channel

[Kalle Valo]

Antonio Quartulli <ordex@autistici.org> writes:

> From: Antonio Quartulli <antonio@open-mesh.com>
>
> 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 <kvalo@qca.qualcomm.com>
> Cc: Nicolas Cavallari <Nicolas.Cavallari@lri.fr>
> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>

Thanks.

Acked-by: Kalle Valo <kvalo@qca.qualcomm.com>

-- 
Kalle Valo

[PATCHv4 4/4] nl80211: allow sending CMD_FRAME without specifying any frequency

[Antonio Quartulli]

From: Antonio Quartulli <antonio@open-mesh.com>

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 <antonio@open-mesh.com>
---

v3:
- moved from 1/4 to 4/4


 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

Re: [PATCHv4 4/4] nl80211: allow sending CMD_FRAME without specifying any frequency

[Johannes Berg]

On Wed, 2013-06-05 at 17:09 +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.
> 
> 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 <antonio@open-mesh.com>
> ---
> 
> v3:
> - moved from 1/4 to 4/4
> 
> 
>  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;
> +	}

I think not specifying a channel but setting
NL80211_ATTR_OFFCHANNEL_TX_OK must be an error.

johannes

Re: [PATCHv4 4/4] nl80211: allow sending CMD_FRAME without specifying any frequency

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

On Tue, Jun 11, 2013 at 01:53:18PM +0200, Johannes Berg wrote:
> > +	/* 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;
> > +	}
> 
> I think not specifying a channel but setting
> NL80211_ATTR_OFFCHANNEL_TX_OK must be an error.

You are right. Indeed this check is done in mac80211. Other drivers do not use
NL80211_ATTR_OFFCHANNEL_TX_OK at all. But for consistency I guess it would be
correct to do the check here too.

I'll send a new version including this change.

Cheers,


-- 
Antonio Quartulli

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv4 1/4] mac80211: make mgmt_tx accept a NULL channel

[Johannes Berg]

On Wed, 2013-06-05 at 17:09 +0200, Antonio Quartulli wrote:

> +	if (need_offchan && !chan)
> +		return -EINVAL;

> @@ -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();

It seems this would be clearer?
http://p.sipsolutions.net/b8a03c85f0e8b89f.txt

johannes

Re: [PATCHv4 1/4] mac80211: make mgmt_tx accept a NULL channel

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

On Tue, Jun 11, 2013 at 01:55:18PM +0200, Johannes Berg wrote:
> On Wed, 2013-06-05 at 17:09 +0200, Antonio Quartulli wrote:
> 
> > +	if (need_offchan && !chan)
> > +		return -EINVAL;
> 
> > @@ -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();
> 
> It seems this would be clearer?
> http://p.sipsolutions.net/b8a03c85f0e8b89f.txt

agreed :)

I'll include this change.

Cheers,

-- 
Antonio Quartulli

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv4 1/4] mac80211: make mgmt_tx accept a NULL channel

[Johannes Berg]

On Tue, 2013-06-11 at 13:55 +0200, Johannes Berg wrote:
> On Wed, 2013-06-05 at 17:09 +0200, Antonio Quartulli wrote:
> 
> > +	if (need_offchan && !chan)
> > +		return -EINVAL;
> 
> > @@ -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();
> 
> It seems this would be clearer?
> http://p.sipsolutions.net/b8a03c85f0e8b89f.txt

Actually, no, I like your version better.

johanes

Re: [PATCHv4 1/4] mac80211: make mgmt_tx accept a NULL channel

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]

On Tue, Jun 11, 2013 at 01:58:07PM +0200, Johannes Berg wrote:
> On Tue, 2013-06-11 at 13:55 +0200, Johannes Berg wrote:
> > On Wed, 2013-06-05 at 17:09 +0200, Antonio Quartulli wrote:
> > 
> > > +	if (need_offchan && !chan)
> > > +		return -EINVAL;
> > 
> > > @@ -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();
> > 
> > It seems this would be clearer?
> > http://p.sipsolutions.net/b8a03c85f0e8b89f.txt
> 
> Actually, no, I like your version better.

uhm, having one else less looks better..why did you change your mind?
Maybe you version is not entirely equivalent?


-- 
Antonio Quartulli

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv4 1/4] mac80211: make mgmt_tx accept a NULL channel

[Johannes Berg]

On Tue, 2013-06-11 at 13:57 +0200, Antonio Quartulli wrote:
> On Tue, Jun 11, 2013 at 01:58:07PM +0200, Johannes Berg wrote:
> > On Tue, 2013-06-11 at 13:55 +0200, Johannes Berg wrote:
> > > On Wed, 2013-06-05 at 17:09 +0200, Antonio Quartulli wrote:
> > > 
> > > > +	if (need_offchan && !chan)
> > > > +		return -EINVAL;
> > > 
> > > > @@ -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();
> > > 
> > > It seems this would be clearer?
> > > http://p.sipsolutions.net/b8a03c85f0e8b89f.txt
> > 
> > Actually, no, I like your version better.
> 
> uhm, having one else less looks better..why did you change your mind?
> Maybe you version is not entirely equivalent?

It's not, but that wouldn't matter, it's different I think only if you
have input from cfg80211 as "!chan && offchan".

It just seems to me that your version is clearer, first you check if you
need offchan, and if you do but don't have a channel you abort. Then you
check if you might need offchan for another reason, etc.

johannes

Re: [PATCHv4 1/4] mac80211: make mgmt_tx accept a NULL channel

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

On Tue, Jun 11, 2013 at 02:07:17PM +0200, Johannes Berg wrote:
> On Tue, 2013-06-11 at 13:57 +0200, Antonio Quartulli wrote:
> > On Tue, Jun 11, 2013 at 01:58:07PM +0200, Johannes Berg wrote:
> > > On Tue, 2013-06-11 at 13:55 +0200, Johannes Berg wrote:
> > > > On Wed, 2013-06-05 at 17:09 +0200, Antonio Quartulli wrote:
> > > > 
> > > > > +	if (need_offchan && !chan)
> > > > > +		return -EINVAL;
> > > > 
> > > > > @@ -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();
> > > > 
> > > > It seems this would be clearer?
> > > > http://p.sipsolutions.net/b8a03c85f0e8b89f.txt
> > > 
> > > Actually, no, I like your version better.
> > 
> > uhm, having one else less looks better..why did you change your mind?
> > Maybe you version is not entirely equivalent?
> 
> It's not, but that wouldn't matter, it's different I think only if you
> have input from cfg80211 as "!chan && offchan".
> 
> It just seems to me that your version is clearer, first you check if you
> need offchan, and if you do but don't have a channel you abort. Then you
> check if you might need offchan for another reason, etc.


yeah. Ok I'll keep my version.

Cheers,

-- 
Antonio Quartulli

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]