On 20 May 2014 09:27, Luis R. Rodriguez <mcgrof@xxxxxxxxxxxxxxxx> wrote:
> On Fri, May 9, 2014 at 1:04 AM, Janusz Dziedzic
> <janusz.dziedzic@xxxxxxxxx> 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,
>> - ®_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
--
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
