GNOME Bugzilla – Bug 760125
Add default/unset wifi.powersave value and use it by default
Last modified: 2016-02-15 23:32:35 UTC
NM probably shouldn't disable powersave when it starts up if it's set on a wifi device; it turns out some drivers (or things outside NM like 'powertop') may turn powersave on. wifi.powersave is already a uint, and we haven't released 1.2 yet, so maybe we should make the default value be 'don't touch' and only change powersave when the value is explicitly set.
got also [1] http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=15ab4bda5bf1b8defad4fab43ca1ebb7236da2e2 This patch implements different behaviors for libnm-utils vs. libnm-core -- which is wrong and needs fixing. Probably (as we didn't yet release 1.2, and as [1] already messes with the default), we should take the time and update the possible values to make it an enum with the following values: DEFAULT DONT_TOUCH ENABLED (formerly TRUE) DISABLED (formerly FALSE) and make the default overwritable via our usual NetworkManager.conf mechanism. This is IMO a must-have before 1.2-rc1
Lubomir suggested to drop this property entirely and make it configurable in a global configuration file or via UDEV.
(In reply to Thomas Haller from comment #2) > Lubomir suggested to drop this property entirely and make it configurable in > a global configuration file or via UDEV. Yeah, I know he did, but when powersave is problematic it's problematic with specific access points, so that wants a per-network toggle. Yes, drivers have problems with powersave too, but that can be fixed by a kernel update. You can't really fix an access point as easily.
Didn't Lubo revert 15ab4bda5bf1b8defad4fab43ca1ebb7236da2e2 though?
(In reply to Dan Williams from comment #4) > Didn't Lubo revert 15ab4bda5bf1b8defad4fab43ca1ebb7236da2e2 though? yes. (In reply to Dan Williams from comment #3) > (In reply to Thomas Haller from comment #2) > > Lubomir suggested to drop this property entirely and make it configurable in > > a global configuration file or via UDEV. > > Yeah, I know he did, but when powersave is problematic it's problematic with > specific access points, so that wants a per-network toggle. Yes, drivers > have problems with powersave too, but that can be fixed by a kernel update. > You can't really fix an access point as easily. Agree. Let's do comment 2 with DEFAULT/DONT_TOUCH/ENABLED/DISABLED
(In reply to Thomas Haller from comment #5) > Let's do comment 2 with DEFAULT/DONT_TOUCH/ENABLED/DISABLED comment 1, I meant
Created attachment 320857 [details] [review] [PATCH] wifi: don't touch by default current powersave setting Please review.
(In reply to Beniamino Galvani from comment #7) > Created attachment 320857 [details] [review] [review] > [PATCH] wifi: don't touch by default current powersave setting > > Please review. - g_object_set (setting, prop, (guint32) powersave_int, NULL); + g_object_set (setting, prop, powersave, NULL); should be cast to (guint). + <listitem><para>If left unspecified, leave the current setting + unchanged.</para></listitem> maybe say more explicitly that the default is "IGNORE"? trailing whitespace in set_powersafe() :) Rest lgtm
(In reply to Thomas Haller from comment #8) > should be cast to (guint). > maybe say more explicitly that the default is "IGNORE"? > trailing whitespace in set_powersafe() :) Fixed, thanks.
lgtm
lgtm, though this will make Ubuntu sad since they cherry-picked the powersave patches with only TRUE/FALSE back to 1.0. Maybe also put a comment in the commit message about how a distro could enable powersave by default for everyone by dropping a file in conf.d with the right defaults?
(In reply to Dan Williams from comment #11) > lgtm, though this will make Ubuntu sad since they cherry-picked the > powersave patches with only TRUE/FALSE back to 1.0. Would it be helpful if we change enum values from: NM_SETTING_WIRELESS_POWERSAVE_DEFAULT = 0, NM_SETTING_WIRELESS_POWERSAVE_IGNORE = 1, NM_SETTING_WIRELESS_POWERSAVE_DISABLE = 2, NM_SETTING_WIRELESS_POWERSAVE_ENABLE = 3, to NM_SETTING_WIRELESS_POWERSAVE_DEFAULT = 0, NM_SETTING_WIRELESS_POWERSAVE_ENABLE = 1, NM_SETTING_WIRELESS_POWERSAVE_DISABLE = 2, NM_SETTING_WIRELESS_POWERSAVE_IGNORE = 3, so that users having 0 in their configuration would still have power saving disabled (unless overridden or changed externally) and 1 would still mean 'enable' after an upgrade?
It seems to me, that all that Ubuntu did to ~enable powersave by default~ was »··»··· g_param_spec_uint (NM_SETTING_WIRELESS_POWERSAVE, "", "", -»··»··· 0, G_MAXUINT32, 0, +»··»··· 0, G_MAXUINT32, 1, »··»··· G_PARAM_READWRITE)); in libnm-utils (!!). That has basically the effect that an older client will not serialize the value 1 via D-Bus, but only the value 0. OTOH, the server uses libnm-core and treats missing values as 0. Also, keyfile will not write the default (0) to keyfile file either. Anyway, I would not worry about this too much. In the worst case, a user managed to enable powersave, but after upgrade it gets reset to "IGNORE".
(In reply to Dan Williams from comment #11) > Maybe also put a comment in the commit message about how a distro could > enable powersave by default for everyone by dropping a file in conf.d with > the right defaults? Sure. (In reply to Thomas Haller from comment #13) > Anyway, I would not worry about this too much. In the worst case, a user > managed to enable powersave, but after upgrade it gets reset to "IGNORE". Ok, merged to master as is: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=10b222288e686f0e37eaf148a8d99100d75a178b