| Message ID | 20240328072916.1164195-3-quic_periyasa@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: Add multi physical hardware iface combination support | expand |
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.
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.
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
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
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
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
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 --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;