[PATCH] ath10k: fix vdev map size for 10.x firmware

[PATCH] ath10k: fix vdev map size for 10.x firmware

[Bartosz Markowski]

Firmware 10.x supports up to 8 virtual AP interfaces
(comparing to 7 for main firmware). Previous vdev map
initialization was missing enough space for 8 + 1 vdevs
(we may spent one for mac monitor), due to wrong define used.

Use correct one - TARGET_10X_NUM_VDEVS - for 10.x firmware.

Signed-off-by: Bartosz Markowski <bartosz.markowski@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 82017f5..e6c56c5 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -795,7 +795,11 @@ int ath10k_core_start(struct ath10k *ar)
 	if (status)
 		goto err_htc_stop;
 
-	ar->free_vdev_map = (1 << TARGET_NUM_VDEVS) - 1;
+	if (test_bit(ATH10K_FW_FEATURE_WMI_10X, ar->fw_features))
+		ar->free_vdev_map = (1 << TARGET_10X_NUM_VDEVS) - 1;
+	else
+		ar->free_vdev_map = (1 << TARGET_NUM_VDEVS) - 1;
+
 	INIT_LIST_HEAD(&ar->arvifs);
 
 	if (!test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags))
-- 
1.8.2

Re: [PATCH] ath10k: fix vdev map size for 10.x firmware

[Ben Greear]

On 05/29/2014 06:36 AM, Bartosz Markowski wrote:
> Firmware 10.x supports up to 8 virtual AP interfaces
> (comparing to 7 for main firmware). Previous vdev map
> initialization was missing enough space for 8 + 1 vdevs
> (we may spent one for mac monitor), due to wrong define used.
>
> Use correct one - TARGET_10X_NUM_VDEVS - for 10.x firmware.

You are bumping total vdevs up to 16 with that patch...have you
actually tested that many?  The stock firmware has quite a bit of
deficiencies in the concurrency handling, at least for stations.

For what it's worth, my firmware will only work on stock kernels
because I ignore the request for 16 vdevs in the firmware and knock
it down to 8 to match the kernel driver (before your change below).

Thanks,
Ben


>
> Signed-off-by: Bartosz Markowski <bartosz.markowski@tieto.com>
> ---
>   drivers/net/wireless/ath/ath10k/core.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> index 82017f5..e6c56c5 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -795,7 +795,11 @@ int ath10k_core_start(struct ath10k *ar)
>   	if (status)
>   		goto err_htc_stop;
>
> -	ar->free_vdev_map = (1 << TARGET_NUM_VDEVS) - 1;
> +	if (test_bit(ATH10K_FW_FEATURE_WMI_10X, ar->fw_features))
> +		ar->free_vdev_map = (1 << TARGET_10X_NUM_VDEVS) - 1;
> +	else
> +		ar->free_vdev_map = (1 << TARGET_NUM_VDEVS) - 1;
> +
>   	INIT_LIST_HEAD(&ar->arvifs);
>
>   	if (!test_bit(ATH10K_FLAG_FIRST_BOOT_DONE, &ar->dev_flags))
>

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] ath10k: fix vdev map size for 10.x firmware

[Bartosz Markowski]

On 29 May 2014 15:47, Ben Greear <greearb@candelatech.com> wrote:
>
>
> On 05/29/2014 06:36 AM, Bartosz Markowski wrote:
>>
>> Firmware 10.x supports up to 8 virtual AP interfaces
>> (comparing to 7 for main firmware). Previous vdev map
>> initialization was missing enough space for 8 + 1 vdevs
>> (we may spent one for mac monitor), due to wrong define used.
>>
>> Use correct one - TARGET_10X_NUM_VDEVS - for 10.x firmware.
>
>
> You are bumping total vdevs up to 16 with that patch...have you
> actually tested that many?  The stock firmware has quite a bit of
> deficiencies in the concurrency handling, at least for stations.

No, I have never tested it with so many. I'm running tests with 8 at
most. And by this patch I just aimed to support 8 VAPS we advertise in
iface_combinations to mac80211 (.max_interfaces = 8). I was managed to
get only 7 before, since 1 vdev was being allocated as monitor.

[  219.960000] ath10k: mac monitor refs: promisc 1 monitor 0 cac 0
[  219.960000] ath10k: mac monitor vdev 1 created

I agree the 10X_NUM_VDEVS is a bit unfortunate with its 16 value, but
that's what was there from FW APIs for a long time and I do not know
exactly if it's still valid (from firmware point of view).

> For what it's worth, my firmware will only work on stock kernels
> because I ignore the request for 16 vdevs in the firmware and knock
> it down to 8 to match the kernel driver (before your change below).

Do you encode your firmware with the FW IE (wmi-10x) also? If som then
it's a bit weird to have different firmware tracks that introduce
themselves as 10.x.

-Bartosz

Re: [PATCH] ath10k: fix vdev map size for 10.x firmware

[Bartosz Markowski]

On 29 May 2014 16:10, Bartosz Markowski <bartosz.markowski@tieto.com> wrote:
> On 29 May 2014 15:47, Ben Greear <greearb@candelatech.com> wrote:
>>
>>
>> On 05/29/2014 06:36 AM, Bartosz Markowski wrote:
>>>
>>> Firmware 10.x supports up to 8 virtual AP interfaces
>>> (comparing to 7 for main firmware). Previous vdev map
>>> initialization was missing enough space for 8 + 1 vdevs
>>> (we may spent one for mac monitor), due to wrong define used.
>>>
>>> Use correct one - TARGET_10X_NUM_VDEVS - for 10.x firmware.
>>
>>
>> You are bumping total vdevs up to 16 with that patch...have you
>> actually tested that many?  The stock firmware has quite a bit of
>> deficiencies in the concurrency handling, at least for stations.
>
> No, I have never tested it with so many. I'm running tests with 8 at
> most. And by this patch I just aimed to support 8 VAPS we advertise in
> iface_combinations to mac80211 (.max_interfaces = 8). I was managed to
> get only 7 before, since 1 vdev was being allocated as monitor.

FYI: I quickly tried to extend the .max_interfaces +
interface_combinations limit in ath10k and check with 15 VAPs (using
hostapd, open network; on MIPS platform) and it just worked.. I did
not perform any exploratory testing, but managed to connect a couple
of clients to various bssids + run some sane iperf traffic etc.

-- 
Bartosz

Re: [PATCH] ath10k: fix vdev map size for 10.x firmware

[Ben Greear]

On 05/29/2014 07:42 AM, Bartosz Markowski wrote:
> On 29 May 2014 16:10, Bartosz Markowski <bartosz.markowski@tieto.com> wrote:
>> On 29 May 2014 15:47, Ben Greear <greearb@candelatech.com> wrote:
>>>
>>>
>>> On 05/29/2014 06:36 AM, Bartosz Markowski wrote:
>>>>
>>>> Firmware 10.x supports up to 8 virtual AP interfaces
>>>> (comparing to 7 for main firmware). Previous vdev map
>>>> initialization was missing enough space for 8 + 1 vdevs
>>>> (we may spent one for mac monitor), due to wrong define used.
>>>>
>>>> Use correct one - TARGET_10X_NUM_VDEVS - for 10.x firmware.
>>>
>>>
>>> You are bumping total vdevs up to 16 with that patch...have you
>>> actually tested that many?  The stock firmware has quite a bit of
>>> deficiencies in the concurrency handling, at least for stations.
>>
>> No, I have never tested it with so many. I'm running tests with 8 at
>> most. And by this patch I just aimed to support 8 VAPS we advertise in
>> iface_combinations to mac80211 (.max_interfaces = 8). I was managed to
>> get only 7 before, since 1 vdev was being allocated as monitor.
> 
> FYI: I quickly tried to extend the .max_interfaces +
> interface_combinations limit in ath10k and check with 15 VAPs (using
> hostapd, open network; on MIPS platform) and it just worked.. I did
> not perform any exploratory testing, but managed to connect a couple
> of clients to various bssids + run some sane iperf traffic etc.

Good to know that many VAPs can work...

If the OS limits are going to remain at 8, then maybe
only set the vdev mask to support 9 instead of 16
vifs to handle the monitor interface?  That should make
it easier (or at least safer) to support my firmware on
un-patched kernels...

Some of the problems with running out of resources in the
firmware (which generally just asserts or crashes in that case)
are only seen under load and/or random-ish cases
in my experience.  I'm all for pushing the firmware to the
max, but I suspect 16 vifs may not be that stable.

That said, I have only been testing multiple stations, so maybe it
will just work fine.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] ath10k: fix vdev map size for 10.x firmware

[Kalle Valo]

Bartosz Markowski <bartosz.markowski@tieto.com> writes:

> Firmware 10.x supports up to 8 virtual AP interfaces
> (comparing to 7 for main firmware). Previous vdev map
> initialization was missing enough space for 8 + 1 vdevs
> (we may spent one for mac monitor), due to wrong define used.
>
> Use correct one - TARGET_10X_NUM_VDEVS - for 10.x firmware.
>
> Signed-off-by: Bartosz Markowski <bartosz.markowski@tieto.com>

So what is the actual bug you are fixing? Previously with 10.x it was
possible to get only 7 VIFs, even though we advertised 8 to user space,
and with your fix we get the full 8 VIFs?

It would be good to clear have that in the commit log so that anyone can
understand what bug is fixed.

-- 
Kalle Valo

Re: [PATCH] ath10k: fix vdev map size for 10.x firmware

[Bartosz Markowski]

On 2 June 2014 18:42, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Bartosz Markowski <bartosz.markowski@tieto.com> writes:
>
>> Firmware 10.x supports up to 8 virtual AP interfaces
>> (comparing to 7 for main firmware). Previous vdev map
>> initialization was missing enough space for 8 + 1 vdevs
>> (we may spent one for mac monitor), due to wrong define used.
>>
>> Use correct one - TARGET_10X_NUM_VDEVS - for 10.x firmware.
>>
>> Signed-off-by: Bartosz Markowski <bartosz.markowski@tieto.com>
>
> So what is the actual bug you are fixing? Previously with 10.x it was
> possible to get only 7 VIFs, even though we advertised 8 to user space,
> and with your fix we get the full 8 VIFs?

For CAC, we use one VDEV to start monitor interface. In case of 10.X
firmware we advertise support up to 8 VAPs, but if we spent one for
monitor interface, only 7 left. I've noticed we fail on .add_interface
when trying to add 8th AP, here:

    bit = ffs(ar->free_vdev_map);
    if (bit == 0) {
        ret = -EBUSY;
        goto err;
    }

and this lead me to initialization code for vdev_map

    ar->free_vdev_map = (1 << TARGET_NUM_VDEVS) - 1;

We have an API split for main and 10.x firmware (incl. number of
vdevs, target fw is able to handle), but here we missed this split.

Ben has a valid point, the TARGET_10X_NUM_VDEVS claims to be 16, so
there's an inconsistency between what we adverts to mac in max
interfaces, but I'm not sure if this is such a big deal.

> It would be good to clear have that in the commit log so that anyone can
> understand what bug is fixed.

Do you want me to send a v2 with just an updated commit (better user
impact description)? (No patch content changes)

-Bartosz

Re: [PATCH] ath10k: fix vdev map size for 10.x firmware

[Kalle Valo]

Bartosz Markowski <bartosz.markowski@tieto.com> writes:

> On 2 June 2014 18:42, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>
>> So what is the actual bug you are fixing? Previously with 10.x it was
>> possible to get only 7 VIFs, even though we advertised 8 to user space,
>> and with your fix we get the full 8 VIFs?
>
> For CAC, we use one VDEV to start monitor interface. In case of 10.X
> firmware we advertise support up to 8 VAPs, but if we spent one for
> monitor interface, only 7 left. I've noticed we fail on .add_interface
> when trying to add 8th AP, here:
>
>     bit = ffs(ar->free_vdev_map);
>     if (bit == 0) {
>         ret = -EBUSY;
>         goto err;
>     }
>
> and this lead me to initialization code for vdev_map
>
>     ar->free_vdev_map = (1 << TARGET_NUM_VDEVS) - 1;
>
> We have an API split for main and 10.x firmware (incl. number of
> vdevs, target fw is able to handle), but here we missed this split.

This is a bit too technical. Basically I need a simple description of
the bug so that both kernel and distro maintainers can quicly understand
what this fix is about. Would this be correct:

"ath10k: fix 8th virtual AP interface with DFS

Firmware 10.x supports up to 8 virtual AP interfaces, but in a DFS
channel it was possible to create only 7 interfaces as ath10k internal
creates a monitor interface for DFS. Previous vdev map initialization
was missing enough space for 8 + 1 vdevs due to wrong define used and
that's why there was no space for 8th interface. Use the correct define
TARGET_10X_NUM_VDEVS with 10.x firmware to make it possible to create
the 8th virtual interface."

> Ben has a valid point, the TARGET_10X_NUM_VDEVS claims to be 16, so
> there's an inconsistency between what we adverts to mac in max
> interfaces, but I'm not sure if this is such a big deal.

I don't see that as a problem as long as we advertise 8 to user space.

>> It would be good to clear have that in the commit log so that anyone
>> can understand what bug is fixed.
>
> Do you want me to send a v2 with just an updated commit (better user
> impact description)? (No patch content changes)

I can update the commit log, we just need to agree on the content.

-- 
Kalle Valo

Re: [PATCH] ath10k: fix vdev map size for 10.x firmware

[Bartosz Markowski]

On 2 June 2014 19:28, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Bartosz Markowski <bartosz.markowski@tieto.com> writes:
>
>> On 2 June 2014 18:42, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>>
>>> So what is the actual bug you are fixing? Previously with 10.x it was
>>> possible to get only 7 VIFs, even though we advertised 8 to user space,
>>> and with your fix we get the full 8 VIFs?
>>
>> For CAC, we use one VDEV to start monitor interface. In case of 10.X
>> firmware we advertise support up to 8 VAPs, but if we spent one for
>> monitor interface, only 7 left. I've noticed we fail on .add_interface
>> when trying to add 8th AP, here:
>>
>>     bit = ffs(ar->free_vdev_map);
>>     if (bit == 0) {
>>         ret = -EBUSY;
>>         goto err;
>>     }
>>
>> and this lead me to initialization code for vdev_map
>>
>>     ar->free_vdev_map = (1 << TARGET_NUM_VDEVS) - 1;
>>
>> We have an API split for main and 10.x firmware (incl. number of
>> vdevs, target fw is able to handle), but here we missed this split.
>
> This is a bit too technical. Basically I need a simple description of
> the bug so that both kernel and distro maintainers can quicly understand
> what this fix is about. Would this be correct:
>
> "ath10k: fix 8th virtual AP interface with DFS
>
> Firmware 10.x supports up to 8 virtual AP interfaces, but in a DFS
> channel it was possible to create only 7 interfaces as ath10k internal
> creates a monitor interface for DFS. Previous vdev map initialization
> was missing enough space for 8 + 1 vdevs due to wrong define used and
> that's why there was no space for 8th interface. Use the correct define
> TARGET_10X_NUM_VDEVS with 10.x firmware to make it possible to create
> the 8th virtual interface."
>
>> Ben has a valid point, the TARGET_10X_NUM_VDEVS claims to be 16, so
>> there's an inconsistency between what we adverts to mac in max
>> interfaces, but I'm not sure if this is such a big deal.
>
> I don't see that as a problem as long as we advertise 8 to user space.
>
>>> It would be good to clear have that in the commit log so that anyone
>>> can understand what bug is fixed.
>>
>> Do you want me to send a v2 with just an updated commit (better user
>> impact description)? (No patch content changes)
>
> I can update the commit log, we just need to agree on the content.

The one you proposed looks good.

Thanks,
Bartosz

Re: [PATCH] ath10k: fix vdev map size for 10.x firmware

[Kalle Valo]

Bartosz Markowski <bartosz.markowski@tieto.com> writes:

> Firmware 10.x supports up to 8 virtual AP interfaces
> (comparing to 7 for main firmware). Previous vdev map
> initialization was missing enough space for 8 + 1 vdevs
> (we may spent one for mac monitor), due to wrong define used.
>
> Use correct one - TARGET_10X_NUM_VDEVS - for 10.x firmware.
>
> Signed-off-by: Bartosz Markowski <bartosz.markowski@tieto.com>

Thanks, applied to ath-current with this commit log:

commit dfa413de1e4388818f7dcdce0a90d6212e74895b
Author: Bartosz Markowski <bartosz.markowski@tieto.com>
Date:   Mon Jun 2 21:19:45 2014 +0300

    ath10k: fix 8th virtual AP interface with DFS
    
    Firmware 10.x supports up to 8 virtual AP interfaces, but in a DFS
    channel it was possible to create only 7 interfaces as ath10k internal
    creates a monitor interface for DFS. Previous vdev map initialization
    was missing enough space for 8 + 1 vdevs due to wrong define used and
    that's why there was no space for 8th interface. Use the correct define
    TARGET_10X_NUM_VDEVS with 10.x firmware to make it possible to create
    the 8th virtual interface.
    
    Signed-off-by: Bartosz Markowski <bartosz.markowski@tieto.com>
    Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>

-- 
Kalle Valo