Re: [PATCH v2 1/1] wiphy: fix regdom change wiphy dump logic

[James Prestwood <prestwoj@gmail.com>]

Hi Denis,

On 5/16/23 5:53 PM, Denis Kenzior wrote:
> Hi James,
> 
> On 5/10/23 15:45, James Prestwood wrote:
>> Certain drivers change the regulatory domain when the client roams.
>> First XX, then the country the roam target is using. Depending on the
>> timing the second regdom event can come while the first is still
>> dumping. This was not handled gracefully/nicely in IWD and it would
>> just issue another dump (it tries to cancel but if the dump started
>> there isn't anything that can be done). AFAIK the kernel should be
>> able to handle this but based on the behavior we saw it seems to not
>> be able to, or at least not reliably.
> 
> Okay.  Might be useful to know what the kernel actually expects to 
> happen in case we send on a socket during an ongoing command.

I can try and get this to happen locally to investigate further, but so 
far its been a rare case. Maybe happening 1 time in a 24 hour period on 
20-30 clients. I'm thinking maybe a locking/race problem in 
nl80211_dump_wiphy(), but its just a guess.

> 
>>
>> When the second dump comes in and GET_WIPHY is issued the kernel
>> stops replying to any messages which obviously causes IWD to completely
>> hang up waiting for various acks (set_station, set_key, etc.).
>>
> 
> Sounds like genl isn't doing the right thing...
> 
>> Now if another dump is ongoing (and cant be safely canceled) a flag
>> will be set when another dump is needed. The first dump will be
>> waited for and only once completed will another start. This also
>> ensures there is at most 1 pending dump when before any number of
>> dumps could be queued which is pointless since only the last dump
>> request matters.
> 
> NAK.  Please stop right there.  genl is already a queue.  There's no 
> sense adding queuing on top of it in this particular case.  If 
> l_genl_family_cancel isn't doing the right thing, then fix that instead.

Well l_genl_family_cancel really cant do anything about it. Once the 
dump hits the kernel you cant cancel it (this is why we added 
l_genl_family_request_sent). So the only alternative I can think of, if 
we cant do it in IWD, would be to make l_genl queue dumps and not issue 
more dumps until the previous one finishes. This is obviously a 
significant behavioral change.

> 
>> ---
>>   src/wiphy.c | 103 +++++++++++++++++++++++++++-------------------------
>>   1 file changed, 54 insertions(+), 49 deletions(-)
>>
> 
> Regards,
> -Denis