diff mbox series

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

Message ID 20240328072916.1164195-3-quic_periyasa@quicinc.com
State New
Headers show
Series wifi: Add multi physical hardware iface combination support | expand

Commit Message

Karthikeyan Periyasamy March 28, 2024, 7:29 a.m. UTC
From: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>

As originally discussed in the RFC [1], when driver supports multiple
physical hardware under one wiphy, wiphy->num_hw != 0, send per-hardware
supported frequency list to user space. List of frequency are reported
inside an index which identifies the hardware as in wiphy->hw_chans[].
This hardware index will be used in follow up patches to identify the
interface combination capability for each of the underlying physical
hardware abstracted under one wiphy.

[1]: https://lore.kernel.org/linux-wireless/20220920100518.19705-3-quic_vthiagar@quicinc.com/

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1

Signed-off-by: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
Co-developed-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
---
 include/uapi/linux/nl80211.h | 28 +++++++++++++++++++++
 net/wireless/nl80211.c       | 47 ++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

Comments

Karthikeyan Periyasamy March 28, 2024, 10:18 a.m. UTC | #1
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.

The hw index used as a sanity check to identify whether the user 
requested channel fall under the different hw or not.

In split-phy hardware, 5GHz band supported by two physical hardware's. 
First supports 5GHz Low band and second supports 5GHz High band.

In this case, user space cannot use band vise check here to identify 
given channel or freq supported in the given hardware.


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


>
>> @@ -3001,6 +3042,12 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
>>   				rdev->wiphy.hw_timestamp_max_peers))
>>   			goto nla_put_failure;
>>   
>> +		state->split_start++;
>> +		break;
>> +	case 17:
>> +		if (nl80211_put_multi_hw_support(&rdev->wiphy, msg))
>> +			goto nla_put_failure;
>>
> This could be (or get) pretty big, are you sure it's not needed to push
> the splitting down into it?

ok, can do the advertise for split.
Jeff Johnson March 28, 2024, 4:14 p.m. UTC | #2
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.
Johannes Berg March 28, 2024, 4:17 p.m. UTC | #3
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
Johannes Berg March 28, 2024, 6:53 p.m. UTC | #4
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
Vasanthakumar Thiagarajan March 29, 2024, 2:21 p.m. UTC | #5
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
Johannes Berg April 10, 2024, 7:59 a.m. UTC | #6
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
Jeff Johnson April 10, 2024, 4:52 p.m. UTC | #7
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
diff mbox series

Patch

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index f917bc6c9b6f..c53c9f941663 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2856,6 +2856,11 @@  enum nl80211_commands {
  *	%NL80211_CMD_ASSOCIATE indicating the SPP A-MSDUs
  *	are used on this connection
  *
+ * @NL80211_ATTR_MULTI_HW: nested attribute to send the hardware specific
+ *	channel capabilities to user space. Drivers registering multiple
+ *	physical hardware under a wiphy can use this attribute,
+ *	see &enum nl80211_multi_hw_mac_attrs.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -3401,6 +3406,8 @@  enum nl80211_attrs {
 
 	NL80211_ATTR_ASSOC_SPP_AMSDU,
 
+	NL80211_ATTR_MULTI_HW,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -7999,4 +8006,25 @@  enum nl80211_ap_settings_flags {
 	NL80211_AP_SETTINGS_SA_QUERY_OFFLOAD_SUPPORT	= 1 << 1,
 };
 
+/**
+ * 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.
+ * @NL80211_MULTI_HW_ATTR_FREQS: array of supported center frequencies
+ * @__NL80211_MULTI_HW_ATTR_LAST: internal use
+ * @NL80211_MULTI_HW_ATTR_MAX: maximum multi-hw mac attribute
+ */
+enum nl80211_multi_hw_attrs {
+	__NL80211_MULTI_HW_ATTR_INVALID,
+
+	NL80211_MULTI_HW_ATTR_IDX,
+	NL80211_MULTI_HW_ATTR_FREQS,
+
+	/* keep last */
+	__NL80211_MULTI_HW_ATTR_LAST,
+	NL80211_MULTI_HW_ATTR_MAX = __NL80211_MULTI_HW_ATTR_LAST - 1
+};
 #endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index b4edba6b0b7b..2a5e395e2e0b 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2392,6 +2392,47 @@  static int nl80211_put_mbssid_support(struct wiphy *wiphy, struct sk_buff *msg)
 	return -ENOBUFS;
 }
 
+static int nl80211_put_multi_hw_support(struct wiphy *wiphy,
+					struct sk_buff *msg)
+{
+	struct nlattr *hw_macs, *hw_mac;
+	struct nlattr *freqs;
+	int i, c;
+
+	if (!wiphy->num_hw)
+		return 0;
+
+	hw_macs = nla_nest_start(msg, NL80211_ATTR_MULTI_HW);
+	if (!hw_macs)
+		return -ENOBUFS;
+
+	for (i = 0; i < wiphy->num_hw; i++) {
+		hw_mac = nla_nest_start(msg, i + 1);
+		if (!hw_mac)
+			return -ENOBUFS;
+
+		if (nla_put_u8(msg, NL80211_MULTI_HW_ATTR_IDX, i))
+			return -ENOBUFS;
+
+		freqs = nla_nest_start(msg,
+				       NL80211_MULTI_HW_ATTR_FREQS);
+		if (!freqs)
+			return -ENOBUFS;
+
+		for (c = 0; c < wiphy->hw_chans[i]->n_chans; c++)
+			if (nla_put_u32(msg, c + 1,
+					wiphy->hw_chans[i]->chans[c].center_freq))
+				return -ENOBUFS;
+
+		nla_nest_end(msg, freqs);
+
+		nla_nest_end(msg, hw_mac);
+	}
+
+	nla_nest_end(msg, hw_macs);
+	return 0;
+}
+
 struct nl80211_dump_wiphy_state {
 	s64 filter_wiphy;
 	long start;
@@ -3001,6 +3042,12 @@  static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 				rdev->wiphy.hw_timestamp_max_peers))
 			goto nla_put_failure;
 
+		state->split_start++;
+		break;
+	case 17:
+		if (nl80211_put_multi_hw_support(&rdev->wiphy, msg))
+			goto nla_put_failure;
+
 		/* done */
 		state->split_start = 0;
 		break;