| Message ID | 1462280620-13000-1-git-send-email-jjmeijer88@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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.
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. -- 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
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".
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? -- 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 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 -- 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
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/
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.
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? -- 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
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;
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(+)