Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

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

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Karthikeyan Periyasamy]

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
--
கார்த்திகேயன் பெரியசாமி

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Thu, 2024-03-28 at 20:40 +0530, Karthikeyan Periyasamy wrote:
> 
> Can you point to any attribute constructed in this way from kernelspace 
> for the reference to explore more ?

I don't have anything directly, looking at the code finds e.g.
devlink_dpipe_entry_ctx_append() but honestly that's really quite
trivial, it just adds that new attribute while iterating whatever list
you have.

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Jeff Johnson]

On 3/28/2024 9:09 AM, Johannes Berg wrote:
> On Thu, 2024-03-28 at 20:40 +0530, Karthikeyan Periyasamy wrote:
>>
>> Can you point to any attribute constructed in this way from kernelspace 
>> for the reference to explore more ?
> 
> I don't have anything directly, looking at the code finds e.g.
> devlink_dpipe_entry_ctx_append() but honestly that's really quite
> trivial, it just adds that new attribute while iterating whatever list
> you have.

Note that we are trying to maintain the same structure used by the current
wiphy global advertisement since we actually refactor and reuse the existing code.

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Jeff Johnson]

On 3/28/2024 9:14 AM, Jeff Johnson wrote:
> On 3/28/2024 9:09 AM, Johannes Berg wrote:
>> On Thu, 2024-03-28 at 20:40 +0530, Karthikeyan Periyasamy wrote:
>>>
>>> Can you point to any attribute constructed in this way from kernelspace 
>>> for the reference to explore more ?
>>
>> I don't have anything directly, looking at the code finds e.g.
>> devlink_dpipe_entry_ctx_append() but honestly that's really quite
>> trivial, it just adds that new attribute while iterating whatever list
>> you have.
> 
> Note that we are trying to maintain the same structure used by the current
> wiphy global advertisement since we actually refactor and reuse the existing code.

Need to check myself -- i'm thinking about the interface combo information here

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Thu, 2024-03-28 at 09:14 -0700, Jeff Johnson wrote:
> On 3/28/2024 9:09 AM, Johannes Berg wrote:
> > On Thu, 2024-03-28 at 20:40 +0530, Karthikeyan Periyasamy wrote:
> > > 
> > > Can you point to any attribute constructed in this way from kernelspace 
> > > for the reference to explore more ?
> > 
> > I don't have anything directly, looking at the code finds e.g.
> > devlink_dpipe_entry_ctx_append() but honestly that's really quite
> > trivial, it just adds that new attribute while iterating whatever list
> > you have.
> 
> Note that we are trying to maintain the same structure used by the current
> wiphy global advertisement since we actually refactor and reuse the existing code.

Partially, yes. That's not true for the one I was discussing _here_,
notably nl80211_put_multi_hw_support(), however.

It's partially true for patch 6, though even there
nl80211_put_per_hw_iface_combinations() doesn't need to do it that way,
since that's a whole new attribute (NL80211_IFACE_COMB_PER_HW_COMB) and
can define the content anew, as long as the part *inside*
NL80211_IFACE_COMB_PER_HW_COMB_LIMITS remains the same to be able to
call nl80211_put_iface_limits().

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Thu, 2024-03-28 at 17:17 +0100, Johannes Berg wrote:
> On Thu, 2024-03-28 at 09:14 -0700, Jeff Johnson wrote:
> > On 3/28/2024 9:09 AM, Johannes Berg wrote:
> > > On Thu, 2024-03-28 at 20:40 +0530, Karthikeyan Periyasamy wrote:
> > > > 
> > > > Can you point to any attribute constructed in this way from kernelspace 
> > > > for the reference to explore more ?
> > > 
> > > I don't have anything directly, looking at the code finds e.g.
> > > devlink_dpipe_entry_ctx_append() but honestly that's really quite
> > > trivial, it just adds that new attribute while iterating whatever list
> > > you have.
> > 
> > Note that we are trying to maintain the same structure used by the current
> > wiphy global advertisement since we actually refactor and reuse the existing code.
> 
> Partially, yes. That's not true for the one I was discussing _here_,
> notably nl80211_put_multi_hw_support(), however.
> 
> It's partially true for patch 6, though even there
> nl80211_put_per_hw_iface_combinations() doesn't need to do it that way,
> since that's a whole new attribute (NL80211_IFACE_COMB_PER_HW_COMB) and
> can define the content anew

I should say "content and how it forms an array in the top-level
message" here.

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Jakub Kicinski]

On Thu, 28 Mar 2024 13:01:55 +0100 Johannes Berg wrote:
> 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.

I think the best way today would be two walks:

	for_each_attr() {
		switch (type):
		case THE_A_ARRAY_1:
			cnt1++;
			break;
		case THE_A_ARRAY_2:
			cnt2++;
			break;
	}

	if (cnt1)
		array_1 = calloc();
	cnt1 = 0; /* we'll use it as index in second loop */
	if (cnt2)
		array_2 = calloc();
	cnt2 = 0;

	for_each_attr() {
		/* [ normal parsing, populating array_1[cnt1++] etc. ] */
	}

Compared to "indexed array" the only practical difference I think is
the fact that all attrs are walked. I think you have to count them
either way before parsing.

I was wondering at some point whether we should require that all
multi-attr attributes are grouped together. Or add an explicit "count"
attribute. But couldn't convince myself that such extra rules will
pay off sufficiently with perf and/or ease of use...

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Thu, 2024-03-28 at 11:49 -0700, Jakub Kicinski wrote:
> > So in that sense, I prefer that, but I'm truly not sure how the (hand-
> > written) userspace code would deal with that.
> 
> I think the best way today would be two walks:
> 
> 	for_each_attr() {
> 		switch (type):
> 		case THE_A_ARRAY_1:
> 			cnt1++;
> 			break;
> 		case THE_A_ARRAY_2:
> 			cnt2++;
> 			break;
> 	}
> 
> 	if (cnt1)
> 		array_1 = calloc();
> 	cnt1 = 0; /* we'll use it as index in second loop */
> 	if (cnt2)
> 		array_2 = calloc();
> 	cnt2 = 0;
> 
> 	for_each_attr() {
> 		/* [ normal parsing, populating array_1[cnt1++] etc. ] */
> 	}

Yeah, that makes sense.

I'm not sure we even need the calloc() all the time, depends what we're
doing with it, of course.

> Compared to "indexed array" the only practical difference I think is
> the fact that all attrs are walked. I think you have to count them
> either way before parsing.

Right, generally the pattern would be something like

nla_for_each_nested(...)
	n++;

// alloc etc.

idx = 0;
nla_for_each_nested(...)
	array[idx++] = whatever(attr);

or something like that.

So I guess the only thing that changes really is that this now becomes

nla_for_each(...)
	if (type != DESIRED)
		continue;

vs.

nla_for_each_nested(...)


I suppose we could even define a

nla_for_each_type(..., type)

for that.

> I was wondering at some point whether we should require that all
> multi-attr attributes are grouped together. Or add an explicit "count"
> attribute. But couldn't convince myself that such extra rules will
> pay off sufficiently with perf and/or ease of use...

That doesn't seem likely, after all, you'll definitely want to double-
check all that ... Personally, unless you have something super perf
critical, I definitely prefer _not_ having a count like that in the API
because it encourages unsafe code that doesn't do the necessary bounds
checks and then crashes ...

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Jakub Kicinski]

On Thu, 28 Mar 2024 19:53:33 +0100 Johannes Berg wrote:
> I suppose we could even define a
> 
> nla_for_each_type(..., type)

That's probably a good idea for the kernel! We already have a bunch of
the loops with the if (type != DESIRED) continue, as you mentioned.

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Thu, 2024-03-28 at 11:57 -0700, Jakub Kicinski wrote:
> On Thu, 28 Mar 2024 19:53:33 +0100 Johannes Berg wrote:
> > I suppose we could even define a
> > 
> > nla_for_each_type(..., type)
> 
> That's probably a good idea for the kernel! We already have a bunch of
> the loops with the if (type != DESIRED) continue, as you mentioned.

https://lore.kernel.org/r/20240328203144.b5a6c895fb80.I1869b44767379f204998ff44dd239803f39c23e0@changeid

:P

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Vasanthakumar Thiagarajan]

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

The only purpose of hw index is to link hw_chans to per-hardware 
interface combination capability so that each hardware can be
identified during interface combination checks capability vs
current state. Thinking if we can embed the channel list also
into per-hardware interface combination signaling by giving the
pointer?

Vasanth

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Johannes Berg]

On Fri, 2024-03-29 at 19:51 +0530, Vasanthakumar Thiagarajan wrote:
> 
> 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.
> > 
> 
> The only purpose of hw index is to link hw_chans to per-hardware 
> interface combination capability so that each hardware can be
> identified during interface combination checks capability vs
> current state. Thinking if we can embed the channel list also
> into per-hardware interface combination signaling by giving the
> pointer?

Maybe? It might mean more allocations where the use is concerned since
it can't be "static const" that way.

I found the code that needs it later, just that Karthikeyan was using
the wrong explanation for it ... I'd hoped he'd understand your own code
better ;-)

johannes

Re: [PATCH 02/13] wifi: nl80211: send underlying multi-hardware channel capabilities to user space

[Jeff Johnson]

On 4/10/2024 12:59 AM, Johannes Berg wrote:
> I found the code that needs it later, just that Karthikeyan was using
> the wrong explanation for it ... I'd hoped he'd understand your own code
> better ;-)

Internally I'm stressing the need to provide sufficient information in these
patches so that you (and I!) can understand the entire scope. Please continue
to let us know when we are failing.

(bcc to our internal development list)

/jeff