[2/2] ath10k: Fix sending NULL/ Qos NULL data frames for QCA99X0 and later

Commit Message

Mohammed Shafi Shajakhan June 23, 2016, 4:40 p.m. UTC

From: Mohammed Shafi Shajakhan <[e-mail]>

For chipsets like QCA99X0, IPQ4019 and later we are not getting proper
NULL func status (always acked/successs !!) when hostapd does a
PROBE_CLIENT via nullfunc frames when the station is powered off
abruptly (inactive timer probes client via null func after the inactive
time reaches beyond the threshold). Fix this by disabling the workaround
(getting the ACK status of NULL func frames by sending via HTT mgmt-tx
 path) introduced by the change ("ath10k: fix beacon loss handling ")
for QCA99X0 and later chipsets. The normal tx path provides the proper
ACK status for NULL data frames. As of now disable this workaround for
chipsets QCA99X0 and later, once the 10.1 firmware is obselete we can
completely get rid of this workaround for all the chipsets

Signed-off-by: Tamizh chelvam <[e-mail]>
Signed-off-by: Mohammed Shafi Shajakhan <[e-mail]>
---
 drivers/net/wireless/ath/ath10k/core.c | 3 +++
 drivers/net/wireless/ath/ath10k/core.h | 6 ++++++
 drivers/net/wireless/ath/ath10k/mac.c  | 1 +
 3 files changed, 10 insertions(+)

Comments

Ben Greear June 23, 2016, 5:12 p.m. UTC | #1

On 06/23/2016 09:40 AM, Mohammed Shafi Shajakhan wrote:
> From: Mohammed Shafi Shajakhan <[e-mail]>
>
> For chipsets like QCA99X0, IPQ4019 and later we are not getting proper
> NULL func status (always acked/successs !!) when hostapd does a
> PROBE_CLIENT via nullfunc frames when the station is powered off
> abruptly (inactive timer probes client via null func after the inactive
> time reaches beyond the threshold). Fix this by disabling the workaround
> (getting the ACK status of NULL func frames by sending via HTT mgmt-tx
>   path) introduced by the change ("ath10k: fix beacon loss handling ")
> for QCA99X0 and later chipsets. The normal tx path provides the proper
> ACK status for NULL data frames. As of now disable this workaround for
> chipsets QCA99X0 and later, once the 10.1 firmware is obselete we can
> completely get rid of this workaround for all the chipsets

Did you see my fix for the tx-status that Kalle posted about recently?
That fixed my problem with 9980, but I normally test status with tx over
the htt mgt path instead of wmi path, so possibly that makes a difference.

Thanks,
Ben

Mohammed Shafi Shajakhan June 25, 2016, 6:53 p.m. UTC | #2

Hello Ben,

On Thu, Jun 23, 2016 at 10:12:01AM -0700, Ben Greear wrote:
> On 06/23/2016 09:40 AM, Mohammed Shafi Shajakhan wrote:
> >From: Mohammed Shafi Shajakhan <[e-mail]>
> >
> >For chipsets like QCA99X0, IPQ4019 and later we are not getting proper
> >NULL func status (always acked/successs !!) when hostapd does a
> >PROBE_CLIENT via nullfunc frames when the station is powered off
> >abruptly (inactive timer probes client via null func after the inactive
> >time reaches beyond the threshold). Fix this by disabling the workaround
> >(getting the ACK status of NULL func frames by sending via HTT mgmt-tx
> >  path) introduced by the change ("ath10k: fix beacon loss handling ")
> >for QCA99X0 and later chipsets. The normal tx path provides the proper
> >ACK status for NULL data frames. As of now disable this workaround for
> >chipsets QCA99X0 and later, once the 10.1 firmware is obselete we can
> >completely get rid of this workaround for all the chipsets
> 
> Did you see my fix for the tx-status that Kalle posted about recently?
> That fixed my problem with 9980, but I normally test status with tx over
> the htt mgt path instead of wmi path, so possibly that makes a difference.

[shafi] Yes Kalle suggested to try your change (BTW we were not aware that you
already discovered this problem !!), https://patchwork.kernel.org/patch/8460831/
but ..


"[ 747.803652] ath10k_pci 0000:01:00.0: htt tx completion msdu_id 0 discard 0
no_ack 0 success 1"

"[ 747.803843] ath10k_pci 0001:01:00.0: htt tx completion msdu_id 0 discard 0
no_ack 0 success 1"

to be more precise looks like we hit this path for MSG type 0xE,( management Tx
completion indication) path(status ok ??) for null func frame without ACK

case HTT_MGMT_TX_STATUS_OK:
	tx_done.status = HTT_TX_COMPL_STATE_ACK;
	break;


a) The success bit being 'set' without the fix so the change you had mentioned may not help
for the upstreamed f/w, should we  give a shot with your change as well to
confirm it ?

b) and also the workaround is for older firmware which got fixed in
10.2.4 and 10.4, if you can help us that 10.1 also reports NULL func frame
status properly via normal data path, we can probably get rid of this workaround
once and for all ? thoughts ?

regards,
shafi
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ben Greear June 26, 2016, 12:27 a.m. UTC | #3

On 06/25/2016 11:53 AM, Mohammed Shafi Shajakhan wrote:
> Hello Ben,
>
> On Thu, Jun 23, 2016 at 10:12:01AM -0700, Ben Greear wrote:
>> On 06/23/2016 09:40 AM, Mohammed Shafi Shajakhan wrote:
>>> From: Mohammed Shafi Shajakhan <[e-mail]>
>>>
>>> For chipsets like QCA99X0, IPQ4019 and later we are not getting proper
>>> NULL func status (always acked/successs !!) when hostapd does a
>>> PROBE_CLIENT via nullfunc frames when the station is powered off
>>> abruptly (inactive timer probes client via null func after the inactive
>>> time reaches beyond the threshold). Fix this by disabling the workaround
>>> (getting the ACK status of NULL func frames by sending via HTT mgmt-tx
>>>   path) introduced by the change ("ath10k: fix beacon loss handling ")
>>> for QCA99X0 and later chipsets. The normal tx path provides the proper
>>> ACK status for NULL data frames. As of now disable this workaround for
>>> chipsets QCA99X0 and later, once the 10.1 firmware is obselete we can
>>> completely get rid of this workaround for all the chipsets
>>
>> Did you see my fix for the tx-status that Kalle posted about recently?
>> That fixed my problem with 9980, but I normally test status with tx over
>> the htt mgt path instead of wmi path, so possibly that makes a difference.
>
> [shafi] Yes Kalle suggested to try your change (BTW we were not aware that you
> already discovered this problem !!), https://patchwork.kernel.org/patch/8460831/
> but ..

Is there a better way than posting to the ath10k mailing list?  There are quite
a few more of my patches that are stuck in pending or ignored state.  If you
could review some of them and add them to your testing trees then it might help
them go upstream.

Maybe someone else with more time and resources could take over maintainer-ship of ath10k
and let Kalle focus on the drivers in general?

> "[ 747.803652] ath10k_pci 0000:01:00.0: htt tx completion msdu_id 0 discard 0
> no_ack 0 success 1"
>
> "[ 747.803843] ath10k_pci 0001:01:00.0: htt tx completion msdu_id 0 discard 0
> no_ack 0 success 1"
>
> to be more precise looks like we hit this path for MSG type 0xE,( management Tx
> completion indication) path(status ok ??) for null func frame without ACK
>
> case HTT_MGMT_TX_STATUS_OK:
> 	tx_done.status = HTT_TX_COMPL_STATE_ACK;
> 	break;
>
>
> a) The success bit being 'set' without the fix so the change you had mentioned may not help
> for the upstreamed f/w, should we  give a shot with your change as well to
> confirm it ?

I have no good way to know what exact source creates the upstream firmware.  But, grep
around for FILTERED and you will probably see your firmware using that tx code.

It could easily be that both of our patches are correct and needed.

> b) and also the workaround is for older firmware which got fixed in
> 10.2.4 and 10.4, if you can help us that 10.1 also reports NULL func frame
> status properly via normal data path, we can probably get rid of this workaround
> once and for all ? thoughts ?

I can fix my 10.1 if it is not already fixed.  If you are ready to have my
10.1 be the only supported 10.1, then we can do the tests.  If that is the
case, then you should add a feature flag for my firmware and fail to load any
other 10.1 firmware since it would then be guaranteed to fail since you are removing
workarounds for it...

I would rather focus on getting some of the debug patches upstream so that
I can actually make progress on firmware bugs found on stock kernels.  Without
them, I have to ask people to run patched drivers in order to get debug info,
and that is painful for all involved.

Thanks,
Ben

Michal Kazior June 27, 2016, 9:27 a.m. UTC | #4

On 23 June 2016 at 18:40, Mohammed Shafi Shajakhan
<[e-mail]> wrote:
> From: Mohammed Shafi Shajakhan <[e-mail]>
>
> For chipsets like QCA99X0, IPQ4019 and later we are not getting proper
> NULL func status (always acked/successs !!) when hostapd does a
> PROBE_CLIENT via nullfunc frames when the station is powered off
> abruptly (inactive timer probes client via null func after the inactive
> time reaches beyond the threshold). Fix this by disabling the workaround
> (getting the ACK status of NULL func frames by sending via HTT mgmt-tx
>  path) introduced by the change ("ath10k: fix beacon loss handling ")
> for QCA99X0 and later chipsets. The normal tx path provides the proper
> ACK status for NULL data frames. As of now disable this workaround for
> chipsets QCA99X0 and later, once the 10.1 firmware is obselete we can
> completely get rid of this workaround for all the chipsets
>
> Signed-off-by: Tamizh chelvam <[e-mail]>
> Signed-off-by: Mohammed Shafi Shajakhan <[e-mail]>
> ---
>  drivers/net/wireless/ath/ath10k/core.c | 3 +++
>  drivers/net/wireless/ath/ath10k/core.h | 6 ++++++
>  drivers/net/wireless/ath/ath10k/mac.c  | 1 +
>  3 files changed, 10 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> index 689d6ce..9978e4a 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -181,6 +181,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
>                         .board = QCA99X0_HW_2_0_BOARD_DATA_FILE,
>                         .board_size = QCA99X0_BOARD_DATA_SZ,
>                         .board_ext_size = QCA99X0_BOARD_EXT_DATA_SZ,
> +                       .disable_null_func_workaround = true,

Tx completion (bugs) are firmware specific, not hardware. This should
be expressed via features bits in ath10k FW API, no?


Michał
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mohammed Shafi Shajakhan June 27, 2016, 2:36 p.m. UTC | #5

Hi Michal,

thanks for the review ..

On Mon, Jun 27, 2016 at 11:27:27AM +0200, Michal Kazior wrote:
> On 23 June 2016 at 18:40, Mohammed Shafi Shajakhan
> <[e-mail]> wrote:
> > From: Mohammed Shafi Shajakhan <[e-mail]>
> >
> > For chipsets like QCA99X0, IPQ4019 and later we are not getting proper
> > NULL func status (always acked/successs !!) when hostapd does a
> > PROBE_CLIENT via nullfunc frames when the station is powered off
> > abruptly (inactive timer probes client via null func after the inactive
> > time reaches beyond the threshold). Fix this by disabling the workaround
> > (getting the ACK status of NULL func frames by sending via HTT mgmt-tx
> >  path) introduced by the change ("ath10k: fix beacon loss handling ")
> > for QCA99X0 and later chipsets. The normal tx path provides the proper
> > ACK status for NULL data frames. As of now disable this workaround for
> > chipsets QCA99X0 and later, once the 10.1 firmware is obselete we can
> > completely get rid of this workaround for all the chipsets
> >
> > Signed-off-by: Tamizh chelvam <[e-mail]>
> > Signed-off-by: Mohammed Shafi Shajakhan <[e-mail]>
> > ---
> >  drivers/net/wireless/ath/ath10k/core.c | 3 +++
> >  drivers/net/wireless/ath/ath10k/core.h | 6 ++++++
> >  drivers/net/wireless/ath/ath10k/mac.c  | 1 +
> >  3 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> > index 689d6ce..9978e4a 100644
> > --- a/drivers/net/wireless/ath/ath10k/core.c
> > +++ b/drivers/net/wireless/ath/ath10k/core.c
> > @@ -181,6 +181,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
> >                         .board = QCA99X0_HW_2_0_BOARD_DATA_FILE,
> >                         .board_size = QCA99X0_BOARD_DATA_SZ,
> >                         .board_ext_size = QCA99X0_BOARD_EXT_DATA_SZ,
> > +                       .disable_null_func_workaround = true,
> 
> Tx completion (bugs) are firmware specific, not hardware. This should
> be expressed via features bits in ath10k FW API, no?
> 
>
[shafi] Are you suggesting me to introduce something like
"ATH10K_FW_FEATURE_SUPPORTS_SKIP_CLOCK_INIT" ? Kalle any suggestions ?

Also how about getting this workaround completely if Ben had fixed this in his tree,
will this affect older 10.2.4 ?

regards,
shafi

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michal Kazior June 28, 2016, 6:48 a.m. UTC | #6

On 27 June 2016 at 16:36, Mohammed Shafi Shajakhan
<[e-mail]> wrote:
> Hi Michal,
>
> thanks for the review ..
>
> On Mon, Jun 27, 2016 at 11:27:27AM +0200, Michal Kazior wrote:
>> On 23 June 2016 at 18:40, Mohammed Shafi Shajakhan
>> <[e-mail]> wrote:
>> > From: Mohammed Shafi Shajakhan <[e-mail]>
>> >
>> > For chipsets like QCA99X0, IPQ4019 and later we are not getting proper
>> > NULL func status (always acked/successs !!) when hostapd does a
>> > PROBE_CLIENT via nullfunc frames when the station is powered off
>> > abruptly (inactive timer probes client via null func after the inactive
>> > time reaches beyond the threshold). Fix this by disabling the workaround
>> > (getting the ACK status of NULL func frames by sending via HTT mgmt-tx
>> >  path) introduced by the change ("ath10k: fix beacon loss handling ")
>> > for QCA99X0 and later chipsets. The normal tx path provides the proper
>> > ACK status for NULL data frames. As of now disable this workaround for
>> > chipsets QCA99X0 and later, once the 10.1 firmware is obselete we can
>> > completely get rid of this workaround for all the chipsets
>> >
>> > Signed-off-by: Tamizh chelvam <[e-mail]>
>> > Signed-off-by: Mohammed Shafi Shajakhan <[e-mail]>
>> > ---
>> >  drivers/net/wireless/ath/ath10k/core.c | 3 +++
>> >  drivers/net/wireless/ath/ath10k/core.h | 6 ++++++
>> >  drivers/net/wireless/ath/ath10k/mac.c  | 1 +
>> >  3 files changed, 10 insertions(+)
>> >
>> > diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
>> > index 689d6ce..9978e4a 100644
>> > --- a/drivers/net/wireless/ath/ath10k/core.c
>> > +++ b/drivers/net/wireless/ath/ath10k/core.c
>> > @@ -181,6 +181,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
>> >                         .board = QCA99X0_HW_2_0_BOARD_DATA_FILE,
>> >                         .board_size = QCA99X0_BOARD_DATA_SZ,
>> >                         .board_ext_size = QCA99X0_BOARD_EXT_DATA_SZ,
>> > +                       .disable_null_func_workaround = true,
>>
>> Tx completion (bugs) are firmware specific, not hardware. This should
>> be expressed via features bits in ath10k FW API, no?
>>
>>
> [shafi] Are you suggesting me to introduce something like
> "ATH10K_FW_FEATURE_SUPPORTS_SKIP_CLOCK_INIT" ? Kalle any suggestions ?
>
> Also how about getting this workaround completely if Ben had fixed this in his tree,
> will this affect older 10.2.4 ?

There's still 636.

We could probably get rid of this as long as:
 - ath10k can express the need to use Probe Requests for AP probing
(in client mode) and beacon loss handling purposes instead of NullFunc
to mac80211
 - everyone uses hostapd with disassoc_low_ack=1 with affected
firmware revisions
 - supplicant uses disassoc_low_ack=1 for p2p go
 - I have no idea about mesh/ibss but they might require some work as well

Otherwise you'll introduce regressions.


Michał
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mohammed Shafi Shajakhan June 29, 2016, 9:47 a.m. UTC | #7

Hello Michal/ Kalle,

On Tue, Jun 28, 2016 at 08:48:38AM +0200, Michal Kazior wrote:
> On 27 June 2016 at 16:36, Mohammed Shafi Shajakhan
> <[e-mail]> wrote:
> > Hi Michal,
> >
> > thanks for the review ..
> >
> > On Mon, Jun 27, 2016 at 11:27:27AM +0200, Michal Kazior wrote:
> >> On 23 June 2016 at 18:40, Mohammed Shafi Shajakhan
> >> <[e-mail]> wrote:
> >> > From: Mohammed Shafi Shajakhan <[e-mail]>
> >> >
> >> > For chipsets like QCA99X0, IPQ4019 and later we are not getting proper
> >> > NULL func status (always acked/successs !!) when hostapd does a
> >> > PROBE_CLIENT via nullfunc frames when the station is powered off
> >> > abruptly (inactive timer probes client via null func after the inactive
> >> > time reaches beyond the threshold). Fix this by disabling the workaround
> >> > (getting the ACK status of NULL func frames by sending via HTT mgmt-tx
> >> >  path) introduced by the change ("ath10k: fix beacon loss handling ")
> >> > for QCA99X0 and later chipsets. The normal tx path provides the proper
> >> > ACK status for NULL data frames. As of now disable this workaround for
> >> > chipsets QCA99X0 and later, once the 10.1 firmware is obselete we can
> >> > completely get rid of this workaround for all the chipsets
> >> >
> >> > Signed-off-by: Tamizh chelvam <[e-mail]>
> >> > Signed-off-by: Mohammed Shafi Shajakhan <[e-mail]>
> >> > ---
> >> >  drivers/net/wireless/ath/ath10k/core.c | 3 +++
> >> >  drivers/net/wireless/ath/ath10k/core.h | 6 ++++++
> >> >  drivers/net/wireless/ath/ath10k/mac.c  | 1 +
> >> >  3 files changed, 10 insertions(+)
> >> >
> >> > diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> >> > index 689d6ce..9978e4a 100644
> >> > --- a/drivers/net/wireless/ath/ath10k/core.c
> >> > +++ b/drivers/net/wireless/ath/ath10k/core.c
> >> > @@ -181,6 +181,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
> >> >                         .board = QCA99X0_HW_2_0_BOARD_DATA_FILE,
> >> >                         .board_size = QCA99X0_BOARD_DATA_SZ,
> >> >                         .board_ext_size = QCA99X0_BOARD_EXT_DATA_SZ,
> >> > +                       .disable_null_func_workaround = true,
> >>
> >> Tx completion (bugs) are firmware specific, not hardware. This should
> >> be expressed via features bits in ath10k FW API, no?
> >>
> >>
> > [shafi] Are you suggesting me to introduce something like
> > "ATH10K_FW_FEATURE_SUPPORTS_SKIP_CLOCK_INIT" ? Kalle any suggestions ?
> >
> > Also how about getting this workaround completely if Ben had fixed this in his tree,
> > will this affect older 10.2.4 ?
> 
> There's still 636.
> 
> We could probably get rid of this as long as:
>  - ath10k can express the need to use Probe Requests for AP probing
> (in client mode) and beacon loss handling purposes instead of NullFunc
> to mac80211
>  - everyone uses hostapd with disassoc_low_ack=1 with affected
> firmware revisions
>  - supplicant uses disassoc_low_ack=1 for p2p go
>  - I have no idea about mesh/ibss but they might require some work as well
> 
> Otherwise you'll introduce regressions.
>

[shafi] sure then we will disable this for 10.4 (QCA99X0 and later)

*firmware feature requires a new firmware updated this feature, so the bug
will be present for all the older firmware, please correct me if my
understanding is wrong

*We discussed wmi_op_version is not the way to go (in the sense just disable it
for 10.4 alone)

Let me know if there is any other suggestion (the existing change though bit
cleanly is very explicit regarding the chipsets that this workaround is not
needed), thank you !

regards,
shafi

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kalle Valo June 30, 2016, 10:25 a.m. UTC | #8

Ben Greear <[e-mail]> writes:

> Is there a better way than posting to the ath10k mailing list?  There are quite
> a few more of my patches that are stuck in pending or ignored state.  If you
> could review some of them and add them to your testing trees then it might help
> them go upstream.

Yes, as you seem to test with your custom ath10k and custom firmware
review and testing feedback from others is more than welcome. That way I
can have more confidence that the patch really works with the mainline
version.

The problem with your patches is that you dump more responsibility on
me. I have no idea if they work with the mainline kernel, or if they
break something, so I need to personally test the patch and that takes
time. Basically I have a rule if that I need to use more than two
minutes on a patch it goes to the Deferred queue and I'll go through
that less often than the normal patch queue.

Ben Greear July 5, 2016, 3:21 p.m. UTC | #9

On 06/30/2016 03:25 AM, Valo, Kalle wrote:
> Ben Greear <[e-mail]> writes:
>
>> Is there a better way than posting to the ath10k mailing list?  There are quite
>> a few more of my patches that are stuck in pending or ignored state.  If you
>> could review some of them and add them to your testing trees then it might help
>> them go upstream.
>
> Yes, as you seem to test with your custom ath10k and custom firmware
> review and testing feedback from others is more than welcome. That way I
> can have more confidence that the patch really works with the mainline
> version.
>
> The problem with your patches is that you dump more responsibility on
> me. I have no idea if they work with the mainline kernel, or if they
> break something, so I need to personally test the patch and that takes
> time. Basically I have a rule if that I need to use more than two
> minutes on a patch it goes to the Deferred queue and I'll go through
> that less often than the normal patch queue.

I can do some testing with stock firmware, but stock firmware is often quite limited
so I cannot do the more interesting test cases that we use internally.
Some of my patches are for fixing edge cases that cannot be easily reproduced,
and that code really needs to be reviewed for correctness more by looking at
the code than by trying to run some exhaustive test case.

I think if there were a specific maintainer for ath10k that could take a more
active role, especially with regard to keeping a fairly wide variety of test rigs
around to run regression tests, then it would help all involved.  I think this person
needs access to firmware source so that they can more easily verify the driver
matches the firmware:  Many of the regressions and bug fixes, for instance with
stats, would be much easier to clean up if you look at firmware while developing
the driver bits.

Thanks,
Ben

Mohammed Shafi Shajakhan July 8, 2016, 11 a.m. UTC | #10

Hi Michal / Kalle / Ben,

is this patch is good to go (or) should i re-work  ?
I had replied to Michal's comment of introducing a new firmware
feature flag will not address the issue in older firmware / code.
Let me know if i had missed something very obvious.


On Tue, Jul 05, 2016 at 08:21:01AM -0700, Ben Greear wrote:
> On 06/30/2016 03:25 AM, Valo, Kalle wrote:
> >Ben Greear <[e-mail]> writes:
> >
> >>Is there a better way than posting to the ath10k mailing list?  There are quite
> >>a few more of my patches that are stuck in pending or ignored state.  If you
> >>could review some of them and add them to your testing trees then it might help
> >>them go upstream.
> >
> >Yes, as you seem to test with your custom ath10k and custom firmware
> >review and testing feedback from others is more than welcome. That way I
> >can have more confidence that the patch really works with the mainline
> >version.
> >
> >The problem with your patches is that you dump more responsibility on
> >me. I have no idea if they work with the mainline kernel, or if they
> >break something, so I need to personally test the patch and that takes
> >time. Basically I have a rule if that I need to use more than two
> >minutes on a patch it goes to the Deferred queue and I'll go through
> >that less often than the normal patch queue.
> 
> I can do some testing with stock firmware, but stock firmware is often quite limited
> so I cannot do the more interesting test cases that we use internally.
> Some of my patches are for fixing edge cases that cannot be easily reproduced,
> and that code really needs to be reviewed for correctness more by looking at
> the code than by trying to run some exhaustive test case.
> 
> I think if there were a specific maintainer for ath10k that could take a more
> active role, especially with regard to keeping a fairly wide variety of test rigs
> around to run regression tests, then it would help all involved.  I think this person
> needs access to firmware source so that they can more easily verify the driver
> matches the firmware:  Many of the regressions and bug fixes, for instance with
> stats, would be much easier to clean up if you look at firmware while developing
> the driver bits.
>

regards,
shafi
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michal Kazior July 20, 2016, 12:22 p.m. UTC | #11

On 20 July 2016 at 13:43, Shajakhan, Mohammed Shafi (Mohammed Shafi)
<[e-mail]> wrote:
> Michal,
>
> Can you please let me know if this change is fine or not ?
> I am waiting infinitely for  your reply long time

Sorry. I was absent for a while and this email slipped by.

Quoting your other email:

> is this patch is good to go (or) should i re-work  ?
> I had replied to Michal's comment of introducing a new firmware
> feature flag will not address the issue in older firmware / code.
> Let me know if i had missed something very obvious.

I couldn't really find any conclusion to this thread in my inbox.

The hw_params approach is definitely wrong. The ACK problem is
firmware-specific, not hardware-specific.

I'm fine with removing the workaround completely if 636 is guaranteed
to not be broken, including AP and P2P GO operation. The client
operation will probably work since wmi-roam event is used for HW
connection monitoring.

Nullfunc tx-status ack/noack reports could be probably fixed up using
sta kickout events (with short timeouts) as a fallback. This would
make it easier for users because otherwise they'd need to enable
disassoc_low_ack in hostapd (which is probably impossible with
wpa_supplicant for p2p, no?).


Michał
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mohammed Shafi Shajakhan July 20, 2016, 4:27 p.m. UTC | #12

On Wed, Jul 20, 2016 at 02:22:29PM +0200, Michal Kazior wrote:
> On 20 July 2016 at 13:43, Shajakhan, Mohammed Shafi (Mohammed Shafi)
> <[e-mail]> wrote:
> > Michal,
> >
> > Can you please let me know if this change is fine or not ?
> > I am waiting infinitely for  your reply long time
> 
> Sorry. I was absent for a while and this email slipped by.

[shafi] ah ok, i thought you had blacklisted this change :)

> 
> Quoting your other email:
> 
> > is this patch is good to go (or) should i re-work  ?
> > I had replied to Michal's comment of introducing a new firmware
> > feature flag will not address the issue in older firmware / code.
> > Let me know if i had missed something very obvious.
> 
> I couldn't really find any conclusion to this thread in my inbox.
> 
> The hw_params approach is definitely wrong. The ACK problem is
> firmware-specific, not hardware-specific.

[shafi] sure let me see if i can figure out an alternative way that shall be
accepted by you and Kalle

> 
> I'm fine with removing the workaround completely if 636 is guaranteed
> to not be broken, including AP and P2P GO operation. The client
> operation will probably work since wmi-roam event is used for HW
> connection monitoring.

[shafi] sorry i am not sure how to validate that, so i will keep this workaround

> 
> Nullfunc tx-status ack/noack reports could be probably fixed up using
> sta kickout events (with short timeouts) as a fallback. This would
> make it easier for users because otherwise they'd need to enable
> disassoc_low_ack in hostapd (which is probably impossible with
> wpa_supplicant for p2p, no?).
> 
>
[shafi] let me check this, i think we usually don't enable disassoc_low_ack to
avoid kicking out stations frequently.

shafi
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 689d6ce..9978e4a 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -181,6 +181,7 @@  static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 			.board = QCA99X0_HW_2_0_BOARD_DATA_FILE,
 			.board_size = QCA99X0_BOARD_DATA_SZ,
 			.board_ext_size = QCA99X0_BOARD_EXT_DATA_SZ,
+			.disable_null_func_workaround = true,
 		},
 	},
 	{
@@ -204,6 +205,7 @@  static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 			.board = QCA9984_HW_1_0_BOARD_DATA_FILE,
 			.board_size = QCA99X0_BOARD_DATA_SZ,
 			.board_ext_size = QCA99X0_BOARD_EXT_DATA_SZ,
+			.disable_null_func_workaround = true,
 		},
 	},
 	{
@@ -262,6 +264,7 @@  static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 			.board = QCA4019_HW_1_0_BOARD_DATA_FILE,
 			.board_size = QCA4019_BOARD_DATA_SZ,
 			.board_ext_size = QCA4019_BOARD_EXT_DATA_SZ,
+			.disable_null_func_workaround = true,
 		},
 	},
 };
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 3da18c9..e8c728c 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -750,6 +750,12 @@  struct ath10k {
 			const char *board;
 			size_t board_size;
 			size_t board_ext_size;
+			/* Workaround of sending NULL data frames via
+			 * HTT mgmt TX and getting the proper ACK status does
+			 * not works for chipsets QCA99X0 and later, while
+			 * Tx data path reports the ACK status properly.
+			 */
+			bool disable_null_func_workaround;
 		} fw;
 	} hw_params;
 
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index d4b7a16..f1e9672 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3236,6 +3236,7 @@  ath10k_mac_tx_h_get_txmode(struct ath10k *ar,
 	 * mode though because AP don't sleep.
 	 */
 	if (ar->htt.target_version_major < 3 &&
+	    !ar->hw_params.fw.disable_null_func_workaround &&
 	    (ieee80211_is_nullfunc(fc) || ieee80211_is_qos_nullfunc(fc)) &&
 	    !test_bit(ATH10K_FW_FEATURE_HAS_WMI_MGMT_TX,
 		      ar->running_fw->fw_file.fw_features))