MIME-Version: 1.0
In-Reply-To: <CAB=NE6Va3XDbUSUi1jk8-EHM8xdcLntU_9_9F0AnUJLkV3PFwA@mail.gmail.com>
References: <1399622690-7202-1-git-send-email-janusz.dziedzic@tieto.com>
	<CAB=NE6Va3XDbUSUi1jk8-EHM8xdcLntU_9_9F0AnUJLkV3PFwA@mail.gmail.com>
Date: Tue, 20 May 2014 15:54:03 +0200
Message-ID: <CALhHN=pn9TQPiH=Pd0OvkBc2RxEpBn2xfZgdNNtLuCtOyHJizA@mail.gmail.com> (sfid-20140520_155426_893440_DA9BA6DF)
Subject: Re: [PATCH v2] cfg80211: reg: track crda request
From: Janusz Dziedzic <janusz.dziedzic@tieto.com>
To: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
Cc: linux-wireless <linux-wireless@vger.kernel.org>,
	Johannes Berg <johannes@sipsolutions.net>
Content-Type: text/plain; charset=UTF-8
Sender: linux-wireless-owner@vger.kernel.org

On 20 May 2014 09:27, Luis R. Rodriguez <mcgrof@do-not-panic.com> wrote:
> On Fri, May 9, 2014 at 1:04 AM, Janusz Dziedzic
> <janusz.dziedzic@tieto.com> wrote:
>> @@ -1803,6 +1807,9 @@ static void reg_process_hint(struct regulatory_request *reg_request)
>>         struct wiphy *wiphy = NULL;
>>         enum reg_request_treatment treatment;
>>
>> +       REG_DBG_PRINT("Process regulatory hint called by %s\n",
>
> "Processing"
>
>> +                     reg_initiator_name(reg_request->initiator));
>> +
>>         if (reg_request->wiphy_idx != WIPHY_IDX_INVALID)
>>                 wiphy = wiphy_idx_to_wiphy(reg_request->wiphy_idx);
>>
>> @@ -1811,12 +1818,7 @@ static void reg_process_hint(struct regulatory_request *reg_request)
>>                 reg_process_hint_core(reg_request);
>>                 return;
>>         case NL80211_REGDOM_SET_BY_USER:
>> -               treatment = reg_process_hint_user(reg_request);
>> -               if (treatment == REG_REQ_IGNORE ||
>> -                   treatment == REG_REQ_ALREADY_SET)
>> -                       return;
>> -               queue_delayed_work(system_power_efficient_wq,
>> -                                  &reg_timeout, msecs_to_jiffies(3142));
>> +               reg_process_hint_user(reg_request);
>
> You do something more than your commit log says it does, in this case
> its ignoring REG_REQ_IGNORE and REG_REQ_ALREADY_SET, explain why that
> is being done, that is very important part of this change.
>
While timer is moved to reg_call_crda() code this check is not
required here while the same is done
in reg_process_hint_user(). So, SET_BY_USER will be handle exactly the
same like before.
So, could you describe this more ...?

> For example you want to explain that we typically never allowed
> retries on the same regulatory hint, but this makes sense in the
> corner case of udev not picking sending hints to CRDA as CRDA may be
> on a partition not mounted yet. It would also be good for you to do
> homework on whether or not udev with systemd guarantees rules using a
> binary on a path will get all its queued up stuff only sent to it once
> the partition it could be on is available. This is the right long term
> solution to this and we need to be mindful of it. If you find that
> systemd+udev will ensure proper synchronization then you can then say
> that this timeout is only then important for legacy systems, if its
> not we should look to see if we can add udev synchronization for
> depending binaries used on rules.
>
I don't care much about user mode code now (in the future we could
have some other staff on the top).
My intention of this patch was to print warning/message in dmesg,
while we don't have udev/crda/regdb/internal_regdb, we didn't have any
indication what was wrong - we could just guessing. With my patch you
will have some hint what could be wrong and I don't care much if that
is udev/systemd problem (I am not sure they are required at all, and
depends on system configuration you are using).

>>                 return;
>>         case NL80211_REGDOM_SET_BY_DRIVER:
>>                 if (!wiphy)
>> @@ -2608,9 +2610,40 @@ void wiphy_regulatory_deregister(struct wiphy *wiphy)
>>
>>  static void reg_timeout_work(struct work_struct *work)
>>  {
>> -       REG_DBG_PRINT("Timeout while waiting for CRDA to reply, restoring regulatory settings\n");
>> +       struct regulatory_request *lr;
>> +
>>         rtnl_lock();
>> -       restore_regulatory_settings(true);
>> +
>> +       lr = get_last_request();
>> +       REG_DBG_PRINT("Timeout while waiting for CRDA to reply %s request\n",
>> +                     reg_initiator_name(lr->initiator));
>> +
>> +       switch (lr->initiator) {
>> +       case NL80211_REGDOM_SET_BY_CORE:
>> +       case NL80211_REGDOM_SET_BY_DRIVER:
>> +#ifdef CONFIG_CFG80211_INTERNAL_REGDB
>
> Can you use config_enabled(CONFIG_CFG80211_INTERNAL_REGDB) instead ?
>
Sure.

>> +               pr_warn("invalid db.txt file, will use limited/restricted regulatory settings\n");
>> +               break;
>> +#else
>
> There is a different between CRDA not being available (we'd know if
> CRDA was present once if at least for one call it was successful, but
> note we don't yet distinguish the source of a regulatory domain
> successful call, whether its from the internal db or not, and a change
> for this is welcomed, although note that upon  restore regulatory
> settings call we could be coming back from a suspend/resume remount),
> a specific alpha2 not being available, and an alpha2 not being
> available on the internal regdb, the above and below does not
> distinguish any of this and it seems it should. If it proves to be
> adding quite a bit of code, don't worry -- splitting this up into
> separate calls for each case might make this code more manageable, at
> least that's what happened with the processing of regulatory hints,
> leave it up to you as you see the code as modify it to handle the
> different cases I just outlined.
>
>> +               if (lr->crda_call_count < REG_CRDA_CALL_COUNT_MAX) {
>> +                       /* Call CRDA again for last request */
>> +                       lr->crda_call_count++;
>> +                       reg_process_hint(lr);
>
> A comment specifying the things I mentioned about udev could be useful
> here, but it'd be a bit nicer if this was instead just properly
> documented on the crda_call_count variable enum documentation, then
> you can go to town on the documentation there.
>
Sure

>> +               } else
>> +                       pr_warn("CRDA not available, will use limited/restricted regulatory settings\n");
>> +
>> +               break;
>> +#endif /* CONFIG_CFG80211_INTERNAL_REGDB */
>> +       case NL80211_REGDOM_SET_BY_USER:
>> +               restore_regulatory_settings(true);
>> +               break;
>> +       case NL80211_REGDOM_SET_BY_COUNTRY_IE:
>> +               restore_regulatory_settings(false);
>> +               break;
>> +       default:
>> +               WARN_ON(1);
>> +               break;
>
> I'd prefer to leave this one out and instead let the compiler deal
> with compilation warning about the switch not handling new cases,
> which it does today because of the enum.
>
Sure

>   Luis