Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e2a8f2d4-01aa-4a3e-d887-6ee6da83ad8e@quicinc.com>
Date:   Fri, 21 Oct 2022 18:27:57 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.0
Subject: Re: [RFC 2/4] wifi: nl80211: send underlying multi-hardware channel
 capabilities to user space
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>
CC:     <linux-wireless@vger.kernel.org>
References: <20220920100518.19705-1-quic_vthiagar@quicinc.com>
 <20220920100518.19705-3-quic_vthiagar@quicinc.com>
 <9d09ce8fa217cdba7a79f4b35e4a4663e3a66e16.camel@sipsolutions.net>
From:   Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
In-Reply-To: <9d09ce8fa217cdba7a79f4b35e4a4663e3a66e16.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 10/21/2022 5:43 PM, Johannes Berg wrote:
> 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?

Yeah, I was not very comfortable calling it multiple MACs either. Sure, 
let me just stick to multi_hw.

> 
>> +/**
>> + * 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?

Sure, thanks.

> 
>> + * @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.
>

we have to do that for every channel? let me check.

> 
>> +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);
> 

I'll try this, thanks!

Vasanth