Re: [PATCH 3/4] brcmfmac: support the forwarding packet

From: Arend Van Spriel <arend.vanspriel@broadcom.com>
To: Wright Feng <wright.feng@cypress.com>, linux-wireless@vger.kernel.org
Cc: brcm80211-dev-list@broadcom.com, brcm80211-dev-list@cypress.com,
	Franky Lin <franky.lin@broadcom.com>,
	Hante Meuleman <hante.meuleman@broadcom.com>,
	Kalle Valo <kvalo@codeaurora.org>,
	chi-hsien.lin@cypress.com,
	Jia-Shyr Chuang <joseph.chuang@cypress.com>,
	Ting-Ying Li <tingying.li@cypress.com>
Subject: Re: [PATCH 3/4] brcmfmac: support the forwarding packet
Date: Tue, 1 Sep 2020 11:39:02 +0200	[thread overview]
Message-ID: <4e44b1b0-50d8-8f03-57f1-a307b759effa@broadcom.com> (raw)
In-Reply-To: <20200901063237.15549-4-wright.feng@cypress.com>

On 9/1/2020 8:32 AM, Wright Feng wrote:
> From: Jia-Shyr Chuang <joseph.chuang@cypress.com>
> 
> Support packet forwarding mechanism for some special usages on PCIE,
> and fix BE/VI priority issue when pumping iperf.

This is a bit to brief for what is being introduced here. Basically, 
your are introducing a shortcut if a packet from an associated STA is 
destined for another associated STA taking the linux network stack and 
its overhead out of the equation. Not sure if this is a desired feature 
for upstream driver as it likely is interfering with features like TSQ. 
So there should be something done to inform the network stack about this 
packet going into transmit path.

I also don't understand what is PCIe specific about this and what BE/VI 
priority issue is being fixed. Please spend more words on the commit 
message.

Some more specific comment below.

Regards,
Arend

> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
> Signed-off-by: Ting-Ying Li <tingying.li@cypress.com>
> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
> ---
>   .../broadcom/brcm80211/brcmfmac/cfg80211.c    |  13 ++-
>   .../broadcom/brcm80211/brcmfmac/core.c        | 100 +++++++++++++++++-
>   .../broadcom/brcm80211/brcmfmac/core.h        |  18 +++-
>   .../broadcom/brcm80211/brcmfmac/msgbuf.c      |  31 +++++-
>   4 files changed, 157 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index d6972420d426..8c7941f85715 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -4799,7 +4799,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
>   		err = -EINVAL;
>   		goto exit;
>   	}
> -
> +	ifp->isap = false;
>   	/* Interface specific setup */
>   	if (dev_role == NL80211_IFTYPE_AP) {
>   		if ((brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MBSS)) && (!mbss))
> @@ -4860,7 +4860,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
>   				 err);
>   			goto exit;
>   		}
> -
> +		ifp->isap = true;
>   		brcmf_dbg(TRACE, "AP mode configuration complete\n");
>   	} else if (dev_role == NL80211_IFTYPE_P2P_GO) {
>   		err = brcmf_fil_iovar_int_set(ifp, "chanspec", chanspec);
> @@ -4892,6 +4892,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev,
>   			goto exit;
>   		}
>   
> +		ifp->isap = true;
>   		brcmf_dbg(TRACE, "GO mode configuration complete\n");
>   	} else {
>   		WARN_ON(1);
> @@ -6045,6 +6046,14 @@ brcmf_notify_connect_status(struct brcmf_if *ifp,
>   	}
>   
>   	if (brcmf_is_apmode(ifp->vif)) {

Given this function I don't think the new struct brcmf_if::isap flag is 
necessary.

> +		if (e->event_code == BRCMF_E_ASSOC_IND ||
> +		    e->event_code == BRCMF_E_REASSOC_IND) {
> +			brcmf_findadd_sta(ifp, e->addr);
> +		} else if ((e->event_code == BRCMF_E_DISASSOC_IND) ||
> +				(e->event_code == BRCMF_E_DEAUTH_IND) ||
> +				(e->event_code == BRCMF_E_DEAUTH)) {
> +			brcmf_del_sta(ifp, e->addr);
> +		}
>   		err = brcmf_notify_connect_status_ap(cfg, ndev, e, data);
>   	} else if (brcmf_is_linkup(ifp->vif, e)) {
>   		brcmf_dbg(CONN, "Linkup\n");
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 20c510dca601..3257b784e019 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -62,6 +62,14 @@ struct wlc_d11rxhdr {
>   	s8 rxpwr[4];
>   } __packed;
>   
> +#define BRCMF_IF_STA_LIST_LOCK_INIT(ifp) spin_lock_init(&(ifp)->sta_list_lock)
> +#define BRCMF_IF_STA_LIST_LOCK(ifp, flags) \
> +	spin_lock_irqsave(&(ifp)->sta_list_lock, (flags))
> +#define BRCMF_IF_STA_LIST_UNLOCK(ifp, flags) \
> +	spin_unlock_irqrestore(&(ifp)->sta_list_lock, (flags))
> +
> +#define BRCMF_STA_NULL ((struct brcmf_sta *)NULL)

Don't hide actual functions with macros like these.