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

[Karthikeyan Periyasamy]

On 4/12/2024 10:57 AM, Karthikeyan Periyasamy wrote:
> 
> 
> 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
>> };
>>
> 

Corrected typo

  I agree this approach. Instead of NL80211_IFACE_COMB_NUM_CHANNELS,
  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
  };



-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி