| Message ID | 20230615192415.1718516-5-prestwoj@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] wiphy: store driver flags directly in wiphy object | expand |
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
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
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; }