| Message ID | 1401370567-13543-1-git-send-email-bartosz.markowski@tieto.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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)) >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>
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))
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(-)