Received-SPF: pass (google.com: domain of linux-kernel+bounces-7614-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <04e4381d-0b74-4bb4-8c53-093b99bba3ff@intel.com>
Date: Wed, 20 Dec 2023 15:00:29 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V2 08/12 net-next] qca_spi: Improve calculation of RX
 buffer size
Content-Language: en-US
To: Stefan Wahren <wahrenst@gmx.net>, "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, "Paolo
 Abeni" <pabeni@redhat.com>
CC: <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20231218232639.33327-1-wahrenst@gmx.net>
 <20231218232639.33327-9-wahrenst@gmx.net>
From: Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20231218232639.33327-9-wahrenst@gmx.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VXRjMUZFZmJlOVY0ODBIRE5aK25UUlEzRHdYUXJxaUhueGNpdzJmVUV3YXVv?=
 =?utf-8?B?S252QTg5MXkxeUtwS0lVZTVaYmI2YmhFMmdQVFRscGowUDI1WXo2ZnRJcUZS?=
 =?utf-8?B?Q1JkYVl6cjdjem1WekhYTXI3V0ltN29ILzhWR1U5d1pTWUQ1UGFBTWdDSHkr?=
 =?utf-8?B?bnZpVVpBNnVDY1M3aElEaTd3OWdxYndGWnh6eXYrY3lPMUJNUzloblZiTHZx?=
 =?utf-8?B?UkcwYjlibDVaOUd6MzRvNzBFMXo1bXFwaWIwU2QyUjdFcEdQRzcvTmxOMHBY?=
 =?utf-8?B?Y25UWGJjSVFuSTYvL1FXSk84NzdSYm1qZHp4ZnV2Q2Z0SExmajRrT1Z3SWNW?=
 =?utf-8?B?L0VrcUYyOC9CMldBaXdxK2ZjNU1XTnIzMWV3b2l5RXIxTTJjUzJwVktjRjJR?=
 =?utf-8?B?anRVd2owTEFvN2tDNW1qK2dvQ242Q1JTYTQ3QzVMRWtwODkyQ0k0Ymg5SUQz?=
 =?utf-8?B?VnR3dnNvb1Z1L3ZkOHYvOXdNek5yYjZ2MnpKNmlXbUMvblRrcm92VTZUK3pr?=
 =?utf-8?B?WFpzMHlQaEVIYmVnZURNK3BDdjJGN1A4eGQ2aTFjTXd2Qm9uZ20yd2JqeE9u?=
 =?utf-8?B?KzNaL0ZGM0ZubnY0NVRDM2p1Ky9wYWlVSmV2OTNVQ2JZZ2ZjbU0zRWttNzFu?=
 =?utf-8?B?WnR1Tnp0VnBFS1V4ZG5kdkdUKzArRzRzenJ2YzZ1bVFPRFZvS09UbUoyWXVS?=
 =?utf-8?B?bXBCZEsybDdkZlRMcDEyb3FpakluY0gwWXQxVll3ZFBTTVlOU2Ywc05DdFZW?=
 =?utf-8?B?b2MwMFc2OHdvYy9UL0JhVXdPaEVseC9DUDEyNU83VkY1T3pCRjNiTHkvMWJ0?=
 =?utf-8?B?QXB0WGFnWUNKazlxMUMzQUkxRFU2MDFOMFpaYnh6d3lGZFZrWi91ZjZVT2NF?=
 =?utf-8?B?Y0daVTFzQ2RUOG5CYXptMFFTSjAzUk5yQ0dKQURqWEdTZUx4bnhHdXlPNGFo?=
 =?utf-8?B?VFlScDlNNU5tRlZJelRsN3VZUFpIcUcrU1JMNDZ1THVNRllISzFFRWdieGFO?=
 =?utf-8?B?d1NCQTdXb1NLbTJkSHo4aC9JMzRUYnJIYWpzalNvem0valRxRktHV0xtZU02?=
 =?utf-8?B?QXA2VkgrcVN4R212Q1BLemF6YS9NNVV4Y2RCSUJoZXAvUlFlQ3lmN2NlL1NY?=
 =?utf-8?B?MjE5V1F0T1h4Z3YyQ3pka3p5SDRWcDFZNXdmdWxPQWVpL04wK2JUamRVVytD?=
 =?utf-8?B?dkhtU090SkZIRkd4VkpiUW1nU2wwNVNWR3huckFCaHZxSUIxem9KVHVZNVZq?=
 =?utf-8?B?dC8wcTJpNXJiQnVCRVYzM3ZGa2pRL0FJR3djencxeWVtYnZEMjdRWnhYWTN5?=
 =?utf-8?B?eUJDTzJ1a2ltMlJyUnYyWXYwU2xuemZ1QjNISVVpWW5xZlRINDJPL2ptUDk3?=
 =?utf-8?B?L3hCN21SUlJ5U05mTFFXYnIvTXdaYWxnOFA5L0VUTC9RbDFZSXJ1YktjWk5X?=
 =?utf-8?B?MVIwSFZlRVRvTlBDMU5VUmJKR3lOVDhLeFYxUVE3bTZyNzZXYllQdlc1M2Nm?=
 =?utf-8?B?T3dySlY4VEE2aTZMc2R3aVZYNjhiOEwwSVVCQS9FbnRSSm56cVFuKzNORndW?=
 =?utf-8?B?Z1ZRMndDbU12allkSVFKQ1YrQVJ4ZmkwQVBjQUt0OUZpNzF6VSt4QXN6bVYv?=
 =?utf-8?B?cjhnWXhOM2EyM2dScjFpT0dVcWZBZk11KytWZURCVno5cmdTNVA5OG53THIv?=
 =?utf-8?B?eW04ZmJrMnF2aGN6T1dWRCtjNU1aam5YS3c2a2dzdnduWUFveDl1RDJ6RWs0?=
 =?utf-8?B?THJIK3JwVlhaVVFib3pQRlpWdFZlSzZZZ3ZMT0QwUUlhL2hCUFpiQmpaNDM3?=
 =?utf-8?B?UEhGdHlRVTlxYkhIUVEwalFEQzcraWgvQlZ6Z1BnN0F4eVMyb0ZnN2piRTF6?=
 =?utf-8?B?amtRQTI5Q1pxSkhCZTc1MlBFMXJTVzlKU1BrL1AwTythLzQwbFlXaUNLUjVp?=
 =?utf-8?B?aFNITTFMbUVtMlYrNjc4UEx1NWdyRThuZzNJKzNDSWU2bmxrT2hDMUxrQmU3?=
 =?utf-8?B?c2dHeEF3akt4VDVtanhObGsrNGhmMHFKSlFTeWg4NTVXUTJxck04S212YmZp?=
 =?utf-8?B?bDA2VnlLTFFWY1pmVGhnWFhQSFdpQWpXcHQ5RU5PVWp6dTlQVTBFVENpTXpH?=
 =?utf-8?B?Ni9VcXNqenFXZzRkeVFKZWpWR0hqdUV3UzVHOGJoK3gyUzNjOThwZC9DRjRX?=
 =?utf-8?B?VUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7331026d-78ed-4a1b-97ab-08dc01af781c
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5095.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Dec 2023 23:00:32.7727
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: XWqw4tD2L9AtCFeAvhkYJJ4r9Te/62k48TUL53/WC2Yy7KH9NqoZk5l2/QdWhbJV2FfhJgkFv27kkfUZtecriu0gbglxXickuHwRwZYm4yY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6073
X-OriginatorOrg: intel.com


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@intel.com>

> Signed-off-by: Stefan Wahren <wahrenst@gmx.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
> 
>