Closed Bug 859486 Opened 11 years ago Closed 11 years ago

Don't don't unload wireless drivers

Categories

(Core :: DOM: Device Interfaces, defect, P1)

All
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla23
blocking-b2g leo+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: m1, Assigned: m1)

Details

(Whiteboard: [caf priority: p1][CR 472163])

Attachments

(2 files, 1 obsolete file)

There have been reports of significant power drain as a result of not unloading the wifi drivers.  Even if the assumption made by bug 801935 [1] is correct, Android does unload the wifi drivers as SOP and bug 801935 was raised due to issues with preproduction hardware/software.   So if unloading wifi drivers still causes issues on Unagi can Moz work with the vendor to get a fix in parallel to backing out bug 801935 for v1.1?

[1] Unloading wireless drivers can trigger bugs on improperly written drivers and is unnecessary on properly written drivers where taking the interface down powers down the hardware.
Unloading wifi drivers is not SOP on Jellybean. AFAICT, there is no module loading at all on Jellybean. Not unloading modules is really the right thing to do here. The driver needs to be fixed, but if it can't be done in time, we can put in a quirk to have it unload.
We're not on a Jellybean-based gonk yet, so Jellybean-era fixes are not necessarily relevant for v1.0.1/v1.1 unfortunately (too much regression risk).

Does the latest unagi kernel still suffer from bug 801935?  

I'm happy to contribute a patch that adds this quirk though.  How about calling it   "ro.moz.wifi.unloadmodules=true"?
(In reply to Michael Vines [:m1] [:evilmachines] from comment #2)
> We're not on a Jellybean-based gonk yet, so Jellybean-era fixes are not
> necessarily relevant for v1.0.1/v1.1 unfortunately (too much regression
> risk).
> 

They're not - I agree. Just pointed that out as a reference for what we'll want going forward.

> I'm happy to contribute a patch that adds this quirk though.  How about
> calling it   "ro.moz.wifi.unloadmodules=true"?

Sounds good to me.
Assignee: nobody → mvines
Summary: Don't Don't unload wireless drivers → Don't unload wireless drivers
Heh, that 2nd don't was intentional but perhaps unclear due to the incorrect capitalization of the 2nd don't.  I've corrected this now.
Summary: Don't unload wireless drivers → Don't don't unload wireless drivers
Attached patch attempt #1 (git flavour) (obsolete) — Splinter Review
|adb shell setprop ro.moz.wifi.unloaddriver 1| to enable
Attachment #735267 - Flags: review?(mwu)
Keywords: unagi
Whiteboard: [CR 472163]
Comment on attachment 735267 [details] [diff] [review]
attempt #1 (git flavour)

Looks good to me, though I think mrbkap is a more appropriate reviewer (if he has time).
Attachment #735267 - Flags: review?(mwu) → review?(mrbkap)
Comment on attachment 735267 [details] [diff] [review]
attempt #1 (git flavour)

Review of attachment 735267 [details] [diff] [review]:
-----------------------------------------------------------------

We probably won't be able to turn off driver unloading for Unagis or Otoros (to wit: Android on those phones doesn't unload the driver) but this looks good to me.

::: dom/wifi/WifiWorker.js
@@ +46,5 @@
>      return parseInt(sdkVersion, 10);
>    }
> +  function getUnloadDriverEnabled() {
> +    let enabled = libcutils.property_get("ro.moz.wifi.unloaddriver");
> +    return parseInt(enabled, 10) == 1;

Nit: === instead of ==.
Attachment #735267 - Flags: review?(mrbkap) → review+
Attached patch m-c versionSplinter Review
Sorry to bug you for r? again Blake.  Turns out I based my patch on v1-train and had to make more changes to it when rebasing to m-c than I'm currently comfortable carrying r+ forward with.  But the good news is that the patch just got much smaller.
Attachment #736114 - Flags: review?(mrbkap)
Attachment #735267 - Attachment is obsolete: true
Attachment #736116 - Flags: review?(mrbkap)
Attachment #736114 - Flags: review?(mrbkap) → review+
Attachment #736116 - Flags: review?(mrbkap) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dbf9ab7dec31
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Flags: in-moztrap-
Whiteboard: [CR 472163] → [caf priority: p1][CR 472163]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: