diff mbox

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

Message ID 1370444989-2095-1-git-send-email-ordex@autistici.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Antonio Quartulli June 5, 2013, 3:09 p.m. UTC 
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(-)

Comments

Johannes Berg June 11, 2013, 11:55 a.m. UTC | #1 
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


--
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
Antonio Quartulli June 11, 2013, 11:56 a.m. UTC | #2 
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 June 11, 2013, 11:57 a.m. UTC | #3 
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?
Johannes Berg June 11, 2013, 11:58 a.m. UTC | #4 
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

--
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
Johannes Berg June 11, 2013, 12:07 p.m. UTC | #5 
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

--
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
Antonio Quartulli June 11, 2013, 12:07 p.m. UTC | #6 
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,
diff mbox

Patch

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