Index | Thread | Search

From:
Kirill A. Korinsky <kirill@korins.ky>
Subject:
Re: sys/iwx: support powersave
To:
Stefan Sperling <stsp@stsp.name>
Cc:
OpenBSD tech <tech@openbsd.org>, Klemens Nanni <kn@openbsd.org>
Date:
Sat, 14 Feb 2026 19:32:29 +0100

Download raw body.

Thread
On Thu, 12 Feb 2026 09:39:54 +0100,
Stefan Sperling <stsp@stsp.name> wrote:
> 
> On Fri, Feb 06, 2026 at 09:52:48PM +0100, Kirill A. Korinsky wrote:
> > On Fri, 06 Feb 2026 20:24:42 +0100,
> > Stuart Henderson <stu@spacehopper.org> wrote:
> > > 
> > > On 2026/02/06 19:45, Kirill A. Korinsky wrote:
> > > > No, it follow the same pattern like iwn and wpi: to enable it, someone needs
> > > > to explicity adds powersave into ifconfig args, and -powersave to disable.
> > > > 
> > > > But I think I should document in man in this diff that it supports
> > > > powersave, similar with iwn and wpi.
> > > 
> > > Those are "older devices" as per
> > > 
> > >    powersave
> > >            Enable 802.11 power saving mode.  This option is generally only
> > >            relevant to older devices where power saving is disabled by
> > >            default.  On modern hardware, drivers will ask the firmware to
> > >            automatically enable any applicable power-saving features.
> > > 
> > > I think some of the older ones were a bit unsure whether it works
> > > properly on all devices (so allowing it to be disabled) but it should
> > > be ok on newer ones..
> > > 
> > 
> > Now it is disabled by default and should be enabled manually, so it should
> > be safe, isn't it?
> 
> Power management in iwm/iwx is not entirely disabled by default at present.
> 
> At present the driver is always setting the POWER_SAVE_ENA_MSK bit which
> allows the device to "save power by turning off receiver and transmitter".
> I assume (not having made any measurements) that this does save some battery.
> It is certain that the firmware is doing something with this because while
> monitoring iwx traffic we can see the device sending Null-Data frames to the
> AP with the PM snooze/awake bit toggling on/off. Which is not something our
> net80211 stack could do by itself.
> 
> The proposed diff turns this feature off by default because all calls to
> iwx_power_mac_update_mode() are removed (even though this function is not
> deleted with this diff?) and replaced with iwx_set_ps_level() which as far
> as I can tell doesn't get called if the ioctl which enables power management
> does not run. So the proposed diff would probably cost people some battery
> because nobody will bother running the additional command to enable power
> management. 
> 
> I do think enabling support for legacy power management (DTIM) will have some
> benefits, so I agree with the direction your diff is taking. If you want to
> investigate further, on modern wifi networks there are additional
> possibilities. See here for an overview of the state of the art in 2014:
> https://mrncciew.com/2014/10/14/cwap-802-11-power-management/
> 
> There are probably even more features in modern standards but the intel
> firmware supports all the important 2014 features mentioned there and
> we leave them all disabled at present apart from POWER_SAVE_ENA_MSK.
> 
> DTIM power management is a good start. Also supporting uAPSD and SMSP would
> probably help save power quite a bit more. These newer features depend on
> negotiation with the AP so having them be controlled by an ioctl which forces
> PM on or off makes zero sense.
> 
> The only use case I see for the ioctl would be to disable all power
> management when using an AP such as ral(4) which cannot handle clients
> using DTIM. At this point most APs in active use should be able to handle
> DTIM power management as enabled by your patch (smartphones would not have
> working wifi otherwise), if not also uAPSD and SMSP, etc.
> So if we're going to be making use of this it should be done by default.
> 
> By the way, I have quite a bit of diffs built up in order to support
> BZ devices which is still work in progress and might clash a little
> bit with your diffs, but not very much.
> 

Thanks for the link and feedback.

Here an updated patch with in this direction. 

I did not touch uAPSD, advanced PM flags, SMPS, or misbehaving-AP fallback.
The change set is already relatively large, so I prefer to move by small steps.

This patch enables power save by default for iwx by setting
IEEE80211_F_PMGTON (and IEEE80211_C_PMGT), and makes iwx react to
SIOCS80211POWER by switching between PM (level 3) and CAM at runtime.


Index: share/man/man4/iwx.4
===================================================================
RCS file: /home/cvs/src/share/man/man4/iwx.4,v
diff -u -p -r1.22 iwx.4
--- share/man/man4/iwx.4	1 Dec 2025 16:44:13 -0000	1.22
+++ share/man/man4/iwx.4	6 Feb 2026 18:33:51 -0000
@@ -76,7 +76,7 @@ driver supports protected management fra
 of management frames such as deauthentication, disassociation, and block-ack
 negotiation, on networks using WPA.
 .Pp
-In BSS mode the driver supports background scanning;
+In BSS mode the driver supports powersave mode and background scanning;
 see
 .Xr ifconfig 8 .
 .Pp
Index: sys/dev/pci/if_iwx.c
===================================================================
RCS file: /home/cvs/src/sys/dev/pci/if_iwx.c,v
diff -u -p -r1.197 if_iwx.c
--- sys/dev/pci/if_iwx.c	11 Feb 2026 11:14:53 -0000	1.197
+++ sys/dev/pci/if_iwx.c	14 Feb 2026 18:13:13 -0000
@@ -402,10 +402,7 @@ int	iwx_flush_sta(struct iwx_softc *, st
 int	iwx_beacon_filter_send_cmd(struct iwx_softc *,
 	    struct iwx_beacon_filter_cmd *);
 int	iwx_update_beacon_abort(struct iwx_softc *, struct iwx_node *, int);
-void	iwx_power_build_cmd(struct iwx_softc *, struct iwx_node *,
-	    struct iwx_mac_power_cmd *);
-int	iwx_power_mac_update_mode(struct iwx_softc *, struct iwx_node *);
-int	iwx_power_update_device(struct iwx_softc *);
+int	iwx_set_pslevel(struct iwx_softc *, int, int, int);
 int	iwx_enable_beacon_filter(struct iwx_softc *, struct iwx_node *);
 int	iwx_disable_beacon_filter(struct iwx_softc *);
 int	iwx_add_sta_cmd(struct iwx_softc *, struct iwx_node *, int);
@@ -6620,68 +6617,80 @@ iwx_update_beacon_abort(struct iwx_softc
 	return iwx_beacon_filter_send_cmd(sc, &cmd);
 }
 
-void
-iwx_power_build_cmd(struct iwx_softc *sc, struct iwx_node *in,
-    struct iwx_mac_power_cmd *cmd)
+int
+iwx_set_pslevel(struct iwx_softc *sc, int dtim, int level, int async)
 {
 	struct ieee80211com *ic = &sc->sc_ic;
-	struct ieee80211_node *ni = &in->in_ni;
+	struct iwx_device_power_cmd dcmd = { };
+	struct iwx_mac_power_cmd mcmd;
+	struct iwx_node *in;
+	struct ieee80211_node *ni;
+	const struct iwx_pmgt *pmgt;
+	int range, skip_dtim, cmd_flags, err;
 	int dtim_period, dtim_msec, keep_alive;
 
-	cmd->id_and_color = htole32(IWX_FW_CMD_ID_AND_COLOR(in->in_id,
-	    in->in_color));
-	if (ni->ni_dtimperiod)
-		dtim_period = ni->ni_dtimperiod;
-	else
-		dtim_period = 1;
-
-	/*
-	 * Regardless of power management state the driver must set
-	 * keep alive period. FW will use it for sending keep alive NDPs
-	 * immediately after association. Check that keep alive period
-	 * is at least 3 * DTIM.
-	 */
-	dtim_msec = dtim_period * ni->ni_intval;
-	keep_alive = MAX(3 * dtim_msec, 1000 * IWX_POWER_KEEP_ALIVE_PERIOD_SEC);
-	keep_alive = roundup(keep_alive, 1000) / 1000;
-	cmd->keep_alive_seconds = htole16(keep_alive);
+	if (ic->ic_opmode == IEEE80211_M_MONITOR)
+		return 0;
 
-	if (ic->ic_opmode != IEEE80211_M_MONITOR)
-		cmd->flags = htole16(IWX_POWER_FLAGS_POWER_SAVE_ENA_MSK);
-}
+	if (dtim == 0) {
+		dtim = 1;
+		skip_dtim = 0;
+	} else
+		skip_dtim = -1;
 
-int
-iwx_power_mac_update_mode(struct iwx_softc *sc, struct iwx_node *in)
-{
-	int err;
-	int ba_enable;
-	struct iwx_mac_power_cmd cmd;
+	if (dtim <= 2)
+		range = 0;
+	else if (dtim <= 10)
+		range = 1;
+	else
+		range = 2;
 
-	memset(&cmd, 0, sizeof(cmd));
+	pmgt = &iwx_pmgt[range][level];
+	if (skip_dtim == -1)
+		skip_dtim = pmgt->skip_dtim;
 
-	iwx_power_build_cmd(sc, in, &cmd);
+	if (level != 0)
+		dcmd.flags = htole16(IWX_DEVICE_POWER_FLAGS_POWER_SAVE_ENA_MSK);
 
-	err = iwx_send_cmd_pdu(sc, IWX_MAC_PM_POWER_TABLE, 0,
-	    sizeof(cmd), &cmd);
-	if (err != 0)
+	cmd_flags = async ? IWX_CMD_ASYNC : 0;
+	err = iwx_send_cmd_pdu(sc, IWX_POWER_TABLE_CMD, cmd_flags,
+	    sizeof(dcmd), &dcmd);
+	if (err)
 		return err;
 
-	ba_enable = !!(cmd.flags &
-	    htole16(IWX_POWER_FLAGS_POWER_MANAGEMENT_ENA_MSK));
-	return iwx_update_beacon_abort(sc, in, ba_enable);
-}
+	if ((sc->sc_flags & IWX_FLAG_MAC_ACTIVE) == 0)
+		return 0;
 
-int
-iwx_power_update_device(struct iwx_softc *sc)
-{
-	struct iwx_device_power_cmd cmd = { };
-	struct ieee80211com *ic = &sc->sc_ic;
+	in = (void *)ic->ic_bss;
+	ni = &in->in_ni;
 
-	if (ic->ic_opmode != IEEE80211_M_MONITOR)
-		cmd.flags = htole16(IWX_DEVICE_POWER_FLAGS_POWER_SAVE_ENA_MSK);
+	memset(&mcmd, 0, sizeof(mcmd));
+	mcmd.id_and_color = htole32(IWX_FW_CMD_ID_AND_COLOR(in->in_id,
+	    in->in_color));
+	dtim_period = ni->ni_dtimperiod ? ni->ni_dtimperiod : 1;
+	dtim_msec = dtim_period * ni->ni_intval;
+	keep_alive = MAX(3 * dtim_msec, 1000 * IWX_POWER_KEEP_ALIVE_PERIOD_SEC);
+	keep_alive = roundup(keep_alive, 1000) / 1000;
+	mcmd.keep_alive_seconds = htole16(keep_alive);
 
-	return iwx_send_cmd_pdu(sc,
-	    IWX_POWER_TABLE_CMD, 0, sizeof(cmd), &cmd);
+	if (level != 0) {
+		mcmd.flags = htole16(IWX_POWER_FLAGS_POWER_SAVE_ENA_MSK |
+		    IWX_POWER_FLAGS_POWER_MANAGEMENT_ENA_MSK);
+		mcmd.rx_data_timeout = htole32(pmgt->rxtimeout * 1024);
+		mcmd.tx_data_timeout = htole32(pmgt->txtimeout * 1024);
+		if (skip_dtim != 0) {
+			mcmd.flags |= htole16(IWX_POWER_FLAGS_SKIP_OVER_DTIM_MSK);
+			mcmd.skip_dtim_periods = skip_dtim + 1;
+		}
+	}
+
+	err = iwx_send_cmd_pdu(sc, IWX_MAC_PM_POWER_TABLE, cmd_flags,
+	    sizeof(mcmd), &mcmd);
+	if (err != 0)
+		return err;
+
+	return iwx_update_beacon_abort(sc, in, !!(mcmd.flags &
+	    htole16(IWX_POWER_FLAGS_POWER_MANAGEMENT_ENA_MSK)));
 }
 
 int
@@ -8712,7 +8721,8 @@ iwx_run(struct iwx_softc *sc)
 		return err;
 	}
 
-	err = iwx_power_update_device(sc);
+	err = iwx_set_pslevel(sc, 0,
+	    (ic->ic_flags & IEEE80211_F_PMGTON) ? 3 : 0, 1);
 	if (err) {
 		printf("%s: could not send power command (error %d)\n",
 		    DEVNAME(sc), err);
@@ -8734,13 +8744,6 @@ iwx_run(struct iwx_softc *sc)
 	if (ic->ic_opmode == IEEE80211_M_MONITOR)
 		return 0;
 
-	err = iwx_power_mac_update_mode(sc, in);
-	if (err) {
-		printf("%s: could not update MAC power (error %d)\n",
-		    DEVNAME(sc), err);
-		return err;
-	}
-
 	/* Start at lowest available bit-rate. Firmware will raise. */
 	in->in_ni.ni_txrate = 0;
 	in->in_ni.ni_txmcs = 0;
@@ -9632,7 +9635,8 @@ iwx_init_hw(struct iwx_softc *sc)
 			goto err;
 	}
 
-	err = iwx_power_update_device(sc);
+	err = iwx_set_pslevel(sc, 0,
+	    (ic->ic_flags & IEEE80211_F_PMGTON) ? 3 : 0, 0);
 	if (err) {
 		printf("%s: could not send power command (error %d)\n",
 		    DEVNAME(sc), err);
@@ -9986,6 +9990,7 @@ int
 iwx_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 {
 	struct iwx_softc *sc = ifp->if_softc;
+	struct ieee80211com *ic = &sc->sc_ic;
 	int s, err = 0, generation = sc->sc_generation;
 
 	/*
@@ -10018,6 +10023,21 @@ iwx_ioctl(struct ifnet *ifp, u_long cmd,
 		}
 		break;
 
+	case SIOCS80211POWER:
+		err = ieee80211_ioctl(ifp, cmd, data);
+		if (err != ENETRESET)
+			break;
+		if (ic->ic_state == IEEE80211_S_RUN) {
+			if (ic->ic_flags & IEEE80211_F_PMGTON)
+				err = iwx_set_pslevel(sc, 0, 3, 0);
+			else	/* back to CAM */
+				err = iwx_set_pslevel(sc, 0, 0, 0);
+		} else {
+			/* Defer until transition to IEEE80211_S_RUN. */
+			err = 0;
+		}
+		break;
+
 	default:
 		err = ieee80211_ioctl(ifp, cmd, data);
 	}
@@ -11902,6 +11922,7 @@ iwx_attach(struct device *parent, struct
 	ic->ic_caps =
 	    IEEE80211_C_QOS | IEEE80211_C_TX_AMPDU | /* A-MPDU */
 	    IEEE80211_C_ADDBA_OFFLOAD | /* device sends ADDBA/DELBA frames */
+	    IEEE80211_C_PMGT |		/* power management */
 	    IEEE80211_C_WEP |		/* WEP */
 	    IEEE80211_C_RSN |		/* WPA/RSN */
 	    IEEE80211_C_SCANALL |	/* device scans all channels at once */
@@ -11910,6 +11931,7 @@ iwx_attach(struct device *parent, struct
 	    IEEE80211_C_SHSLOT |	/* short slot time supported */
 	    IEEE80211_C_SHPREAMBLE |	/* short preamble supported */
 	    IEEE80211_C_MFP;		/* management frame protection */
+	ic->ic_flags |= IEEE80211_F_PMGTON;
 
 	ic->ic_htcaps = IEEE80211_HTCAP_SGI20 | IEEE80211_HTCAP_SGI40;
 	ic->ic_htcaps |= IEEE80211_HTCAP_CBW20_40;
Index: sys/dev/pci/if_iwxreg.h
===================================================================
RCS file: /home/cvs/src/sys/dev/pci/if_iwxreg.h,v
diff -u -p -r1.58 if_iwxreg.h
--- sys/dev/pci/if_iwxreg.h	1 Dec 2025 16:44:13 -0000	1.58
+++ sys/dev/pci/if_iwxreg.h	30 Jan 2026 23:52:35 -0000
@@ -5096,6 +5096,42 @@ struct iwx_mac_power_cmd {
 #define IWX_DEFAULT_PS_TX_DATA_TIMEOUT      (100 * 1000)
 #define IWX_DEFAULT_PS_RX_DATA_TIMEOUT      (100 * 1000)
 
+#define IWX_NDTIMRANGES		3
+#define IWX_NPOWERLEVELS	6
+static const struct iwx_pmgt {
+	uint32_t	rxtimeout;
+	uint32_t	txtimeout;
+	int		skip_dtim;
+} iwx_pmgt[IWX_NDTIMRANGES][IWX_NPOWERLEVELS] = {
+	/* DTIM <= 2 */
+	{
+	{   0,   0, 0 },	/* CAM */
+	{ 200, 500, 0 },	/* PS level 1 */
+	{ 200, 300, 0 },	/* PS level 2 */
+	{  50, 100, 0 },	/* PS level 3 */
+	{  50,  25, 1 },	/* PS level 4 */
+	{  25,  25, 2 }		/* PS level 5 */
+	},
+	/* 3 <= DTIM <= 10 */
+	{
+	{   0,   0, 0 },	/* CAM */
+	{ 200, 500, 0 },	/* PS level 1 */
+	{ 200, 300, 0 },	/* PS level 2 */
+	{  50, 100, 0 },	/* PS level 3 */
+	{  50,  25, 1 },	/* PS level 4 */
+	{  25,  25, 2 }		/* PS level 5 */
+	},
+	/* DTIM >= 11 */
+	{
+	{   0,   0, 0 },	/* CAM */
+	{ 200, 500, 0 },	/* PS level 1 */
+	{ 200, 300, 0 },	/* PS level 2 */
+	{  50, 100, 0 },	/* PS level 3 */
+	{  50,  25, 0 },	/* PS level 4 */
+	{  25,  25, 0 }		/* PS level 5 */
+	}
+};
+
 /*
  * struct iwx_uapsd_misbehaving_ap_notif - FW sends this notification when
  * associated AP is identified as improperly implementing uAPSD protocol.