RE: [PATCH net-next 09/18] ravb: Factorise ravb_ring_free function

Hi Sergei,

Subject: RE: [PATCH net-next 09/18] ravb: Factorise ravb_ring_free
function

Hi Sergei,

Thanks for the feedback.

quoted

Subject: Re: [PATCH net-next 09/18] ravb: Factorise ravb_ring_free
function

Hello!

On 7/22/21 5:13 PM, Biju Das wrote:

quoted

Extended descriptor support in RX is available for R-Car where as it
is a normal descriptor for RZ/G2L. Factorise ravb_ring_free function
so that it can support later SoC.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  5 +++
 drivers/net/ethernet/renesas/ravb_main.c | 49
++++++++++++++++--------
 2 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h

b/drivers/net/ethernet/renesas/ravb.h
index a474ed68db22..3a9cf6e8671a 100644

--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h

@@ -988,7 +988,12 @@ enum ravb_chip_id {
 	RCAR_GEN3,
 };

+struct ravb_ops {
+	void (*ring_free)(struct net_device *ndev, int q);

   Hmm, why not store it right in the *struct* ravb_drv_data?

OK.

quoted

quoted

+};
+
 struct ravb_drv_data {
+	const struct ravb_ops *ravb_ops;
 	netdev_features_t net_features;
 	netdev_features_t net_hw_features;
 	const char (*gstrings_stats)[ETH_GSTRING_LEN];

diff --git a/drivers/net/ethernet/renesas/ravb_main.c

b/drivers/net/ethernet/renesas/ravb_main.c
index 4ef2565534d2..a3b8b243fd54 100644

--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c

@@ -247,30 +247,39 @@ static int ravb_tx_free(struct net_device

*ndev, int q, bool free_txed_only)  }

 /* Free skb's and DMA buffers for Ethernet AVB */ -static void
ravb_ring_free(struct net_device *ndev, int q)
+static void ravb_ring_free_rx(struct net_device *ndev, int q)

   How about ravb_rx_ring_free() instead?

Agreed.

quoted

quoted

 {
 	struct ravb_private *priv = netdev_priv(ndev);
-	int num_tx_desc = priv->num_tx_desc;
 	int ring_size;
 	int i;

-	if (priv->rx_ring[q]) {
-		for (i = 0; i < priv->num_rx_ring[q]; i++) {
-			struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
+	for (i = 0; i < priv->num_rx_ring[q]; i++) {
+		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];

-			if (!dma_mapping_error(ndev->dev.parent,
-					       le32_to_cpu(desc->dptr)))
-				dma_unmap_single(ndev->dev.parent,
-						 le32_to_cpu(desc->dptr),
-						 RX_BUF_SZ,
-						 DMA_FROM_DEVICE);
-		}
-		ring_size = sizeof(struct ravb_ex_rx_desc) *
-			    (priv->num_rx_ring[q] + 1);
-		dma_free_coherent(ndev->dev.parent, ring_size, priv-
rx_ring[q],
-				  priv->rx_desc_dma[q]);
-		priv->rx_ring[q] = NULL;
+		if (!dma_mapping_error(ndev->dev.parent,
+				       le32_to_cpu(desc->dptr)))
+			dma_unmap_single(ndev->dev.parent,
+					 le32_to_cpu(desc->dptr),
+					 RX_BUF_SZ,
+					 DMA_FROM_DEVICE);
 	}
+	ring_size = sizeof(struct ravb_ex_rx_desc) *
+		    (priv->num_rx_ring[q] + 1);
+	dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
+			  priv->rx_desc_dma[q]);
+	priv->rx_ring[q] = NULL;

   Couldn't this be moved into the new ravb_ring_free(), like the
initial NULL check?

For RZ/G2L, it is priv->rgeth_rx_ring, that is the reason.

I can move the initial NULL check here, so the generic ravb_ring_free does
not differentiate between
priv->rx_ring and priv->rgeth_rx_ring. See below.

quoted

quoted

+}
+
+static void ravb_ring_free(struct net_device *ndev, int q) {
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_drv_data *info = priv->info;
+	int num_tx_desc = priv->num_tx_desc;
+	int ring_size;
+	int i;
+
+	if (priv->rx_ring[q])
+		info->ravb_ops->ring_free(ndev, q);

   ... here?

     It will be just  info->ravb_ops->ring_free(ndev, q); And NULL check
will be handled respective rx helper function.

I will be sending next set of patches incorporating the above comments
within couple of days.

Regards,
Biju