From: James Prestwood <prestwoj@gmail.com>
To: iwd@lists.linux.dev
Cc: James Prestwood <prestwoj@gmail.com>
Subject: [PATCH] netdev: move power save disabling until after interface is up
Date: Tue, 21 Nov 2023 10:33:21 -0800 [thread overview]
Message-ID: <20231121183321.179230-1-prestwoj@gmail.com> (raw)
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(-)
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;
--
2.34.1
next reply other threads:[~2023-11-21 18:33 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-21 18:33 James Prestwood [this message]
2023-11-23 15:22 ` [PATCH] netdev: move power save disabling until after interface is up Denis Kenzior
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231121183321.179230-1-prestwoj@gmail.com \
--to=prestwoj@gmail.com \
--cc=iwd@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).