[PATCH 1/7] wiphy: store driver flags directly in wiphy object

[PATCH 1/7] wiphy: store driver flags directly in wiphy object

[James Prestwood]

Rather than keep a pointer to the driver_info entry copy the flags
into the wiphy object. This preps for supporting driver flags via
a configuration file, specifically allowing for entries that are a
subset of others. For example:

{ "rtl88*",          DEFAULT_IF },
{ "rtl88x2bu",       FORCE_PAE },

Before it was not possible to add entires like this since only the
last entry match would get set. Now DEFAULT_IF would get set to all
matches, and FORCE_PAE to only rtl88x2bu. This isn't especially
important for the static list since it could be modified to work
correctly, but will be needed when parsing flags from a
configuration file that may contain duplicates or subsets of the
static list.
---
 src/wiphy.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/wiphy.c b/src/wiphy.c
index d06b2447..ca8a4958 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -111,7 +111,7 @@ struct wiphy {
 	char *model_str;
 	char *vendor_str;
 	char *driver_str;
-	const struct driver_info *driver_info;
+	uint32_t driver_flags;
 	struct watchlist state_watches;
 	uint8_t extended_capabilities[EXT_CAP_LEN + 2]; /* max bitmap size + IE header */
 	uint8_t *iftype_extended_capabilities[NUM_NL80211_IFTYPES];
@@ -685,8 +685,7 @@ bool wiphy_uses_default_if(struct wiphy *wiphy)
 	if (!wiphy_get_driver(wiphy))
 		return true;
 
-	if (wiphy->driver_info &&
-			wiphy->driver_info->flags & DEFAULT_IF)
+	if (wiphy->driver_flags & DEFAULT_IF)
 		return true;
 
 	return false;
@@ -697,8 +696,7 @@ bool wiphy_control_port_enabled(struct wiphy *wiphy)
 	const struct l_settings *settings = iwd_get_config();
 	bool enabled;
 
-	if (wiphy->driver_info &&
-			wiphy->driver_info->flags & FORCE_PAE) {
+	if (wiphy->driver_flags & FORCE_PAE) {
 		l_info("Not using Control Port due to driver quirks: %s",
 				wiphy_get_driver(wiphy));
 		return false;
@@ -1885,7 +1883,7 @@ static bool wiphy_get_driver_name(struct wiphy *wiphy)
 
 	for (i = 0; i < L_ARRAY_SIZE(driver_infos); i++)
 		if (!fnmatch(driver_infos[i].prefix, wiphy->driver_str, 0))
-			wiphy->driver_info = &driver_infos[i];
+			wiphy->driver_flags |= driver_infos[i].flags;
 
 	return true;
 }
-- 
2.25.1

[PATCH 2/7] wiphy: allow for user-defined driver flags

[James Prestwood]

The driver_infos list in wiphy.c is hard coded and, naturally,
not configurable from a user perspective. As drivers are updated
or added users may be left with their system being broken until the
driver is added, IWD released, and packaged.

This adds the ability to define driver flags inside main.conf under
the "DriverFlags" group. Keys in this group correspond to values in
enum driver_flag and values are a list of glob matches for specific
drivers:

[DriverFlags]
DefaultInterface=rtl81*,rtl87*,rtl88*,rtw_*,brcmfmac,bcmsdh_sdmmc
ForcePae=buggy_pae_*
---
 src/wiphy.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/src/wiphy.c b/src/wiphy.c
index ca8a4958..6f8f6826 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -73,6 +73,11 @@ enum driver_flag {
 	FORCE_PAE = 0x2,
 };
 
+struct driver_flag_name {
+	const char *name;
+	enum driver_flag flag;
+};
+
 struct driver_info {
 	const char *prefix;
 	unsigned int flags;
@@ -93,6 +98,11 @@ static const struct driver_info driver_infos[] = {
 	{ "bcmsdh_sdmmc",    DEFAULT_IF },
 };
 
+static const struct driver_flag_name driver_flag_names[] = {
+	{ "DefaultInterface", DEFAULT_IF },
+	{ "ForcePae",         FORCE_PAE },
+};
+
 struct wiphy {
 	uint32_t id;
 	char name[20];
@@ -1868,6 +1878,9 @@ static bool wiphy_get_driver_name(struct wiphy *wiphy)
 	char driver_path[256];
 	ssize_t len;
 	unsigned int i;
+	unsigned int j;
+	const struct l_settings *config = iwd_get_config();
+	char **flag_list;
 
 	driver_link = l_strdup_printf("/sys/class/ieee80211/%s/device/driver",
 					wiphy->name);
@@ -1885,6 +1898,24 @@ static bool wiphy_get_driver_name(struct wiphy *wiphy)
 		if (!fnmatch(driver_infos[i].prefix, wiphy->driver_str, 0))
 			wiphy->driver_flags |= driver_infos[i].flags;
 
+	/* Check for any user-defined driver flags */
+	if (!l_settings_has_group(config, "DriverFlags"))
+		return true;
+
+	for (i = 0; i < L_ARRAY_SIZE(driver_flag_names); i++) {
+		flag_list = l_settings_get_string_list(config, "DriverFlags",
+						driver_flag_names[i].name, ',');
+		if (!flag_list)
+			continue;
+
+		for (j = 0; flag_list[j]; j++)
+			if (!fnmatch(flag_list[j], wiphy->driver_str, 0))
+				wiphy->driver_flags |=
+						driver_flag_names[i].flag;
+
+		l_strv_free(flag_list);
+	}
+
 	return true;
 }
 
-- 
2.25.1

[PATCH 3/7] doc: document [DriverFlags] group settings

[James Prestwood]

---
 src/iwd.config.rst | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/src/iwd.config.rst b/src/iwd.config.rst
index b3dc22d4..190f2c69 100644
--- a/src/iwd.config.rst
+++ b/src/iwd.config.rst
@@ -378,6 +378,32 @@ The group ``[IPv4]`` contains settings related to IPv4 network configuration.
        will limit the number of access points that can be running
        simultaneously on different interfaces.
 
+DriverFlags
+-----------
+
+The group ``[DriverFlags]`` contains special flags associated with drivers that
+are buggy or just don't behave similar enough to the majority of other drivers.
+
+.. list-table::
+   :header-rows: 0
+   :stub-columns: 0
+   :widths: 20 80
+   :align: left
+
+   * - DefaultInterface
+     - Values: comma-separated list of drivers or glob matches
+
+       If a driver in use matches one in this list IWD will not attempt to
+       remove and re-create the default interface.
+
+   * - ForcePae
+     - Values: comma-separated list of drivers or glob matches
+
+       If a driver in use matches one in this list ControlPortOverNL80211 will
+       not be used, and PAE will be used instead. Some drivers do not properly
+       support ControlPortOverNL80211 even though they advertise support for it.
+
+
 SEE ALSO
 ========
 
-- 
2.25.1

[PATCH 4/7] wiphy: add [DriverFlags].PowerSaveDisable flag

[James Prestwood]

Certain drivers do not handle power save very well resulting in
missed frames, firmware crashes, or other bad behavior. Its easy
enough to disable power save via iw, iwconfig, etc but since IWD
removes and creates the interface on startup it blows away any
previous power save setting. The setting must be done *after* IWD
creates the interface which can be done, but needs to be via some
external daemon monitoring IWD's state. For minimal systems,
e.g. without NetworkManager, it becomes difficult and annoying to
persistently disable power save.

For this reason a new driver flag POWER_SAVE_DISABLE is being
added. This can then be referenced when creating the interfaces
and if set, disable power save.
---
 src/wiphy.c | 13 +++++++++++++
 src/wiphy.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/src/wiphy.c b/src/wiphy.c
index 6f8f6826..2c09d47a 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -71,6 +71,7 @@ static unsigned int wiphy_dump_id;
 enum driver_flag {
 	DEFAULT_IF = 0x1,
 	FORCE_PAE = 0x2,
+	POWER_SAVE_DISABLE = 0x4,
 };
 
 struct driver_flag_name {
@@ -101,6 +102,7 @@ static const struct driver_info driver_infos[] = {
 static const struct driver_flag_name driver_flag_names[] = {
 	{ "DefaultInterface", DEFAULT_IF },
 	{ "ForcePae",         FORCE_PAE },
+	{ "PowerSaveDisable", POWER_SAVE_DISABLE },
 };
 
 struct wiphy {
@@ -723,6 +725,17 @@ bool wiphy_control_port_enabled(struct wiphy *wiphy)
 	return enabled;
 }
 
+bool wiphy_disable_power_save(struct wiphy *wiphy)
+{
+	if (wiphy->driver_flags & POWER_SAVE_DISABLE) {
+		l_info("Disabling power save due to driver quirks: %s",
+				wiphy_get_driver(wiphy));
+		return true;
+	}
+
+	return false;
+}
+
 const uint8_t *wiphy_get_permanent_address(struct wiphy *wiphy)
 {
 	return wiphy->permanent_addr;
diff --git a/src/wiphy.h b/src/wiphy.h
index f4f205ad..39837366 100644
--- a/src/wiphy.h
+++ b/src/wiphy.h
@@ -135,6 +135,7 @@ const char *wiphy_get_driver(struct wiphy *wiphy);
 const char *wiphy_get_name(struct wiphy *wiphy);
 bool wiphy_uses_default_if(struct wiphy *wiphy);
 bool wiphy_control_port_enabled(struct wiphy *wiphy);
+bool wiphy_disable_power_save(struct wiphy *wiphy);
 const uint8_t *wiphy_get_permanent_address(struct wiphy *wiphy);
 const uint8_t *wiphy_get_extended_capabilities(struct wiphy *wiphy,
 							uint32_t iftype);
-- 
2.25.1

[PATCH 5/7] netdev: disable power save if required

[James Prestwood]

Disable power save if the wiphy indicates its needed. Do this
before issuing GET_LINK so the netdev doesn't signal its up until
power save is disabled.
---
 src/netdev.c | 80 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 66 insertions(+), 14 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index 4ee17f3b..1b44944e 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -6212,6 +6212,65 @@ error:
 	return NULL;
 }
 
+static void netdev_get_link(uint32_t ifindex)
+{
+	struct ifinfomsg *rtmmsg;
+	size_t bufsize;
+
+	/* Query interface flags */
+	bufsize = NLMSG_ALIGN(sizeof(struct ifinfomsg));
+	rtmmsg = l_malloc(bufsize);
+	memset(rtmmsg, 0, bufsize);
+
+	rtmmsg->ifi_family = AF_UNSPEC;
+	rtmmsg->ifi_index = ifindex;
+
+	l_netlink_send(rtnl, RTM_GETLINK, 0, rtmmsg, bufsize,
+					netdev_getlink_cb, NULL, NULL);
+
+	l_free(rtmmsg);
+}
+
+static void netdev_disable_ps_cb(struct l_genl_msg *msg, void *user_data)
+{
+	int err = l_genl_msg_get_error(msg);
+	uint32_t ifindex = L_PTR_TO_UINT(user_data);
+
+	/* 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)",
+				ifindex, strerror(-err), err);
+		return;
+	}
+
+	l_debug("Disabled power save for ifindex %u", ifindex);
+}
+
+static void netdev_disable_ps_destroy(void *user_data)
+{
+	uint32_t ifindex = L_PTR_TO_UINT(user_data);
+
+	netdev_get_link(ifindex);
+}
+
+static bool netdev_disable_power_save(uint32_t ifindex)
+{
+	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, &ifindex);
+	l_genl_msg_append_attr(msg, NL80211_ATTR_PS_STATE, 4, &disabled);
+
+	if (!l_genl_family_send(nl80211, msg, netdev_disable_ps_cb,
+				L_UINT_TO_PTR(ifindex),
+				netdev_disable_ps_destroy)) {
+		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)
 {
@@ -6223,8 +6282,6 @@ struct netdev *netdev_create_from_genl(struct l_genl_msg *msg,
 	uint32_t wiphy_id;
 	struct netdev *netdev;
 	struct wiphy *wiphy = NULL;
-	struct ifinfomsg *rtmmsg;
-	size_t bufsize;
 	struct l_io *pae_io = NULL;
 
 	if (nl80211_parse_attrs(msg, NL80211_ATTR_IFINDEX, &ifindex,
@@ -6283,20 +6340,15 @@ struct netdev *netdev_create_from_genl(struct l_genl_msg *msg,
 	l_debug("Created interface %s[%d %" PRIx64 "]", netdev->name,
 		netdev->index, netdev->wdev_id);
 
-	/* Query interface flags */
-	bufsize = NLMSG_ALIGN(sizeof(struct ifinfomsg));
-	rtmmsg = l_malloc(bufsize);
-	memset(rtmmsg, 0, bufsize);
-
-	rtmmsg->ifi_family = AF_UNSPEC;
-	rtmmsg->ifi_index = ifindex;
-
-	l_netlink_send(rtnl, RTM_GETLINK, 0, rtmmsg, bufsize,
-					netdev_getlink_cb, NULL, NULL);
+	netdev_setup_interface(netdev);
 
-	l_free(rtmmsg);
+	if (wiphy_disable_power_save(wiphy)) {
+		/* Wait to issue GET_LINK until PS is disabled */
+		if (netdev_disable_power_save(ifindex))
+			return netdev;
+	}
 
-	netdev_setup_interface(netdev);
+	netdev_get_link(ifindex);
 
 	return netdev;
 }
-- 
2.25.1

[PATCH 6/7] wiphy: print driver flags on startup

[James Prestwood]

Similar to other basic information, print the driver flags so the
user is informed what is set.
---
 src/wiphy.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/src/wiphy.c b/src/wiphy.c
index 2c09d47a..ebc91206 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -1333,6 +1333,27 @@ static void wiphy_print_basic_info(struct wiphy *wiphy)
 		l_free(joined);
 		l_strfreev(iftypes);
 	}
+
+	if (wiphy->driver_flags) {
+		char **flags = l_strv_new();
+		char *joined;
+
+		if (wiphy->driver_flags & DEFAULT_IF)
+			flags = l_strv_append(flags, "DefaultInterface");
+
+		if (wiphy->driver_flags & FORCE_PAE)
+			flags = l_strv_append(flags, "ForcePae");
+
+		if (wiphy->driver_flags & POWER_SAVE_DISABLE)
+			flags = l_strv_append(flags, "PowerSaveDisable");
+
+		joined = l_strjoinv(flags, ' ');
+
+		l_info("\tDriver Flags: %s", joined);
+
+		l_free(joined);
+		l_strfreev(flags);
+	}
 }
 
 static void parse_supported_commands(struct wiphy *wiphy,
-- 
2.25.1

[PATCH 7/7] doc: Document [DriverFlags].PowerSaveDisable

[James Prestwood]

---
 src/iwd.config.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/iwd.config.rst b/src/iwd.config.rst
index 190f2c69..6b52b040 100644
--- a/src/iwd.config.rst
+++ b/src/iwd.config.rst
@@ -403,6 +403,10 @@ are buggy or just don't behave similar enough to the majority of other drivers.
        not be used, and PAE will be used instead. Some drivers do not properly
        support ControlPortOverNL80211 even though they advertise support for it.
 
+   * - PowerSaveDisable
+     - Values: comma-separated list of drivers or glob matches
+
+       If a driver in user matches one in this list power save will be disabled.
 
 SEE ALSO
 ========
-- 
2.25.1

Re: [PATCH 2/7] wiphy: allow for user-defined driver flags

[Denis Kenzior]

Hi James,

On 6/15/23 14:24, James Prestwood wrote:
> The driver_infos list in wiphy.c is hard coded and, naturally,
> not configurable from a user perspective. As drivers are updated
> or added users may be left with their system being broken until the
> driver is added, IWD released, and packaged.
> 
> This adds the ability to define driver flags inside main.conf under
> the "DriverFlags" group. Keys in this group correspond to values in

So I renamed this to "DriverQuirks" instead to be a little more future-proof.

> enum driver_flag and values are a list of glob matches for specific
> drivers:
> 
> [DriverFlags]
> DefaultInterface=rtl81*,rtl87*,rtl88*,rtw_*,brcmfmac,bcmsdh_sdmmc
> ForcePae=buggy_pae_*
> ---
>   src/wiphy.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
> 

Patch 1-3 applied, thanks.

Regards,
-Denis

Re: [PATCH 4/7] wiphy: add [DriverFlags].PowerSaveDisable flag

[Denis Kenzior]

Hi James,

On 6/15/23 14:24, James Prestwood wrote:
> Certain drivers do not handle power save very well resulting in
> missed frames, firmware crashes, or other bad behavior. Its easy
> enough to disable power save via iw, iwconfig, etc but since IWD
> removes and creates the interface on startup it blows away any
> previous power save setting. The setting must be done *after* IWD
> creates the interface which can be done, but needs to be via some
> external daemon monitoring IWD's state. For minimal systems,
> e.g. without NetworkManager, it becomes difficult and annoying to
> persistently disable power save.
> 
> For this reason a new driver flag POWER_SAVE_DISABLE is being
> added. This can then be referenced when creating the interfaces
> and if set, disable power save.
> ---
>   src/wiphy.c | 13 +++++++++++++
>   src/wiphy.h |  1 +
>   2 files changed, 14 insertions(+)
> 

<snip>

> @@ -723,6 +725,17 @@ bool wiphy_control_port_enabled(struct wiphy *wiphy)
>   	return enabled;
>   }
>   
> +bool wiphy_disable_power_save(struct wiphy *wiphy)

I named this wiphy_power_save_disabled to be consistent with 
wiphy_control_port_enabled()...

I went ahead and applied this, but I think we have to fix the behavior of this 
and wiphy_control_port_enabled().  Namely:

> +{
> +	if (wiphy->driver_flags & POWER_SAVE_DISABLE) {
> +		l_info("Disabling power save due to driver quirks: %s",
> +				wiphy_get_driver(wiphy));

We shouldn't really use l_info here.  This method is meant more as a getter and 
might have other users in the future.  Same case with 
wiphy_control_port_enabled(), but that one is a little bit more complex to fix.

Patches 4, 6 and 7 applied.

Regards,
-Denis

Re: [PATCH 5/7] netdev: disable power save if required

[Denis Kenzior]

Hi James,

On 6/15/23 14:24, James Prestwood wrote:
> Disable power save if the wiphy indicates its needed. Do this
> before issuing GET_LINK so the netdev doesn't signal its up until
> power save is disabled.
> ---
>   src/netdev.c | 80 +++++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 66 insertions(+), 14 deletions(-)
> 

<snip>

> +static void netdev_disable_ps_cb(struct l_genl_msg *msg, void *user_data)
> +{
> +	int err = l_genl_msg_get_error(msg);
> +	uint32_t ifindex = L_PTR_TO_UINT(user_data);
> +
> +	/* 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)",
> +				ifindex, strerror(-err), err);
> +		return;
> +	}
> +
> +	l_debug("Disabled power save for ifindex %u", ifindex);
> +}
> +
> +static void netdev_disable_ps_destroy(void *user_data)
> +{
> +	uint32_t ifindex = L_PTR_TO_UINT(user_data);
> +
> +	netdev_get_link(ifindex);

So why do we do this in the destroy callback?  What happens if this operation is 
canceled (maybe by hot-unplug?)

> +}
> +

<snip>

>   
> -	l_free(rtmmsg);
> +	if (wiphy_disable_power_save(wiphy)) {
> +		/* Wait to issue GET_LINK until PS is disabled */
> +		if (netdev_disable_power_save(ifindex))

Should we be saving a command id here so we can cancel this operation in case of 
hot-unplug?

Ideally we should switch to using l_genl_family_new per netdev so that all the 
outstanding commands are auto-canceled, but this might require some care.

> +			return netdev;
> +	}
>   
> -	netdev_setup_interface(netdev);
> +	netdev_get_link(ifindex);

We should be saving the command id here too, but as a separate fix.

>   
>   	return netdev;
>   }

Regards,
-Denis

Re: [PATCH 4/7] wiphy: add [DriverFlags].PowerSaveDisable flag

[James Prestwood]

Hi Denis,

On 6/18/23 12:07 PM, Denis Kenzior wrote:
> Hi James,
> 
> On 6/15/23 14:24, James Prestwood wrote:
>> Certain drivers do not handle power save very well resulting in
>> missed frames, firmware crashes, or other bad behavior. Its easy
>> enough to disable power save via iw, iwconfig, etc but since IWD
>> removes and creates the interface on startup it blows away any
>> previous power save setting. The setting must be done *after* IWD
>> creates the interface which can be done, but needs to be via some
>> external daemon monitoring IWD's state. For minimal systems,
>> e.g. without NetworkManager, it becomes difficult and annoying to
>> persistently disable power save.
>>
>> For this reason a new driver flag POWER_SAVE_DISABLE is being
>> added. This can then be referenced when creating the interfaces
>> and if set, disable power save.
>> ---
>>   src/wiphy.c | 13 +++++++++++++
>>   src/wiphy.h |  1 +
>>   2 files changed, 14 insertions(+)
>>
> 
> <snip>
> 
>> @@ -723,6 +725,17 @@ bool wiphy_control_port_enabled(struct wiphy *wiphy)
>>       return enabled;
>>   }
>> +bool wiphy_disable_power_save(struct wiphy *wiphy)
> 
> I named this wiphy_power_save_disabled to be consistent with 
> wiphy_control_port_enabled()...
> 
> I went ahead and applied this, but I think we have to fix the behavior 
> of this and wiphy_control_port_enabled().  Namely:
> 
>> +{
>> +    if (wiphy->driver_flags & POWER_SAVE_DISABLE) {
>> +        l_info("Disabling power save due to driver quirks: %s",
>> +                wiphy_get_driver(wiphy));
> 
> We shouldn't really use l_info here.  This method is meant more as a 
> getter and might have other users in the future.  Same case with 
> wiphy_control_port_enabled(), but that one is a little bit more complex 
> to fix.

Ok I can send a follow up patch. The only use for all these APIs is when 
creating the netdev so we *should* only see it then but now that we 
print these flags with wiphy_print_basic_info I'll just remove them 
entirely.

> 
> Patches 4, 6 and 7 applied.
> 
> Regards,
> -Denis

Re: [PATCH 5/7] netdev: disable power save if required

[James Prestwood]

Hi Denis,

On 6/18/23 12:11 PM, Denis Kenzior wrote:
> Hi James,
> 
> On 6/15/23 14:24, James Prestwood wrote:
>> Disable power save if the wiphy indicates its needed. Do this
>> before issuing GET_LINK so the netdev doesn't signal its up until
>> power save is disabled.
>> ---
>>   src/netdev.c | 80 +++++++++++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 66 insertions(+), 14 deletions(-)
>>
> 
> <snip>
> 
>> +static void netdev_disable_ps_cb(struct l_genl_msg *msg, void 
>> *user_data)
>> +{
>> +    int err = l_genl_msg_get_error(msg);
>> +    uint32_t ifindex = L_PTR_TO_UINT(user_data);
>> +
>> +    /* 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)",
>> +                ifindex, strerror(-err), err);
>> +        return;
>> +    }
>> +
>> +    l_debug("Disabled power save for ifindex %u", ifindex);
>> +}
>> +
>> +static void netdev_disable_ps_destroy(void *user_data)
>> +{
>> +    uint32_t ifindex = L_PTR_TO_UINT(user_data);
>> +
>> +    netdev_get_link(ifindex);
> 
> So why do we do this in the destroy callback?  What happens if this 
> operation is canceled (maybe by hot-unplug?)
> 
>> +}
>> +
> 
> <snip>
> 
>> -    l_free(rtmmsg);
>> +    if (wiphy_disable_power_save(wiphy)) {
>> +        /* Wait to issue GET_LINK until PS is disabled */
>> +        if (netdev_disable_power_save(ifindex))
> 
> Should we be saving a command id here so we can cancel this operation in 
> case of hot-unplug?

Yeah I can do this. My theory for not was if it was a hot-unplug the 
cb/destroy were never accessing netdev directly, just the ifindex. But 
an explicit cancel (for getlink too) is better.

> 
> Ideally we should switch to using l_genl_family_new per netdev so that 
> all the outstanding commands are auto-canceled, but this might require 
> some care.

That would actually be pretty nice, and remove that massive block of 
cancels.

> 
>> +            return netdev;
>> +    }
>> -    netdev_setup_interface(netdev);
>> +    netdev_get_link(ifindex);
> 
> We should be saving the command id here too, but as a separate fix.
> 
>>       return netdev;
>>   }
> 
> Regards,
> -Denis