[PATCH] Fix regression in Android due to rework .get_station() callback

[PATCH] Fix regression in Android due to rework .get_station() callback

[meijjaa]

After f654d13 Android is not able to get station signal strength.
To much was removed, this patch brings back the needed functionality.

Signed-off-by: meijjaa <jjmeijer88@gmail.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
index deb5f78..0275474 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
@@ -2428,6 +2428,8 @@ brcmf_cfg80211_get_station(struct wiphy *wiphy, struct net_device *ndev,
 {
 	struct brcmf_if *ifp = netdev_priv(ndev);
 	s32 err = 0;
+	struct brcmf_scb_val_le scb_val;
+	int rssi;
 	struct brcmf_sta_info_le sta_info_le;
 	u32 sta_flags;
 	u32 is_tdls_peer;
@@ -2506,6 +2508,21 @@ brcmf_cfg80211_get_station(struct wiphy *wiphy, struct net_device *ndev,
 				count_rssi++;
 			}
 		}
+		if (test_bit(BRCMF_VIF_STATUS_CONNECTED,
+			     &ifp->vif->sme_state)) {
+			memset(&scb_val, 0, sizeof(scb_val));
+			err = brcmf_fil_cmd_data_get(ifp, BRCMF_C_GET_RSSI,
+						     &scb_val, sizeof(scb_val));
+			if (err) {
+				brcmf_err("Could not get rssi (%d)\n", err);
+				goto done;
+			} else {
+				rssi = le32_to_cpu(scb_val.val);
+				sinfo->filled |= BIT(NL80211_STA_INFO_SIGNAL);
+				sinfo->signal = rssi;
+				brcmf_dbg(CONN, "RSSI %d dBm\n", rssi);
+			}
+		}
 		if (count_rssi) {
 			sinfo->filled |= BIT(NL80211_STA_INFO_CHAIN_SIGNAL);
 			sinfo->chains = count_rssi;
-- 
1.9.1

Re: [PATCH] Fix regression in Android due to rework .get_station() callback

[Kalle Valo]

meijjaa <jjmeijer88@gmail.com> writes:

> After f654d13 Android is not able to get station signal strength.
> To much was removed, this patch brings back the needed functionality.
>
> Signed-off-by: meijjaa <jjmeijer88@gmail.com>

This has multiple issues:

o Use your full name.

o Use prefix "brcmfmac: " in the title.

o I can't find commit f654d13, is the commit id really correct?

o Also check from SubmittingPatches how you should reference commit ids.

-- 
Kalle Valo

Re: [PATCH] Fix regression in Android due to rework .get_station() callback

[Jaap Jan Meijer]

2016-05-06 16:12 GMT+01:00 Kalle Valo <kvalo@codeaurora.org>:
> Jaap Jan Meijer <jjmeijer88@gmail.com> writes:
>
>> Hi Kalle,
>>
>> Op vr 6 mei 2016 12:52 schreef Kalle Valo <kvalo@codeaurora.org>:
>>
>>
>>     This has multiple issues:
>>
>>     o Use your full name.
>>     o Use prefix "brcmfmac: " in the title.
>>
>>     o I can't find commit f654d13, is the commit id really correct?
>>     o Also check from SubmittingPatches how you should reference commit ids.
>>
>>
>>
>> Thank you for the feedback, I will send a reworked patch as soon as I get home
>> next week. Also I did this against v4.4.8 so I'll have to rebase it as well.
>>
>> I'm not sure what went wrong with the commit hash, its actually this commit:
>> 1f0dc59a6de93586fcfc04696a61946408ffc56a.
>
> That commit id looks to be valid.
>
>> I see you did this commit, maybe you can check if this actually is the root
>> cause? I'm sure you have a lot more insight into this issue than I do.
>
> Please CC the list so that everyone can join the discussion.
>
> --
> Kalle Valo

Sorry, I shouldn't do this on a mobile device.

Re: [PATCH] Fix regression in Android due to rework .get_station() callback

[Kalle Valo]

Jaap Jan Meijer <jjmeijer88@gmail.com> writes:

> 2016-05-06 16:12 GMT+01:00 Kalle Valo <kvalo@codeaurora.org>:
>> Jaap Jan Meijer <jjmeijer88@gmail.com> writes:
>>
>>> Hi Kalle,
>>>
>>> Op vr 6 mei 2016 12:52 schreef Kalle Valo <kvalo@codeaurora.org>:
>>>
>>>
>>>     This has multiple issues:
>>>
>>>     o Use your full name.
>>>     o Use prefix "brcmfmac: " in the title.
>>>
>>>     o I can't find commit f654d13, is the commit id really correct?
>>>     o Also check from SubmittingPatches how you should reference commit ids.
>>>
>>>
>>>
>>> Thank you for the feedback, I will send a reworked patch as soon as I get home
>>> next week. Also I did this against v4.4.8 so I'll have to rebase it as well.
>>>
>>> I'm not sure what went wrong with the commit hash, its actually this commit:
>>> 1f0dc59a6de93586fcfc04696a61946408ffc56a.
>>
>> That commit id looks to be valid.
>>
>>> I see you did this commit, maybe you can check if this actually is the root
>>> cause? I'm sure you have a lot more insight into this issue than I do.

I just commited the patch. Broadcom folks (CCed) should be able to
answer better, most likely they missed this patch as the title didn't
have "brcmfmac".

-- 
Kalle Valo

Re: [PATCH] Fix regression in Android due to rework .get_station() callback

[Jaap Jan Meijer]

2016-05-06 17:02 GMT+01:00 Kalle Valo <kvalo@codeaurora.org>:
> Jaap Jan Meijer <jjmeijer88@gmail.com> writes:
>
>> 2016-05-06 16:12 GMT+01:00 Kalle Valo <kvalo@codeaurora.org>:
>>> Jaap Jan Meijer <jjmeijer88@gmail.com> writes:
>>>
>>>> Hi Kalle,
>>>>
>>>> Op vr 6 mei 2016 12:52 schreef Kalle Valo <kvalo@codeaurora.org>:
>>>>
>>>>
>>>>     This has multiple issues:
>>>>
>>>>     o Use your full name.
>>>>     o Use prefix "brcmfmac: " in the title.
>>>>
>>>>     o I can't find commit f654d13, is the commit id really correct?
>>>>     o Also check from SubmittingPatches how you should reference commit ids.
>>>>
>>>>
>>>>
>>>> Thank you for the feedback, I will send a reworked patch as soon as I get home
>>>> next week. Also I did this against v4.4.8 so I'll have to rebase it as well.
>>>>
>>>> I'm not sure what went wrong with the commit hash, its actually this commit:
>>>> 1f0dc59a6de93586fcfc04696a61946408ffc56a.
>>>
>>> That commit id looks to be valid.
>>>
>>>> I see you did this commit, maybe you can check if this actually is the root
>>>> cause? I'm sure you have a lot more insight into this issue than I do.
>
> I just commited the patch. Broadcom folks (CCed) should be able to
> answer better, most likely they missed this patch as the title didn't
> have "brcmfmac".
>
> --
> Kalle Valo

Thanks, so no need to send a revised patch? Also, could you please
point me to the right repository so I can follow the progress?

Re: [PATCH] Fix regression in Android due to rework .get_station() callback

[Kalle Valo]

Jaap Jan Meijer <jjmeijer88@gmail.com> writes:

> 2016-05-06 17:02 GMT+01:00 Kalle Valo <kvalo@codeaurora.org>:
>> Jaap Jan Meijer <jjmeijer88@gmail.com> writes:
>>
>>> 2016-05-06 16:12 GMT+01:00 Kalle Valo <kvalo@codeaurora.org>:
>>>> Jaap Jan Meijer <jjmeijer88@gmail.com> writes:
>>>>
>>>>> I'm not sure what went wrong with the commit hash, its actually this commit:
>>>>> 1f0dc59a6de93586fcfc04696a61946408ffc56a.
>>>>
>>>> That commit id looks to be valid.
>>>>
>>>>> I see you did this commit, maybe you can check if this actually is the root
>>>>> cause? I'm sure you have a lot more insight into this issue than I do.
>>
>> I just commited the patch. Broadcom folks (CCed) should be able to
>> answer better, most likely they missed this patch as the title didn't
>> have "brcmfmac".
>
> Thanks, so no need to send a revised patch? Also, could you please
> point me to the right repository so I can follow the progress?

Sorry, I had very bad choise of words. I meant that I only commit
patches (like the commit 1f0dc59a6d we are discussing here), I'm not
familiar with brcmfmac internals. So I have NOT applied this patch and
you need to send v2. I see that Arend also found some issues, please
follow his advice.

> Also, could you please point me to the right repository so I can
> follow the progress?

You can check that from MAINTAINERS file:

NETWORKING DRIVERS (WIRELESS)
M:      Kalle Valo <kvalo@codeaurora.org>
L:      linux-wireless@vger.kernel.org
Q:      http://patchwork.kernel.org/project/linux-wireless/list/
T:      git git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git
T:      git git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git
S:      Maintained
F:      drivers/net/wireless/

But wireless-testing tree, maintained by Bob Copeland, is handy to
follow the status because it contains all the wireless trees:

https://git.kernel.org/cgit/linux/kernel/git/wireless/wireless-testing.git/

-- 
Kalle Valo

Re: [PATCH] Fix regression in Android due to rework .get_station() callback

[Arend Van Spriel]

On 6-5-2016 18:02, Kalle Valo wrote:
> Jaap Jan Meijer <jjmeijer88@gmail.com> writes:
> 
>> 2016-05-06 16:12 GMT+01:00 Kalle Valo <kvalo@codeaurora.org>:
>>> Jaap Jan Meijer <jjmeijer88@gmail.com> writes:
>>>
>>>> Hi Kalle,
>>>>
>>>> Op vr 6 mei 2016 12:52 schreef Kalle Valo <kvalo@codeaurora.org>:
>>>>
>>>>
>>>>     This has multiple issues:
>>>>
>>>>     o Use your full name.
>>>>     o Use prefix "brcmfmac: " in the title.
>>>>
>>>>     o I can't find commit f654d13, is the commit id really correct?
>>>>     o Also check from SubmittingPatches how you should reference commit ids.
>>>>
>>>>
>>>>
>>>> Thank you for the feedback, I will send a reworked patch as soon as I get home
>>>> next week. Also I did this against v4.4.8 so I'll have to rebase it as well.
>>>>
>>>> I'm not sure what went wrong with the commit hash, its actually this commit:
>>>> 1f0dc59a6de93586fcfc04696a61946408ffc56a.
>>>
>>> That commit id looks to be valid.
>>>
>>>> I see you did this commit, maybe you can check if this actually is the root
>>>> cause? I'm sure you have a lot more insight into this issue than I do.
> 
> I just commited the patch. Broadcom folks (CCed) should be able to
> answer better, most likely they missed this patch as the title didn't
> have "brcmfmac".

Hi Kalle,

I did see the patch and noticed the procedural issues as well. However,
last week was a short week over here and I did not get to it to respond.
The fix is not done properly. The function determines the RSSI from the
per-chain values. I suspect that Jaap Jan is using a device which does
not report per-chain values so his solution should be used as fallback.
So can you revert the patch so Jaap Jan can rework the patch, ie.:

	if (count_rssi) {
		:
	} else if (test_bit(BRCMF_VIF_STATUS_CONNECTED,
			    &ifp->vif->sme_state)) {
		memset(&scb_val, 0, sizeof(scb_val));
		err = brcmf_fil_cmd_data_get(ifp, BRCMF_C_GET_RSSI,
					     &scb_val, sizeof(scb_val));
		if (err) {
			brcmf_err("Could not get rssi (%d)\n", err);
			goto done;
		} else {
			rssi = le32_to_cpu(scb_val.val);
			sinfo->filled |= BIT(NL80211_STA_INFO_SIGNAL);
			sinfo->signal = rssi;
			brcmf_dbg(CONN, "RSSI %d dBm\n", rssi);
		}
	}

Let me know if that is ok or should I submit a fixup patch.

Regards,
Arend

Re: [PATCH] Fix regression in Android due to rework .get_station() callback

[Kalle Valo]

Arend Van Spriel <arend.vanspriel@broadcom.com> writes:

> On 6-5-2016 18:02, Kalle Valo wrote:
>> Jaap Jan Meijer <jjmeijer88@gmail.com> writes:
>> 
>>> 2016-05-06 16:12 GMT+01:00 Kalle Valo <kvalo@codeaurora.org>:
>>>> Jaap Jan Meijer <jjmeijer88@gmail.com> writes:
>>>>
>>>>> Hi Kalle,
>>>>>
>>>>> Op vr 6 mei 2016 12:52 schreef Kalle Valo <kvalo@codeaurora.org>:
>>>>>
>>>>>
>>>>>     This has multiple issues:
>>>>>
>>>>>     o Use your full name.
>>>>>     o Use prefix "brcmfmac: " in the title.
>>>>>
>>>>>     o I can't find commit f654d13, is the commit id really correct?
>>>>>     o Also check from SubmittingPatches how you should reference commit ids.
>>>>>
>>>>>
>>>>>
>>>>> Thank you for the feedback, I will send a reworked patch as soon as I get home
>>>>> next week. Also I did this against v4.4.8 so I'll have to rebase it as well.
>>>>>
>>>>> I'm not sure what went wrong with the commit hash, its actually this commit:
>>>>> 1f0dc59a6de93586fcfc04696a61946408ffc56a.
>>>>
>>>> That commit id looks to be valid.
>>>>
>>>>> I see you did this commit, maybe you can check if this actually is the root
>>>>> cause? I'm sure you have a lot more insight into this issue than I do.
>> 
>> I just commited the patch. Broadcom folks (CCed) should be able to
>> answer better, most likely they missed this patch as the title didn't
>> have "brcmfmac".
>
> I did see the patch and noticed the procedural issues as well. However,
> last week was a short week over here and I did not get to it to respond.
> The fix is not done properly. The function determines the RSSI from the
> per-chain values. I suspect that Jaap Jan is using a device which does
> not report per-chain values so his solution should be used as fallback.
> So can you revert the patch so Jaap Jan can rework the patch, ie.:
>
> 	if (count_rssi) {
> 		:
> 	} else if (test_bit(BRCMF_VIF_STATUS_CONNECTED,
> 			    &ifp->vif->sme_state)) {
> 		memset(&scb_val, 0, sizeof(scb_val));
> 		err = brcmf_fil_cmd_data_get(ifp, BRCMF_C_GET_RSSI,
> 					     &scb_val, sizeof(scb_val));
> 		if (err) {
> 			brcmf_err("Could not get rssi (%d)\n", err);
> 			goto done;
> 		} else {
> 			rssi = le32_to_cpu(scb_val.val);
> 			sinfo->filled |= BIT(NL80211_STA_INFO_SIGNAL);
> 			sinfo->signal = rssi;
> 			brcmf_dbg(CONN, "RSSI %d dBm\n", rssi);
> 		}
> 	}
>
> Let me know if that is ok or should I submit a fixup patch.

I haven't applied Jaap's patch yet so he can send v2. Sorry for the
confusion.

-- 
Kalle Valo

Re: [PATCH] Fix regression in Android due to rework .get_station() callback

[Jaap Jan Meijer]

2016-05-09 16:21 GMT+02:00 Kalle Valo <kvalo@codeaurora.org>:
> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
>> I did see the patch and noticed the procedural issues as well. However,
>> last week was a short week over here and I did not get to it to respond.
>> The fix is not done properly. The function determines the RSSI from the
>> per-chain values. I suspect that Jaap Jan is using a device which does
>> not report per-chain values so his solution should be used as fallback.
>> So can you revert the patch so Jaap Jan can rework the patch, ie.:
>>
>>       if (count_rssi) {
>>               :
>>       } else if (test_bit(BRCMF_VIF_STATUS_CONNECTED,
>>                           &ifp->vif->sme_state)) {
>>               memset(&scb_val, 0, sizeof(scb_val));
>>               err = brcmf_fil_cmd_data_get(ifp, BRCMF_C_GET_RSSI,
>>                                            &scb_val, sizeof(scb_val));
>>               if (err) {
>>                       brcmf_err("Could not get rssi (%d)\n", err);
>>                       goto done;
>>               } else {
>>                       rssi = le32_to_cpu(scb_val.val);
>>                       sinfo->filled |= BIT(NL80211_STA_INFO_SIGNAL);
>>                       sinfo->signal = rssi;
>>                       brcmf_dbg(CONN, "RSSI %d dBm\n", rssi);
>>               }
>>       }
>>


Thank you for the feedback, I would like to contribute so I'll build a
new patch.

One more question: I only included the signal strength because I
needed it for Android, should I also include Beacon peroid and DTIM
period again in this fallback? If so, I would probably need to
separate this code in a new fallback method?