Re: [PATCH 2/3] brcmfmac: add credit numbers updating support

[Wright Feng <Wright.Feng@cypress.com>]

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@broadcom.com>
>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>> ---
>>  .../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);
>