DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5621920827
Subject: Re: [PATCH 2/3] brcmfmac: add credit numbers updating support
To:     Wright Feng <Wright.Feng@cypress.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
References: <1540808552-28738-1-git-send-email-wright.feng@cypress.com>
 <1540808552-28738-3-git-send-email-wright.feng@cypress.com>
Cc:     "franky.lin@broadcom.com" <franky.lin@broadcom.com>,
        "hante.meuleman@broadcom.com" <hante.meuleman@broadcom.com>,
        "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        Chi-Hsien Lin <Chi-Hsien.Lin@cypress.com>,
        "brcm80211-dev-list.pdl@broadcom.com" 
        <brcm80211-dev-list.pdl@broadcom.com>
From:   Arend van Spriel <arend.vanspriel@broadcom.com>
Message-ID: <aa65d65d-f610-3136-2e7d-45d150ec183d@broadcom.com>
Date:   Tue, 30 Oct 2018 12:04:38 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <1540808552-28738-3-git-send-email-wright.feng@cypress.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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.

>  	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?

>  	}
>  	brcmf_fws_schedule_deq(fws);
>  	brcmf_fws_unlock(fws);