Message-ID: <9d0f309da45ae657cd2ce0bc11a93d66e856ef64.camel@sipsolutions.net>
Date: Thu, 28 Mar 2024 13:01:55 +0100
From: Johannes Berg <johannes@...solutions.net>
To: Karthikeyan Periyasamy <quic_periyasa@...cinc.com>, 
	ath12k@...ts.infradead.org
Cc: linux-wireless@...r.kernel.org, Vasanthakumar Thiagarajan
	 <quic_vthiagar@...cinc.com>, netdev@...r.kernel.org, Jakub Kicinski
	 <kuba@...nel.org>
Subject: Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware
 channel capabilities to user space

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.

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

So in that sense, I prefer that, but I'm truly not sure how the (hand-
written) userspace code would deal with that.

johannes

Powered by blists - more mailing lists