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: <bd8d9fe60b4ff7a420ab597b1db64b5b42616d77.camel@sipsolutions.net>
Subject: Re: [RFC 4/4] wifi: nl80211: send iface combination to user space
 in multi-hardware wiphy
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
Cc:     linux-wireless@vger.kernel.org
Date:   Fri, 21 Oct 2022 14:25:38 +0200
In-Reply-To: <20220920100518.19705-5-quic_vthiagar@quicinc.com>
References: <20220920100518.19705-1-quic_vthiagar@quicinc.com>
         <20220920100518.19705-5-quic_vthiagar@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

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?)

> +		for (l =3D 0; l < c->iface_hw_list->n_limits; l++) {
> +			struct nlattr *limit;
> +
> +			limit =3D 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?

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

johannes