netdev - Re: [PATCH V2 08/12 net-next] qca_spi: Improve calculation of RX buffer size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <04e4381d-0b74-4bb4-8c53-093b99bba3ff@intel.com>
Date: Wed, 20 Dec 2023 15:00:29 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Stefan Wahren <wahrenst@....net>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, "Paolo
 Abeni" <pabeni@...hat.com>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 08/12 net-next] qca_spi: Improve calculation of RX
 buffer size



On 12/18/2023 3:26 PM, Stefan Wahren wrote:
> There are two points with the calculation of RX buffer size which are
> not optimal:
> 1. mtu is a mutual parameter, but actually we need the maximum possible
>    MTU. So better use the define directly.
> 2. This magic number 4 represent the hardware generated frame length
>    which is specific to SPI. We better replace this with the suitable
>    define.
> 
> There is no functional change.
> 

This is a bit confusing why you said no functional change. At first my
thought was that dev->mtu could easily be smaller than the maximum MTU.
(thats why its configurable after all?). But actually the diff context
cuts off a few lines that are extremely relevant:

>         struct qcaspi *qca = netdev_priv(dev);
> 
>         dev->mtu = QCAFRM_MAX_MTU;

We assign dev->mtu here to QCAFRM_MAX_MTU

>         dev->type = ARPHRD_ETHER;
>         qca->clkspeed = qcaspi_clkspeed;
>         qca->burst_len = qcaspi_burst_len;
>         qca->spi_thread = NULL;
>         qca->buffer_size = (dev->mtu + VLAN_ETH_HLEN + QCAFRM_HEADER_LEN +
>                 QCAFRM_FOOTER_LEN + 4) * 4;

And then use it here.

> 
>         memset(&qca->stats, 0, sizeof(struct qcaspi_stats));
> 

Makes sense to use the constant directly.

It might have helped to have a bit more of this context in the commit
message, but its still a good cleanup.

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>

> Signed-off-by: Stefan Wahren <wahrenst@....net>
> ---
>  drivers/net/ethernet/qualcomm/qca_spi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c
> index 80182f8a4a50..6d2859aad921 100644
> --- a/drivers/net/ethernet/qualcomm/qca_spi.c
> +++ b/drivers/net/ethernet/qualcomm/qca_spi.c
> @@ -813,8 +813,8 @@ qcaspi_netdev_init(struct net_device *dev)
>  	qca->clkspeed = qcaspi_clkspeed;
>  	qca->burst_len = qcaspi_burst_len;
>  	qca->spi_thread = NULL;
> -	qca->buffer_size = (dev->mtu + VLAN_ETH_HLEN + QCAFRM_HEADER_LEN +
> -		QCAFRM_FOOTER_LEN + 4) * QCASPI_RX_MAX_FRAMES;
> +	qca->buffer_size = (QCAFRM_MAX_MTU + VLAN_ETH_HLEN + QCAFRM_HEADER_LEN +
> +		QCAFRM_FOOTER_LEN + QCASPI_HW_PKT_LEN) * QCASPI_RX_MAX_FRAMES;
> 
>  	memset(&qca->stats, 0, sizeof(struct qcaspi_stats));
> 
> --
> 2.34.1
> 
>