diff mbox series

netdev: move power save disabling until after interface is up

Message ID 20231121183321.179230-1-prestwoj@gmail.com (mailing list archive)
State Accepted, archived
Headers show
Series netdev: move power save disabling until after interface is up | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
prestwoj/iwd-alpine-ci-fetch success Fetch PR
prestwoj/iwd-ci-gitlint fail netdev: move power save disabling until after interface is up 25: B1 Line exceeds max length (84>80): "iwd[1286641]: src/netdev.c:netdev_disable_ps_cb() Disabled power save for ifindex 54"
prestwoj/iwd-ci-fetch success Fetch PR
prestwoj/iwd-ci-makedistcheck success Make Distcheck
prestwoj/iwd-ci-incremental_build success Incremental build not run PASS
prestwoj/iwd-alpine-ci-makedistcheck success Make Distcheck
prestwoj/iwd-alpine-ci-incremental_build success Incremental build not run PASS
prestwoj/iwd-ci-build success Build - Configure
prestwoj/iwd-alpine-ci-build success Build - Configure
prestwoj/iwd-ci-makecheckvalgrind success Make Check w/Valgrind
prestwoj/iwd-ci-clang success clang PASS
prestwoj/iwd-ci-makecheck success Make Check
prestwoj/iwd-alpine-ci-makecheckvalgrind success Make Check w/Valgrind
prestwoj/iwd-alpine-ci-makecheck success Make Check
prestwoj/iwd-ci-testrunner success test-runner PASS

Commit Message

James Prestwood Nov. 21, 2023, 6:33 p.m. UTC 
Very rarely on ath10k (potentially other ath cards), disabling
power save while the interface is down causes a timeout when
bringing the interface back up. This seems to be a race in the
driver or firmware but it causes IWD to never start up properly
since there is no retry logic on that path.

Retrying is an option, but a more straight forward approach is
to just reorder the logic to set power save off after the
interface is already up. If the power save setting fails we can
just log it, ignore the failure, and continue. From a users point
of view there is no real difference in doing it this way as
PS still gets disabled prior to IWD connecting/sending data.

Changing behavior based on a buggy driver isn't something we
should be doing, but in this instance the change shouldn't have
any downside and actually isn't any different than how it has
been done prior to the driver quirks change (i.e. use network
manager, iw, or iwconfig to set power save after IWD starts).

For reference, this problem is quite rare and difficult to say
exactly how often but certainly <1% of the time:

iwd[1286641]: src/netdev.c:netdev_disable_ps_cb() Disabled power save for ifindex 54
kernel: ath10k_pci 0000:02:00.0: wmi service ready event not received
iwd[1286641]: Error bringing interface 54 up: Connection timed out
kernel: ath10k_pci 0000:02:00.0: Could not init core: -110

After this IWD just sits idle as it has no interface to start using.

This is even reproducable outside of IWD if you loop and run:

ip link set <wlan> down
iw dev <wlan> set power_save off
ip link set <wlan> up

Eventually the 'up' command will fail with a timeout.

I've brought this to the linux-wireless/ath10k mailing list but
even if its fixed in future kernels we'd still need to support
older kernels, so a workaround/change in IWD is still required.
---
 src/netdev.c | 86 +++++++++++++++++++++++++++-------------------------
 1 file changed, 44 insertions(+), 42 deletions(-)

Comments

Denis Kenzior Nov. 23, 2023, 3:22 p.m. UTC | #1 
Hi James,

On 11/21/23 12:33, James Prestwood wrote:
> Very rarely on ath10k (potentially other ath cards), disabling
> power save while the interface is down causes a timeout when
> bringing the interface back up. This seems to be a race in the
> driver or firmware but it causes IWD to never start up properly
> since there is no retry logic on that path.
> 
> Retrying is an option, but a more straight forward approach is
> to just reorder the logic to set power save off after the
> interface is already up. If the power save setting fails we can
> just log it, ignore the failure, and continue. From a users point
> of view there is no real difference in doing it this way as
> PS still gets disabled prior to IWD connecting/sending data.
> 
> Changing behavior based on a buggy driver isn't something we
> should be doing, but in this instance the change shouldn't have
> any downside and actually isn't any different than how it has
> been done prior to the driver quirks change (i.e. use network
> manager, iw, or iwconfig to set power save after IWD starts).
> 
> For reference, this problem is quite rare and difficult to say
> exactly how often but certainly <1% of the time:
> 
> iwd[1286641]: src/netdev.c:netdev_disable_ps_cb() Disabled power save for ifindex 54
> kernel: ath10k_pci 0000:02:00.0: wmi service ready event not received
> iwd[1286641]: Error bringing interface 54 up: Connection timed out
> kernel: ath10k_pci 0000:02:00.0: Could not init core: -110
> 
> After this IWD just sits idle as it has no interface to start using.
> 
> This is even reproducable outside of IWD if you loop and run:
> 
> ip link set <wlan> down
> iw dev <wlan> set power_save off
> ip link set <wlan> up
> 
> Eventually the 'up' command will fail with a timeout.
> 
> I've brought this to the linux-wireless/ath10k mailing list but
> even if its fixed in future kernels we'd still need to support
> older kernels, so a workaround/change in IWD is still required.
> ---
>   src/netdev.c | 86 +++++++++++++++++++++++++++-------------------------
>   1 file changed, 44 insertions(+), 42 deletions(-)
> 

Applied, thanks.

Regards,
-Denis
diff mbox series

Patch

diff --git a/src/netdev.c b/src/netdev.c
index e31a5161..f23ca59e 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -5935,6 +5935,44 @@  static void netdev_dellink_notify(const struct ifinfomsg *ifi, int bytes)
 	netdev_free(netdev);
 }
 
+static void netdev_disable_ps_cb(struct l_genl_msg *msg, void *user_data)
+{
+	struct netdev *netdev = user_data;
+	int err = l_genl_msg_get_error(msg);
+
+	netdev->power_save_cmd_id = 0;
+
+	/* Can't do anything about it but inform the user */
+	if (err < 0)
+		l_error("Failed to disable power save for ifindex %u (%s: %d)",
+				netdev->index, strerror(-err), err);
+	else
+		l_debug("Disabled power save for ifindex %u", netdev->index);
+
+	WATCHLIST_NOTIFY(&netdev_watches, netdev_watch_func_t,
+				netdev, NETDEV_WATCH_EVENT_NEW);
+	netdev->events_ready = true;
+}
+
+static bool netdev_disable_power_save(struct netdev *netdev)
+{
+	struct l_genl_msg *msg = l_genl_msg_new(NL80211_CMD_SET_POWER_SAVE);
+	uint32_t disabled = NL80211_PS_DISABLED;
+
+	l_genl_msg_append_attr(msg, NL80211_ATTR_IFINDEX, 4, &netdev->index);
+	l_genl_msg_append_attr(msg, NL80211_ATTR_PS_STATE, 4, &disabled);
+
+	netdev->power_save_cmd_id = l_genl_family_send(nl80211, msg,
+							netdev_disable_ps_cb,
+							netdev, NULL);
+	if (!netdev->power_save_cmd_id) {
+		l_error("Failed to send SET_POWER_SAVE (-EIO)");
+		return false;
+	}
+
+	return true;
+}
+
 static void netdev_initial_up_cb(int error, uint16_t type, const void *data,
 					uint32_t len, void *user_data)
 {
@@ -5967,6 +6005,12 @@  static void netdev_initial_up_cb(int error, uint16_t type, const void *data,
 
 	scan_wdev_add(netdev->wdev_id);
 
+	if (wiphy_power_save_disabled(netdev->wiphy)) {
+		/* Wait to issue EVENT_NEW until power save is disabled */
+		if (netdev_disable_power_save(netdev))
+			return;
+	}
+
 	WATCHLIST_NOTIFY(&netdev_watches, netdev_watch_func_t,
 				netdev, NETDEV_WATCH_EVENT_NEW);
 	netdev->events_ready = true;
@@ -6130,42 +6174,6 @@  static void netdev_get_link(struct netdev *netdev)
 	l_free(rtmmsg);
 }
 
-static void netdev_disable_ps_cb(struct l_genl_msg *msg, void *user_data)
-{
-	struct netdev *netdev = user_data;
-	int err = l_genl_msg_get_error(msg);
-
-	netdev->power_save_cmd_id = 0;
-
-	/* Can't do anything about it but inform the user */
-	if (err < 0)
-		l_error("Failed to disable power save for ifindex %u (%s: %d)",
-				netdev->index, strerror(-err), err);
-	else
-		l_debug("Disabled power save for ifindex %u", netdev->index);
-
-	netdev_get_link(netdev);
-}
-
-static bool netdev_disable_power_save(struct netdev *netdev)
-{
-	struct l_genl_msg *msg = l_genl_msg_new(NL80211_CMD_SET_POWER_SAVE);
-	uint32_t disabled = NL80211_PS_DISABLED;
-
-	l_genl_msg_append_attr(msg, NL80211_ATTR_IFINDEX, 4, &netdev->index);
-	l_genl_msg_append_attr(msg, NL80211_ATTR_PS_STATE, 4, &disabled);
-
-	netdev->power_save_cmd_id = l_genl_family_send(nl80211, msg,
-							netdev_disable_ps_cb,
-							netdev, NULL);
-	if (!netdev->power_save_cmd_id) {
-		l_error("Failed to send SET_POWER_SAVE (-EIO)");
-		return false;
-	}
-
-	return true;
-}
-
 struct netdev *netdev_create_from_genl(struct l_genl_msg *msg,
 					const uint8_t *set_mac)
 {
@@ -6237,12 +6245,6 @@  struct netdev *netdev_create_from_genl(struct l_genl_msg *msg,
 
 	netdev_setup_interface(netdev);
 
-	if (wiphy_power_save_disabled(wiphy)) {
-		/* Wait to issue GET_LINK until PS is disabled */
-		if (netdev_disable_power_save(netdev))
-			return netdev;
-	}
-
 	netdev_get_link(netdev);
 
 	return netdev;