1093951 - WiFi Multicast/Broadcast filter don't work when system is sleep

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[None]

User Agent: Mozilla/5.0 (compatible; MSIE 10.0; Windows NT 6.1; Trident/6.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; InfoPath.3; .NET4.0E; Tablet PC 2.0; CNS_UA; AD_LOGON=4C47452E4E4554)

Steps to reproduce:

1. WiFi ON.
2. WiFi connect to some AP.
3. Phone sleep.


Actual results:

Phone wake up many times because of multicast packet.(ex. HSRP multicast packet) So power consumption is up.


Expected results:

It filter multicast / broadcast packet. So phone don't wake up by multicast packet.

Comment 1

[bhavana bajaj [:bajaj]]

wayne I was informed by CAF that this is a TA blocker highlighted as *critical*. Can you help follow-up on how madai didn't' flag us here?

Chuck,

This could be related to https://bugzilla.mozilla.org/show_bug.cgi?id=950124, can you help here?

Comment 2

[None]

The issue(950124) is solved.
But is there any modification that enable filter when phone is sleep?

FFOS filter enabling scinario like that
 - wifi.screen_off_timeout is expired and wifi lock is held.

Maybe I think that they don't know why filter is needed when phone is sleep.

When phone is sleep with WiFi connected, phone can get many multicast packet that just needs network management entity.(ex HSRP..) So in some network phone can't sleep because of multicast packet. In android platform, they set multicast filter when screen is off. 

I think FFOS alos need this logic.

Comment 3

[Vincent Chang[:vchang][changyihsin]]

(In reply to None from comment #2)
> The issue(950124) is solved.
> But is there any modification that enable filter when phone is sleep?
> 
> FFOS filter enabling scinario like that
>  - wifi.screen_off_timeout is expired and wifi lock is held.
> 
> Maybe I think that they don't know why filter is needed when phone is sleep.
> 
> When phone is sleep with WiFi connected, phone can get many multicast packet
> that just needs network management entity.(ex HSRP..) So in some network
> phone can't sleep because of multicast packet. In android platform, they set
> multicast filter when screen is off. 
> 
> I think FFOS alos need this logic.

Does it mean that we should start multicast filter[1] when set wifi power save mode[2]?

[1]http://dxr.mozilla.org/mozilla-central/source/dom/wifi/WifiCommand.jsm?from=wificommand.jsm&case=true#196
[2]http://dxr.mozilla.org/mozilla-central/source/dom/wifi/WifiWorker.js#3367

Comment 4

[None]

I thinks packet filter working when LCD is off. And when LCD is on filter is releaved.

Already this API is exist( wifiManager.setPowerSavingMode() ).

wifi.js(gaia) use that API.

Comment 5

[Henry Chang [:hchang]]

It seems android disable multicast by default but provides
a multicast lock [1] whenever needed. We can do as how
android does as long as there's no current app supposing
to be able to receive multicast packet.

[1] http://developer.android.com/reference/android/net/wifi/WifiManager.MulticastLock.html

Comment 6

[Henry Chang [:hchang]]

(In reply to None from comment #4)
> I thinks packet filter working when LCD is off. And when LCD is on filter is
> releaved.
> 
> Already this API is exist( wifiManager.setPowerSavingMode() ).
> 
> wifi.js(gaia) use that API.

If no app needs to receive multicast packets, we can always turn on 
multicast filter. Perfect!

However, if some apps need, I doubt if the app still needs to receive 
multicast packet during power saving mode ...

Comment 7

[None]

(In reply to Henry Chang [:henry] from comment #6)
> (In reply to None from comment #4)
> I thinks packet filter working when LCD
> is off. And when LCD is on filter is
> releaved.
> 
> Already this API is
> exist( wifiManager.setPowerSavingMode() ).
> 
> wifi.js(gaia) use that API.
> If no app needs to receive multicast packets, we can always turn on 
> multicast filter. Perfect!

However, if some apps need, I doubt if the app
> still needs to receive 
multicast packet during power saving mode ...



Most of mobile WiFi chip vendor's implementations of host off-load or filter can block just unnessary packet. 

There are parameters that determin which packet can pass this filter.(All multicast/broadcast or Just nessary multicast...)

We can set like this ... If some app bind specific multicast IP address, WiFi chip remember multicast MAC orginated multicast IP address and don't block packets that have same multicast MAC.

Broadcast filter has same mechanism. Normally they call MC/BC filter(Broadcast and Multicast filter)

Comment 8

[Henry Chang [:hchang]]

Attachment: Bug1093951.patch

Comment 9

[Henry Chang [:hchang]]

Since the multicast filter will be enabled when we request the wifi driver to go to 
power save mode, what we only have to do is to make sure WifiManager.setPowerSavingMode
is called at the proper timing. We choose to enable wifi power saving mode
when the screen becomes off and the phone is not charging.

To sum up, there are two check points:

1) To confirm we do call WifiManager.setPowerSavingMode when we turn off the screen.
2) After calling WifiManager.setPowerSavingMode, the filter takes effect.

attachment 8518801 [details] [diff] [review] is a fast fix to invoke to go to power saving mode
no matter the wifi lock is held.

Comment 10

[None]

Thanks to make patch.

We test your patch. The filter is setted correctly. But it don't remove filter.

So we modify code.(wifi.js - gaig/apps/system/js ) You can find our code comment - //LGE patch.

The if statement(if (!this.wifiDisabledByWakelock) ) block to remove filter.

--wifi.js----

    // ... and quietly turn it back on or cancel the timer otherwise
    else {
      if (this._alarmId) {
        navigator.mozAlarms.remove(this._alarmId);
        this._alarmId = null;
      }

      // If wifi is enabled but disconnected.
      // we would need to call getNetworks() so we could join known wifi network
      if (this.wifiEnabled && wifiManager.connection.status == 'disconnected') {
        wifiManager.getNetworks();
      }

      //LGE patch - This code needed to apply Bug 1093951 from Bugzilla
      //                    Mozilla patch don't reset PowerSavemode.
      if (this.wifiEnabled) {
        // Restore from power save mode is wifi is already enabled.
        wifiManager.setPowerSavingMode(false);
        releaseCpuLock();
        return;
      }

      // We don't need to do anything if we didn't disable wifi at first place.
      if (!this.wifiDisabledByWakelock) {
        releaseCpuLock();
        return;
      }

Thanks.

Comment 11

[Wayne Chang [:wchang]]

ni?Henry for comment 10

Comment 12

[Henry Chang [:hchang]]

(In reply to None from comment #10)
> Thanks to make patch.
> 
> We test your patch. The filter is setted correctly. But it don't remove
> filter.
> 
> So we modify code.(wifi.js - gaig/apps/system/js ) You can find our code
> comment - //LGE patch.
> 
> The if statement(if (!this.wifiDisabledByWakelock) ) block to remove filter.
> 
> --wifi.js----
> 
>     // ... and quietly turn it back on or cancel the timer otherwise
>     else {
>       if (this._alarmId) {
>         navigator.mozAlarms.remove(this._alarmId);
>         this._alarmId = null;
>       }
> 
>       // If wifi is enabled but disconnected.
>       // we would need to call getNetworks() so we could join known wifi
> network
>       if (this.wifiEnabled && wifiManager.connection.status ==
> 'disconnected') {
>         wifiManager.getNetworks();
>       }
> 
>       //LGE patch - This code needed to apply Bug 1093951 from Bugzilla
>       //                    Mozilla patch don't reset PowerSavemode.
>       if (this.wifiEnabled) {
>         // Restore from power save mode is wifi is already enabled.
>         wifiManager.setPowerSavingMode(false);
>         releaseCpuLock();
>         return;
>       }
> 
>       // We don't need to do anything if we didn't disable wifi at first
> place.
>       if (!this.wifiDisabledByWakelock) {
>         releaseCpuLock();
>         return;
>       }
> 
> Thanks.

Thanks for pointing this out! By the way, you mentioned you
set the screen timeout to 0 to avoid wifi being disabled.
However, roughly reading the code, it seems it only makes 
wifi turned off right after screen is off. Have you confirmed
this? Thanks!

Comment 13

[None]

(In reply to Henry Chang [:henry] from comment #12)
> (In reply to None from comment #10)
> Thanks to make patch.
> 
> We test
> your patch. The filter is setted correctly. But it don't remove
> filter.
> 
> > So we modify code.(wifi.js - gaig/apps/system/js ) You can find our code
>
> comment - //LGE patch.
> 
> The if statement(if
> (!this.wifiDisabledByWakelock) ) block to remove filter.
> 
> --wifi.js----
> > 
>     // ... and quietly turn it back on or cancel the timer otherwise
> 
> else {
>       if (this._alarmId) {
>        
> navigator.mozAlarms.remove(this._alarmId);
>         this._alarmId = null;
>
> }
> 
>       // If wifi is enabled but disconnected.
>       // we would
> need to call getNetworks() so we could join known wifi
> network
>       if
> (this.wifiEnabled && wifiManager.connection.status ==
> 'disconnected') {
> 
> wifiManager.getNetworks();
>       }
> 
>       //LGE patch - This code
> needed to apply Bug 1093951 from Bugzilla
>       //                   
> Mozilla patch don't reset PowerSavemode.
>       if (this.wifiEnabled) {
>  
> // Restore from power save mode is wifi is already enabled.
>        
> wifiManager.setPowerSavingMode(false);
>         releaseCpuLock();
>        
> return;
>       }
> 
>       // We don't need to do anything if we didn't
> disable wifi at first
> place.
>       if (!this.wifiDisabledByWakelock) {
>
> releaseCpuLock();
>         return;
>       }
> 
> Thanks.

Thanks for
> pointing this out! By the way, you mentioned you
set the screen timeout to 0
> to avoid wifi being disabled.
However, roughly reading the code, it seems it
> only makes 
wifi turned off right after screen is off. Have you confirmed
> this? Thanks!

We roll back wifi.screen_off_timeout to 600000. But this timer value is never set because of [comment 8] patch. If WiFi on and screen off/no charing, it just setup power-saving mode. Then return.

If wifi.screen_off_timeout is 0, it just return and never set up filter.
Any way, thanks a lot.

Comment 14

[Michael Vines [:m1] [:evilmachines]]

(re-opening, as we'd like this fix landed in m-c at least)

Comment 15

[Wayne Chang [:wchang]]

ni?Vincent for comment 14.

Can you assess and work with Henry on landing a solution to M-C?

Thanks

Comment 16

[Henry Chang [:hchang]]

(In reply to Wayne Chang [:wchang] from comment #15)
> ni?Vincent for comment 14.
> 
> Can you assess and work with Henry on landing a solution to M-C?
> 
> Thanks

It's actually a gaia patch :)

Comment 17

[Inder]

[Blocking Requested - why for this release]:

This is a critical power saving fix and needs to land on 2.1 also.

Comment 18

[Henry Chang [:hchang]]

We are going to land the quick fix on 2.0/2.1. But for master, we are planning
to introduce more sophisticated power saving strategy and more configurable 
behavior like what Android's done.

Comment 19

[Henry Chang [:hchang]]

Attachment: gaia PR for 2.0

Comment 20

[Henry Chang [:hchang]]

Attachment: gaia PR for 2.1

Comment 21

[Henry Chang [:hchang]]

Comment on attachment 8521962 [details] [review]
gaia PR for 2.0

Hi alive,

Could you help review this patch for the quick fix regarding wifi power save mode on 2.0/2.1? We will always enter wifi power save mode when screen off and usb unplugged to reduce the power consumption before screen timeout. Wifi would still be turned off after screen timeout. Feel free to direct the review to someone you consider suitable if you are not. Thanks!

Chuck, could you also give some feedback? Thanks!

Comment 22

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8521962 [details] [review]
gaia PR for 2.0

Looks good to me.

Comment 23

[Ethan Tseng [:ethan]]

(In reply to Henry Chang [:henry] from comment #18)
> We are going to land the quick fix on 2.0/2.1. But for master, we are planning
> to introduce more sophisticated power saving strategy and more configurable 
> behavior like what Android's done.

We had a discussion of how to exploit Power Save Mode or even aggressively turn off Wi-Fi in order to 
optimize power consumption.
The design is in a draft phase. You can refer to the diagram:
https://wiki.mozilla.org/images/c/c6/WifiPowerModeState.svg

Comment 24

[Henry Chang [:hchang]]

Attachment: gaia PR for master

Comment 25

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #24)
> Created attachment 8522059 [details] [review]
> gaia PR for master

Thanks alive and chuck!

Comment 26

[Vincent Chang[:vchang][changyihsin]]

(In reply to Ethan Tseng [:ethan] from comment #23)
> (In reply to Henry Chang [:henry] from comment #18)
> > We are going to land the quick fix on 2.0/2.1. But for master, we are planning
> > to introduce more sophisticated power saving strategy and more configurable 
> > behavior like what Android's done.
> 
> We had a discussion of how to exploit Power Save Mode or even aggressively
> turn off Wi-Fi in order to 
> optimize power consumption.
> The design is in a draft phase. You can refer to the diagram:
> https://wiki.mozilla.org/images/c/c6/WifiPowerModeState.svg

I think we can do this in bug 1084156.

Comment 27

[bhavana bajaj [:bajaj]]

Inder, feel free to try the patch and let us know if it fixes the issue. Since its r+ it will land in our trees soon.

Comment 28

[bhavana bajaj [:bajaj]]

Henry, This has a review+ already so can we land it on master and get it uplifted on branches? Unclear if we are waiting for anything else here.

Comment 29

[Henry Chang [:hchang]]

(In reply to bhavana bajaj [:bajaj] from comment #28)
> Henry, This has a review+ already so can we land it on master and get it
> uplifted on branches? Unclear if we are waiting for anything else here.

Hi,

I was working on fixing some gaia test errors for the past few days.
Actually what I was doing is to accommodate the gaia unit test cases to
the new policy. Everything seems to be okay now. Going to ask someone
to help me merge soon.

Thanks!

Comment 30

[Henry Chang [:hchang]]

Comment on attachment 8522059 [details] [review]
gaia PR for master

Hi Alive,

Based on the previous r+, I further modified test cases to fit 
the new policy and all the relevant failure has been fixed. 
Could you have another review and help merge (if r+) the PR for 
master? (because I have no permission to merge.) Thanks!

Comment 31

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8522059 [details] [review]
gaia PR for master

r=me

Comment 32

[Henry Chang [:hchang]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #31)
> Comment on attachment 8522059 [details] [review]
> gaia PR for master
> 
> r=me

Thanks alive! Could you also help merge this PR for master? Thanks!

Comment 33

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/gaia/commit/bd9507aa5ca35eef9541f65298d64f4bfad66de0

Comment 34

[Henry Chang [:hchang]]

Comment on attachment 8521962 [details] [review]
gaia PR for 2.0

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Wifi
[User impact] if declined: Wifi would consume more power before sleeping
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): No
[String changes made]: No

Comment 35

[Henry Chang [:hchang]]

Comment on attachment 8521962 [details] [review]
gaia PR for 2.0

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Wifi
[User impact] if declined: Wifi would consume more power before sleeping
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): No
[String changes made]: No

Comment 36

[Henry Chang [:hchang]]

Comment on attachment 8521964 [details] [review]
gaia PR for 2.1

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Wifi
[User impact] if declined: Wifi would consume more power before sleeping
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): No
[String changes made]: No

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

2.1: https://github.com/mozilla-b2g/gaia/commit/4297952929674ab2bf1747380ee2d6cd2dcd5269
2.0: https://github.com/mozilla-b2g/gaia/commit/a4b7067b62dadfe3573d2ccaffedb3f43cd4eb84