On 4/23/2025 9:06 PM, Johannes Berg wrote:
> On Fri, 2025-03-28 at 17:55 +0530, Roopni Devanathan wrote:
>>
>> -static int ath6kl_cfg80211_set_wiphy_params(struct wiphy *wiphy, u32 changed)
>> +static int ath6kl_cfg80211_set_wiphy_params(struct wiphy *wiphy, s8 radio_id, u32 changed)
>
> nit: all those lines could be shorter. I'm surprised you even did that
> by hand rather than spatch ;-)
>
Will address this in next version.
>> #define NL80211_WIPHY_NAME_MAXLEN 64
>> +#define NL80211_WIPHY_RADIO_ID_DEFAULT -1
>
> It seems to me that should't be in nl80211.h, since it cannot be used by
> userspace, and is internal anyway?
>
> And why should the attribute even be signed, when you explicitly reject
> negative values anyway? Seems like it should simply be unsigned?
>
Sure, will change this in next version.
>> + /* Allocate radio configuration space for multi-radio wiphy
>> + */
>
> what happened there
>
>> + if (wiphy->n_radio > 0) {
>> + 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.
>
> unnecessarily long lines
>
Will resolve this in next version.
>> @@ -1185,6 +1202,7 @@ void wiphy_unregister(struct wiphy *wiphy)
>> cfg80211_rdev_free_wowlan(rdev);
>> cfg80211_free_coalesce(rdev->coalesce);
>> rdev->coalesce = NULL;
>> + kfree(wiphy->radio_cfg);
>> }
>
> Hm. Would be safer in wiphy_free()?
>
Sure, will port it to wiphy_free().
>> @@ -3875,50 +3887,79 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
>>
>> if (changed) {
>> u8 old_retry_short, old_retry_long;
>> - u32 old_frag_threshold, old_rts_threshold;
>> - u8 old_coverage_class;
>> + u32 old_frag_threshold, old_rts_threshold, *old_radio_rts_threshold;
>> + u8 old_coverage_class, i;
>
> also long lines
>
>> + old_radio_rts_threshold = kcalloc(rdev->wiphy.n_radio, sizeof(u32),
>> + GFP_KERNEL);
>
> long line
>
> allocations can fail
>
> you leak it
>
Will resolve this in next version.
>
>> +
>> + if (radio_id < rdev->wiphy.n_radio && radio_id >= 0) {
>> + old_rts_threshold =
>> + rdev->wiphy.radio_cfg[radio_id].rts_threshold;
>> +
>> + if (changed & WIPHY_PARAM_RTS_THRESHOLD)
>> + rdev->wiphy.radio_cfg[radio_id].rts_threshold =
>> + rts_threshold;
>> +
>> + result = rdev_set_wiphy_params(rdev, radio_id, changed);
>> + if (result)
>> + rdev->wiphy.radio_cfg[radio_id].rts_threshold =
>> + old_rts_threshold;
>
> this is also getting really deep - probably better to refactor this into
> new functions for the two cases (with and without radio idx)
>
Sure, will split the code.
>> static inline int
>> -rdev_set_wiphy_params(struct cfg80211_registered_device *rdev, u32 changed)
>> +rdev_set_wiphy_params(struct cfg80211_registered_device *rdev, s8 radio_id, u32 changed)
>> {
>
> also unnecessarily long
>
Will resolve this in next version.
> johannes
