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 <[email protected]>
---
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
Yeoh Chun-Yeow <[email protected]> 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
On Thu, Apr 24, 2014 at 4:45 PM, Kalle Valo <[email protected]> wrote:
> Yeoh Chun-Yeow <[email protected]> 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
On Thu, Apr 24, 2014 at 4:22 PM, Kalle Valo <[email protected]> wrote:
> Chun-Yeow Yeoh <[email protected]> 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 <[email protected]>
>
> [...]
>
>> @@ -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
On 24 April 2014 10:50, Yeoh Chun-Yeow <[email protected]> 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ł
> 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
Yeoh Chun-Yeow <[email protected]> writes:
> On Thu, Apr 24, 2014 at 4:45 PM, Kalle Valo <[email protected]> wrote:
>> Yeoh Chun-Yeow <[email protected]> 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
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
On 24 April 2014 10:50, Johannes Berg <[email protected]> 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ł
Chun-Yeow Yeoh <[email protected]> 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 <[email protected]>
[...]
> @@ -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
On 24 April 2014 10:14, Chun-Yeow Yeoh <[email protected]> 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 <[email protected]>
> ---
> 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ł
>
> 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
On Thu, Apr 24, 2014 at 4:53 PM, Michal Kazior <[email protected]> wrote:
> On 24 April 2014 10:50, Yeoh Chun-Yeow <[email protected]> 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