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.
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.
> 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 ?
> + 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.
> + } 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.
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
