On 2018/10/30 下午 07:04, Arend van Spriel wrote:
> On 10/29/2018 11:27 AM, Wright Feng wrote:
>> The credit numbers are static and tunable per chip in firmware side.
>> However the credit number may be changed that is based on packet pool
>> length and will send BRCMF_E_FIFO_CREDIT_MAP event to notify host driver
>> updates the credit numbers during interface up.
>> The purpose of this patch is making host driver has ability of updating
>> the credit numbers when receiving the BRCMF_E_FIFO_CREDIT_MAP event.
>
> Reviewed-by: Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx>
>> Signed-off-by: Wright Feng <wright.feng@xxxxxxxxxxx>
>> ---
>> .../broadcom/brcm80211/brcmfmac/fwsignal.c | 23
>> ++++++++++++++--------
>> 1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git
>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>> index f3cbf78..e0910c5 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>
> [...]
>
>> @@ -1595,19 +1599,21 @@ static int brcmf_fws_notify_credit_map(struct
>> brcmf_if *ifp,
>> brcmf_err("event payload too small (%d)\n", e->datalen);
>> return -EINVAL;
>> }
>> - if (fws->creditmap_received)
>> - return 0;
>>
>> fws->creditmap_received = true;
>
> I think the creditmap_received struct member is no longer needed.
I will keep this because we still need it for checking whether flow
control is active or not.
>
>> brcmf_dbg(TRACE, "enter: credits %pM\n", credits);
>> brcmf_fws_lock(fws);
>> for (i = 0; i < ARRAY_SIZE(fws->fifo_credit); i++) {
>> - if (*credits)
>> + fws->fifo_credit[i] += credits[i] - fws->init_fifo_credit[i];
>> + fws->init_fifo_credit[i] = credits[i];
>> + if (fws->fifo_credit[i] > 0)
>> fws->fifo_credit_map |= 1 << i;
>> else
>> fws->fifo_credit_map &= ~(1 << i);
>> - fws->fifo_credit[i] = *credits++;
>> + if (fws->fifo_credit[i] < 0)
>> + brcmf_err("fifo_credit[%d] value is negative(%d)\n",
>> + i, fws->fifo_credit[i]);
>
> This looks like it should not happen so maybe warrants a WARN or WARN_ONCE?
I will replace those 2 lines with WARN_ONCE.
Thanks for the suggestion.
-Wright
>
>> }
>> brcmf_fws_schedule_deq(fws);
>> brcmf_fws_unlock(fws);
>
