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: <eefaa8d8-f84a-6f1f-1e06-adebdd949d0d@quicinc.com>
Date:   Fri, 21 Oct 2022 19:01:34 +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 4/4] wifi: nl80211: send iface combination to user space in
 multi-hardware wiphy
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-5-quic_vthiagar@quicinc.com>
 <bd8d9fe60b4ff7a420ab597b1db64b5b42616d77.camel@sipsolutions.net>
From:   Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
In-Reply-To: <bd8d9fe60b4ff7a420ab597b1db64b5b42616d77.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 10/21/2022 5:55 PM, Johannes Berg wrote:
> On Tue, 2022-09-20 at 15:35 +0530, Vasanthakumar Thiagarajan wrote:
>> A new nested nl attribute is added to the same existing NL command to
>> advertise the iface combination capability for each underlying hardware
>> when driver groups more than one physical hardware under one wiphy to
>> enable MLO among them.
> 
> That's a good point - are we assuming this implies you can do MLO across
> the groups? Maybe somebody would want to use this advertisement without
> allowing MLO? (Not sure that's useful vs. multiple wiphys though, but
> maybe to simplify driver if there are some devices w/o MLO?)

Good point. Yes, this can be used even without MLO (i.e. not quite tied 
to MLO feature). Ill update the doc accordingly. Thanks.

> 
>> +		for (l = 0; l < c->iface_hw_list->n_limits; l++) {
>> +			struct nlattr *limit;
>> +
>> +			limit = nla_nest_start(msg, l + 1);
>> +			if (!limit)
>> +				return -ENOBUFS;
>> +
>> +			if (nla_put_u16(msg, NL80211_IFACE_LIMIT_MAX,
>> +					c->iface_hw_list[i].limits[l].max))
>> +				return -ENOBUFS;
>> +
>> +			if (nla_put_u16(msg, NL80211_IFACE_LIMIT_TYPES,
>> +					c->iface_hw_list[i].limits[l].types))
>> +				return -ENOBUFS;
>> +
>> +			nla_nest_end(msg, limit);
>> +		}
>> +		nla_nest_end(msg, limits);
> 
> 
> Feels like this part is kind of pre-existing code, or should be, could
> it be refactored?
> 

let me check, thanks.

>> +		if (nla_put_u32(msg,
>> +				NL80211_IFACE_COMB_PER_HW_COMB_NUM_CHANNELS,
>> +				c->iface_hw_list[i].num_different_channels))
>> +			return -ENOBUFS;
>> +
>> +		if (nla_put_u16(msg,
>> +				NL80211_IFACE_COMB_PER_HW_COMB_MAXIMUM,
>> +				c->iface_hw_list[i].max_interfaces))
>> +			return -ENOBUFS;
>> +
>> +		nla_nest_end(msg, hw_combi);
> 
> And even this feels like it must already exist in some way? Wouldn't it
> even be easier to parse for userspace if it wasn't a separate set of
> attributes?
>

Yes, reusing existing attribute will be simpler. let me check that.

Thanks for the review!


Vasanth