Re: [PATCH v2] Revert "cfg80211: fix processing world regdomain when non modular"

["Hodaszi, Robert" <Robert.Hodaszi@xxxxxxxx>]

------------------------------------------------------------------------
*From:* Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
*Sent:* Friday, June 14, 2019 3:30PM
*To:* Hodaszi, Robert <Robert.Hodaszi@xxxxxxxx>
*Cc:* Linux-wireless <linux-wireless@xxxxxxxxxxxxxxx>
*Subject:* Re: [PATCH v2] Revert "cfg80211: fix processing world 
regdomain when non modular"

> On Fri, 2019-06-14 at 13:16 +0000, Hodaszi, Robert wrote:
>> This reverts commit 96cce12ff6e0bc9d9fcb2235e08b7fc150f96fd2.
>>
>> Re-triggering a reg_process_hint with the last request on all events,
>> can make the regulatory domain fail in case of multiple WiFi modules. On
>> slower boards (espacially with mdev), enumeration of the WiFi modules
>> can end up in an intersected regulatory domain, and user cannot set it
>> with 'iw reg set' anymore.
>>
>> This is happening, because:
>> - 1st module enumerates, queues up a regulatory request
>> - request gets processed by __reg_process_hint_driver():
>>    - checks if previous was set by CORE -> yes
>>      - checks if regulator domain changed -> yes, from '00' to e.g. 'US'
>>        -> sends request to the 'crda'
>> - 2nd module enumerates, queues up a regulator request (which triggers
>> the reg_todo() work)
>> - reg_todo() -> reg_process_pending_hints() sees, that the last request
>> is not processed yet, so it tries to process it again.
>> __reg_process_hint driver() will run again, and:
>>    - checks if the last request's initiator was the core -> no, it was
>> the driver (1st WiFi module)
>>    - checks, if the previous initiator was the driver -> yes
>>      - checks if the regulator domain changed -> yes, it was '00' (set by
>> core, and crda call did not return yet), and should be changed to 'US'
>>
>> ------> __reg_process_hint_driver calls an intersect
>>
>> Besides, the reg_process_hint call with the last request is meaningless
>> since the crda call has a timeout work. If that timeout expires, the
>> first module's request will lost.
> It's pointless to resend when I still have the original patch pending,
> at least without any changes.
>
> That said, I looked at this today and I'm not sure how the code doesn't
> now have the original issue again?
>
> johannes
>

I didn't just resend that. I just realized, accidentally I forgot to fix 
the debug message printing function, that define doesn't exist anymore. 
Sorry for the confusion!

Under "original issue", you mean the issue, which commit 
96cce12ff6e0bc9d9fcb2235e08b7fc150f96fd2 (cfg80211: fix processing world 
regdomain when non modular) supposed to fix? That still won't work, but 
that didn't work neither before I reverted the patch, because crda call 
timeout will just drop the last packet. Also, as it re-processed the 
last request, not just resent it, it caused undesired states. Like when 
I used 2 WiFi modules with US regulatory domains, after enumeration, my 
global regulator domain was set to "Country 98".

To fix my issue, why I reverted the patch, and also fix the issue the 
reverted commit supposed to fix, I could imagine something like this. 
But I'm not sure, it doesn't have any side effect:

diff --git a/linux/net/wireless/reg.c b/linux/net/wireless/reg.c
index 6fdb01b20b..13d564558d 100644
--- a/linux/net/wireless/reg.c
+++ b/linux/net/wireless/reg.c
@@ -2798,7 +2798,8 @@ static void reg_process_pending_hints(void)

diff --git a/linux/net/wireless/reg.c b/linux/net/wireless/reg.c
index 6fdb01b20b..13d564558d 100644
--- a/linux/net/wireless/reg.c
+++ b/linux/net/wireless/reg.c
@@ -2798,7 +2798,8 @@ static void reg_process_pending_hints(void)

         /* When last_request->processed becomes true this will be 
rescheduled */
         if (lr && !lr->processed) {
-               reg_process_hint(lr);
+               if (!reg_query_database(lr))
+                       reg_free_request(lr);
                 return;
         }

@@ -3175,6 +3176,7 @@ static void restore_regulatory_settings(bool 
reset_user, bool cached)
         struct reg_beacon *reg_beacon, *btmp;
         LIST_HEAD(tmp_reg_req_list);
         struct cfg80211_registered_device *rdev;
+       struct regulatory_request *lr;

         ASSERT_RTNL();

@@ -3190,6 +3192,13 @@ static void restore_regulatory_settings(bool 
reset_user, bool cached)
         }
         spin_unlock(&reg_indoor_lock);

+       /* If last request is pending, save it, will resubmit it */
+       lr = get_last_request();
+       if (lr && !lr->processed)
+               rcu_assign_pointer(last_request, NULL);
+       else
+               lr = NULL;
+
         reset_regdomains(true, &world_regdom);
         restore_alpha2(alpha2, reset_user);

@@ -3267,6 +3276,9 @@ static void restore_regulatory_settings(bool 
reset_user, bool cached)
         list_splice_tail_init(&tmp_reg_req_list, &reg_requests_list);
         spin_unlock(&reg_requests_lock);

+       if (lr != NULL)
+               rcu_assign_pointer(last_request, lr);
+
         pr_debug("Kicking the queue\n");

         schedule_work(&reg_work);


Best regards,
Robert Hodaszi