Re: [PATCH 6/7] wifi: cfg80211: Add support for interface usage notification

[Kalle Valo <kvalo@kernel.org>]

"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/

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches