[PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Chun-Yeow Yeoh]

Firmware 999.999.0.636 does not allow stand alone monitor
mode. This means that bridging the STA mode and put it into
promiscuous mode will also cause the firmware to crash. Avoid
this.

Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index e2c01dc..f640328 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -647,10 +647,15 @@ static int ath10k_monitor_vdev_delete(struct ath10k *ar)
 
 static int ath10k_monitor_start(struct ath10k *ar)
 {
-	int ret;
+	int ret = -1;
 
 	lockdep_assert_held(&ar->conf_mutex);
 
+	if (ar->fw_version_build == 636) {
+		ath10k_warn("stand alone monitor mode is not supported\n");
+		return ret;
+	}
+
 	if (!ath10k_monitor_is_enabled(ar)) {
 		ath10k_warn("trying to start monitor with no references\n");
 		return 0;
-- 
1.9.2


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Kalle Valo]

Chun-Yeow Yeoh <yeohchunyeow@gmail.com> writes:

> Firmware 999.999.0.636 does not allow stand alone monitor
> mode. This means that bridging the STA mode and put it into
> promiscuous mode will also cause the firmware to crash. Avoid
> this.
>
> Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com>

[...]

> @@ -647,10 +647,15 @@ static int ath10k_monitor_vdev_delete(struct ath10k *ar)
>  
>  static int ath10k_monitor_start(struct ath10k *ar)
>  {
> -	int ret;
> +	int ret = -1;

I prefer to avoid initialising ret variables. And -1 is not a proper
error value.

>  	lockdep_assert_held(&ar->conf_mutex);
>  
> +	if (ar->fw_version_build == 636) {

Checking for firmware version in ath10k is a big no. For a functinality
change like this you should add a new feature flag to enum
ath10k_fw_features (and I need to then recreate the firmware image).

> +		ath10k_warn("stand alone monitor mode is not supported\n");

I would prefer not to print a warning for a situation like this. Can't
we instead return an error value back to the caller?

> +		return ret;

return -EOPNOTSUPP or similar is better approach than initialising ret
to -1.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Yeoh Chun-Yeow]

On Thu, Apr 24, 2014 at 4:22 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Chun-Yeow Yeoh <yeohchunyeow@gmail.com> writes:
>
>> Firmware 999.999.0.636 does not allow stand alone monitor
>> mode. This means that bridging the STA mode and put it into
>> promiscuous mode will also cause the firmware to crash. Avoid
>> this.
>>
>> Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com>
>
> [...]
>
>> @@ -647,10 +647,15 @@ static int ath10k_monitor_vdev_delete(struct ath10k *ar)
>>
>>  static int ath10k_monitor_start(struct ath10k *ar)
>>  {
>> -     int ret;
>> +     int ret = -1;
>
> I prefer to avoid initialising ret variables. And -1 is not a proper
> error value.
>
Ok.

>>       lockdep_assert_held(&ar->conf_mutex);
>>
>> +     if (ar->fw_version_build == 636) {
>
> Checking for firmware version in ath10k is a big no. For a functinality
> change like this you should add a new feature flag to enum
> ath10k_fw_features (and I need to then recreate the firmware image).
>
Should we just use the ATH10K_FW_FEATURE_WMI_10X?

>> +             ath10k_warn("stand alone monitor mode is not supported\n");
>
> I would prefer not to print a warning for a situation like this. Can't
> we instead return an error value back to the caller?
>
Yes.

>> +             return ret;
>
> return -EOPNOTSUPP or similar is better approach than initialising ret
> to -1.
Sure.

----
Chun-Yeow

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Kalle Valo]

Yeoh Chun-Yeow <yeohchunyeow@gmail.com> writes:

>>> +     if (ar->fw_version_build == 636) {
>>
>> Checking for firmware version in ath10k is a big no. For a functinality
>> change like this you should add a new feature flag to enum
>> ath10k_fw_features (and I need to then recreate the firmware image).
>>
> Should we just use the ATH10K_FW_FEATURE_WMI_10X?

That's a bit dangerous if in the future there's a new firmware which
doesn't have ATH10K_FW_FEATURE_WMI_10X but still doesn't support stand
alone monitor mode.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Yeoh Chun-Yeow]

On Thu, Apr 24, 2014 at 4:45 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Yeoh Chun-Yeow <yeohchunyeow@gmail.com> writes:
>
>>>> +     if (ar->fw_version_build == 636) {
>>>
>>> Checking for firmware version in ath10k is a big no. For a functinality
>>> change like this you should add a new feature flag to enum
>>> ath10k_fw_features (and I need to then recreate the firmware image).
>>>
>> Should we just use the ATH10K_FW_FEATURE_WMI_10X?
>
> That's a bit dangerous if in the future there's a new firmware which
> doesn't have ATH10K_FW_FEATURE_WMI_10X but still doesn't support stand
> alone monitor mode.
>

Then, we may need to introduce the new feature flag.

But then I just wondering if the firmware 636 claimed to support STA
mode "well" but then not allowed to be bridged. This may cause
confusion to end user which is the best firmware for STA mode. FYI, AP
firmware has no such issue if using as STA mode and put into
promiscuous mode.

----
Chun-Yeow

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Kalle Valo]

Yeoh Chun-Yeow <yeohchunyeow@gmail.com> writes:

> On Thu, Apr 24, 2014 at 4:45 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Yeoh Chun-Yeow <yeohchunyeow@gmail.com> writes:
>>
>>>>> +     if (ar->fw_version_build == 636) {
>>>>
>>>> Checking for firmware version in ath10k is a big no. For a functinality
>>>> change like this you should add a new feature flag to enum
>>>> ath10k_fw_features (and I need to then recreate the firmware image).
>>>>
>>> Should we just use the ATH10K_FW_FEATURE_WMI_10X?
>>
>> That's a bit dangerous if in the future there's a new firmware which
>> doesn't have ATH10K_FW_FEATURE_WMI_10X but still doesn't support stand
>> alone monitor mode.
>
> Then, we may need to introduce the new feature flag.

And that will create other problems. It's better to bite the bullet now
than trying to postpone it. Besides, adding the feature flag is trivial.

> But then I just wondering if the firmware 636 claimed to support STA
> mode "well" but then not allowed to be bridged. This may cause
> confusion to end user which is the best firmware for STA mode. FYI, AP
> firmware has no such issue if using as STA mode and put into
> promiscuous mode.

Yeah, maybe should change the documentation to recommend using 10.1
branch for AP, STA and monitor modes? And recommended main branch only
for Ad-Hoc and P2P?

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Yeoh Chun-Yeow]

>
> Yeah, maybe should change the documentation to recommend using 10.1
> branch for AP, STA and monitor modes? And recommended main branch only
> for Ad-Hoc and P2P?
>
Eventually, we need a single firmware that can support all the modes,
including mesh.

For the 10.1, it cited " STA specific features might not work". Can
someone comment what working and what not working?

----
Chun-Yeow

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Michal Kazior]

On 24 April 2014 10:14, Chun-Yeow Yeoh <yeohchunyeow@gmail.com> wrote:
> Firmware 999.999.0.636 does not allow stand alone monitor
> mode. This means that bridging the STA mode and put it into
> promiscuous mode will also cause the firmware to crash. Avoid
> this.
>
> Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com>
> ---
>  drivers/net/wireless/ath/ath10k/mac.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index e2c01dc..f640328 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -647,10 +647,15 @@ static int ath10k_monitor_vdev_delete(struct ath10k *ar)
>
>  static int ath10k_monitor_start(struct ath10k *ar)
>  {
> -       int ret;
> +       int ret = -1;
>
>         lockdep_assert_held(&ar->conf_mutex);
>
> +       if (ar->fw_version_build == 636) {
> +               ath10k_warn("stand alone monitor mode is not supported\n");
> +               return ret;
> +       }

I think Monitor operation should be performed on a best effort basis.
This means monitor_start/stop should be attempted once number of
non-monitor vdevs changes.

We should probably introduce a ath10k_recalc_monitor() for that purpose.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Johannes Berg]

On Thu, 2014-04-24 at 10:44 +0200, Michal Kazior wrote:

> I think Monitor operation should be performed on a best effort basis.
> This means monitor_start/stop should be attempted once number of
> non-monitor vdevs changes.
> 
> We should probably introduce a ath10k_recalc_monitor() for that purpose.

Doesn't mac80211 do that for you?

See IEEE80211_HW_WANT_MONITOR_VIF.

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Michal Kazior]

On 24 April 2014 10:50, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2014-04-24 at 10:44 +0200, Michal Kazior wrote:
>
>> I think Monitor operation should be performed on a best effort basis.
>> This means monitor_start/stop should be attempted once number of
>> non-monitor vdevs changes.
>>
>> We should probably introduce a ath10k_recalc_monitor() for that purpose.
>
> Doesn't mac80211 do that for you?
>
> See IEEE80211_HW_WANT_MONITOR_VIF.

This is not sufficient in this case.

E.g. If you add a disconnected 4addr sta interface to bridge the
interface enters promisc mode. This attempts to start monitor vdev in
ath10k before sta vdev is started internally. We could probably make
it start earlier (in add_interface) but there still remains a problem
when you stop last non-monitor interface (monitor vif will be created
_after_ last non-monitor is removed which is too late).


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Yeoh Chun-Yeow]

> I think Monitor operation should be performed on a best effort basis.
> This means monitor_start/stop should be attempted once number of
> non-monitor vdevs changes.

I am too clear about this. Do you mean that actually we can have
monitor mode on non-monitor vdevs in firmware 636?

----
Chun-Yeow

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Michal Kazior]

On 24 April 2014 10:50, Yeoh Chun-Yeow <yeohchunyeow@gmail.com> wrote:
>> I think Monitor operation should be performed on a best effort basis.
>> This means monitor_start/stop should be attempted once number of
>> non-monitor vdevs changes.
>
> I am too clear about this. Do you mean that actually we can have
> monitor mode on non-monitor vdevs in firmware 636?

No. What I mean is we should attempt to start monitor vdev once at
least 1 non-monitor vdev is present and stop monitor vdev is last
non-monitor vdev is about to be stopped on 636.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: don't allow stand alone monitor mode for non-AP firmware

[Yeoh Chun-Yeow]

On Thu, Apr 24, 2014 at 4:53 PM, Michal Kazior <michal.kazior@tieto.com> wrote:
> On 24 April 2014 10:50, Yeoh Chun-Yeow <yeohchunyeow@gmail.com> wrote:
>>> I think Monitor operation should be performed on a best effort basis.
>>> This means monitor_start/stop should be attempted once number of
>>> non-monitor vdevs changes.
>>
>> I am too clear about this. Do you mean that actually we can have
>> monitor mode on non-monitor vdevs in firmware 636?
>
> No. What I mean is we should attempt to start monitor vdev once at
> least 1 non-monitor vdev is present and stop monitor vdev is last
> non-monitor vdev is about to be stopped on 636.
>

Got it. But my investigation on firmware 636 shows that the firmware
crashes even though 1 non-monitor vdev is present. So can we conclude
actually monitor mode is not allowed in firmware 636.

---
Chun-Yeow

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k