| Message ID | ab294ba62856f00c06dde10b1bf57f5f341a7198.1447351137.git.poh@qca.qualcomm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, 2015-11-12 at 09:59 -0800, Peter Oh wrote: > Add NL80211_EXT_FEATURE_VHT_MESH flag to allow drivers > to indicate support for VHT in Mesh mode. > This flag will be used by wireless drivers to indicate > the capability of VHT Mesh support and wpa_supplicant to inspect > wireless drivers' capability of VHT Mesh. > In addition to this patch, separate patches to ath10k driver and > wpa_supplicant are also sent out. > Sorry, still doesn't make sense. What does userspace even need to know this *for*? johannes
On 11/12/2015 12:03 PM, Johannes Berg wrote: > On Thu, 2015-11-12 at 09:59 -0800, Peter Oh wrote: >> Add NL80211_EXT_FEATURE_VHT_MESH flag to allow drivers >> to indicate support for VHT in Mesh mode. >> This flag will be used by wireless drivers to indicate >> the capability of VHT Mesh support and wpa_supplicant to inspect >> wireless drivers' capability of VHT Mesh. >> In addition to this patch, separate patches to ath10k driver and >> wpa_supplicant are also sent out. >> > Sorry, still doesn't make sense. What does userspace even need to know > this *for*? At this moment Mesh Point is not able to be brought up by userspace such as wap_supplicant at VHT data rates because there is no events/NL80211 defined that userspace can check out if drivers can support Mesh Point (802.11s) at VHT data rates or not, hence only HT data rates can be used for Mesh Point by userspace now. This patch is introducing NL80211 event to cover the issue. > johannes > > _______________________________________________ > ath10k mailing list > ath10k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath10k Thanks, Peter
On Thu, 2015-11-12 at 13:33 -0800, Peter Oh wrote: > > At this moment Mesh Point is not able to be brought up by userspace > such > as wap_supplicant at VHT data rates because there is no > events/NL80211 > defined that userspace can check out if drivers can support Mesh > Point > (802.11s) at VHT data rates or not, hence only HT data rates can be > used > for > Mesh Point by userspace now. > This patch is introducing NL80211 event to cover the issue. No... this patch doesn't introduce an nl80211 event. It introduces a feature flag, but I'm arguing it's useless. Look - what's the difference that userspace would do based on this flag? Clearly, setting the flag in the driver has no effect on kernel code whatsoever, so your "is not able to be brought up" makes no sense. Perhaps it'll become clearer once you post the wpa_supplicant code, but you should still coherently explain in the commit message why this is necessary. So far, I don't see it, since no kernel code ever depends on this flag. johannes
On 11/12/2015 01:40 PM, Johannes Berg wrote: > On Thu, 2015-11-12 at 13:33 -0800, Peter Oh wrote: >> At this moment Mesh Point is not able to be brought up by userspace >> such >> as wap_supplicant at VHT data rates because there is no >> events/NL80211 >> defined that userspace can check out if drivers can support Mesh >> Point >> (802.11s) at VHT data rates or not, hence only HT data rates can be >> used >> for >> Mesh Point by userspace now. >> This patch is introducing NL80211 event to cover the issue. > No... this patch doesn't introduce an nl80211 event. It introduces a > feature flag, but I'm arguing it's useless. > > Look - what's the difference that userspace would do based on this > flag? Clearly, setting the flag in the driver has no effect on kernel > code whatsoever, so your "is not able to be brought up" makes no sense. Yes directly but no indirectly. Setting the flag in the driver will have effect to the driver eventually by userspace since the flag will be sent up to userspace and userspace will select data rates according to the flag. setting NL80211_VHT_MESH flag in driver -> wpa_supplicant requests driver capabilities via NL80211 -> driver sends up its capabilities including VHT_MESH -> wpa_supplicant parses and check if VHT_MESH is set -> wpa_supplicant configures the driver with VHT data rates if the flag set or configure HT data rates. > > Perhaps it'll become clearer once you post the wpa_supplicant code, This is ath10k driver diff I sent out for upstream review. @@ -7266,6 +7266,7 @@ int ath10k_mac_register(struct ath10k *ar) } wiphy_ext_feature_set(ar->hw->wiphy, NL80211_EXT_FEATURE_VHT_IBSS); + wiphy_ext_feature_set(ar->hw->wiphy, NL80211_EXT_FEATURE_VHT_MESH); and a part of wpa_supplicant changes. @@ -359,6 +359,9 @@ static void wiphy_info_ext_feature_flags(struct wiphy_info_d if (ext_feature_isset(nla_data(tb), nla_len(tb), NL80211_EXT_FEATURE_VHT_IBSS)) capa->flags |= WPA_DRIVER_FLAGS_VHT_IBSS; + if (ext_feature_isset(nla_data(tb), nla_len(tb), + NL80211_EXT_FEATURE_VHT_MESH)) + capa->flags |= WPA_DRIVER_FLAGS_VHT_MESH; @@ -1885,12 +1885,13 @@ void ibss_mesh_setup_freq(struct wpa_supplicant *wpa_s, - /* Not sure if mesh is ready for VHT */ + /* For IBSS/Mesh check VHT flag */ + if (ssid->mode == WPAS_MODE_MESH && + !(wpa_s->drv_flags & WPA_DRIVER_FLAGS_VHT_MESH)) return; > but > you should still coherently explain in the commit message why this is > necessary. The both of wpa_supplicant and ath10k patches have explanation in their commit message already. > So far, I don't see it, since no kernel code ever depends on > this flag. A ath10k driver patch in upstream review is using the flag. Exactly the same communication mechanism and purpose are used with NL80211_EXT_FEATURE_VHT_IBSS which is already a part of NL80211 feature flag. The new feature flag, NL80211_EXT_FEATURE_VHT_MESH, follows the same purpose and usage. > > johannes > > _______________________________________________ > ath10k mailing list > ath10k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath10k
On Thu, 2015-11-12 at 14:28 -0800, Peter Oh wrote: > > Exactly the same communication mechanism and purpose are used with > NL80211_EXT_FEATURE_VHT_IBSS which is already a part of NL80211 > feature > flag. > The new feature flag, NL80211_EXT_FEATURE_VHT_MESH, follows the same > purpose and usage. No, it doesn't. Check how the _IBSS one is used in the code to actually *do* something. johannes
On 11/12/2015 02:32 PM, Johannes Berg wrote: > On Thu, 2015-11-12 at 14:28 -0800, Peter Oh wrote: >> >> Exactly the same communication mechanism and purpose are used with >> NL80211_EXT_FEATURE_VHT_IBSS which is already a part of NL80211 >> feature >> flag. >> The new feature flag, NL80211_EXT_FEATURE_VHT_MESH, follows the same >> purpose and usage. > No, it doesn't. Check how the _IBSS one is used in the code to actually > *do* something. that's right. so take a look reset of explanation for this patch. > johannes Thanks, Peter
On Thu, 2015-11-12 at 15:05 -0800, Peter Oh wrote: > On 11/12/2015 02:32 PM, Johannes Berg wrote: > > On Thu, 2015-11-12 at 14:28 -0800, Peter Oh wrote: > > > > > > Exactly the same communication mechanism and purpose are used > > > with > > > NL80211_EXT_FEATURE_VHT_IBSS which is already a part of NL80211 > > > feature > > > flag. > > > The new feature flag, NL80211_EXT_FEATURE_VHT_MESH, follows the > > > same > > > purpose and usage. > > No, it doesn't. Check how the _IBSS one is used in the code to > > actually > > *do* something. > that's right. so take a look reset of explanation for this patch. > Still not making sense. I *suspect* that you think that the existing code is broken, and can't use VHT mesh and requires driver changes for it, but that's not what your ath10k change shows since it also does nothing at all. Right now, I see no reason whatsoever to apply either one of those two patches. There are no functional changes, so wpa_supplicant could enable VHT mesh by checking VHT capabilities or so instead of a special feature flag. I also suspect that perhaps mesh *should* be checking like IBSS does, although I also would actually *prefer* that we can assume VHT mesh works if the driver advertises VHT support and mesh support separately, i.e. a new feature flag really isn't necessary. In any case, the arguments for this patch haven't convinced me. I'm not going to apply this without much better ones. johannes
On 11/12/2015 11:37 PM, Johannes Berg wrote: > On Thu, 2015-11-12 at 15:05 -0800, Peter Oh wrote: >> On 11/12/2015 02:32 PM, Johannes Berg wrote: >>> On Thu, 2015-11-12 at 14:28 -0800, Peter Oh wrote: >>>> >>>> Exactly the same communication mechanism and purpose are used >>>> with >>>> NL80211_EXT_FEATURE_VHT_IBSS which is already a part of NL80211 >>>> feature >>>> flag. >>>> The new feature flag, NL80211_EXT_FEATURE_VHT_MESH, follows the >>>> same >>>> purpose and usage. >>> No, it doesn't. Check how the _IBSS one is used in the code to >>> actually >>> *do* something. >> that's right. so take a look reset of explanation for this patch. >> > Still not making sense. > > I *suspect* that you think that the existing code is broken, and can't > use VHT mesh and requires driver changes for it, I'm saying, cannot use VHT Mesh by wpa_supplicant because a lack of communication method between driver and wpa_supplicant, hence extending NL80211 feature flag. > but that's not what > your ath10k change shows since it also does nothing at all. ath10k is advertising its VHT Mesh capability using NL80211 feature flag in the patch which wpa_supplicant can listen and catch up. > > Right now, I see no reason whatsoever to apply either one of those two > patches. There are no functional changes, so wpa_supplicant could > enable VHT mesh by checking VHT capabilities or so instead of a special > feature flag. You're asking wpa_supplicant enhancement which is not fair to engineers that did not involve the design implementation. Also if you think Mesh should check VHT capabilities instead of a special feature flag, you had to ask the same thing to _VHT_IBSS. _VHT_IBSS feature flag is also used to advertise driver's capability of VHT for IBSS instead of checking driver's information elements. if IBSS had read VHT capability from driver's IE, _VHT_IBSS flag used at nl80211_join_ibss also shouldn't have existed. > > I also suspect that perhaps mesh *should* be checking like IBSS does, > although I also would actually *prefer* that we can assume VHT mesh > works if the driver advertises VHT support and mesh support separately, > i.e. a new feature flag really isn't necessary. Both IBSS and Mesh may support VHT without any feature flags. However this patch's approach is come from _VHT_IBSS feature flag which you already approved and so exists on upstream. If you're asking not to use _VHT_MESH feature flag and look for another way, your request affects to wpa_supplicant design of ibss and mesh, since mesh and ibss frequency info are handled at the same function and the function is designed to use _VHT_IBSS feature flag. If Mesh cannot use the _VHT_MESH feature flag, the design of function cannot keep the consistency of capability check. If you're saying _VHT_MESH feature flag is different from _VHT_IBSS because of what it is doing at nl80211_join_ibss function, I will add another patch to nl80211_join_mesh function to make _VHT_MESH feature flag the same as _VHT_IBSS. Will you be convinced then? > > In any case, the arguments for this patch haven't convinced me. I'm not > going to apply this without much better ones. > > johannes > > _______________________________________________ > ath10k mailing list > ath10k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath10k Thanks, Peter
On Fri, 2015-11-13 at 15:50 -0800, Peter Oh wrote: > > I'm saying, cannot use VHT Mesh by wpa_supplicant because a lack of > communication method between driver and wpa_supplicant, hence > extending NL80211 feature flag. > > And I'm arguing that you don't need such a communication method since, unlike IBSS, mesh doesn't restrict the channel width usable further than the HT/VHT capabilities. It might, on the other hand, very well be that mesh stuff is missing some VHT compatibility today - that would be an argument for a more comprehensive patch that then includes the feature flag to say "we've fixed the VHT issues in mesh mode". That's quite obviously not what you're doing though. johannes
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index 1f0b4cf..4a16ed5 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -4389,12 +4389,14 @@ enum nl80211_feature_flags { /** * enum nl80211_ext_feature_index - bit index of extended features. * @NL80211_EXT_FEATURE_VHT_IBSS: This driver supports IBSS with VHT datarates. + * @NL80211_EXT_FEATURE_VHT_MESH: This driver supports Mesh with VHT datarates. * * @NUM_NL80211_EXT_FEATURES: number of extended features. * @MAX_NL80211_EXT_FEATURES: highest extended feature index. */ enum nl80211_ext_feature_index { NL80211_EXT_FEATURE_VHT_IBSS, + NL80211_EXT_FEATURE_VHT_MESH, /* add new features before the definition below */ NUM_NL80211_EXT_FEATURES,
Add NL80211_EXT_FEATURE_VHT_MESH flag to allow drivers to indicate support for VHT in Mesh mode. This flag will be used by wireless drivers to indicate the capability of VHT Mesh support and wpa_supplicant to inspect wireless drivers' capability of VHT Mesh. In addition to this patch, separate patches to ath10k driver and wpa_supplicant are also sent out. Signed-off-by: Peter Oh <poh@qca.qualcomm.com> --- v2: - updated commit message for details --- include/uapi/linux/nl80211.h | 2 ++ 1 file changed, 2 insertions(+)