Re: [PATCH] ath10k: only advertise mesh support in raw mode

[Kalle Valo]

Bob Copeland  writes:

> Rather than always advertising mesh support and then generating
> an error when the user attempts to bring up the device without
> rawmode, only advertise it when available: that is, if the module
> is loaded with rawmode=1, the firmware supports it, and the
> device/firmware supports an AP virtual interface internally.
>
> Signed-off-by: Bob Copeland 
> ---
> Actually, I think this is perhaps worse than just spitting out an
> error on interface up, but I'm sending anyway because it was requested
> a couple of times in review.
>
> The reason this is so ugly is that the structures in question are
> defined const in the API; when also declared const in the driver,
> they go into the .rodata section and the interface limit types
> cannot be changed without causing a fault.  So this patch removes
> const in a bunch of places and adds casts to const where required.
>
> Another approach might be changing cfg80211 to silently remove interface
> types from advertised combinations when the given mode is not also
> in interface_modes, instead of warning and failing registration as it
> does today.  But this would make errors here harder to spot.
>
> Or we can keep it the way it is with the driver failing and showing an
> error on add interface.  I'm okay with any of these approaches really.

Yeah, I'm with you. It would be nice to advertise the mesh feature on
when the firmware supports it, but I think this is just too complicated.
So I'm dropping this for now, but we can revisit later if there's a need
for that.

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ath10k: only advertise mesh support in raw mode

[Peter Oh]

On 09/30/2015 07:54 PM, Bob Copeland wrote:

On Mon, Sep 28, 2015 at 10:27:30AM -0700, Peter Oh wrote:

I prefer the current design to this new approach because drivers already
know if it's mesh capable or not at build time, hence adding runtime
configuration is redundant.

I do think the proposed patch is a bit overboard, so I suppose my vote is
to
keep it the way it is, even if a little user-unfriendly.


One more thing we have to think about is enabling mesh in only raw mode.
In standard view point, mesh is only available in raw mode on ath10k,
however ath10k is also able to run mesh in native WiFi mode in current
mac80211 design since mac80211 handles packets per interface. So that
mac80211 knows that packets are for mesh without looking at mesh control
present bit when they come into mesh interface.

This is true, it'll work with mac80211, but it violates the standard
(802.11-2012 8.2.4.7.3).

I agree.


For the benefit of others, as I just retested non-rawmode -- the issue is
that QoS control in nwifi frames is missing the "Mesh Control Present"
bit.
However, it still includes the mesh control field, which is the part
that has the mesh sequence number and address extension fields.

As a result, a packet parser might misinterpret the mesh control field as
the LLC header -- the wireshark dissector at least gets confused like
this.

I would think a small change to ath10k firmware could fix this though,
vendor willing.
I'm working with firmware guy and there is possibility that firmware 
handles this issue properly.



Thanks,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ath10k: only advertise mesh support in raw mode

[Yeoh Chun-Yeow]

>
> I would think a small change to ath10k firmware could fix this though,
> vendor willing.
>
+1 for this. Or even coming out with mesh mode instead of using AP
mode in ath10k firmware.

---
Chun-Yeow
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ath10k: only advertise mesh support in raw mode

[Bob Copeland]

On Mon, Sep 28, 2015 at 10:27:30AM -0700, Peter Oh wrote:
> I prefer the current design to this new approach because drivers already
> know if it's mesh capable or not at build time, hence adding runtime
> configuration is redundant.

I do think the proposed patch is a bit overboard, so I suppose my vote is to
keep it the way it is, even if a little user-unfriendly.

> One more thing we have to think about is enabling mesh in only raw mode.
> In standard view point, mesh is only available in raw mode on ath10k,
> however ath10k is also able to run mesh in native WiFi mode in current
> mac80211 design since mac80211 handles packets per interface. So that
> mac80211 knows that packets are for mesh without looking at mesh control
> present bit when they come into mesh interface.

This is true, it'll work with mac80211, but it violates the standard
(802.11-2012 8.2.4.7.3).

For the benefit of others, as I just retested non-rawmode -- the issue is
that QoS control in nwifi frames is missing the "Mesh Control Present" bit.
However, it still includes the mesh control field, which is the part
that has the mesh sequence number and address extension fields.

As a result, a packet parser might misinterpret the mesh control field as
the LLC header -- the wireshark dissector at least gets confused like this.

I would think a small change to ath10k firmware could fix this though,
vendor willing.

-- 
Bob Copeland %% http://bobcopeland.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ath10k: only advertise mesh support in raw mode

[Peter Oh]

Hi,

On 09/28/2015 05:59 AM, Bob Copeland wrote:

Rather than always advertising mesh support and then generating
an error when the user attempts to bring up the device without
rawmode, only advertise it when available: that is, if the module
is loaded with rawmode=1, the firmware supports it, and the
device/firmware supports an AP virtual interface internally.

Signed-off-by: Bob Copeland 
---
Actually, I think this is perhaps worse than just spitting out an
error on interface up, but I'm sending anyway because it was requested
a couple of times in review.

The reason this is so ugly is that the structures in question are
defined const in the API; when also declared const in the driver,
they go into the .rodata section and the interface limit types
cannot be changed without causing a fault.  So this patch removes
const in a bunch of places and adds casts to const where required.

Another approach might be changing cfg80211 to silently remove interface
types from advertised combinations when the given mode is not also
in interface_modes, instead of warning and failing registration as it
does today.  But this would make errors here harder to spot.

Or we can keep it the way it is with the driver failing and showing an
error on add interface.  I'm okay with any of these approaches really.
I prefer the current design to this new approach because drivers already 
know if it's mesh capable or not at build time, hence adding runtime 
configuration is redundant.

One more thing we have to think about is enabling mesh in only raw mode.
In standard view point, mesh is only available in raw mode on ath10k, 
however ath10k is also able to run mesh in native WiFi mode in current 
mac80211 design since mac80211 handles packets per interface. So that 
mac80211 knows that packets are for mesh without looking at mesh control 
present bit when they come into mesh interface.
Because of the reason allowing mesh in raw mode on ath10k is too much 
strict in my opinion.

  drivers/net/wireless/ath/ath10k/mac.c | 98
++-
  1 file changed, 63 insertions(+), 35 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c
b/drivers/net/wireless/ath/ath10k/mac.c
index 20d002c..c689613 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4209,11 +4209,6 @@ static int ath10k_add_interface(struct ieee80211_hw
*hw,
arvif->vdev_type = WMI_VDEV_TYPE_IBSS;
break;
case NL80211_IFTYPE_MESH_POINT:
-   if (!test_bit(ATH10K_FLAG_RAW_MODE, &ar->dev_flags)) {
-   ret = -EINVAL;
-   ath10k_warn(ar, "must load driver with rawmode=1
to add mesh interfaces\n");
-   goto err;
-   }
arvif->vdev_type = WMI_VDEV_TYPE_AP;
break;
case NL80211_IFTYPE_AP:
@@ -6755,7 +6750,7 @@ void ath10k_mac_destroy(struct ath10k *ar)
ieee80211_free_hw(ar->hw);
  }
  
-static const struct ieee80211_iface_limit ath10k_if_limits[] = {

+static struct ieee80211_iface_limit ath10k_if_limits[] = {
{
.max= 8,
.types  = BIT(NL80211_IFTYPE_STATION)
@@ -6772,23 +6767,17 @@ static const struct ieee80211_iface_limit
ath10k_if_limits[] = {
{
.max= 7,
.types  = BIT(NL80211_IFTYPE_AP)
-#ifdef CONFIG_MAC80211_MESH
-   | BIT(NL80211_IFTYPE_MESH_POINT)
-#endif
},
  };
  
-static const struct ieee80211_iface_limit ath10k_10x_if_limits[] = {

+static struct ieee80211_iface_limit ath10k_10x_if_limits[] = {
{
.max= 8,
.types  = BIT(NL80211_IFTYPE_AP)
-#ifdef CONFIG_MAC80211_MESH
-   | BIT(NL80211_IFTYPE_MESH_POINT)
-#endif
},
  };
  
-static const struct ieee80211_iface_combination ath10k_if_comb[] = {

+static struct ieee80211_iface_combination ath10k_if_comb[] = {
{
.limits = ath10k_if_limits,
.n_limits = ARRAY_SIZE(ath10k_if_limits),
@@ -6798,7 +6787,7 @@ static const struct ieee80211_iface_combination
ath10k_if_comb[] = {
},
  };
  
-static const struct ieee80211_iface_combination ath10k_10x_if_comb[] = {

+static struct ieee80211_iface_combination ath10k_10x_if_comb[] = {
{
.limits = ath10k_10x_if_limits,
.n_limits = ARRAY_SIZE(ath10k_10x_if_limits),
@@ -6814,7 +6803,7 @@ static const struct ieee80211_iface_combination
ath10k_10x_if_comb[] = {
},
  };
  
-static const struct ieee80211_iface_limit ath10k_tlv_if_limit[] = {

+static struct ieee80211_iface_limit ath10k_tlv_if_limit[] = {
{
.max = 2,
.types = BIT(NL80211_IFTYPE_STATION),
@@ -6822,9 +6811,6 @@ static const struct ieee80211_iface_limit
ath10k_tlv_if_limit[] = {
{
.max = 2,
.types = BIT(NL80211_IFTYPE_AP) |
-#ifdef CONFIG_MAC80211_MESH
-BIT(NL80211_IFTYPE_MESH_PO

[PATCH] ath10k: only advertise mesh support in raw mode

[Bob Copeland]

Rather than always advertising mesh support and then generating
an error when the user attempts to bring up the device without
rawmode, only advertise it when available: that is, if the module
is loaded with rawmode=1, the firmware supports it, and the
device/firmware supports an AP virtual interface internally.

Signed-off-by: Bob Copeland 
---
Actually, I think this is perhaps worse than just spitting out an
error on interface up, but I'm sending anyway because it was requested
a couple of times in review.

The reason this is so ugly is that the structures in question are
defined const in the API; when also declared const in the driver,
they go into the .rodata section and the interface limit types
cannot be changed without causing a fault.  So this patch removes
const in a bunch of places and adds casts to const where required.

Another approach might be changing cfg80211 to silently remove interface
types from advertised combinations when the given mode is not also
in interface_modes, instead of warning and failing registration as it
does today.  But this would make errors here harder to spot.

Or we can keep it the way it is with the driver failing and showing an
error on add interface.  I'm okay with any of these approaches really.

 drivers/net/wireless/ath/ath10k/mac.c | 98 ++-
 1 file changed, 63 insertions(+), 35 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index 20d002c..c689613 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4209,11 +4209,6 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
arvif->vdev_type = WMI_VDEV_TYPE_IBSS;
break;
case NL80211_IFTYPE_MESH_POINT:
-   if (!test_bit(ATH10K_FLAG_RAW_MODE, &ar->dev_flags)) {
-   ret = -EINVAL;
-   ath10k_warn(ar, "must load driver with rawmode=1 to add 
mesh interfaces\n");
-   goto err;
-   }
arvif->vdev_type = WMI_VDEV_TYPE_AP;
break;
case NL80211_IFTYPE_AP:
@@ -6755,7 +6750,7 @@ void ath10k_mac_destroy(struct ath10k *ar)
ieee80211_free_hw(ar->hw);
 }
 
-static const struct ieee80211_iface_limit ath10k_if_limits[] = {
+static struct ieee80211_iface_limit ath10k_if_limits[] = {
{
.max= 8,
.types  = BIT(NL80211_IFTYPE_STATION)
@@ -6772,23 +6767,17 @@ static const struct ieee80211_iface_limit 
ath10k_if_limits[] = {
{
.max= 7,
.types  = BIT(NL80211_IFTYPE_AP)
-#ifdef CONFIG_MAC80211_MESH
-   | BIT(NL80211_IFTYPE_MESH_POINT)
-#endif
},
 };
 
-static const struct ieee80211_iface_limit ath10k_10x_if_limits[] = {
+static struct ieee80211_iface_limit ath10k_10x_if_limits[] = {
{
.max= 8,
.types  = BIT(NL80211_IFTYPE_AP)
-#ifdef CONFIG_MAC80211_MESH
-   | BIT(NL80211_IFTYPE_MESH_POINT)
-#endif
},
 };
 
-static const struct ieee80211_iface_combination ath10k_if_comb[] = {
+static struct ieee80211_iface_combination ath10k_if_comb[] = {
{
.limits = ath10k_if_limits,
.n_limits = ARRAY_SIZE(ath10k_if_limits),
@@ -6798,7 +6787,7 @@ static const struct ieee80211_iface_combination 
ath10k_if_comb[] = {
},
 };
 
-static const struct ieee80211_iface_combination ath10k_10x_if_comb[] = {
+static struct ieee80211_iface_combination ath10k_10x_if_comb[] = {
{
.limits = ath10k_10x_if_limits,
.n_limits = ARRAY_SIZE(ath10k_10x_if_limits),
@@ -6814,7 +6803,7 @@ static const struct ieee80211_iface_combination 
ath10k_10x_if_comb[] = {
},
 };
 
-static const struct ieee80211_iface_limit ath10k_tlv_if_limit[] = {
+static struct ieee80211_iface_limit ath10k_tlv_if_limit[] = {
{
.max = 2,
.types = BIT(NL80211_IFTYPE_STATION),
@@ -6822,9 +6811,6 @@ static const struct ieee80211_iface_limit 
ath10k_tlv_if_limit[] = {
{
.max = 2,
.types = BIT(NL80211_IFTYPE_AP) |
-#ifdef CONFIG_MAC80211_MESH
-BIT(NL80211_IFTYPE_MESH_POINT) |
-#endif
 BIT(NL80211_IFTYPE_P2P_CLIENT) |
 BIT(NL80211_IFTYPE_P2P_GO),
},
@@ -6834,7 +6820,7 @@ static const struct ieee80211_iface_limit 
ath10k_tlv_if_limit[] = {
},
 };
 
-static const struct ieee80211_iface_limit ath10k_tlv_qcs_if_limit[] = {
+static struct ieee80211_iface_limit ath10k_tlv_qcs_if_limit[] = {
{
.max = 2,
.types = BIT(NL80211_IFTYPE_STATION),
@@ -6846,9 +6832,6 @@ static const struct ieee80211_iface_limit 
ath10k_tlv_qcs_if_limit[] = {
{
.max = 1,
.types = BIT(NL80211_IFTYPE_AP) |
-#ifdef CONFIG_MAC80211_MESH
-BIT(NL80211_IFTYPE_MESH_POI