On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>
> * @new_beacon_int: set this to the beacon interval of a new interface
> * that's not operating yet, if such is to be checked as part of
> * the verification
> + * @chandef: Channel definition for which the interface combination is to be
> + * checked, when checking during interface preparation on a new channel,
> + * for example. This will be used when the driver advertises underlying
> + * hw specific interface combination in a multi physical hardware device.
> + * This will be NULL when the interface combination check is not due to
> + * channel or the interface combination does not include per-hw
> + * advertisement.
This is input, so "will be" doesn't make much sense, more like "must
be"?
But I'm confused as to how that works with num_different_channels being
here too?
This function was, as far as I can tell, always checking the _full_
state. Now you're changing that, and I'm neither sure why, nor does it
seem well documented.
> + * @n_per_hw: number of Per-HW interface combinations.
> + * @per_hw: @n_per_hw of hw specific interface combinations. Per-hw channel
> + * list index as advertised in wiphy @hw_chans is used as index
> + * in @per_hw to maintain the interface combination of the corresponding
> + * hw.
What?
If I'm reading that correctly, which is all but guaranteed, doesn't that
actually mean you don't need n_per_hw at all, since it necessarily equal
to n_hw_chans?
> +/**
> + * cfg80211_per_hw_iface_comb_advertised - if per-hw iface combination supported
> + *
> + * @wiphy: the wiphy
> + *
> + * This function is used to check underlying per-hw interface combination is
> + * advertised by the driver.
> + */
> +bool cfg80211_per_hw_iface_comb_advertised(struct wiphy *wiphy);
Is that even worth an export rather than being inline? Is it even needed
outside of cfg80211 itself?
Also for cfg80211_get_hw_idx_by_chan(), is it really needed?
I'm also wondering if we really should use the "hw_idx" everywhere.
Maybe that'd be more useful as a pointer to struct
ieee80211_chans_per_hw in most places (other than nl80211, obviously)?
The index always feels pretty fragile, a pointer at least gives us type-
checking?
Even in the interface combination advertising, perhaps, though not sure
how that'd work for the drivers.
> +static const struct ieee80211_iface_per_hw *
> +cfg80211_get_hw_iface_comb_by_idx(struct wiphy *wiphy,
> + const struct ieee80211_iface_combination *c,
> + int idx)
> +{
> + int i;
> +
> + for (i = 0; i < c->n_hw_list; i++)
> + if (c->iface_hw_list[i].hw_chans_idx == idx)
> + break;
> +
> + if (i == c->n_hw_list)
> + return NULL;
> +
> + return &c->iface_hw_list[i];
> +}
???
Hint: it's perfectly legal to return directly from a loop.
> +static int
> +cfg80211_validate_iface_comb_per_hw_limits(struct wiphy *wiphy,
> + struct iface_combination_params *params,
> + const struct ieee80211_iface_combination *c,
> + u16 *num_ifaces, u32 *all_iftypes)
> +{
> + struct ieee80211_iface_limit *limits;
> + const struct iface_comb_per_hw_params *per_hw;
> + const struct ieee80211_iface_per_hw *per_hw_comb;
> + int i, ret = 0;
The = 0 doesn't seem needed.
> + ret = cfg80211_validate_iface_limits(wiphy,
> + per_hw->iftype_num,
> + limits,
> + per_hw_comb->n_limits,
> + all_iftypes);
> +
> + kfree(limits);
> +
> + if (ret)
> + goto out;
> + }
> +
> +out:
> + return ret;
That 'out' label is pointless.
> +static void cfg80211_put_iface_comb_iftypes(u16 *num_ifaces)
> +{
> + kfree(num_ifaces);
> +}
Not sure I see value in that indirection?
> +static u16*
missing space
> +cfg80211_get_iface_comb_iftypes(struct wiphy *wiphy,
> + struct iface_combination_params *params,
> + u32 *used_iftypes)
> +{
> + const struct iface_comb_per_hw_params *per_hw;
> + u16 *num_ifaces;
> + int i;
> + u8 num_hw;
> +
> + num_hw = params->n_per_hw ? params->n_per_hw : 1;
I think we're allowed to use the "?:" shortcut.
> + num_ifaces = kcalloc(num_hw, sizeof(*num_ifaces), GFP_KERNEL);
> + if (!num_ifaces)
> + return NULL;
But ... maybe we should just cap num_hw to a reasonable limit (4? 5?)
and use a static array in the caller instead of allocating here.
> + is_per_hw = cfg80211_per_hw_iface_comb_advertised(wiphy);
Maybe call that "have_per_hw_combinations" or so? Or "check_per_hw"
even, "is" seems not clear - "what is?"
> + /* check per HW validation */
> + if (params->n_per_hw) {
> + if (!is_per_hw)
> + return -EINVAL;
> +
> + if (params->n_per_hw > wiphy->num_hw)
> + return -EINVAL;
> + }
> +
> + if (is_per_hw && params->chandef &&
> + cfg80211_chandef_valid(params->chandef))
> + hw_chan_idx = cfg80211_get_hw_idx_by_chan(wiphy,
> + params->chandef->chan);
> +
> + num_ifaces = cfg80211_get_iface_comb_iftypes(wiphy,
> + params,
> + &used_iftypes);
> + if (!num_ifaces)
> + return -ENOMEM;
But still like I said above, all this code seems really odd to me now,
it's checking *either* the per-hw and then only for a single HW, *or*
the global, but ... seems it should do full checks for both, if needed?
johannes
