| Message ID | 20240605135233.23d15e758640.I7a62740a6868416acaed01e41157b3c0a7a41b4d@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | cfg80211/mac80211 patches from our internal tree 2024-06-05 | expand |
Miri Korenblit <miriam.rachel.korenblit@intel.com> writes: > From: Ilan Peer <ilan.peer@intel.com> > > In some cases, when an interface is added by user space, user space > might not know yet what is the intended type of the interface, e.g., > before a P2P Group Ownership Negotiation (GON) an interface is added > but only at the end of the GON flow the final interface type is > determined. This doesn't allow the kernel drivers to prepare for the > actual interface type, e.g., make resources available for the > interface type etc. > > Generally, adding an interface doesn't necessarily imply that it will > actually be used soon, and as described the interface might not be used > with the type it's added as. > > This new API allows user space to indicate that it does indeed intend to > use the interface soon, along with the types (of which the interface > must be one) that may be selected for that usage. This will allow the > underlying driver to adjust resources accordingly. > > Signed-off-by: Ilan Peer <ilan.peer@intel.com> > Reviewed-by: Johannes Berg <johannes.berg@intel.com> > Tested-by: iil_jenkins iil_jenkins <EC.GER.UNIX.IIL.JENKINS@INTEL.COM> This new command just looks weird to me, do we really need it? I would expect to see a workaround like this in out-of-tree drivers but not in upstream.
Hi, > -----Original Message----- > From: Kalle Valo <kvalo@kernel.org> > Sent: Thursday, 6 June 2024 12:28 > To: Korenblit, Miriam Rachel <miriam.rachel.korenblit@intel.com> > Cc: johannes@sipsolutions.net; linux-wireless@vger.kernel.org; Peer, Ilan > <ilan.peer@intel.com>; Berg, Johannes <johannes.berg@intel.com>; > iil_jenkins iil_jenkins <EC.GER.UNIX.IIL.JENKINS@INTEL.COM> > Subject: Re: [PATCH 6/7] wifi: cfg80211: Add support for interface usage > notification > > Miri Korenblit <miriam.rachel.korenblit@intel.com> writes: > > > From: Ilan Peer <ilan.peer@intel.com> > > > > In some cases, when an interface is added by user space, user space > > might not know yet what is the intended type of the interface, e.g., > > before a P2P Group Ownership Negotiation (GON) an interface is added > > but only at the end of the GON flow the final interface type is > > determined. This doesn't allow the kernel drivers to prepare for the > > actual interface type, e.g., make resources available for the > > interface type etc. > > > > Generally, adding an interface doesn't necessarily imply that it will > > actually be used soon, and as described the interface might not be > > used with the type it's added as. > > > > This new API allows user space to indicate that it does indeed intend > > to use the interface soon, along with the types (of which the > > interface must be one) that may be selected for that usage. This will > > allow the underlying driver to adjust resources accordingly. > > > > Signed-off-by: Ilan Peer <ilan.peer@intel.com> > > Reviewed-by: Johannes Berg <johannes.berg@intel.com> > > Tested-by: iil_jenkins iil_jenkins <EC.GER.UNIX.IIL.JENKINS@INTEL.COM> > > This new command just looks weird to me, do we really need it? I would > expect to see a workaround like this in out-of-tree drivers but not in upstream. > As depicted above, the need to inform the driver about the intended usage of the interface is real. We encountered several P2P cases in which an interface was added and P2P Group Ownership Negotiation and P2P Invitation signalling were completed successfully, but the P2P Group Session establishment failed since the interface type changed from P2P Client to P2P GO and the local device was no longer able to accommodate the P2P GO operation due to resource constraints. With this new API, user space can now inform the driver about the intended usage of the interface so the driver will make the resources available for all possible interface types. With this the information exchanged during the P2P signalling would correctly reflect state and the P2P group session would be able to be established. I think that this could be useful in other use cases where the interface type is changed, e.g., when starting AP operation on the interface, but before the AP is setup, the interface type is set to station mode to allow scanning operation etc. Regards, Ilan.
"Peer, Ilan" <ilan.peer@intel.com> writes: > Hi, > >> -----Original Message----- >> From: Kalle Valo <kvalo@kernel.org> >> Sent: Thursday, 6 June 2024 12:28 >> To: Korenblit, Miriam Rachel <miriam.rachel.korenblit@intel.com> >> Cc: johannes@sipsolutions.net; linux-wireless@vger.kernel.org; Peer, Ilan >> <ilan.peer@intel.com>; Berg, Johannes <johannes.berg@intel.com>; >> iil_jenkins iil_jenkins <EC.GER.UNIX.IIL.JENKINS@INTEL.COM> >> Subject: Re: [PATCH 6/7] wifi: cfg80211: Add support for interface usage >> notification >> >> Miri Korenblit <miriam.rachel.korenblit@intel.com> writes: >> >> > From: Ilan Peer <ilan.peer@intel.com> >> > >> > In some cases, when an interface is added by user space, user space >> > might not know yet what is the intended type of the interface, e.g., >> > before a P2P Group Ownership Negotiation (GON) an interface is added >> > but only at the end of the GON flow the final interface type is >> > determined. This doesn't allow the kernel drivers to prepare for the >> > actual interface type, e.g., make resources available for the >> > interface type etc. >> > >> > Generally, adding an interface doesn't necessarily imply that it will >> > actually be used soon, and as described the interface might not be >> > used with the type it's added as. >> > >> > This new API allows user space to indicate that it does indeed intend >> > to use the interface soon, along with the types (of which the >> > interface must be one) that may be selected for that usage. This will >> > allow the underlying driver to adjust resources accordingly. >> > >> > Signed-off-by: Ilan Peer <ilan.peer@intel.com> >> > Reviewed-by: Johannes Berg <johannes.berg@intel.com> >> > Tested-by: iil_jenkins iil_jenkins <EC.GER.UNIX.IIL.JENKINS@INTEL.COM> >> >> This new command just looks weird to me, do we really need it? I would >> expect to see a workaround like this in out-of-tree drivers but not in upstream. >> > > As depicted above, the need to inform the driver about the intended > usage of the interface is real. Sure, I can understand the need is real. This just feels like an ugly workaround, not a proper solution. And the documentation for this is quite vague, I'm worried how do we get similarly working drivers? Let's say if I were to implement a user space application for this, or a driver implementation for that matter, it would be a guessing game for me. For example, what's "soon" in this context? 5 mins, 50 secs or 5 secs? Can the mac80211 operation sleep? So user space is now always supposed to always call this nl80211 command and at what stage exactly? Or is it optional? But if it's optional what's the point of adding it? > We encountered several P2P cases in which an interface was added and > P2P Group Ownership Negotiation and P2P Invitation signalling were > completed successfully, but the P2P Group Session establishment failed > since the interface type changed from P2P Client to P2P GO and the > local device was no longer able to accommodate the P2P GO operation > due to resource constraints. > > With this new API, user space can now inform the driver about the > intended usage of the interface so the driver will > make the resources available for all possible interface types. With > this the information exchanged during the P2P signalling > would correctly reflect state and the P2P group session would be able > to be established. Why not allocate the resources during driver initialisation? Or when changing the interface? Why need this weird interface? For easier reading below are all the patches, including the iwlwifi patch. Honestly, this just looks like something like a workaround for a problem in your firmware or something like that. https://patchwork.kernel.org/project/linux-wireless/patch/20240605135233.23d15e758640.I7a62740a6868416acaed01e41157b3c0a7a41b4d@changeid/ https://patchwork.kernel.org/project/linux-wireless/patch/20240605135233.4d602acf0e65.I01fecab3b41961038f37ca6e0e3039c5fe9bb6bf@changeid/ https://patchwork.kernel.org/project/linux-wireless/patch/20240605140556.21582e74a0e0.I7c423d03b4412d77509bd31bd41e4573f76c0e84@changeid/
Hi, > > > > As depicted above, the need to inform the driver about the intended > > usage of the interface is real. > > Sure, I can understand the need is real. This just feels like an ugly workaround, > not a proper solution. > If you have a different solution in mind, please share. > And the documentation for this is quite vague, I'm worried how do we get > similarly working drivers? Let's say if I were to implement a user space > application for this, or a driver implementation for that matter, it would be a > guessing game for me. For example, what's "soon" in this context? 5 mins, 50 > secs or 5 secs? Can the mac80211 operation sleep? > I understand this is not clear. The intention was to say that by the time the interface is enabled, the interface type might change, and that the driver should be aware of that. I can try to better express this in the command and documentation. > So user space is now always supposed to always call this nl80211 command > and at what stage exactly? Or is it optional? But if it's optional what's the point > of adding it? > It is optional. User space should use it when it expects the interface type to change before the interface is activated. > > We encountered several P2P cases in which an interface was added and > > P2P Group Ownership Negotiation and P2P Invitation signalling were > > completed successfully, but the P2P Group Session establishment failed > > since the interface type changed from P2P Client to P2P GO and the > > local device was no longer able to accommodate the P2P GO operation > > due to resource constraints. > > > > With this new API, user space can now inform the driver about the > > intended usage of the interface so the driver will make the resources > > available for all possible interface types. With this the information > > exchanged during the P2P signalling would correctly reflect state and > > the P2P group session would be able to be established. > > Why not allocate the resources during driver initialisation? Or when changing > the interface? Why need this weird interface? > Allocating resources to all possible interface combinations etc. is waste as not all allocations would eventually be used. Regards, Ilan. p.s.: sorry for the late response (was OOO).
"Peer, Ilan" <ilan.peer@intel.com> writes: > Hi, > >> > >> > As depicted above, the need to inform the driver about the intended >> > usage of the interface is real. >> >> Sure, I can understand the need is real. This just feels like an ugly workaround, >> not a proper solution. >> > > If you have a different solution in mind, please share. Yeah, fix the root cause :) >> And the documentation for this is quite vague, I'm worried how do we get >> similarly working drivers? Let's say if I were to implement a user space >> application for this, or a driver implementation for that matter, it would be a >> guessing game for me. For example, what's "soon" in this context? 5 mins, 50 >> secs or 5 secs? Can the mac80211 operation sleep? >> > > I understand this is not clear. The intention was to say that by the > time the interface is enabled, > the interface type might change, and that the driver should be aware > of that. I can try to better express > this in the command and documentation. > >> So user space is now always supposed to always call this nl80211 command >> and at what stage exactly? Or is it optional? But if it's optional what's the point >> of adding it? >> > > It is optional. User space should use it when it expects the interface type to > change before the interface is activated. If this is optional for user space (wpasupplicant, iwd etc.) then the driver cannot rely on it being called, no? So this command cannot be used for anything important because it's optional. Also I'm worried how this will give a different user experience based on if the user space calls this optional command or not. The way I see that this is designed just to workaround one iwlwifi bug, not really as a generic nl80211 command which could be useful for all drivers. But I'm more than happy to be proven wrong! >> > We encountered several P2P cases in which an interface was added and >> > P2P Group Ownership Negotiation and P2P Invitation signalling were >> > completed successfully, but the P2P Group Session establishment failed >> > since the interface type changed from P2P Client to P2P GO and the >> > local device was no longer able to accommodate the P2P GO operation >> > due to resource constraints. >> > >> > With this new API, user space can now inform the driver about the >> > intended usage of the interface so the driver will make the resources >> > available for all possible interface types. With this the information >> > exchanged during the P2P signalling would correctly reflect state and >> > the P2P group session would be able to be established. >> >> Why not allocate the resources during driver initialisation? Or when changing >> the interface? Why need this weird interface? >> > > Allocating resources to all possible interface combinations etc. is waste as > not all allocations would eventually be used. Sure, in ath11k/ath12k we have problems with resource allocation as well, for example how to allocate firmware memory (number of clients vs size data buffers etc), and we really should find a solution for that. Quite a few people are pushing for INI files to be able to configure wireless driver resource allocation but I cannot see how that would be accepted to upstream. It would be great find some type of dynamic configuration solution for wireless drivers, for example devlink or similar.
HI, > -----Original Message----- > From: Kalle Valo <kvalo@kernel.org> > Sent: Monday, 24 June 2024 15:50 > To: Peer, Ilan <ilan.peer@intel.com> > Cc: Korenblit, Miriam Rachel <miriam.rachel.korenblit@intel.com>; > johannes@sipsolutions.net; linux-wireless@vger.kernel.org; Berg, Johannes > <johannes.berg@intel.com> > Subject: Re: [PATCH 6/7] wifi: cfg80211: Add support for interface usage > notification > > "Peer, Ilan" <ilan.peer@intel.com> writes: > > > Hi, > > > >> > > >> > As depicted above, the need to inform the driver about the intended > >> > usage of the interface is real. > >> > >> Sure, I can understand the need is real. This just feels like an ugly > >> workaround, not a proper solution. > >> > > > > If you have a different solution in mind, please share. > > Yeah, fix the root cause :) > > >> And the documentation for this is quite vague, I'm worried how do we > >> get similarly working drivers? Let's say if I were to implement a > >> user space application for this, or a driver implementation for that > >> matter, it would be a guessing game for me. For example, what's > >> "soon" in this context? 5 mins, 50 secs or 5 secs? Can the mac80211 operation > sleep? > >> > > > > I understand this is not clear. The intention was to say that by the > > time the interface is enabled, the interface type might change, and > > that the driver should be aware of that. I can try to better express > > this in the command and documentation. > > > >> So user space is now always supposed to always call this nl80211 > >> command and at what stage exactly? Or is it optional? But if it's > >> optional what's the point of adding it? > >> > > > > It is optional. User space should use it when it expects the interface > > type to change before the interface is activated. > > If this is optional for user space (wpasupplicant, iwd etc.) then the driver cannot > rely on it being called, no? So this command cannot be used for anything > important because it's optional. Also I'm worried how this will give a different > user experience based on if the user space calls this optional command or not. > > The way I see that this is designed just to workaround one iwlwifi bug, not really > as a generic nl80211 command which could be useful for all drivers. But I'm more > than happy to be proven wrong! > It was more an attempt to tell the driver what to expect, so it could prepare itself for the upcoming flows. Since as you mentioned the API is clear in a way that can be used by the drivers, we can drop this patch. I'll try to work on an solution that doesn't involve user space. Thanks for your inputs and feedback. Regards, Ilan.
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 5da9bb0ac6a4..f2defbfd758e 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -4176,6 +4176,15 @@ struct mgmt_frame_regs { u32 global_mcast_stypes, interface_mcast_stypes; }; +/** + * struct cfg80211_iface_usage - Notify about intended interface usage + * + * @types_mask: mask of interface types that are going to be used. + */ +struct cfg80211_iface_usage { + u32 types_mask; +}; + /** * struct cfg80211_ops - backend description for wireless configuration * @@ -4577,6 +4586,7 @@ struct mgmt_frame_regs { * * @set_hw_timestamp: Enable/disable HW timestamping of TM/FTM frames. * @set_ttlm: set the TID to link mapping. + * @iface_usage: notify about intended usage of added interfaces. */ struct cfg80211_ops { int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow); @@ -4938,6 +4948,8 @@ struct cfg80211_ops { struct cfg80211_set_hw_timestamp *hwts); int (*set_ttlm)(struct wiphy *wiphy, struct net_device *dev, struct cfg80211_ttlm_params *params); + void (*iface_usage)(struct wiphy *wiphy, struct net_device *dev, + struct cfg80211_iface_usage *usage); }; /* diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index f917bc6c9b6f..92dbbb0589f0 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -1329,6 +1329,15 @@ * %NL80211_ATTR_MLO_TTLM_ULINK attributes are used to specify the * TID to Link mapping for downlink/uplink traffic. * + * @NL80211_CMD_IFACE_USAGE_NOTIF: Notify kernel about the expected interface + * usage. Allows user space to indicate to the kernel that it intends to + * use the interface soon and what is the expected usage of the interface + * so resources could be prepared etc. This is useful in case an added + * interface is not going to be used immediately but soon, and its type + * might change. The %NL80211_ATTR_IFACE_USAGE_IFTYPES attribute is used to + * provide the mask of intended interface types (the current type must be + * included in the mask). + * * @NL80211_CMD_MAX: highest used command number * @__NL80211_CMD_AFTER_LAST: internal use */ @@ -1586,6 +1595,8 @@ enum nl80211_commands { NL80211_CMD_SET_TID_TO_LINK_MAPPING, + NL80211_CMD_IFACE_USAGE_NOTIF, + /* add new commands above here */ /* used to define NL80211_CMD_MAX below */ @@ -2856,6 +2867,9 @@ enum nl80211_commands { * %NL80211_CMD_ASSOCIATE indicating the SPP A-MSDUs * are used on this connection * + * @NL80211_ATTR_IFACE_USAGE_IFTYPES: a bitmask of interface types that might be + * used with the interface. + * * @NUM_NL80211_ATTR: total number of nl80211_attrs available * @NL80211_ATTR_MAX: highest attribute number currently defined * @__NL80211_ATTR_AFTER_LAST: internal use @@ -3401,6 +3415,8 @@ enum nl80211_attrs { NL80211_ATTR_ASSOC_SPP_AMSDU, + NL80211_ATTR_IFACE_USAGE_IFTYPES, + /* add attributes here, update the policy in nl80211.c */ __NL80211_ATTR_AFTER_LAST, diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 296acd2a2a1b..04110b74547d 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -825,6 +825,11 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = { [NL80211_ATTR_MLO_TTLM_DLINK] = NLA_POLICY_EXACT_LEN(sizeof(u16) * 8), [NL80211_ATTR_MLO_TTLM_ULINK] = NLA_POLICY_EXACT_LEN(sizeof(u16) * 8), [NL80211_ATTR_ASSOC_SPP_AMSDU] = { .type = NLA_FLAG }, + + /* Set the limits to not allow NL80211_IFTYPE_UNSPECIFIED */ + [NL80211_ATTR_IFACE_USAGE_IFTYPES] = + NLA_POLICY_RANGE(NLA_U32, BIT(NL80211_IFTYPE_UNSPECIFIED + 1), + BIT(NUM_NL80211_IFTYPES) - 2), }; /* policy for the key attributes */ @@ -16319,6 +16324,31 @@ nl80211_set_ttlm(struct sk_buff *skb, struct genl_info *info) return rdev_set_ttlm(rdev, dev, ¶ms); } +static int +nl80211_iface_usage(struct sk_buff *skb, struct genl_info *info) +{ + struct cfg80211_iface_usage iface_usage = {}; + struct cfg80211_registered_device *rdev = info->user_ptr[0]; + struct net_device *dev = info->user_ptr[1]; + struct wireless_dev *wdev = dev->ieee80211_ptr; + + /* once the interface is up and running its type can no longer change */ + if (wdev_running(wdev)) + return -EINVAL; + + if (!info->attrs[NL80211_ATTR_IFACE_USAGE_IFTYPES]) + return -EINVAL; + + iface_usage.types_mask = + nla_get_u32(info->attrs[NL80211_ATTR_IFACE_USAGE_IFTYPES]); + + if (!(BIT(wdev->iftype) & iface_usage.types_mask)) + return -EINVAL; + + rdev_iface_usage(rdev, dev, &iface_usage); + return 0; +} + #define NL80211_FLAG_NEED_WIPHY 0x01 #define NL80211_FLAG_NEED_NETDEV 0x02 #define NL80211_FLAG_NEED_RTNL 0x04 @@ -17511,6 +17541,12 @@ static const struct genl_small_ops nl80211_small_ops[] = { .flags = GENL_UNS_ADMIN_PERM, .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP), }, + { + .cmd = NL80211_CMD_IFACE_USAGE_NOTIF, + .doit = nl80211_iface_usage, + .flags = GENL_UNS_ADMIN_PERM, + .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV), + }, }; static struct genl_family nl80211_fam __ro_after_init = { diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h index 43897a5269b6..0a13212d9c8d 100644 --- a/net/wireless/rdev-ops.h +++ b/net/wireless/rdev-ops.h @@ -1542,4 +1542,17 @@ rdev_set_ttlm(struct cfg80211_registered_device *rdev, return ret; } + +static inline void +rdev_iface_usage(struct cfg80211_registered_device *rdev, + struct net_device *dev, + struct cfg80211_iface_usage *iface_usage) +{ + struct wiphy *wiphy = &rdev->wiphy; + + trace_rdev_iface_usage(wiphy, dev, iface_usage); + if (rdev->ops->iface_usage) + rdev->ops->iface_usage(wiphy, dev, iface_usage); + trace_rdev_return_void(wiphy); +} #endif /* __CFG80211_RDEV_OPS */ diff --git a/net/wireless/trace.h b/net/wireless/trace.h index 14cfa0aba93a..2f5df52918cf 100644 --- a/net/wireless/trace.h +++ b/net/wireless/trace.h @@ -3032,6 +3032,24 @@ TRACE_EVENT(rdev_set_ttlm, WIPHY_PR_ARG, NETDEV_PR_ARG) ); +TRACE_EVENT(rdev_iface_usage, + TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, + struct cfg80211_iface_usage *iface_usage), + TP_ARGS(wiphy, netdev, iface_usage), + TP_STRUCT__entry( + WIPHY_ENTRY + NETDEV_ENTRY + __field(u32, types_mask) + ), + TP_fast_assign( + WIPHY_ASSIGN; + NETDEV_ASSIGN; + __entry->types_mask = iface_usage->types_mask; + ), + TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", types_mask=0x%x", + WIPHY_PR_ARG, NETDEV_PR_ARG, __entry->types_mask) +); + /************************************************************* * cfg80211 exported functions traces * *************************************************************/