Received-SPF: pass (google.com: domain of linux-wireless+bounces-5467-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <14b739c9-18da-0d58-b58d-cccebc505950@quicinc.com>
Date: Thu, 28 Mar 2024 20:40:59 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware
 channel capabilities to user space
To: Johannes Berg <johannes@sipsolutions.net>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>,
        Vasanthakumar Thiagarajan
	<quic_vthiagar@quicinc.com>,
        <netdev@vger.kernel.org>, Jakub Kicinski
	<kuba@kernel.org>
References: <20240328072916.1164195-1-quic_periyasa@quicinc.com>
 <20240328072916.1164195-3-quic_periyasa@quicinc.com>
 <6d92d0ba4a8764cd91cc20c4bd35613bcc41dfcd.camel@sipsolutions.net>
 <9d5c2f9f-19b5-af4d-8018-1eb97fac10d6@quicinc.com>
 <9d0f309da45ae657cd2ce0bc11a93d66e856ef64.camel@sipsolutions.net>
Content-Language: en-US
From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
In-Reply-To: <9d0f309da45ae657cd2ce0bc11a93d66e856ef64.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


On 3/28/2024 5:31 PM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 15:48 +0530, Karthikeyan Periyasamy wrote:
>> On 3/28/2024 1:19 PM, Johannes Berg wrote:
>>> On Thu, 2024-03-28 at 12:59 +0530, Karthikeyan Periyasamy wrote:
>>>> +/**
>>>> + * nl80211_multi_hw_attrs - multi-hw attributes
>>>> + *
>>>> + * @NL80211_MULTI_HW_ATTR_INVALID: invalid
>>>> + * @NL80211_MULTI_HW_ATTR_IDX: (u8) multi-HW index to refer the underlying HW
>>>> + *	for which the supported channel list is advertised. Internally refer
>>>> + *	the index of the wiphy's @hw_chans array.
>>> Is there a good reason to expose this? Seems pretty internal to me, and
>>> not sure what userspace would do with it?
>> Hostapd use this hw index for the channel switch cmd.
> What, where? I don't see that in this patchset? And why??
>
> In any case, unless I just missed it and you're going to tell me to look
> at patch N, we don't need it here, and then I'd prefer to keep it an
> internal detail until it's needed.
>
>> The hw index used as a sanity check to identify whether the user
>> requested channel fall under the different hw or not.
> You mean within hostapd itself? That doesn't make sense, it can derive
> that information differently.
>
>> In split-phy hardware, 5GHz band supported by two physical hardware's.
>> First supports 5GHz Low band and second supports 5GHz High band.
> In your hardware design anyway, but yeah, I get it.
>
>> In this case, user space cannot use band vise check here to identify
>> given channel or freq supported in the given hardware.
> No, but it also doesn't need an index assigned by the kernel for that.

Yes


>>>> +	for (i = 0; i < wiphy->num_hw; i++) {
>>>> +		hw_mac = nla_nest_start(msg, i + 1);
>>> And you kind of even have it here already ...
>> Then user and kernel have to make an assumption that implicit index used
>> in the life cycle.
> Agree that's maybe not a great idea, for all we care this could just use
> 0 as the index anyway.
>
> Which reminds me ...
>
> Right now, the way you have it, we have the following structure:
>
> NL80211_ATTR_MULTI_HW
> - 1
>    - NL80211_MULTI_HW_ATTR_IDX: 0
>    - NL80211_MULTI_HW_ATTR_FREQS
>      - 0: 2412
>      - 1: 2417
>      ...
> - 2
>    - NL80211_MULTI_HW_ATTR_IDX: 1
>    - NL80211_MULTI_HW_ATTR_FREQS
>      - ... as above with 5 GHz etc.
> ...
>
>
> Netlink is kind of moving away from nested arrays though:
>
> https://kernel.org/doc/html/latest/userspace-api/netlink/specs.html#multi-attr-arrays
> https://kernel.org/doc/html/latest/userspace-api/netlink/genetlink-legacy.html#attribute-type-nests
>
> This talks about families, but maybe we shouldn't read that literally
> and do the new style for new arrays in existing families as well, not
> just new families.
>
> If we do that, including NL80211_MULTI_HW_ATTR_IDX for illustrative
> purposes though I think it should be removed, we'd end up with:
>
> NL80211_ATTR_MULTI_HW
>   - NL80211_MULTI_HW_ATTR_IDX: 0
>   - NL80211_MULTI_HW_ATTR_FREQ: 2412
>   - NL80211_MULTI_HW_ATTR_FREQ: 2417
>   ...
> NL80211_ATTR_MULTI_HW
>   - NL80211_MULTI_HW_ATTR_IDX: 1
>   - NL80211_MULTI_HW_ATTR_FREQ: 5180
>   - NL80211_MULTI_HW_ATTR_FREQ: 5200
>   ...
>
> which _is_ a lot more compact, and removes all the uninteresting mid-
> level indexing.

Can you point to any attribute constructed in this way from kernelspace 
for the reference to explore more ?

-- 

Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி