On Mon, Mar 18, 2013 at 08:25:10PM +0100, Johannes Berg wrote:
> On Sun, 2013-03-17 at 21:30 +0100, Karl Beldan wrote:
>
> > + if (conf->chandef.chan)
>
> > + else
> > + wiphy_debug(hw->wiphy,
> > + "%s (freq=0/%s idle=%d ps=%d smps=%s)\n",
>
> I don't think the else should be allowed to happen.
>
> > --- a/net/mac80211/cfg.c
> > +++ b/net/mac80211/cfg.c
> > @@ -805,8 +805,7 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy,
> > IEEE80211_CHANCTX_EXCLUSIVE);
> > }
> > } else if (local->open_count == local->monitors) {
> > - local->_oper_channel = chandef->chan;
> > - local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> > + memcpy(&local->oper_chandef, chandef, sizeof(local->oper_chandef));
>
> Somehow I'd prefer an assignment:
>
> local->oper_chandef = *chandef;
>
> I know it'll just be a memcpy() again, but ... reads nicer, imho.
>
> > + memcpy(chandef, &local->oper_chandef, sizeof(chandef));
>
> ditto
>
> > @@ -22,7 +22,9 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
> > drv_change_chanctx(local, ctx, IEEE80211_CHANCTX_CHANGE_WIDTH);
> >
> > if (!local->use_chanctx) {
> > - local->_oper_channel_type = cfg80211_get_chandef_type(chandef);
> > + local->oper_chandef.width = chandef->width;
> > + local->oper_chandef.center_freq1 = chandef->center_freq1;
> > + local->oper_chandef.center_freq2 = chandef->center_freq2;
>
> Why not assign all here as well? Is the channel pointer itself special?
>
> > - /* For backward compatibility only -- do not use */
> > - struct ieee80211_channel *_oper_channel;
> > - enum nl80211_channel_type _oper_channel_type;
> > + struct cfg80211_chan_def oper_chandef;
>
> You shouldn't remove the comment, and I think you should also keep the
> leading underscore -- this is still for backward compatibility with
> non-chanctx drivers only, and mac80211 internally must use chanctxes all
> the way except in SW scan and SW roc code.
>
> > +static int ieee80211_chandef_cmp(const struct cfg80211_chan_def *def1,
> > + const struct cfg80211_chan_def *def2)
>
> indentation
>
> However this already exists in cfg80211.h --
> cfg80211_chandef_identical().
>
> > static u32 ieee80211_hw_conf_chan(struct ieee80211_local *local)
> > {
> > struct ieee80211_sub_if_data *sdata;
> > + struct cfg80211_chan_def chandef;
>
> better initialize this: struct ... chandef = {};
>
Ok, even though I have done it in the v2 I think I get why now (after
the comment of Mahesh on center_freq2 != 0).
And the reason I'd see is that you take care of zeroing center_freq{1,2}
when they are not used (i.e wrt to the width), and the code behaves
accordingly ?
Then, indeed I could obviously drop cfg80211_chandef_identical for
cfg80211_chandef_identical.
Karl
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
