Re: [RFC V2 1/2] mac80211: Use a cfg80211_chan_def in ieee80211_hw_conf_chan

[Karl Beldan <karl.beldan@gmail.com>]

On Fri, Mar 22, 2013 at 11:55:37AM +0100, Johannes Berg wrote:
> On Tue, 2013-03-19 at 01:23 +0100, Karl Beldan wrote:
> 
> > @@ -106,7 +104,7 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
> >  	WARN_ON_ONCE(ctx->refcount != 0);
> >  
> >  	if (!local->use_chanctx) {
> > -		local->_oper_channel_type = NL80211_CHAN_NO_HT;
> > +		local->_oper_chandef.width = NL80211_CHAN_WIDTH_20_NOHT;
> >  		ieee80211_hw_config(local, 0);
> 
> You also have to reset center_freq1/2 here to make it a valid chandef in
> all cases, otherwise you can end up with e.g. "2412 noht20 2422 0" which
> makes no sense.
> 
> I think it'd be worth sticking a
> 
> WARN_ON(!cfg80211_chandef_valid(...))
> 
> into ieee80211_hw_conf_chan().
> 
I addressed the reset of center_freq{1,2} when I understood how we
handle them, will add the WARN_ON.

> > --- a/net/mac80211/main.c
> > +++ b/net/mac80211/main.c
> > @@ -92,45 +92,54 @@ static void ieee80211_reconfig_filter(struct work_struct *work)
> >  	ieee80211_configure_filter(local);
> >  }
> >  
> > +static int ieee80211_chandef_cmp(const struct cfg80211_chan_def *def1,
> > +				 const struct cfg80211_chan_def *def2)
> 
> Shouldn't be needed?
> 
> > -			local->hw.conf.channel =
> > -			local->_oper_channel = &sband->channels[0];
> > -			local->hw.conf.channel_type = NL80211_CHAN_NO_HT;
> > +			local->hw.conf.chandef.chan =
> > +			local->_oper_chandef.chan = &sband->channels[0];
> > +			local->hw.conf.chandef.width = NL80211_CHAN_NO_HT;
> 
> wrong constant
> 

Above issues already addressed.

> > +++ b/net/mac80211/mlme.c
> > @@ -985,7 +985,9 @@ static void ieee80211_chswitch_work(struct work_struct *work)
> >  {
> >  	struct ieee80211_sub_if_data *sdata =
> >  		container_of(work, struct ieee80211_sub_if_data, u.mgd.chswitch_work);
> > +	struct ieee80211_local *local = sdata->local;
> >  	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
> > +	int cfreq_off;
> >  
> >  	if (!ieee80211_sdata_running(sdata))
> >  		return;
> > @@ -994,21 +996,25 @@ static void ieee80211_chswitch_work(struct work_struct *work)
> >  	if (!ifmgd->associated)
> >  		goto out;
> >  
> > -	sdata->local->_oper_channel = sdata->local->csa_channel;
> > -	if (!sdata->local->ops->channel_switch) {
> > +	cfreq_off = local->csa_channel->center_freq -
> > +		local->_oper_chandef.chan->center_freq;
> > +
> > +	local->_oper_chandef.center_freq1 += cfreq_off;
> > +	local->_oper_chandef.center_freq2 += cfreq_off;
> 
> Adjusting the center_freq1 is quite clearly wrong. :-)
> 
> Besides, it might not even be supported by the driver.
> 
Yes, the next serie will downgrade to NL80211_CHAN_WIDTH_20_NOHT.
I know this is not acceptable, I will get back to this very soon.

> Since we really don't handle anything but 20 MHz channel switches
> correctly, maybe we should just disconnect in that case, like we do in
> the chanctx case today. I'd prefer that as a separate patch (coming
> before this one) though.
> 
> Of course, patches to make it handle it correctly would also be welcome
> -- I started playing with it here:
> 
> http://git.kernel.org/cgit/linux/kernel/git/jberg/mac80211-next.git/commit/?h=wip&id=cb0308430993e96c79b3449cf0b3c744ef62b50e
> 
> (and the parent commit)
> 
Ok, thanks !
I should get back to this very soon.

> > +++ b/net/mac80211/trace.h
> > @@ -269,7 +269,6 @@ DEFINE_EVENT(local_sdata_addr_evt, drv_remove_interface,
> >  		 struct ieee80211_sub_if_data *sdata),
> >  	TP_ARGS(local, sdata)
> >  );
> > -
> >  TRACE_EVENT(drv_config,
> 
> please keep that :-)
> 
Oops, did it again.

> >  	TP_PROTO(struct ieee80211_local *local,
> >  		 u32 changed),
> > @@ -286,8 +285,10 @@ TRACE_EVENT(drv_config,
> >  		__field(u16, listen_interval)
> >  		__field(u8, long_frame_max_tx_count)
> >  		__field(u8, short_frame_max_tx_count)
> > -		__field(int, center_freq)
> > -		__field(int, channel_type)
> > +		__field(int, control_freq)
> > +		__field(int, center_freq1)
> > +		__field(int, center_freq2)
> > +		__field(int, chan_width)
> 
> There should be ready macros for chandef tracing -- can you use them?
> 
> > +		LOCAL_PR_FMT " ch:%#x" CHANDEF_PR_FMT,
> > +		LOCAL_PR_ARG, __entry->changed, CHANDEF_PR_ARG
> 
> Hm so you used some? Why not all?
> 
I would have liked to use CHANDEF_ENTRY and CHANDEF_ASSIGN but they
don't check for "chandef.chan != NULL" which would Oops when using
chanctxes (and I hesitated to make it too invasive although I would
gladly add the check if that doesn't bother you.

Thanks for your feedback.
 
Karl