diff mbox series

[06/13] wifi: nl80211: send iface combination to user space in multi-hardware wiphy

Message ID 20240328072916.1164195-7-quic_periyasa@quicinc.com (mailing list archive)
State RFC
Delegated to: Johannes Berg
Headers show
Series wifi: Add multi physical hardware iface combination support | expand

Commit Message

Karthikeyan Periyasamy March 28, 2024, 7:29 a.m. UTC
From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

As originally discussed in the RFC [1], add a new nested attribute to the
existing NL80211_CMD_NEW_WIPHY command. This attribute should advertise the
iface combination capability for each underlying physical hardware device.
This is necessary when the driver groups more than one physical hardware
under single wiphy.

[1]: https://lore.kernel.org/linux-wireless/20220920100518.19705-5-quic_vthiagar@quicinc.com/

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1

Signed-off-by: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
Co-developed-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
---
 include/uapi/linux/nl80211.h | 50 ++++++++++++++++++++++++++++++-
 net/wireless/nl80211.c       | 58 ++++++++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+), 1 deletion(-)

Comments

Johannes Berg March 28, 2024, 1:33 p.m. UTC | #1
On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
> 
> + *	When describing per-hardware combinations in multi-hardware in
> + *	one wiphy model, the first possibility can further include the finer
> + *	capabilities like below

Not sure I'd say "below" rather than e.g. "like this:"

> + *	hw_chan_idx = 0, numbers = [ #{STA} <= 1, #{AP} <= 1 ],
> + *	channels = 1, max = 2
> + *	=> allows a STA plus an AP interface on the underlying hardware mac
> + *	   advertised at index 0 in wiphy @hw_chans array.
> + *	hw_chan_idx = 1, numbers = [ #{STA} <= 1, #{AP} <= 2 ],
> + *	channels = 1, max = 3
> + *	=> allows a STA plus two AP interfaces on the underlying hardware mac
> + *	   advertised at index 1 in wiphy @hw_chans array.

Have you checked the rst output for this? Seems likely that's not going
to be great with that formatting, but I haven't checked.

> + * @NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX: u8 attribute specifying the index
> + *	to the wiphy @hw_chans list for which the iface combination is being
> + *	described.

"@hw_chans" doesn't make sense here, this is nl80211, it should refer to
some attribute

but why didn't you just _say_ in the patch 2 discussion that it's used
here ...

> + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute containing the
> + *	limits for the given interface types, see
> + *	&enum nl80211_iface_limit_attrs.
> + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the maximum
> + *	number of interfaces that can be created in this group. This number
> + *	does not apply to the interfaces purely managed in software.
> + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute specifying the
> + *	number of different channels that can be used in this group.
> + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
> + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
> + */
> +enum nl80211_if_comb_per_hw_comb_attrs {
> +	NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
> +	NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
> +	NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
> +	NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
> +	NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,

Almost all these attributes duplicate - including their docs -
attributes from enum nl80211_if_combination_attrs. Is it really worth
doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
the different uses of the attribute set?

> +	hw_combis = nla_nest_start(msg, NL80211_IFACE_COMB_PER_HW_COMB);
> +	if (!hw_combis)
> +		return -ENOBUFS;
> +
> +	for (i = 0; i < c->n_hw_list; i++) {
> +		struct nlattr *hw_combi, *limits;
> +
> +		hw_combi = nla_nest_start(msg, i + 1);

And of course the array and splitting discussions apply here too.

johannes
Jeff Johnson March 28, 2024, 4:19 p.m. UTC | #2
On 3/28/2024 6:33 AM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
[...]
>> +	hw_combis = nla_nest_start(msg, NL80211_IFACE_COMB_PER_HW_COMB);
>> +	if (!hw_combis)
>> +		return -ENOBUFS;
>> +
>> +	for (i = 0; i < c->n_hw_list; i++) {
>> +		struct nlattr *hw_combi, *limits;
>> +
>> +		hw_combi = nla_nest_start(msg, i + 1);
> 
> And of course the array and splitting discussions apply here too.

This is where I meant to say that I believe we are trying to reuse the same
structure and code as the wiphy-global combinations
Vasanthakumar Thiagarajan March 29, 2024, 2:34 p.m. UTC | #3
On 3/28/2024 7:03 PM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>>
>> + *	When describing per-hardware combinations in multi-hardware in
>> + *	one wiphy model, the first possibility can further include the finer
>> + *	capabilities like below
> 
> Not sure I'd say "below" rather than e.g. "like this:"
> 
>> + *	hw_chan_idx = 0, numbers = [ #{STA} <= 1, #{AP} <= 1 ],
>> + *	channels = 1, max = 2
>> + *	=> allows a STA plus an AP interface on the underlying hardware mac
>> + *	   advertised at index 0 in wiphy @hw_chans array.
>> + *	hw_chan_idx = 1, numbers = [ #{STA} <= 1, #{AP} <= 2 ],
>> + *	channels = 1, max = 3
>> + *	=> allows a STA plus two AP interfaces on the underlying hardware mac
>> + *	   advertised at index 1 in wiphy @hw_chans array.
> 
> Have you checked the rst output for this? Seems likely that's not going
> to be great with that formatting, but I haven't checked.
> 
>> + * @NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX: u8 attribute specifying the index
>> + *	to the wiphy @hw_chans list for which the iface combination is being
>> + *	described.
> 
> "@hw_chans" doesn't make sense here, this is nl80211, it should refer to
> some attribute
> 
> but why didn't you just _say_ in the patch 2 discussion that it's used
> here ...
> 
>> + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute containing the
>> + *	limits for the given interface types, see
>> + *	&enum nl80211_iface_limit_attrs.
>> + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the maximum
>> + *	number of interfaces that can be created in this group. This number
>> + *	does not apply to the interfaces purely managed in software.
>> + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute specifying the
>> + *	number of different channels that can be used in this group.
>> + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
>> + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
>> + */
>> +enum nl80211_if_comb_per_hw_comb_attrs {
>> +	NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>> +	NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
>> +	NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
>> +	NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
>> +	NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
> 
> Almost all these attributes duplicate - including their docs -
> attributes from enum nl80211_if_combination_attrs. Is it really worth
> doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
> the different uses of the attribute set?
> 

I agree, more duplication. So we have to have the per_hw_comb_attrs
defines like below?

enum nl80211_if_comb_per_hw_comb_attrs {
	NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
	NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX = 			
			NL80211_IFACE_COMB_NUM_CHANNELS + 1,
	/* keep last */
	NUM_NL80211_IFACE_COMB_PER_HW_COMB,
	MAX_NL80211_IFACE_COMB_PER_HW_COMB =
			NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
};

Vasanth
Karthikeyan Periyasamy April 10, 2024, 4:10 a.m. UTC | #4
On 3/29/2024 8:04 PM, Vasanthakumar Thiagarajan wrote:
> 
> 
> On 3/28/2024 7:03 PM, Johannes Berg wrote:
>> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>>>
>>> + *    When describing per-hardware combinations in multi-hardware in
>>> + *    one wiphy model, the first possibility can further include the 
>>> finer
>>> + *    capabilities like below
>>
>> Not sure I'd say "below" rather than e.g. "like this:"
>>
>>> + *    hw_chan_idx = 0, numbers = [ #{STA} <= 1, #{AP} <= 1 ],
>>> + *    channels = 1, max = 2
>>> + *    => allows a STA plus an AP interface on the underlying 
>>> hardware mac
>>> + *       advertised at index 0 in wiphy @hw_chans array.
>>> + *    hw_chan_idx = 1, numbers = [ #{STA} <= 1, #{AP} <= 2 ],
>>> + *    channels = 1, max = 3
>>> + *    => allows a STA plus two AP interfaces on the underlying 
>>> hardware mac
>>> + *       advertised at index 1 in wiphy @hw_chans array.
>>
>> Have you checked the rst output for this? Seems likely that's not going
>> to be great with that formatting, but I haven't checked.
>>
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX: u8 attribute specifying 
>>> the index
>>> + *    to the wiphy @hw_chans list for which the iface combination is 
>>> being
>>> + *    described.
>>
>> "@hw_chans" doesn't make sense here, this is nl80211, it should refer to
>> some attribute
>>
>> but why didn't you just _say_ in the patch 2 discussion that it's used
>> here ...
>>
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute 
>>> containing the
>>> + *    limits for the given interface types, see
>>> + *    &enum nl80211_iface_limit_attrs.
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the 
>>> maximum
>>> + *    number of interfaces that can be created in this group. This 
>>> number
>>> + *    does not apply to the interfaces purely managed in software.
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute 
>>> specifying the
>>> + *    number of different channels that can be used in this group.
>>> + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
>>> + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
>>> + */
>>> +enum nl80211_if_comb_per_hw_comb_attrs {
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
>>
>> Almost all these attributes duplicate - including their docs -
>> attributes from enum nl80211_if_combination_attrs. Is it really worth
>> doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
>> the different uses of the attribute set?
>>
> 
> I agree, more duplication. So we have to have the per_hw_comb_attrs
> defines like below?
> 
> enum nl80211_if_comb_per_hw_comb_attrs {
>      NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>      NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
>              NL80211_IFACE_COMB_NUM_CHANNELS + 1,
>      /* keep last */
>      NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>      MAX_NL80211_IFACE_COMB_PER_HW_COMB =
>              NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
> };
> 

@johannes Berg


Any comments on the vasanth suggested approach ?
Johannes Berg April 10, 2024, 6:59 a.m. UTC | #5
On Wed, 2024-04-10 at 09:40 +0530, Karthikeyan Periyasamy wrote:
> > I agree, more duplication. So we have to have the per_hw_comb_attrs
> > defines like below?
> > 
> > enum nl80211_if_comb_per_hw_comb_attrs {
> >      NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
> >      NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
> >              NL80211_IFACE_COMB_NUM_CHANNELS + 1,
> >      /* keep last */
> >      NUM_NL80211_IFACE_COMB_PER_HW_COMB,
> >      MAX_NL80211_IFACE_COMB_PER_HW_COMB =
> >              NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
> > };
> > 
> 
> @johannes Berg
> 
> 
> Any comments on the vasanth suggested approach ?

Think about it, do you have any comments? I mean, you've _already_ sent
an email, why not actually say something about the topic?

johannes
Karthikeyan Periyasamy April 12, 2024, 5:27 a.m. UTC | #6
On 3/29/2024 8:04 PM, Vasanthakumar Thiagarajan wrote:
> 
> 
> On 3/28/2024 7:03 PM, Johannes Berg wrote:
>> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>>>
>>> + *    When describing per-hardware combinations in multi-hardware in
>>> + *    one wiphy model, the first possibility can further include the 
>>> finer
>>> + *    capabilities like below
>>
>> Not sure I'd say "below" rather than e.g. "like this:"
>>
>>> + *    hw_chan_idx = 0, numbers = [ #{STA} <= 1, #{AP} <= 1 ],
>>> + *    channels = 1, max = 2
>>> + *    => allows a STA plus an AP interface on the underlying 
>>> hardware mac
>>> + *       advertised at index 0 in wiphy @hw_chans array.
>>> + *    hw_chan_idx = 1, numbers = [ #{STA} <= 1, #{AP} <= 2 ],
>>> + *    channels = 1, max = 3
>>> + *    => allows a STA plus two AP interfaces on the underlying 
>>> hardware mac
>>> + *       advertised at index 1 in wiphy @hw_chans array.
>>
>> Have you checked the rst output for this? Seems likely that's not going
>> to be great with that formatting, but I haven't checked.
>>
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX: u8 attribute specifying 
>>> the index
>>> + *    to the wiphy @hw_chans list for which the iface combination is 
>>> being
>>> + *    described.
>>
>> "@hw_chans" doesn't make sense here, this is nl80211, it should refer to
>> some attribute
>>
>> but why didn't you just _say_ in the patch 2 discussion that it's used
>> here ...
>>
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute 
>>> containing the
>>> + *    limits for the given interface types, see
>>> + *    &enum nl80211_iface_limit_attrs.
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the 
>>> maximum
>>> + *    number of interfaces that can be created in this group. This 
>>> number
>>> + *    does not apply to the interfaces purely managed in software.
>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute 
>>> specifying the
>>> + *    number of different channels that can be used in this group.
>>> + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
>>> + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
>>> + */
>>> +enum nl80211_if_comb_per_hw_comb_attrs {
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
>>> +    NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
>>
>> Almost all these attributes duplicate - including their docs -
>> attributes from enum nl80211_if_combination_attrs. Is it really worth
>> doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
>> the different uses of the attribute set?
>>
> 
> I agree, more duplication. So we have to have the per_hw_comb_attrs
> defines like below?
> 
> enum nl80211_if_comb_per_hw_comb_attrs {
>      NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>      NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
>              NL80211_IFACE_COMB_NUM_CHANNELS + 1,
>      /* keep last */
>      NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>      MAX_NL80211_IFACE_COMB_PER_HW_COMB =
>              NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
> };
> 

I agree this approach. Instead of NUM_NL80211_IFACE_COMB_PER_HW_COMB, 
shall we have MAX_NL80211_IFACE_COMB like below so that hw_idx attribute 
value will not get conflict to any IFACE combination attributes

  enum nl80211_if_comb_per_hw_comb_attrs {
       NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
       NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
               MAX_NL80211_IFACE_COMB + 1,
       /* keep last */
       NUM_NL80211_IFACE_COMB_PER_HW_COMB,
       MAX_NL80211_IFACE_COMB_PER_HW_COMB =
               NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
};
Johannes Berg April 15, 2024, 2:27 p.m. UTC | #7
On Fri, 2024-04-12 at 10:57 +0530, Karthikeyan Periyasamy wrote:
> > > > + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute 
> > > > containing the
> > > > + *    limits for the given interface types, see
> > > > + *    &enum nl80211_iface_limit_attrs.
> > > > + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the 
> > > > maximum
> > > > + *    number of interfaces that can be created in this group. This 
> > > > number
> > > > + *    does not apply to the interfaces purely managed in software.
> > > > + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute 
> > > > specifying the
> > > > + *    number of different channels that can be used in this group.
> > > > + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
> > > > + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
> > > > + */
> > > > +enum nl80211_if_comb_per_hw_comb_attrs {
> > > > +    NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
> > > > +    NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
> > > > +    NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
> > > > +    NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
> > > > +    NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
> > > 
> > > Almost all these attributes duplicate - including their docs -
> > > attributes from enum nl80211_if_combination_attrs. Is it really worth
> > > doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
> > > the different uses of the attribute set?
> > > 
> > 
> > I agree, more duplication. So we have to have the per_hw_comb_attrs
> > defines like below?
> > 
> > enum nl80211_if_comb_per_hw_comb_attrs {
> >      NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
> >      NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
> >              NL80211_IFACE_COMB_NUM_CHANNELS + 1,
> >      /* keep last */
> >      NUM_NL80211_IFACE_COMB_PER_HW_COMB,
> >      MAX_NL80211_IFACE_COMB_PER_HW_COMB =
> >              NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
> > };
> > 
> 
> I agree this approach. Instead of NUM_NL80211_IFACE_COMB_PER_HW_COMB, 
> shall we have MAX_NL80211_IFACE_COMB like below so that hw_idx attribute 
> value will not get conflict to any IFACE combination attributes
> 
>   enum nl80211_if_comb_per_hw_comb_attrs {
>        NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>        NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
>                MAX_NL80211_IFACE_COMB + 1,
>        /* keep last */
>        NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>        MAX_NL80211_IFACE_COMB_PER_HW_COMB =
>                NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
> };
> 

You haven't thought this through - what happens here if something is
added to enum nl80211_if_combination_attrs?

johannes
Karthikeyan Periyasamy April 15, 2024, 3:57 p.m. UTC | #8
On 4/15/2024 7:57 PM, Johannes Berg wrote:
> On Fri, 2024-04-12 at 10:57 +0530, Karthikeyan Periyasamy wrote:
>>>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute
>>>>> containing the
>>>>> + *    limits for the given interface types, see
>>>>> + *    &enum nl80211_iface_limit_attrs.
>>>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the
>>>>> maximum
>>>>> + *    number of interfaces that can be created in this group. This
>>>>> number
>>>>> + *    does not apply to the interfaces purely managed in software.
>>>>> + * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute
>>>>> specifying the
>>>>> + *    number of different channels that can be used in this group.
>>>>> + * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
>>>>> + * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
>>>>> + */
>>>>> +enum nl80211_if_comb_per_hw_comb_attrs {
>>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
>>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
>>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
>>>>> +    NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
>>>>
>>>> Almost all these attributes duplicate - including their docs -
>>>> attributes from enum nl80211_if_combination_attrs. Is it really worth
>>>> doing that, rather than adding NL80211_IFACE_COMB_HW_IDX and documenting
>>>> the different uses of the attribute set?
>>>>
>>>
>>> I agree, more duplication. So we have to have the per_hw_comb_attrs
>>> defines like below?
>>>
>>> enum nl80211_if_comb_per_hw_comb_attrs {
>>>       NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>>>       NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
>>>               NL80211_IFACE_COMB_NUM_CHANNELS + 1,
>>>       /* keep last */
>>>       NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>>>       MAX_NL80211_IFACE_COMB_PER_HW_COMB =
>>>               NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
>>> };
>>>
>>
>> I agree this approach. Instead of NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>> shall we have MAX_NL80211_IFACE_COMB like below so that hw_idx attribute
>> value will not get conflict to any IFACE combination attributes
>>
>>    enum nl80211_if_comb_per_hw_comb_attrs {
>>         NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
>>         NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX =
>>                 MAX_NL80211_IFACE_COMB + 1,
>>         /* keep last */
>>         NUM_NL80211_IFACE_COMB_PER_HW_COMB,
>>         MAX_NL80211_IFACE_COMB_PER_HW_COMB =
>>                 NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
>> };
>>
> 
> You haven't thought this through - what happens here if something is
> added to enum nl80211_if_combination_attrs?
> 

Yeah, it break backward compatibility.
diff mbox series

Patch

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index c53c9f941663..aa0988be1f2a 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -6004,6 +6004,10 @@  enum nl80211_iface_limit_attrs {
  * @NL80211_IFACE_COMB_BI_MIN_GCD: u32 attribute specifying the minimum GCD of
  *	different beacon intervals supported by all the interface combinations
  *	in this group (if not present, all beacon intervals be identical).
+ * @NL80211_IFACE_COMB_PER_HW_COMB: nested attribute specifying the interface
+ *	combination for each underlying hardware when multiple hardware are
+ *	registered under one wiphy,
+ *	see &enum nl80211_if_comb_per_hw_comb_attrs.
  * @NUM_NL80211_IFACE_COMB: number of attributes
  * @MAX_NL80211_IFACE_COMB: highest attribute number
  *
@@ -6020,7 +6024,19 @@  enum nl80211_iface_limit_attrs {
  *	numbers = [ #{STA} <= 1, #{P2P-client,P2P-GO} <= 3 ], max = 4
  *	=> allows a STA plus three P2P interfaces
  *
- * The list of these four possibilities could completely be contained
+ *	When describing per-hardware combinations in multi-hardware in
+ *	one wiphy model, the first possibility can further include the finer
+ *	capabilities like below
+ *	hw_chan_idx = 0, numbers = [ #{STA} <= 1, #{AP} <= 1 ],
+ *	channels = 1, max = 2
+ *	=> allows a STA plus an AP interface on the underlying hardware mac
+ *	   advertised at index 0 in wiphy @hw_chans array.
+ *	hw_chan_idx = 1, numbers = [ #{STA} <= 1, #{AP} <= 2 ],
+ *	channels = 1, max = 3
+ *	=> allows a STA plus two AP interfaces on the underlying hardware mac
+ *	   advertised at index 1 in wiphy @hw_chans array.
+ *
+ * The list of these five possibilities could completely be contained
  * within the %NL80211_ATTR_INTERFACE_COMBINATIONS attribute to indicate
  * that any of these groups must match.
  *
@@ -6039,12 +6055,44 @@  enum nl80211_if_combination_attrs {
 	NL80211_IFACE_COMB_RADAR_DETECT_WIDTHS,
 	NL80211_IFACE_COMB_RADAR_DETECT_REGIONS,
 	NL80211_IFACE_COMB_BI_MIN_GCD,
+	NL80211_IFACE_COMB_PER_HW_COMB,
 
 	/* keep last */
 	NUM_NL80211_IFACE_COMB,
 	MAX_NL80211_IFACE_COMB = NUM_NL80211_IFACE_COMB - 1
 };
 
+/**
+ * enum nl80211_if_comb_per_hw_comb_attrs - per-hardware iface combination
+ * attributes with multi-hw radios in one wiphy model
+ *
+ * @NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC: (reserved)
+ * @NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX: u8 attribute specifying the index
+ *	to the wiphy @hw_chans list for which the iface combination is being
+ *	described.
+ * @NL80211_IFACE_COMB_PER_HW_COMB_LIMITS: nested attribute containing the
+ *	limits for the given interface types, see
+ *	&enum nl80211_iface_limit_attrs.
+ * @NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM: u32 attribute giving the maximum
+ *	number of interfaces that can be created in this group. This number
+ *	does not apply to the interfaces purely managed in software.
+ * @NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS: u32 attribute specifying the
+ *	number of different channels that can be used in this group.
+ * @NUM_NL80211_IFACE_COMB_PER_HW_COMB: number of attributes
+ * @MAX_NL80211_IFACE_COMB_PER_HW_COMB: highest attribute number
+ */
+enum nl80211_if_comb_per_hw_comb_attrs {
+	NL80211_IFACE_COMB_PER_HW_COMB_UNSPEC,
+	NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
+	NL80211_IFACE_COMB_PER_HW_COMB_LIMITS,
+	NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
+	NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
+
+	/* keep last */
+	NUM_NL80211_IFACE_COMB_PER_HW_COMB,
+	MAX_NL80211_IFACE_COMB_PER_HW_COMB =
+			NUM_NL80211_IFACE_COMB_PER_HW_COMB - 1
+};
 
 /**
  * enum nl80211_plink_state - state of a mesh peer link finite state machine
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 37524a61f417..87436924834f 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1653,6 +1653,61 @@  nl80211_put_iface_limits(struct sk_buff *msg,
 	return -ENOBUFS;
 }
 
+static int
+nl80211_put_per_hw_iface_combinations(struct wiphy *wiphy, struct sk_buff *msg,
+				      const struct ieee80211_iface_combination *c)
+{
+	struct nlattr *hw_combis;
+	int i;
+
+	if (!wiphy->num_hw)
+		return 0;
+
+	hw_combis = nla_nest_start(msg, NL80211_IFACE_COMB_PER_HW_COMB);
+	if (!hw_combis)
+		return -ENOBUFS;
+
+	for (i = 0; i < c->n_hw_list; i++) {
+		struct nlattr *hw_combi, *limits;
+
+		hw_combi = nla_nest_start(msg, i + 1);
+		if (!hw_combi)
+			return -ENOBUFS;
+
+		if (nla_put_u8(msg, NL80211_IFACE_COMB_PER_HW_COMB_HW_IDX,
+			       c->iface_hw_list[i].hw_chans_idx))
+			return -ENOBUFS;
+
+		limits = nla_nest_start(msg,
+					NL80211_IFACE_COMB_PER_HW_COMB_LIMITS);
+		if (!limits)
+			return -ENOBUFS;
+
+		if (nl80211_put_iface_limits(msg,
+					     c->iface_hw_list[i].limits,
+					     c->iface_hw_list[i].n_limits))
+			return -ENOBUFS;
+
+		nla_nest_end(msg, limits);
+
+		if (nla_put_u32(msg,
+				NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
+				c->iface_hw_list[i].num_different_channels))
+			return -ENOBUFS;
+
+		if (nla_put_u16(msg,
+				NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
+				c->iface_hw_list[i].max_interfaces))
+			return -ENOBUFS;
+
+		nla_nest_end(msg, hw_combi);
+	}
+
+	nla_nest_end(msg, hw_combis);
+
+	return 0;
+}
+
 static int nl80211_put_iface_combinations(struct wiphy *wiphy,
 					  struct sk_buff *msg,
 					  bool large)
@@ -1704,6 +1759,9 @@  static int nl80211_put_iface_combinations(struct wiphy *wiphy,
 				c->beacon_int_min_gcd))
 			goto nla_put_failure;
 
+		if (large && nl80211_put_per_hw_iface_combinations(wiphy, msg, c))
+			goto nla_put_failure;
+
 		nla_nest_end(msg, nl_combi);
 	}