diff mbox

wpa_supplicant: send event to all interfaces

Message ID CAFED-jkMKsLY6ECMdr-8zqxgrL8QcquBny26jOsQCh5tXek23A@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Janusz Dziedzic April 2, 2016, 5:20 p.m. UTC
2016-04-02 18:56 GMT+02:00 Janusz Dziedzic <janusz.dziedzic@gmail.com>:
> =
>
> 2016-04-02 12:30 GMT+02:00 Jouni Malinen <j@w1.fi>:
>> On Thu, Mar 31, 2016 at 08:53:14PM +0200, Janusz Dziedzic wrote:
>>> In case we don't have ifidx and wdev_id pass
>>> such event to all interfaces and bss.
>>> Before we send event only to first interface and
>>> in case we are using p2p-dev we send event only
>>> to this one iface:
>>>
>>> p2p-dev-wlan0: CTRL-EVENT-REGDOM-CHANGE init=CORE type=WORLD
>>>
>>> Because of that we fail hwsim test cases eg. ibss_5ghz,
>>> while we never get CTRL-EVENT-REGDOM-CHANGE on wlan0.
>>>
>>> I also remove for_each() from wpa_supplicant_update_channel_list()
>>> while this function will be called now for each interface.
>>> So, seems this for_each() was a workaround for a real issue.
>>
>> Could you please clarify what exactly you mean with hwsim test cases
>> failing? I don't see such a failure in my testing.. Is this only
>> relevant for the remote test cases that you show in the example below?
>>
> It should also failed in case your driver using p2p-dev. In such case you will
> get only REGDOM-CHANGE on p2p-dev-wlan0 (only on first interface in iface
> iterations), and never get such event on wlan0.
> I think you can start mac80211_hwsim with support_p2p_device=1 and run
> only ibss_5ghz to reproduce this problem.
>
> For sure my patch fix this issue and each interface get REGODOM -CHANGE.
> In my case p2p-dev-wlan0 and wlan0.
> But I am not sure if this will not introduce some other issue. So
> clarification is required here, how should we handle events when
> ifidx==-1 and wdev_id is not set. If we should send such event to all
> interfaces, then my patch do exactly that.
>

This is test with hwsim:

janusz@dell:~/work/hostap/tests/hwsim$ git diff
 sudo ifconfig hwsim0 up
 sudo $WLANTEST -i hwsim0 -n $LOGDIR/hwsim0.pcapng -c -dtN -L $LOGDIR/hwsim0 &


janusz@dell:~/work/hostap/tests/hwsim$ sudo ./run-tests.py ibss_5ghz
DEV: wlan0: 42:00:00:00:00:00
DEV: wlan1: 42:00:00:00:01:00
DEV: wlan2: 42:00:00:00:02:00
APDEV: wlan3
APDEV: wlan4
START ibss_5ghz 1/1
FAIL ibss_5ghz 5.19917 2016-04-02 19:13:49.208650

And error: No regdom change event

You didn't hit this problem because support_p2p_device=0 by default in
hwsim. In case of remote test case I have different drivers in the
stack (intel7260, ath10k, ath9k) and in case of remote tests and hwsim
I always load mac80211_hwsim by hand (seems p2p_dev support is enabled
by default).

BR
Janusz

> BR
> Janusz
>>> Now, seems we pass test where we wait for REGDOM event
>>> janusz@dell6430:/home/work/hostap/tests/remote$ ./run-tests.py -r hwsim0 -r hwsim1 -r hwsim2 -d hwsim3 -d hwsim4 -h ibss_5ghz
>>
>> tests/remote does not exist in hostap.git, so it is a bit difficult to
>> understand this type of comments before either the changes have been
>> added for that or the commit message is clearer on what exactly is being
>> fixed if the same issue cannot be reproduced in the current hostap.git
>> snapshot.
>>
>> --
>> Jouni Malinen                                            PGP id EFC895FA
>>
>> _______________________________________________
>> Hostap mailing list
>> Hostap@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/hostap
>
>
>
> --
> Janusz Dziedzic

Comments

Jouni Malinen April 24, 2016, 8:42 a.m. UTC | #1
On Sat, Apr 02, 2016 at 07:20:45PM +0200, Janusz Dziedzic wrote:
> This is test with hwsim:

> diff --git a/tests/hwsim/start.sh b/tests/hwsim/start.sh
> @@ -100,7 +100,7 @@ else
> -test -f /proc/modules && sudo modprobe mac80211_hwsim radios=7
> channels=$NUM_CH support_p2p_device=0
> +test -f /proc/modules && sudo modprobe mac80211_hwsim radios=7
> channels=$NUM_CH support_p2p_device=1

> janusz@dell:~/work/hostap/tests/hwsim$ sudo ./run-tests.py ibss_5ghz
> START ibss_5ghz 1/1
> FAIL ibss_5ghz 5.19917 2016-04-02 19:13:49.208650
> 
> And error: No regdom change event

I cannot reproduce this with such a change in start.sh. Are you saying
this fails for you every time with "No regdom change event"?
Janusz.Dziedzic@tieto.com April 25, 2016, 4:57 a.m. UTC | #2
On 24 April 2016 at 10:42, Jouni Malinen <j@w1.fi> wrote:
> On Sat, Apr 02, 2016 at 07:20:45PM +0200, Janusz Dziedzic wrote:
>> This is test with hwsim:
>
>> diff --git a/tests/hwsim/start.sh b/tests/hwsim/start.sh
>> @@ -100,7 +100,7 @@ else
>> -test -f /proc/modules && sudo modprobe mac80211_hwsim radios=7
>> channels=$NUM_CH support_p2p_device=0
>> +test -f /proc/modules && sudo modprobe mac80211_hwsim radios=7
>> channels=$NUM_CH support_p2p_device=1
>
>> janusz@dell:~/work/hostap/tests/hwsim$ sudo ./run-tests.py ibss_5ghz
>> START ibss_5ghz 1/1
>> FAIL ibss_5ghz 5.19917 2016-04-02 19:13:49.208650
>>
>> And error: No regdom change event
>
> I cannot reproduce this with such a change in start.sh. Are you saying
> this fails for you every time with "No regdom change event"?
>
Yes, exactly.
In ibss_5ghz.log I see we only get this event for p2p-dev-wlan1/p2p-dev-wlan0.

Kernel I am using:
4.5.0-rc6-ath+

BR
Janusz


> --
> Jouni Malinen                                            PGP id EFC895FA
Peer, Ilan April 25, 2016, 6:35 a.m. UTC | #3
> -----Original Message-----

> From: Hostap [mailto:hostap-bounces@lists.infradead.org] On Behalf Of

> Janusz Dziedzic

> Sent: Monday, April 25, 2016 07:58

> To: Jouni Malinen

> Cc: hostap@lists.infradead.org; Janusz Dziedzic

> Subject: Re: [PATCH] wpa_supplicant: send event to all interfaces

> 

> On 24 April 2016 at 10:42, Jouni Malinen <j@w1.fi> wrote:

> > On Sat, Apr 02, 2016 at 07:20:45PM +0200, Janusz Dziedzic wrote:

> >> This is test with hwsim:

> >

> >> diff --git a/tests/hwsim/start.sh b/tests/hwsim/start.sh @@ -100,7

> >> +100,7 @@ else -test -f /proc/modules && sudo modprobe

> mac80211_hwsim

> >> radios=7 channels=$NUM_CH support_p2p_device=0

> >> +test -f /proc/modules && sudo modprobe mac80211_hwsim radios=7

> >> channels=$NUM_CH support_p2p_device=1

> >

> >> janusz@dell:~/work/hostap/tests/hwsim$ sudo ./run-tests.py ibss_5ghz

> >> START ibss_5ghz 1/1 FAIL ibss_5ghz 5.19917 2016-04-02

> 19:13:49.208650

> >>

> >> And error: No regdom change event

> >

> > I cannot reproduce this with such a change in start.sh. Are you saying

> > this fails for you every time with "No regdom change event"?

> >

> Yes, exactly.

> In ibss_5ghz.log I see we only get this event for p2p-dev-wlan1/p2p-dev-

> wlan0.

> 


I think that is the same issue that was addressed in http://lists.shmoo.com/pipermail/hostap/2015-June/033085.html

The attached patch also handles this issue. 

Regards,

Ilan.
Jouni Malinen April 25, 2016, 8:55 a.m. UTC | #4
On Mon, Apr 25, 2016 at 06:57:59AM +0200, Janusz Dziedzic wrote:
> On 24 April 2016 at 10:42, Jouni Malinen <j@w1.fi> wrote:
> > On Sat, Apr 02, 2016 at 07:20:45PM +0200, Janusz Dziedzic wrote:
> >> This is test with hwsim:
> >
> >> diff --git a/tests/hwsim/start.sh b/tests/hwsim/start.sh
> >> @@ -100,7 +100,7 @@ else
> >> -test -f /proc/modules && sudo modprobe mac80211_hwsim radios=7
> >> channels=$NUM_CH support_p2p_device=0
> >> +test -f /proc/modules && sudo modprobe mac80211_hwsim radios=7
> >> channels=$NUM_CH support_p2p_device=1

Ah.. I was not looking at this closely enough. This start.sh change does
not actually do anything for my main test setup, since I use a VM and a
kernel without any modules..

> In ibss_5ghz.log I see we only get this event for p2p-dev-wlan1/p2p-dev-wlan0.

I can see this now with vm/vm-run.sh handling the support_p2p_device
modification.

Now that I can reproduce this issue, lets try to figure out what's the
best way of addressing this. Your patch is doing quite a bit more than
just modifying the specfici CTRL-EVENT-REGDOM-CHANGE event. Was there a
reason for doing that or is that all just trying to make these test
cases pass? Would the simpler patch that Ilan sent today work for the
cases where you can see this issue with remote hosts? It is simply
finding the highest level parent to get the IFNAME=wlan0 style prefix
for the event regardless of whether the P2P Device interface is used.
Janusz.Dziedzic@tieto.com April 25, 2016, 9:14 a.m. UTC | #5
On 25 April 2016 at 10:55, Jouni Malinen <j@w1.fi> wrote:
> On Mon, Apr 25, 2016 at 06:57:59AM +0200, Janusz Dziedzic wrote:
>> On 24 April 2016 at 10:42, Jouni Malinen <j@w1.fi> wrote:
>> > On Sat, Apr 02, 2016 at 07:20:45PM +0200, Janusz Dziedzic wrote:
>> >> This is test with hwsim:
>> >
>> >> diff --git a/tests/hwsim/start.sh b/tests/hwsim/start.sh
>> >> @@ -100,7 +100,7 @@ else
>> >> -test -f /proc/modules && sudo modprobe mac80211_hwsim radios=7
>> >> channels=$NUM_CH support_p2p_device=0
>> >> +test -f /proc/modules && sudo modprobe mac80211_hwsim radios=7
>> >> channels=$NUM_CH support_p2p_device=1
>
> Ah.. I was not looking at this closely enough. This start.sh change does
> not actually do anything for my main test setup, since I use a VM and a
> kernel without any modules..
>
>> In ibss_5ghz.log I see we only get this event for p2p-dev-wlan1/p2p-dev-wlan0.
>
> I can see this now with vm/vm-run.sh handling the support_p2p_device
> modification.
>
> Now that I can reproduce this issue, lets try to figure out what's the
> best way of addressing this. Your patch is doing quite a bit more than
> just modifying the specfici CTRL-EVENT-REGDOM-CHANGE event. Was there a
> reason for doing that or is that all just trying to make these test
> cases pass? Would the simpler patch that Ilan sent today work for the
> cases where you can see this issue with remote hosts? It is simply
> finding the highest level parent to get the IFNAME=wlan0 style prefix
> for the event regardless of whether the P2P Device interface is used.
>

I also remove for_each() from wpa_supplicant_update_channel_list()
while this function will be called now for each interface.
So, seems this for_each() was a workaround for a real issue (not
sending this message to all ifaces).

This is general question:
how should we handle events when ifidx==-1 and wdev_id is not set.

BR
Janusz



> --
> Jouni Malinen                                            PGP id EFC895FA
Jouni Malinen April 28, 2016, 6:11 p.m. UTC | #6
On Mon, Apr 25, 2016 at 11:14:12AM +0200, Janusz Dziedzic wrote:
> I also remove for_each() from wpa_supplicant_update_channel_list()
> while this function will be called now for each interface.
> So, seems this for_each() was a workaround for a real issue (not
> sending this message to all ifaces).

Please see commit 0f4bccdbbe642fc620d2b70f60c8ddc4ad09a5db ('Refactor
channel list update event in wpa_supplicant'). That dl_list_for_each()
in wpa_supplicant_update_channel_list() would seem to be needed to call
wpas_p2p_update_channel_list() at the end of the updates. The changes
you are proposing in this patch would seem to undo at least parts of
that earlier fix. As such, I'm not really keen on applying this without
much clearer justification on this fixing something. In other words, I'd
much rather use the simpler patch to allow the test cases pass unless
someone can go through all the possible channel list update sequences in
more detail.

> This is general question:
> how should we handle events when ifidx==-1 and wdev_id is not set.

Which events are these (other than the channel list change indication)?
Janusz.Dziedzic@tieto.com April 29, 2016, 5:03 a.m. UTC | #7
On 28 April 2016 at 20:11, Jouni Malinen <j@w1.fi> wrote:
> On Mon, Apr 25, 2016 at 11:14:12AM +0200, Janusz Dziedzic wrote:
>> I also remove for_each() from wpa_supplicant_update_channel_list()
>> while this function will be called now for each interface.
>> So, seems this for_each() was a workaround for a real issue (not
>> sending this message to all ifaces).
>
> Please see commit 0f4bccdbbe642fc620d2b70f60c8ddc4ad09a5db ('Refactor
> channel list update event in wpa_supplicant'). That dl_list_for_each()
> in wpa_supplicant_update_channel_list() would seem to be needed to call
> wpas_p2p_update_channel_list() at the end of the updates. The changes
> you are proposing in this patch would seem to undo at least parts of
> that earlier fix. As such, I'm not really keen on applying this without
> much clearer justification on this fixing something. In other words, I'd
> much rather use the simpler patch to allow the test cases pass unless
> someone can go through all the possible channel list update sequences in
> more detail.
>
fixing something: In my opinion remove workarounds, applied before :)

But I understand this could have "unknown" impact on other parts of code,
so simple patch is also fine for me.

BR
Janusz

>> This is general question:
>> how should we handle events when ifidx==-1 and wdev_id is not set.
>
> Which events are these (other than the channel list change indication)?
>
> --
> Jouni Malinen                                            PGP id EFC895FA
Jouni Malinen May 14, 2016, 5:08 p.m. UTC | #8
On Mon, Apr 25, 2016 at 06:35:05AM +0000, Peer, Ilan wrote:
> I think that is the same issue that was addressed in http://lists.shmoo.com/pipermail/hostap/2015-June/033085.html
> 
> The attached patch also handles this issue. 

Thanks, applied.
diff mbox

Patch

diff --git a/tests/hwsim/start.sh b/tests/hwsim/start.sh
index a2885e4..c0d1366 100755
--- a/tests/hwsim/start.sh
+++ b/tests/hwsim/start.sh
@@ -100,7 +100,7 @@  else
        NUM_CH=1
 fi

-test -f /proc/modules && sudo modprobe mac80211_hwsim radios=7
channels=$NUM_CH support_p2p_device=0
+test -f /proc/modules && sudo modprobe mac80211_hwsim radios=7
channels=$NUM_CH support_p2p_device=1