Re: [RFC 2/4] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote:
> 
> +++ b/include/uapi/linux/nl80211.h
> @@ -2749,6 +2749,12 @@ enum nl80211_commands {
>   *	When used with %NL80211_CMD_FRAME_TX_STATUS, indicates the ack RX
>   *	timestamp. When used with %NL80211_CMD_FRAME RX notification, indicates
>   *	the incoming frame RX timestamp.
> + *
> + * @NL80211_ATTR_MULTI_HW_MACS: nested attribute to send the hardware mac

Not sure I'd call this multiple MACs? It's multiple devices in some
sense, but from a spec POV at least, I'd think our NIC also has multiple
MACs when it doesn't use this infrastructure. Might get a bit confusing?

Maybe just stick to "multi_hw" or so?

> +/**
> + * nl80211_multi_hw_mac_attrs - multi-hw mac attributes
> + *
> + * @NL80211_MULTI_HW_MAC_ATTR_INVALID: invalid
> + * @NL80211_MULTI_HW_MAC_ATTR_IDX: (u8) array index in wiphy @hw_chans to refer an
> + *	underlying hw mac for which the supported channel list is advertised.

I'd prefer this to be primarily written from a userspace POV, so the
whole "@hw_chans" etc isn't really right. Maybe say something like

"(u8) multi-HW index used to refer to an underlying HW ...; internally
the index of the wiphy's @hw_chans array."

or so?

> + * @NL80211_MULTI_HW_MAC_ATTR_FREQS: array of supported center frequencies

FWIW, Jakub has started advertising for using the same attribute
multiple times to have arrays, so you'd just have

 {NL80211_MULTI_HW_ATTR_FREQ: 2412},
 {NL80211_MULTI_HW_ATTR_FREQ: 2417},
 {NL80211_MULTI_HW_ATTR_FREQ: 2422},

etc. in the message. Not sure we want to try that here, but it'd also
simplify splitting messages for dumps.


> +static int nl80211_put_multi_hw_support(struct wiphy *wiphy,
> +					struct sk_buff *msg)
> +{
> +	struct nlattr *hw_macs, *hw_mac;
> +	struct nlattr *freqs;
> +	int i, c;
> +
> +	if (!wiphy->num_hw)
> +		return 0;
> +
> +	hw_macs = nla_nest_start(msg, NL80211_ATTR_MULTI_HW_MACS);
> +	if (!hw_macs)
> +		return -ENOBUFS;
> +
> +	for (i = 0; i < wiphy->num_hw; i++) {
> +		hw_mac = nla_nest_start(msg, i + 1);
> +		if (!hw_mac)
> +			return -ENOBUFS;
> +
> +		if (nla_put_u8(msg, NL80211_MULTI_HW_MAC_ATTR_IDX, i))
> +			return -ENOBUFS;
> +
> +		freqs = nla_nest_start(msg,
> +				       NL80211_MULTI_HW_MAC_ATTR_FREQS);
> +		if (!freqs)
> +			return -ENOBUFS;
> +
> +		for (c = 0; c < wiphy->hw_chans[i]->n_chans; c++)
> +			if (nla_put_u32(msg, c + 1,
> +					wiphy->hw_chans[i]->chans[c].center_freq))
> +				return -ENOBUFS;

Ah you used a nested array even.

So the argument for using a real array would've been that it's smaller,
but I guess with nested that argument goes way.

Would you mind trying Jakub's preferred approach here and see how that
works out?

For the generator basically you'd just have

hw_mac = nla_nest_start();
nla_put_u8(IDX, i)
for (c = 0; c < ...; c++)
	nla_put_u32(MULTI_HW_ATTR_FREQ, ...->chans[c].center_freq);


johannes