Received-SPF: pass (google.com: domain of linux-kernel+bounces-55725-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 6 Feb 2024 17:29:31 -0600
From: Andrew Halaney <ahalaney@redhat.com>
To: Abhishek Chauhan <quic_abchauha@quicinc.com>
Cc: Vinod Koul <vkoul@kernel.org>, 
	Bhupesh Sharma <bhupesh.sharma@linaro.org>, Andy Gross <agross@kernel.org>, 
	Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, 
	"David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, 
	Jakub Kicinski <kuba@kernel.org>, Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Alexandre Torgue <alexandre.torgue@foss.st.com>, Jose Abreu <joabreu@synopsys.com>, 
	Maxime Coquelin <mcoquelin.stm32@gmail.com>, netdev@vger.kernel.org, linux-arm-msm@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-stm32@st-md-mailman.stormreply.com, Prasad Sodagudi <psodagud@quicinc.com>, 
	Rob Herring <robh@kernel.org>, kernel@quicinc.com
Subject: Re: [PATCH v1] TSO and TBS cannot co-exist. TBS requires special
 descriptor to be allocated at bootup. Initialising Tx queues at probe to
 support TSO and TBS can help in allocating those resources at bootup.
Message-ID: <uihm7fwg5s6vqdmkz3poasuukdhuc4ztep7doqxnvhhsbfmfop@iikjtrb4jwoi>
References: <20240206212734.1209920-1-quic_abchauha@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240206212734.1209920-1-quic_abchauha@quicinc.com>

Hey Abhishek,

Thanks for the patch!

This is a new feature for netdev, so the Subject should have "net-next"
in it: https://docs.kernel.org/process/maintainer-netdev.html#tl-dr

Another thing, the kernel is very particular about the commit messages.
I thought checkpatch.pl would complain about the subject line, but
surprisingly it didn't.

Usually going with a "when in rome" approach is good (i.e. take a look
at the git log). So here something like:

    "net: stmmac: dwmac-qcom-ethqos: Enable TBS on all queues but 0"

would be better. Its a short one line subject, that has the appropriate
prefix, etc. You could then embed the information about TSO and TBS
being exclusive, and maybe explain your reasoning on why this allocation
of queues (TSO on 0, TBS on the rest) was done.. etc in the body. Maybe even
pointing to the similar NXP related patch Esben posted recently would be
smart since your motivation is the same:

    commit 3b12ec8f618ebaccfe43ea4621a6f5fb586edef8
    Author: Esben Haabendal <esben@geanix.com>
    Date:   Fri Jan 26 10:10:42 2024 +0100

        net: stmmac: dwmac-imx: set TSO/TBS TX queues default settings

        TSO and TBS cannot coexist. For now we set i.MX Ethernet QOS controller to
        use the first TX queue with TSO and the rest for TBS.

        TX queues with TBS can support etf qdisc hw offload.

        Signed-off-by: Esben Haabendal <esben@geanix.com>
        Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
        Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
        Signed-off-by: Paolo Abeni <pabeni@redhat.com>

i.e. something like (please make it your own):

    In a similar vein to 3b12ec8f618e
    ("net: stmmac: dwmac-imx: set TSO/TBS TX queues default settings"),
    let's leave TSO enabled on queue 0 and enable TBS on all other queues.
    This allows using the etf qdisc with hw offload on the TBS enabled
    queues.

On Tue, Feb 06, 2024 at 01:27:34PM -0800, Abhishek Chauhan wrote:
> TX queues with TBS can support etf qdisc hw offload.
> 
> Signed-off-by: Abhishek Chauhan <quic_abchauha@quicinc.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 31631e3f89d0..d2f9b8f6c027 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -728,7 +728,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	struct stmmac_resources stmmac_res;
>  	struct device *dev = &pdev->dev;
>  	struct qcom_ethqos *ethqos;
> -	int ret;
> +	int ret, i;
>  
>  	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
>  	if (ret)
> @@ -822,6 +822,10 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  		plat_dat->serdes_powerdown  = qcom_ethqos_serdes_powerdown;
>  	}
>  
> +	/*Enable TSO on queue0 and enable TBS on rest of the queues*/

nitpicky: Please put spaces between the comments

> +	for (i = 1; i < plat_dat->tx_queues_to_use; i++)
> +		plat_dat->tx_queues_cfg[i].tbs_en = 1;
> +
>  	return devm_stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);
>  }
>  
> -- 
> 2.25.1
> 
>