On 2/28/2025 6:31 PM, Johannes Berg wrote:
>> +/**
>> + * struct wiphy_radio_cfg - physical radio config of a wiphy
>> + * This structure describes the configurations of a physical radio in a
>> + * wiphy. It is used to denote per-radio attributes belonging to a wiphy.
>>
>
> Seems like there should be a blank line after the short description so
> it doesn't all end up in there?
>
Sure, I'll add one.
>> + * @NL80211_ATTR_WIPHY_RADIO_INDEX: Integer attribute denoting the index of
>> + * the radio in interest. Internally a value of 0xff is used to indicate
>> + * this attribute is not present, and hence any associated attributes are
>> + * deemed to be applicable to all radios
>
> Please document the type here. Also, the description of the internal
> 0xff handling and all that is inappropriate in the public API
> documentation.
>
Sure, I'll add a detailed description of those here.
> However, it seems using -1, would be nicer? Also,
> NL80211_WIPHY_RADIO_ID_MAX is a _really_ bad name for that.>
We are using u8 to store the radio_id to serve two purposes. First is to check if
the number of radios in the device is greater than the value in wiphy->n_radio.
Second is to check if the number of radios is given in the user-space. Both these
checks culminate in using a common - if (radio_id > wiphy->n_radio), to check if
radio_id is valid. If -1 is used, we might need to check if radio_id is -1 and is
greater than wiphy->n_radio separately.
Can we stick to using 0xff if radio_id is not given in userspace?
>> +++ b/net/wireless/core.c
>> @@ -1077,6 +1077,23 @@ int wiphy_register(struct wiphy *wiphy)
>> return res;
>> }
>>
>> + /* Allocate radio configuration space for multi-radio wiphy.
>> + */
>> + if (wiphy->n_radio) {
>> + int idx;
>> +
>> + wiphy->radio_cfg = kcalloc(wiphy->n_radio, sizeof(*wiphy->radio_cfg),
>> + GFP_KERNEL);
>> + if (!wiphy->radio_cfg)
>> + return -ENOMEM;
>> + /*
>> + * Initialize wiphy radio parameters to IEEE 802.11 MIB default values.
>> + * RTS threshold is disabled by default with the special -1 value.
>> + */
>> + for (idx = 0; idx < wiphy->n_radio; idx++)
>> + wiphy->radio_cfg[idx].rts_threshold = (u32)-1;
>> + }
>
> This error handling is obviously all wrong. Please ask someone else to
> review before you resubmit.
>
Sure, I'll make modifications, get it reviewed internally and re-send.
>
> The later code in nl80211.c could also use some refactoring, rather than
> just indent it a bit and call it done.
>
I caught your review comments in your other reply to this patch, thanks.
> johannes
