Re: [net-next,v2 6/6] ravb: Unify Rx ring maintenance code paths

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 6/6] ravb: Unify Rx ring maintenance code paths
Date: Sun, 3 Mar 2024 13:00:08 +0300	[thread overview]
Message-ID: <7fb71160-a0b5-d4e9-7c83-271754ba98a9@omp.ru> (raw)
In-Reply-To: <20240227223305.910452-7-niklas.soderlund+renesas@ragnatech.se>

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

> The R-Car and RZ/G2L Rx code paths was split in two separate

   s/was/were/.

> implementations when support for RZ/G2L was added due to the fact that
> R-Car uses the extended descriptor format while RZ/G2L uses normal
> descriptors. This has lead to a duplication of Rx logic with the only

   s/lead/led/.

> difference being the different Rx descriptors types used. The
> implementation however neglects to take into account that extended
> descriptors are normal descriptors with additional metadata at the end
> to carry hardware timestamp information.
> 
> The hardware timestamps information is only consumed in the R-Car Rx

   Timestamp, as above...

> loop and all the maintenance code around the Rx ring can be shared
> between the two implementations if the difference in descriptor length
> is carefully considered.
> 
> This change merges the two implementations for Rx ring maintenance by
> adding a method to access both types of descriptors as normal
> descriptors, as this part covers all the fields needed for Rx ring
> maintenance the only difference between using normal or extended
> descriptor is the size of the memory region to allocate/free and the
> step size between each descriptor in the ring.
> 
> 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 4ef4be9e152e..fa48ff4aba2d 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -202,6 +202,13 @@ static const struct mdiobb_ops bb_ops = {
>  	.get_mdio_data = ravb_get_mdio_data,
>  };
>  
> +static struct ravb_rx_desc *
> +ravb_rx_get_desc(struct ravb_private *priv, unsigned int q,

   Please make it 'int q' for consistency. We can change the q's type
universally later...

[...]
> @@ -202,6 +202,13 @@ static const struct mdiobb_ops bb_ops = {
>  	.get_mdio_data = ravb_get_mdio_data,
>  };
>  
> +static struct ravb_rx_desc *

   Not 'void *'?

[...]

MBR, Sergey