Re: [PATCH v9 2/5] cfg80211/mac80211: refactor cfg80211_chandef_dfs_required()

[Luca Coelho <luca@coelho.fi>]

On Tue, 2014-03-11 at 12:45 +0200, Eliad Peller wrote:
> On Mon, Mar 10, 2014 at 11:31 PM, Luciano Coelho
> <luciano.coelho@intel.com> wrote:
> > Some interface types don't require DFS (such as STATION, P2P_CLIENT
> > etc).  In order to centralize these decisions, make
> > cfg80211_chandef_dfs_required() take the iftype into consideration.
> >
> > Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
> > ---
> [...]
> 
> >  int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
> > -                                 const struct cfg80211_chan_def *chandef)
> > +                                 const struct cfg80211_chan_def *chandef,
> > +                                 enum nl80211_iftype iftype)
> >  {
> >         int width;
> > -       int r;
> > +       int ret;
> >
> >         if (WARN_ON(!cfg80211_chandef_valid(chandef)))
> >                 return -EINVAL;
> >
> > -       width = cfg80211_chandef_get_width(chandef);
> > -       if (width < 0)
> > -               return -EINVAL;
> > +       switch (iftype) {
> > +       case NL80211_IFTYPE_ADHOC:
> > +       case NL80211_IFTYPE_AP:
> > +       case NL80211_IFTYPE_P2P_GO:
> > +       case NL80211_IFTYPE_MESH_POINT:
> > +               width = cfg80211_chandef_get_width(chandef);
> > +               if (width < 0)
> > +                       return -EINVAL;
> >
> it's a matter of style, but i think bailing out in non-relevant cases
> and taking the code out of the switch looks better (less indentation,
> etc.).

Yeah, it's a style issue and I tend to agree with you.  But I also think
it's good to have a switch where we can easily see what happens with
each interface type.

I won't change this now, but we can refactor this at a later point if it
starts getting annoying. ;)



> [...]
> 
> > +       case NL80211_IFTYPE_STATION:
> > +       case NL80211_IFTYPE_P2P_CLIENT:
> > +       case NL80211_IFTYPE_MONITOR:
> > +       case NL80211_IFTYPE_AP_VLAN:
> > +       case NL80211_IFTYPE_WDS:
> > +       case NL80211_IFTYPE_P2P_DEVICE:
> > +               break;
> > +       case NL80211_IFTYPE_UNSPECIFIED:
> > +       case NUM_NL80211_IFTYPES:
> > +               WARN_ON(1);
> > +       }
> 
> [...]
> 
> > @@ -671,7 +700,8 @@ bool cfg80211_reg_can_beacon(struct wiphy *wiphy,
> > -       if (cfg80211_chandef_dfs_required(wiphy, chandef) > 0 &&
> > +       if (cfg80211_chandef_dfs_required(wiphy, chandef,
> > +                                         NL80211_IFTYPE_UNSPECIFIED) > 0 &&
> wouldn't this WARN_ON()?

Yep, you're right.  Previously I was calling this and I didn't really
care what was the interface type.  But Johannes asked me to change the
cfg80211_dfs_required() code to warn if unspecified was passed and I
forgot to change it here.  I'll either have to add the iftype to the
cfg80211_reg_can_beacon() function arguments (which is a bit ugly,
because it's not really needed) or I'll have to move the UNSPECIFIED to
some sort of "don't care" in the cfg80211_chandef_dfs_required()
function.


> > @@ -5796,7 +5799,8 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
> >         if (wdev->cac_started)
> >                 return -EBUSY;
> >
> > -       err = cfg80211_chandef_dfs_required(wdev->wiphy, &chandef);
> > +       err = cfg80211_chandef_dfs_required(wdev->wiphy, &chandef,
> > +                                           NL80211_IFTYPE_UNSPECIFIED);
> ditto.

Ditto. :)

--
Cheers,
Luca.