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

Karthikeyan Periyasamy
2024-04-15T15:57:50Z

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.

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