2016-05-06 16:12 GMT+01:00 Kalle Valo <[email protected]>:
> Jaap Jan Meijer <[email protected]> writes:
>
>> Hi Kalle,
>>
>> Op vr 6 mei 2016 12:52 schreef Kalle Valo <[email protected]>:
>>
>>
>> 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.
Arend Van Spriel <[email protected]> writes:
> On 6-5-2016 18:02, Kalle Valo wrote:
>> Jaap Jan Meijer <[email protected]> writes:
>>
>>> 2016-05-06 16:12 GMT+01:00 Kalle Valo <[email protected]>:
>>>> Jaap Jan Meijer <[email protected]> writes:
>>>>
>>>>> Hi Kalle,
>>>>>
>>>>> Op vr 6 mei 2016 12:52 schreef Kalle Valo <[email protected]>:
>>>>>
>>>>>
>>>>> 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
2016-05-09 16:21 GMT+02:00 Kalle Valo <[email protected]>:
> Arend Van Spriel <[email protected]> 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?
On 6-5-2016 18:02, Kalle Valo wrote:
> Jaap Jan Meijer <[email protected]> writes:
>
>> 2016-05-06 16:12 GMT+01:00 Kalle Valo <[email protected]>:
>>> Jaap Jan Meijer <[email protected]> writes:
>>>
>>>> Hi Kalle,
>>>>
>>>> Op vr 6 mei 2016 12:52 schreef Kalle Valo <[email protected]>:
>>>>
>>>>
>>>> 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
Jaap Jan Meijer <[email protected]> writes:
> 2016-05-06 16:12 GMT+01:00 Kalle Valo <[email protected]>:
>> Jaap Jan Meijer <[email protected]> writes:
>>
>>> Hi Kalle,
>>>
>>> Op vr 6 mei 2016 12:52 schreef Kalle Valo <[email protected]>:
>>>
>>>
>>> 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