Re: [net-next,v2 3/6] ravb: Create helper to allocate skb and align it

From: Sergey Shtylyov <s.shtylyov@omp.ru>
To: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
	"David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Biju Das" <biju.das.jz@bp.renesas.com>,
	"Claudiu Beznea" <claudiu.beznea.uj@bp.renesas.com>,
	"Yoshihiro Shimoda" <yoshihiro.shimoda.uh@renesas.com>,
	netdev@vger.kernel.org
Cc: <linux-renesas-soc@vger.kernel.org>
Subject: Re: [net-next,v2 3/6] ravb: Create helper to allocate skb and align it
Date: Sun, 3 Mar 2024 12:57:20 +0300	[thread overview]
Message-ID: <5e01055d-cb63-3bb2-496b-b3222fab06fc@omp.ru> (raw)
In-Reply-To: <20240227223305.910452-4-niklas.soderlund+renesas@ragnatech.se>

On 2/28/24 1:33 AM, Niklas Söderlund wrote:

> The RAVB device requires the SKB data to be aligned to 128 bytes. The

   I prefer calling it Ether[net]AVB (like the manuals do), that "ravb" is
the driver's name... :-)

> alignment is done by allocating a skb 128 bytes larger than the maximum

   s/a/an/.

> frame size supported by the device and adjusting the headroom to fit the
> requirement.
> 
> This code has been refactored a few times and small issues have been
> added along the way. The issues are not harmful but prevents merging

   Prevent.

> parts of the Rx code which have been split in two implementations with
> the addition of RZ/G2L support, a device that supports larger frame
> sizes.
> 
> This change removes the need for duplicated and somewhat inaccurate
> hardware alignment constrains stored in the hardware information struct
> by creating a helper to handle the allocation of a skb and alignment of

   s/a/an/.

> a skb data.

   s/a/an/.

> For the R-Car class of devices the maximum frame size is 4K and each
> descriptor is limited to 2K of data. The current implementation does not
> support split descriptors, this limits the frame size to 2K. The
> current hardware information however records the descriptor size just
> under 2K due to bad understanding of the device when larger MTUs where
> added.
> 
> For the RZ/G2L device the maximum frame size is 8K and each descriptor
> is limited to 4K of data. The current hardware information records this
> correctly, but it gets the alignment constrains wrong as just aligns it
> by 128, it does not extend it by 128 bytes to allow the full frame to be
> stored. This works because the RZ/G2L device supports split descriptors
> and allocates each skb to 8K and aligns each 4K descriptor in this
> space.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 5c72b780d623..e6b025058847 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -372,7 +383,7 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
>  		rx_desc = &priv->rx_ring[q].ex_desc[i];
>  		rx_desc->ds_cc = cpu_to_le16(RX_BUF_SZ);

   Don't we need to also update this?

>  		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
> -					  RX_BUF_SZ,
> +					  priv->info->rx_max_frame_size,
>  					  DMA_FROM_DEVICE);
>  		/* We just set the data size to 0 for a failed mapping which
>  		 * should prevent DMA from happening...
[...]

MBR, Sergey