Hi Luis,
Thanks for the review :)
Ilan.
> > Note that this is a permissive approach to the FCC definitions, that
> > require a clear assessment that either the platform device is an
> > indoor device, or the device operating the AP is an indoor device,
> > i.e., AC powered.
> > It is assumed that these restrictions are enforced by user space.
> > Furthermore, it is assumed, that if the conditions that allowed for
> > the operation of the GO on such a channel change, it is the
> > responsibility of user space to evacuate the GO from the channel.
>
> In the context of strict regulatory data we currently do not differentiate
> specifically between a device that can initiate radiation between AP and Group
> Owner (GO) but in your earlier patches you proposed a way to do that. I
> reviewed feedback on that patch there. It may make sense however to define
> clearly what you mean by why the flag is being introduced by documenting the
> use case, ie what you describe here. Other regulatory bodies may follow and it
> helps to provide context here.
Sure, I'll add such a use case to the previous patch
> > diff --git a/net/wireless/Kconfig b/net/wireless/Kconfig index
> > de76078..d9e2be7 100644
> > --- a/net/wireless/Kconfig
> > +++ b/net/wireless/Kconfig
> > @@ -102,6 +102,16 @@ config CFG80211_REG_CELLULAR_HINTS
> > This option adds support for drivers that can receive regulatory
> > hints from cellular base stations
> >
> > +config CFG80211_REG_SOFT_CONFIGURATIONS
> > + bool "cfg80211 support for GO operation on additional channels"
> > + depends on CFG80211 && CFG80211_CERTIFICATION_ONUS
> > + ---help---
> > + This option enables the operation of a P2P Group Owner on
> > + additional channels, if there is a clear assessment that
> > + the platform device operates in an indoor environment or
> > + in case that there is an additional BSS interface which is
> > + connected to an AP which is an indoor device.
> > +
>
> I like this approach, indeed this is great work and reflects usage of the onus
> kconfig option appropriately in this case due dilligence required more in part
> on userspace / other components for full regulatory compliance.
>
The credit here should go to Johannes :)
> > +#ifdef CPTCFG_CFG80211_REG_SOFT_CONFIGURATIONS
> > +static int cfg80211_get_unii_band(int freq) {
> > + /* UNII-1 */
> > + if (freq >= 5150 && freq <= 5250)
> > + return 0;
> > +
> > + /* UNII-2 */
> > + if (freq > 5250 && freq <= 5350)
> > + return 1;
> > +
> > + /* UNII-2E */
> > + if (freq >= 5470 && freq <= 5725)
> > + return 2;
> > +
> > + /* UNII-3 */
> > + if (freq > 5725 && freq <= 5825)
> > + return 3;
> > +
> > + WARN_ON(1);
> > + return -EINVAL;
> > +}
> > +#endif
>
> #else?
We considered this approach, but preferred to move the ifdef inside the function.
>
> Are you aware of UNII-1, UNII-2, UNII-2E, UNII-3 are specific world regulatory
> language bands? When I last tried to look for a clear definition I could not find
> a clear definition of them and its why on the ath module on for QCA modules I
> state "we call these" in reference to the UNII nomenclature. If we can get a
> clear resource for its definition that would help here.
>
Maybe these link will help.
http://www.gpo.gov/fdsys/pkg/CFR-2010-title47-vol1/xml/CFR-2010-title47-vol1-part15.xml#seqnum15.403
http://hraunfoss.fcc.gov/edocs_public/attachmatch/FCC-13-22A1.pdf
Anyway, I will use your ''we call these" notation as well :)
> > +
> > +/* For GO only, check if the channel can be used under permissive
> > +conditions
> > + * mandated by the FCC, i.e., the channel is marked as:
>
> Ah -- note - FCC, you are making an FCC specific rule global, that doesn't seems
> right. The implementation should reflect that *some* regulatory bodies are
> implicating software requirements for GO operation and in that case this tries
> to implement such known current interpretations.
>
> Hope is that regulatory bodies will stick to this singular interpretation /
> preference when required.
>
> So my point: your code treats this appropriately as agnostic to the regulatory
> body but your comments do not, just modify the comments more to make it
> more agnostic.
>
Will fix.
> > + * 1. Indoor only: a GO can be operational on such a channel, iff there is
> > + * clear assessment that the platform device is indoor.
> > + * 2. Concurrent GO: a GO can be operational on such a channel, iff there is
> an
> > + * additional station interface connected to an AP on this channel.
> > + *
> > + * TODO: The function is too permissive, as it does not verify the
> > + platform
> > + * device type is indeed indoor, or that the AP is indoor/AC powered.
>
> So to do this properly we need an eventual userspace API to throw to
> kernelspace if we are AC powered? We'll need this to enhance this routine
> here?
>
Not sure about this point. I prefer leaving the exact knowledge of the device type, being AC Powered or not (and which type of AC power) etc. out side of the kernel. The approach I chose with this patch was to only allow to start a GO on such a channel, making the basic verification done above, assuming that the user space component guarantees that all the full restrictions are satisfied.
> > + */
> > +static bool cfg80211_can_go_use_chan(struct cfg80211_registered_device
> *rdev,
> > + struct ieee80211_channel *chan) {
> > +#ifdef CPTCFG_CFG80211_REG_SOFT_CONFIGURATIONS
> > + struct wireless_dev *wdev_iter;
> > +
> > + ASSERT_RTNL();
> > +
> > + if (!(chan->flags & (IEEE80211_CHAN_INDOOR_ONLY |
> > + IEEE80211_CHAN_GO_CONCURRENT)))
> > + return false;
> > +
> > + if (chan->band == IEEE80211_BAND_60GHZ)
> > + return false;
> > +
> > + list_for_each_entry(wdev_iter, &rdev->wdev_list, list) {
> > + struct ieee80211_channel *other_chan = NULL;
> > +
> > + if (wdev_iter->iftype != NL80211_IFTYPE_STATION ||
> > + (!netif_running(wdev_iter->netdev)))
> > + continue;
> > +
> > +
> > + wdev_lock(wdev_iter);
> > + if (wdev_iter->current_bss)
> > + other_chan = wdev_iter->current_bss->pub.channel;
> > + wdev_unlock(wdev_iter);
> > +
> > + if (!other_chan)
> > + continue;
> > +
> > + if (chan == other_chan)
> > + return true;
> > + else if ((chan->band == IEEE80211_BAND_5GHZ) &&
> > + (cfg80211_get_unii_band(chan->center_freq) ==
> > + cfg80211_get_unii_band(other_chan->center_freq)))
> > + return true;
> > + }
> > +#endif
> > + return false;
> > +}
>
> Please wrap code as follows:
>
> #ifdef FOO
> static int foo(void)
> {
> return true;
> }
> #else
> static int foo(void)
> return false;
> }
> #endif
Ok. I can do that.
> In the meantime, while you get all your patch sets properly developed I'd
> recommend to define the code returning false there strictly or perhaps for any
> flag on the channel requiring DFS / no-ibss, or passive scan. The stricter case,
> defining false always, seems to be what you are suggesting here.
I do not think that this is needed here. Returning false, means that the code should test if the PASSIVE_SCAN and NO_IBSS are not set on the channel we want to start beaconing on.
> Do you have a white list that can exclude some channels for now globally or
> somehow as all this lines up?
>
Note sure I understood what you are looking for. Can you please clarify this point?
��.n��������+%������w��{.n�����{���zW����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
