To reduce code duplication, we add a new RX ring refill function which
can handle both the initial RX ring population (which was split between
ravb_ring_init() and ravb_ring_format()) and the RX ring refill after
polling (in ravb_rx()).
Signed-off-by: Paul Barker <[email protected]>
---
drivers/net/ethernet/renesas/ravb_main.c | 141 +++++++++--------------
1 file changed, 52 insertions(+), 89 deletions(-)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 1ac599a044b2..baa01bd81f2d 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -317,35 +317,42 @@ static void ravb_ring_free(struct net_device *ndev, int q)
priv->tx_skb[q] = NULL;
}
-static void ravb_rx_ring_format(struct net_device *ndev, int q)
+static u32
+ravb_rx_ring_refill(struct net_device *ndev, int q, u32 count, gfp_t gfp_mask)
{
struct ravb_private *priv = netdev_priv(ndev);
+ const struct ravb_hw_info *info = priv->info;
struct ravb_rx_desc *rx_desc;
- unsigned int rx_ring_size;
dma_addr_t dma_addr;
- unsigned int i;
+ u32 i, entry;
- rx_ring_size = priv->info->rx_desc_size * priv->num_rx_ring[q];
- memset(priv->rx_ring[q].raw, 0, rx_ring_size);
- /* Build RX ring buffer */
- for (i = 0; i < priv->num_rx_ring[q]; i++) {
- /* RX descriptor */
- rx_desc = ravb_rx_get_desc(priv, q, i);
- rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
- dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
- 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...
- */
- if (dma_mapping_error(ndev->dev.parent, dma_addr))
- rx_desc->ds_cc = cpu_to_le16(0);
- rx_desc->dptr = cpu_to_le32(dma_addr);
+ for (i = 0; i < count; i++) {
+ entry = (priv->dirty_rx[q] + i) % priv->num_rx_ring[q];
+ rx_desc = ravb_rx_get_desc(priv, q, entry);
+ rx_desc->ds_cc = cpu_to_le16(info->rx_max_desc_use);
+
+ if (!priv->rx_skb[q][entry]) {
+ priv->rx_skb[q][entry] = ravb_alloc_skb(ndev, info, gfp_mask);
+ if (!priv->rx_skb[q][entry])
+ break;
+ dma_addr = dma_map_single(ndev->dev.parent,
+ priv->rx_skb[q][entry]->data,
+ priv->info->rx_max_frame_size,
+ DMA_FROM_DEVICE);
+ skb_checksum_none_assert(priv->rx_skb[q][entry]);
+ /* We just set the data size to 0 for a failed mapping
+ * which should prevent DMA from happening...
+ */
+ if (dma_mapping_error(ndev->dev.parent, dma_addr))
+ rx_desc->ds_cc = cpu_to_le16(0);
+ rx_desc->dptr = cpu_to_le32(dma_addr);
+ }
+ /* Descriptor type must be set after all the above writes */
+ dma_wmb();
rx_desc->die_dt = DT_FEMPTY;
}
- rx_desc = ravb_rx_get_desc(priv, q, i);
- rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
- rx_desc->die_dt = DT_LINKFIX; /* type */
+
+ return i;
}
/* Format skb and descriptor buffer for Ethernet AVB */
@@ -353,6 +360,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
{
struct ravb_private *priv = netdev_priv(ndev);
unsigned int num_tx_desc = priv->num_tx_desc;
+ struct ravb_rx_desc *rx_desc;
struct ravb_tx_desc *tx_desc;
struct ravb_desc *desc;
unsigned int tx_ring_size = sizeof(*tx_desc) * priv->num_tx_ring[q] *
@@ -364,8 +372,6 @@ static void ravb_ring_format(struct net_device *ndev, int q)
priv->dirty_rx[q] = 0;
priv->dirty_tx[q] = 0;
- ravb_rx_ring_format(ndev, q);
-
memset(priv->tx_ring[q], 0, tx_ring_size);
/* Build TX ring buffer */
for (i = 0, tx_desc = priv->tx_ring[q]; i < priv->num_tx_ring[q];
@@ -379,6 +385,14 @@ static void ravb_ring_format(struct net_device *ndev, int q)
tx_desc->dptr = cpu_to_le32((u32)priv->tx_desc_dma[q]);
tx_desc->die_dt = DT_LINKFIX; /* type */
+ /* Regular RX descriptors have already been initialized by
+ * ravb_rx_ring_refill(), we just need to initialize the final link
+ * descriptor.
+ */
+ rx_desc = ravb_rx_get_desc(priv, q, priv->num_rx_ring[q]);
+ rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
+ rx_desc->die_dt = DT_LINKFIX; /* type */
+
/* RX descriptor base address for best effort */
desc = &priv->desc_bat[RX_QUEUE_OFFSET + q];
desc->die_dt = DT_LINKFIX; /* type */
@@ -408,11 +422,9 @@ static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)
static int ravb_ring_init(struct net_device *ndev, int q)
{
struct ravb_private *priv = netdev_priv(ndev);
- const struct ravb_hw_info *info = priv->info;
unsigned int num_tx_desc = priv->num_tx_desc;
unsigned int ring_size;
- struct sk_buff *skb;
- unsigned int i;
+ u32 num_filled;
/* Allocate RX and TX skb rings */
priv->rx_skb[q] = kcalloc(priv->num_rx_ring[q],
@@ -422,13 +434,6 @@ static int ravb_ring_init(struct net_device *ndev, int q)
if (!priv->rx_skb[q] || !priv->tx_skb[q])
goto error;
- for (i = 0; i < priv->num_rx_ring[q]; i++) {
- skb = ravb_alloc_skb(ndev, info, GFP_KERNEL);
- if (!skb)
- goto error;
- priv->rx_skb[q][i] = skb;
- }
-
if (num_tx_desc > 1) {
/* Allocate rings for the aligned buffers */
priv->tx_align[q] = kmalloc(DPTR_ALIGN * priv->num_tx_ring[q] +
@@ -443,6 +448,13 @@ static int ravb_ring_init(struct net_device *ndev, int q)
priv->dirty_rx[q] = 0;
+ /* Populate RX ring buffer. */
+ ring_size = priv->info->rx_desc_size * priv->num_rx_ring[q];
+ memset(priv->rx_ring[q].raw, 0, ring_size);
+ num_filled = ravb_rx_ring_refill(ndev, q, priv->num_rx_ring[q], GFP_KERNEL);
+ if (num_filled != priv->num_rx_ring[q])
+ goto error;
+
/* Allocate all TX descriptors. */
ring_size = sizeof(struct ravb_tx_desc) *
(priv->num_tx_ring[q] * num_tx_desc + 1);
@@ -762,11 +774,9 @@ static struct sk_buff *ravb_get_skb_gbeth(struct net_device *ndev, int entry,
static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
{
struct ravb_private *priv = netdev_priv(ndev);
- const struct ravb_hw_info *info = priv->info;
struct net_device_stats *stats;
struct ravb_rx_desc *desc;
struct sk_buff *skb;
- dma_addr_t dma_addr;
int rx_packets = 0;
u8 desc_status;
u16 desc_len;
@@ -854,32 +864,9 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
}
/* Refill the RX ring buffers. */
- for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
- entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
- desc = &priv->rx_ring[q].desc[entry];
- desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
-
- if (!priv->rx_skb[q][entry]) {
- skb = ravb_alloc_skb(ndev, info, GFP_ATOMIC);
- if (!skb)
- break;
- dma_addr = dma_map_single(ndev->dev.parent,
- skb->data,
- priv->info->rx_max_frame_size,
- DMA_FROM_DEVICE);
- skb_checksum_none_assert(skb);
- /* We just set the data size to 0 for a failed mapping
- * which should prevent DMA from happening...
- */
- if (dma_mapping_error(ndev->dev.parent, dma_addr))
- desc->ds_cc = cpu_to_le16(0);
- desc->dptr = cpu_to_le32(dma_addr);
- priv->rx_skb[q][entry] = skb;
- }
- /* Descriptor type must be set after all the above writes */
- dma_wmb();
- desc->die_dt = DT_FEMPTY;
- }
+ priv->dirty_rx[q] += ravb_rx_ring_refill(ndev, q,
+ priv->cur_rx[q] - priv->dirty_rx[q],
+ GFP_ATOMIC);
return rx_packets;
}
@@ -888,11 +875,9 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
{
struct ravb_private *priv = netdev_priv(ndev);
- const struct ravb_hw_info *info = priv->info;
struct net_device_stats *stats = &priv->stats[q];
struct ravb_ex_rx_desc *desc;
struct sk_buff *skb;
- dma_addr_t dma_addr;
struct timespec64 ts;
int rx_packets = 0;
u8 desc_status;
@@ -964,31 +949,9 @@ static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
}
/* Refill the RX ring buffers. */
- for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
- entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
- desc = &priv->rx_ring[q].ex_desc[entry];
- desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
-
- if (!priv->rx_skb[q][entry]) {
- skb = ravb_alloc_skb(ndev, info, GFP_ATOMIC);
- if (!skb)
- break; /* Better luck next round. */
- dma_addr = dma_map_single(ndev->dev.parent, skb->data,
- priv->info->rx_max_frame_size,
- DMA_FROM_DEVICE);
- skb_checksum_none_assert(skb);
- /* We just set the data size to 0 for a failed mapping
- * which should prevent DMA from happening...
- */
- if (dma_mapping_error(ndev->dev.parent, dma_addr))
- desc->ds_cc = cpu_to_le16(0);
- desc->dptr = cpu_to_le32(dma_addr);
- priv->rx_skb[q][entry] = skb;
- }
- /* Descriptor type must be set after all the above writes */
- dma_wmb();
- desc->die_dt = DT_FEMPTY;
- }
+ priv->dirty_rx[q] += ravb_rx_ring_refill(ndev, q,
+ priv->cur_rx[q] - priv->dirty_rx[q],
+ GFP_ATOMIC);
return rx_packets;
}
--
2.39.2
Hi Paul,
Thanks for your work, I really like this deduplication of code!
On 2024-04-15 10:48:00 +0100, Paul Barker wrote:
> To reduce code duplication, we add a new RX ring refill function which
> can handle both the initial RX ring population (which was split between
> ravb_ring_init() and ravb_ring_format()) and the RX ring refill after
> polling (in ravb_rx()).
>
> Signed-off-by: Paul Barker <[email protected]>
> ---
> drivers/net/ethernet/renesas/ravb_main.c | 141 +++++++++--------------
> 1 file changed, 52 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 1ac599a044b2..baa01bd81f2d 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -317,35 +317,42 @@ static void ravb_ring_free(struct net_device *ndev, int q)
> priv->tx_skb[q] = NULL;
> }
>
> -static void ravb_rx_ring_format(struct net_device *ndev, int q)
> +static u32
> +ravb_rx_ring_refill(struct net_device *ndev, int q, u32 count, gfp_t gfp_mask)
> {
> struct ravb_private *priv = netdev_priv(ndev);
> + const struct ravb_hw_info *info = priv->info;
> struct ravb_rx_desc *rx_desc;
> - unsigned int rx_ring_size;
> dma_addr_t dma_addr;
> - unsigned int i;
> + u32 i, entry;
>
> - rx_ring_size = priv->info->rx_desc_size * priv->num_rx_ring[q];
> - memset(priv->rx_ring[q].raw, 0, rx_ring_size);
> - /* Build RX ring buffer */
> - for (i = 0; i < priv->num_rx_ring[q]; i++) {
> - /* RX descriptor */
> - rx_desc = ravb_rx_get_desc(priv, q, i);
> - rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
> - dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
> - 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...
> - */
> - if (dma_mapping_error(ndev->dev.parent, dma_addr))
> - rx_desc->ds_cc = cpu_to_le16(0);
> - rx_desc->dptr = cpu_to_le32(dma_addr);
> + for (i = 0; i < count; i++) {
> + entry = (priv->dirty_rx[q] + i) % priv->num_rx_ring[q];
> + rx_desc = ravb_rx_get_desc(priv, q, entry);
> + rx_desc->ds_cc = cpu_to_le16(info->rx_max_desc_use);
> +
> + if (!priv->rx_skb[q][entry]) {
> + priv->rx_skb[q][entry] = ravb_alloc_skb(ndev, info, gfp_mask);
> + if (!priv->rx_skb[q][entry])
> + break;
> + dma_addr = dma_map_single(ndev->dev.parent,
> + priv->rx_skb[q][entry]->data,
> + priv->info->rx_max_frame_size,
> + DMA_FROM_DEVICE);
> + skb_checksum_none_assert(priv->rx_skb[q][entry]);
> + /* We just set the data size to 0 for a failed mapping
> + * which should prevent DMA from happening...
> + */
> + if (dma_mapping_error(ndev->dev.parent, dma_addr))
> + rx_desc->ds_cc = cpu_to_le16(0);
> + rx_desc->dptr = cpu_to_le32(dma_addr);
> + }
> + /* Descriptor type must be set after all the above writes */
> + dma_wmb();
> rx_desc->die_dt = DT_FEMPTY;
> }
> - rx_desc = ravb_rx_get_desc(priv, q, i);
> - rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
> - rx_desc->die_dt = DT_LINKFIX; /* type */
> +
> + return i;
> }
>
> /* Format skb and descriptor buffer for Ethernet AVB */
> @@ -353,6 +360,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
> {
> struct ravb_private *priv = netdev_priv(ndev);
> unsigned int num_tx_desc = priv->num_tx_desc;
> + struct ravb_rx_desc *rx_desc;
> struct ravb_tx_desc *tx_desc;
> struct ravb_desc *desc;
> unsigned int tx_ring_size = sizeof(*tx_desc) * priv->num_tx_ring[q] *
> @@ -364,8 +372,6 @@ static void ravb_ring_format(struct net_device *ndev, int q)
> priv->dirty_rx[q] = 0;
> priv->dirty_tx[q] = 0;
>
> - ravb_rx_ring_format(ndev, q);
> -
> memset(priv->tx_ring[q], 0, tx_ring_size);
> /* Build TX ring buffer */
> for (i = 0, tx_desc = priv->tx_ring[q]; i < priv->num_tx_ring[q];
> @@ -379,6 +385,14 @@ static void ravb_ring_format(struct net_device *ndev, int q)
> tx_desc->dptr = cpu_to_le32((u32)priv->tx_desc_dma[q]);
> tx_desc->die_dt = DT_LINKFIX; /* type */
>
> + /* Regular RX descriptors have already been initialized by
> + * ravb_rx_ring_refill(), we just need to initialize the final link
> + * descriptor.
> + */
> + rx_desc = ravb_rx_get_desc(priv, q, priv->num_rx_ring[q]);
> + rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
> + rx_desc->die_dt = DT_LINKFIX; /* type */
> +
super-nit: Should you not move this addition up to where you removed the
call to ravb_rx_ring_format()? Before this change the order of things
are,
/* Init RX ring */
/* Init TX ring */
/* Set RX descriptor base address */
/* Set TX descriptor base address */
While after it is,
/* Init TX ring */
/* Init RX ring */
/* Set RX descriptor base address */
/* Set TX descriptor base address */
My OCD is itching ;-)
> /* RX descriptor base address for best effort */
> desc = &priv->desc_bat[RX_QUEUE_OFFSET + q];
> desc->die_dt = DT_LINKFIX; /* type */
> @@ -408,11 +422,9 @@ static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)
> static int ravb_ring_init(struct net_device *ndev, int q)
> {
> struct ravb_private *priv = netdev_priv(ndev);
> - const struct ravb_hw_info *info = priv->info;
> unsigned int num_tx_desc = priv->num_tx_desc;
> unsigned int ring_size;
> - struct sk_buff *skb;
> - unsigned int i;
> + u32 num_filled;
>
> /* Allocate RX and TX skb rings */
> priv->rx_skb[q] = kcalloc(priv->num_rx_ring[q],
> @@ -422,13 +434,6 @@ static int ravb_ring_init(struct net_device *ndev, int q)
> if (!priv->rx_skb[q] || !priv->tx_skb[q])
> goto error;
>
> - for (i = 0; i < priv->num_rx_ring[q]; i++) {
> - skb = ravb_alloc_skb(ndev, info, GFP_KERNEL);
> - if (!skb)
> - goto error;
> - priv->rx_skb[q][i] = skb;
> - }
> -
> if (num_tx_desc > 1) {
> /* Allocate rings for the aligned buffers */
> priv->tx_align[q] = kmalloc(DPTR_ALIGN * priv->num_tx_ring[q] +
> @@ -443,6 +448,13 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>
> priv->dirty_rx[q] = 0;
>
> + /* Populate RX ring buffer. */
> + ring_size = priv->info->rx_desc_size * priv->num_rx_ring[q];
> + memset(priv->rx_ring[q].raw, 0, ring_size);
> + num_filled = ravb_rx_ring_refill(ndev, q, priv->num_rx_ring[q], GFP_KERNEL);
> + if (num_filled != priv->num_rx_ring[q])
> + goto error;
> +
Here you also change the order, but it make sense here as you first deal
with all TX and then all RX ;-)
> /* Allocate all TX descriptors. */
> ring_size = sizeof(struct ravb_tx_desc) *
> (priv->num_tx_ring[q] * num_tx_desc + 1);
> @@ -762,11 +774,9 @@ static struct sk_buff *ravb_get_skb_gbeth(struct net_device *ndev, int entry,
> static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
> {
> struct ravb_private *priv = netdev_priv(ndev);
> - const struct ravb_hw_info *info = priv->info;
> struct net_device_stats *stats;
> struct ravb_rx_desc *desc;
> struct sk_buff *skb;
> - dma_addr_t dma_addr;
> int rx_packets = 0;
> u8 desc_status;
> u16 desc_len;
> @@ -854,32 +864,9 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
> }
>
> /* Refill the RX ring buffers. */
> - for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
> - entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
> - desc = &priv->rx_ring[q].desc[entry];
> - desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
> -
> - if (!priv->rx_skb[q][entry]) {
> - skb = ravb_alloc_skb(ndev, info, GFP_ATOMIC);
> - if (!skb)
> - break;
> - dma_addr = dma_map_single(ndev->dev.parent,
> - skb->data,
> - priv->info->rx_max_frame_size,
> - DMA_FROM_DEVICE);
> - skb_checksum_none_assert(skb);
> - /* We just set the data size to 0 for a failed mapping
> - * which should prevent DMA from happening...
> - */
> - if (dma_mapping_error(ndev->dev.parent, dma_addr))
> - desc->ds_cc = cpu_to_le16(0);
> - desc->dptr = cpu_to_le32(dma_addr);
> - priv->rx_skb[q][entry] = skb;
> - }
> - /* Descriptor type must be set after all the above writes */
> - dma_wmb();
> - desc->die_dt = DT_FEMPTY;
> - }
> + priv->dirty_rx[q] += ravb_rx_ring_refill(ndev, q,
> + priv->cur_rx[q] - priv->dirty_rx[q],
> + GFP_ATOMIC);
>
> return rx_packets;
> }
> @@ -888,11 +875,9 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
> static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
> {
> struct ravb_private *priv = netdev_priv(ndev);
> - const struct ravb_hw_info *info = priv->info;
> struct net_device_stats *stats = &priv->stats[q];
> struct ravb_ex_rx_desc *desc;
> struct sk_buff *skb;
> - dma_addr_t dma_addr;
> struct timespec64 ts;
> int rx_packets = 0;
> u8 desc_status;
> @@ -964,31 +949,9 @@ static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
> }
>
> /* Refill the RX ring buffers. */
> - for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
> - entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
> - desc = &priv->rx_ring[q].ex_desc[entry];
> - desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
> -
> - if (!priv->rx_skb[q][entry]) {
> - skb = ravb_alloc_skb(ndev, info, GFP_ATOMIC);
> - if (!skb)
> - break; /* Better luck next round. */
> - dma_addr = dma_map_single(ndev->dev.parent, skb->data,
> - priv->info->rx_max_frame_size,
> - DMA_FROM_DEVICE);
> - skb_checksum_none_assert(skb);
> - /* We just set the data size to 0 for a failed mapping
> - * which should prevent DMA from happening...
> - */
> - if (dma_mapping_error(ndev->dev.parent, dma_addr))
> - desc->ds_cc = cpu_to_le16(0);
> - desc->dptr = cpu_to_le32(dma_addr);
> - priv->rx_skb[q][entry] = skb;
> - }
> - /* Descriptor type must be set after all the above writes */
> - dma_wmb();
> - desc->die_dt = DT_FEMPTY;
> - }
> + priv->dirty_rx[q] += ravb_rx_ring_refill(ndev, q,
> + priv->cur_rx[q] - priv->dirty_rx[q],
> + GFP_ATOMIC);
>
> return rx_packets;
> }
> --
> 2.39.2
>
--
Kind Regards,
Niklas Söderlund
On 15/04/2024 12:57, Niklas Söderlund wrote:
> Hi Paul,
>
> Thanks for your work, I really like this deduplication of code!
>
> On 2024-04-15 10:48:00 +0100, Paul Barker wrote:
>> To reduce code duplication, we add a new RX ring refill function which
>> can handle both the initial RX ring population (which was split between
>> ravb_ring_init() and ravb_ring_format()) and the RX ring refill after
>> polling (in ravb_rx()).
>>
>> Signed-off-by: Paul Barker <[email protected]>
>> ---
>> drivers/net/ethernet/renesas/ravb_main.c | 141 +++++++++--------------
>> 1 file changed, 52 insertions(+), 89 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 1ac599a044b2..baa01bd81f2d 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -317,35 +317,42 @@ static void ravb_ring_free(struct net_device *ndev, int q)
>> priv->tx_skb[q] = NULL;
>> }
>>
>> -static void ravb_rx_ring_format(struct net_device *ndev, int q)
>> +static u32
>> +ravb_rx_ring_refill(struct net_device *ndev, int q, u32 count, gfp_t gfp_mask)
>> {
>> struct ravb_private *priv = netdev_priv(ndev);
>> + const struct ravb_hw_info *info = priv->info;
>> struct ravb_rx_desc *rx_desc;
>> - unsigned int rx_ring_size;
>> dma_addr_t dma_addr;
>> - unsigned int i;
>> + u32 i, entry;
>>
>> - rx_ring_size = priv->info->rx_desc_size * priv->num_rx_ring[q];
>> - memset(priv->rx_ring[q].raw, 0, rx_ring_size);
>> - /* Build RX ring buffer */
>> - for (i = 0; i < priv->num_rx_ring[q]; i++) {
>> - /* RX descriptor */
>> - rx_desc = ravb_rx_get_desc(priv, q, i);
>> - rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
>> - dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
>> - 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...
>> - */
>> - if (dma_mapping_error(ndev->dev.parent, dma_addr))
>> - rx_desc->ds_cc = cpu_to_le16(0);
>> - rx_desc->dptr = cpu_to_le32(dma_addr);
>> + for (i = 0; i < count; i++) {
>> + entry = (priv->dirty_rx[q] + i) % priv->num_rx_ring[q];
>> + rx_desc = ravb_rx_get_desc(priv, q, entry);
>> + rx_desc->ds_cc = cpu_to_le16(info->rx_max_desc_use);
>> +
>> + if (!priv->rx_skb[q][entry]) {
>> + priv->rx_skb[q][entry] = ravb_alloc_skb(ndev, info, gfp_mask);
>> + if (!priv->rx_skb[q][entry])
>> + break;
>> + dma_addr = dma_map_single(ndev->dev.parent,
>> + priv->rx_skb[q][entry]->data,
>> + priv->info->rx_max_frame_size,
>> + DMA_FROM_DEVICE);
>> + skb_checksum_none_assert(priv->rx_skb[q][entry]);
>> + /* We just set the data size to 0 for a failed mapping
>> + * which should prevent DMA from happening...
>> + */
>> + if (dma_mapping_error(ndev->dev.parent, dma_addr))
>> + rx_desc->ds_cc = cpu_to_le16(0);
>> + rx_desc->dptr = cpu_to_le32(dma_addr);
>> + }
>> + /* Descriptor type must be set after all the above writes */
>> + dma_wmb();
>> rx_desc->die_dt = DT_FEMPTY;
>> }
>> - rx_desc = ravb_rx_get_desc(priv, q, i);
>> - rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
>> - rx_desc->die_dt = DT_LINKFIX; /* type */
>> +
>> + return i;
>> }
>>
>> /* Format skb and descriptor buffer for Ethernet AVB */
>> @@ -353,6 +360,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>> {
>> struct ravb_private *priv = netdev_priv(ndev);
>> unsigned int num_tx_desc = priv->num_tx_desc;
>> + struct ravb_rx_desc *rx_desc;
>> struct ravb_tx_desc *tx_desc;
>> struct ravb_desc *desc;
>> unsigned int tx_ring_size = sizeof(*tx_desc) * priv->num_tx_ring[q] *
>> @@ -364,8 +372,6 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>> priv->dirty_rx[q] = 0;
>> priv->dirty_tx[q] = 0;
>>
>> - ravb_rx_ring_format(ndev, q);
>> -
>> memset(priv->tx_ring[q], 0, tx_ring_size);
>> /* Build TX ring buffer */
>> for (i = 0, tx_desc = priv->tx_ring[q]; i < priv->num_tx_ring[q];
>> @@ -379,6 +385,14 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>> tx_desc->dptr = cpu_to_le32((u32)priv->tx_desc_dma[q]);
>> tx_desc->die_dt = DT_LINKFIX; /* type */
>>
>> + /* Regular RX descriptors have already been initialized by
>> + * ravb_rx_ring_refill(), we just need to initialize the final link
>> + * descriptor.
>> + */
>> + rx_desc = ravb_rx_get_desc(priv, q, priv->num_rx_ring[q]);
>> + rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
>> + rx_desc->die_dt = DT_LINKFIX; /* type */
>> +
>
> super-nit: Should you not move this addition up to where you removed the
> call to ravb_rx_ring_format()? Before this change the order of things
> are,
>
> /* Init RX ring */
> /* Init TX ring */
> /* Set RX descriptor base address */
> /* Set TX descriptor base address */
>
>
> While after it is,
>
> /* Init TX ring */
> /* Init RX ring */
> /* Set RX descriptor base address */
> /* Set TX descriptor base address */
>
> My OCD is itching ;-)
Since I'll need to re-spin this series anyway, I may as well tidy that
up :)
>
>> /* RX descriptor base address for best effort */
>> desc = &priv->desc_bat[RX_QUEUE_OFFSET + q];
>> desc->die_dt = DT_LINKFIX; /* type */
>> @@ -408,11 +422,9 @@ static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)
>> static int ravb_ring_init(struct net_device *ndev, int q)
>> {
>> struct ravb_private *priv = netdev_priv(ndev);
>> - const struct ravb_hw_info *info = priv->info;
>> unsigned int num_tx_desc = priv->num_tx_desc;
>> unsigned int ring_size;
>> - struct sk_buff *skb;
>> - unsigned int i;
>> + u32 num_filled;
>>
>> /* Allocate RX and TX skb rings */
>> priv->rx_skb[q] = kcalloc(priv->num_rx_ring[q],
>> @@ -422,13 +434,6 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>> if (!priv->rx_skb[q] || !priv->tx_skb[q])
>> goto error;
>>
>> - for (i = 0; i < priv->num_rx_ring[q]; i++) {
>> - skb = ravb_alloc_skb(ndev, info, GFP_KERNEL);
>> - if (!skb)
>> - goto error;
>> - priv->rx_skb[q][i] = skb;
>> - }
>> -
>> if (num_tx_desc > 1) {
>> /* Allocate rings for the aligned buffers */
>> priv->tx_align[q] = kmalloc(DPTR_ALIGN * priv->num_tx_ring[q] +
>> @@ -443,6 +448,13 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>>
>> priv->dirty_rx[q] = 0;
>>
>> + /* Populate RX ring buffer. */
>> + ring_size = priv->info->rx_desc_size * priv->num_rx_ring[q];
>> + memset(priv->rx_ring[q].raw, 0, ring_size);
>> + num_filled = ravb_rx_ring_refill(ndev, q, priv->num_rx_ring[q], GFP_KERNEL);
>> + if (num_filled != priv->num_rx_ring[q])
>> + goto error;
>> +
>
> Here you also change the order, but it make sense here as you first deal
> with all TX and then all RX ;-)
The placement here is because we can't call ravb_rx_ring_refill() until
priv->dirty_rx[q] has been zero'd.
The init order right now is actually:
RX page pool
RX buffers
TX SKBs
RX descriptors
RX ring buffer
TX descriptors
So maybe this should be re-ordered.
I considered breaking this all apart, so ravb_ring_init() would call
ravb_rx_ring_init()/ravb_tx_ring_init() and ravb_ring_format() would
call ravb_rx_ring_format()/ravb_tx_ring_format(). There are several
steps happening for TX & RX in both init and format stages. Does that
sound cleaner to you?
Thanks,
--
Paul Barker
On 2024-04-15 13:18:08 +0100, Paul Barker wrote:
> On 15/04/2024 12:57, Niklas Söderlund wrote:
> > Hi Paul,
> >
> > Thanks for your work, I really like this deduplication of code!
> >
> > On 2024-04-15 10:48:00 +0100, Paul Barker wrote:
> >> To reduce code duplication, we add a new RX ring refill function which
> >> can handle both the initial RX ring population (which was split between
> >> ravb_ring_init() and ravb_ring_format()) and the RX ring refill after
> >> polling (in ravb_rx()).
> >>
> >> Signed-off-by: Paul Barker <[email protected]>
> >> ---
> >> drivers/net/ethernet/renesas/ravb_main.c | 141 +++++++++--------------
> >> 1 file changed, 52 insertions(+), 89 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >> index 1ac599a044b2..baa01bd81f2d 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -317,35 +317,42 @@ static void ravb_ring_free(struct net_device *ndev, int q)
> >> priv->tx_skb[q] = NULL;
> >> }
> >>
> >> -static void ravb_rx_ring_format(struct net_device *ndev, int q)
> >> +static u32
> >> +ravb_rx_ring_refill(struct net_device *ndev, int q, u32 count, gfp_t gfp_mask)
> >> {
> >> struct ravb_private *priv = netdev_priv(ndev);
> >> + const struct ravb_hw_info *info = priv->info;
> >> struct ravb_rx_desc *rx_desc;
> >> - unsigned int rx_ring_size;
> >> dma_addr_t dma_addr;
> >> - unsigned int i;
> >> + u32 i, entry;
> >>
> >> - rx_ring_size = priv->info->rx_desc_size * priv->num_rx_ring[q];
> >> - memset(priv->rx_ring[q].raw, 0, rx_ring_size);
> >> - /* Build RX ring buffer */
> >> - for (i = 0; i < priv->num_rx_ring[q]; i++) {
> >> - /* RX descriptor */
> >> - rx_desc = ravb_rx_get_desc(priv, q, i);
> >> - rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
> >> - dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
> >> - 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...
> >> - */
> >> - if (dma_mapping_error(ndev->dev.parent, dma_addr))
> >> - rx_desc->ds_cc = cpu_to_le16(0);
> >> - rx_desc->dptr = cpu_to_le32(dma_addr);
> >> + for (i = 0; i < count; i++) {
> >> + entry = (priv->dirty_rx[q] + i) % priv->num_rx_ring[q];
> >> + rx_desc = ravb_rx_get_desc(priv, q, entry);
> >> + rx_desc->ds_cc = cpu_to_le16(info->rx_max_desc_use);
> >> +
> >> + if (!priv->rx_skb[q][entry]) {
> >> + priv->rx_skb[q][entry] = ravb_alloc_skb(ndev, info, gfp_mask);
> >> + if (!priv->rx_skb[q][entry])
> >> + break;
> >> + dma_addr = dma_map_single(ndev->dev.parent,
> >> + priv->rx_skb[q][entry]->data,
> >> + priv->info->rx_max_frame_size,
> >> + DMA_FROM_DEVICE);
> >> + skb_checksum_none_assert(priv->rx_skb[q][entry]);
> >> + /* We just set the data size to 0 for a failed mapping
> >> + * which should prevent DMA from happening...
> >> + */
> >> + if (dma_mapping_error(ndev->dev.parent, dma_addr))
> >> + rx_desc->ds_cc = cpu_to_le16(0);
> >> + rx_desc->dptr = cpu_to_le32(dma_addr);
> >> + }
> >> + /* Descriptor type must be set after all the above writes */
> >> + dma_wmb();
> >> rx_desc->die_dt = DT_FEMPTY;
> >> }
> >> - rx_desc = ravb_rx_get_desc(priv, q, i);
> >> - rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
> >> - rx_desc->die_dt = DT_LINKFIX; /* type */
> >> +
> >> + return i;
> >> }
> >>
> >> /* Format skb and descriptor buffer for Ethernet AVB */
> >> @@ -353,6 +360,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
> >> {
> >> struct ravb_private *priv = netdev_priv(ndev);
> >> unsigned int num_tx_desc = priv->num_tx_desc;
> >> + struct ravb_rx_desc *rx_desc;
> >> struct ravb_tx_desc *tx_desc;
> >> struct ravb_desc *desc;
> >> unsigned int tx_ring_size = sizeof(*tx_desc) * priv->num_tx_ring[q] *
> >> @@ -364,8 +372,6 @@ static void ravb_ring_format(struct net_device *ndev, int q)
> >> priv->dirty_rx[q] = 0;
> >> priv->dirty_tx[q] = 0;
> >>
> >> - ravb_rx_ring_format(ndev, q);
> >> -
> >> memset(priv->tx_ring[q], 0, tx_ring_size);
> >> /* Build TX ring buffer */
> >> for (i = 0, tx_desc = priv->tx_ring[q]; i < priv->num_tx_ring[q];
> >> @@ -379,6 +385,14 @@ static void ravb_ring_format(struct net_device *ndev, int q)
> >> tx_desc->dptr = cpu_to_le32((u32)priv->tx_desc_dma[q]);
> >> tx_desc->die_dt = DT_LINKFIX; /* type */
> >>
> >> + /* Regular RX descriptors have already been initialized by
> >> + * ravb_rx_ring_refill(), we just need to initialize the final link
> >> + * descriptor.
> >> + */
> >> + rx_desc = ravb_rx_get_desc(priv, q, priv->num_rx_ring[q]);
> >> + rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
> >> + rx_desc->die_dt = DT_LINKFIX; /* type */
> >> +
> >
> > super-nit: Should you not move this addition up to where you removed the
> > call to ravb_rx_ring_format()? Before this change the order of things
> > are,
> >
> > /* Init RX ring */
> > /* Init TX ring */
> > /* Set RX descriptor base address */
> > /* Set TX descriptor base address */
> >
> >
> > While after it is,
> >
> > /* Init TX ring */
> > /* Init RX ring */
> > /* Set RX descriptor base address */
> > /* Set TX descriptor base address */
> >
> > My OCD is itching ;-)
>
> Since I'll need to re-spin this series anyway, I may as well tidy that
> up :)
>
> >
> >> /* RX descriptor base address for best effort */
> >> desc = &priv->desc_bat[RX_QUEUE_OFFSET + q];
> >> desc->die_dt = DT_LINKFIX; /* type */
> >> @@ -408,11 +422,9 @@ static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)
> >> static int ravb_ring_init(struct net_device *ndev, int q)
> >> {
> >> struct ravb_private *priv = netdev_priv(ndev);
> >> - const struct ravb_hw_info *info = priv->info;
> >> unsigned int num_tx_desc = priv->num_tx_desc;
> >> unsigned int ring_size;
> >> - struct sk_buff *skb;
> >> - unsigned int i;
> >> + u32 num_filled;
> >>
> >> /* Allocate RX and TX skb rings */
> >> priv->rx_skb[q] = kcalloc(priv->num_rx_ring[q],
> >> @@ -422,13 +434,6 @@ static int ravb_ring_init(struct net_device *ndev, int q)
> >> if (!priv->rx_skb[q] || !priv->tx_skb[q])
> >> goto error;
> >>
> >> - for (i = 0; i < priv->num_rx_ring[q]; i++) {
> >> - skb = ravb_alloc_skb(ndev, info, GFP_KERNEL);
> >> - if (!skb)
> >> - goto error;
> >> - priv->rx_skb[q][i] = skb;
> >> - }
> >> -
> >> if (num_tx_desc > 1) {
> >> /* Allocate rings for the aligned buffers */
> >> priv->tx_align[q] = kmalloc(DPTR_ALIGN * priv->num_tx_ring[q] +
> >> @@ -443,6 +448,13 @@ static int ravb_ring_init(struct net_device *ndev, int q)
> >>
> >> priv->dirty_rx[q] = 0;
> >>
> >> + /* Populate RX ring buffer. */
> >> + ring_size = priv->info->rx_desc_size * priv->num_rx_ring[q];
> >> + memset(priv->rx_ring[q].raw, 0, ring_size);
> >> + num_filled = ravb_rx_ring_refill(ndev, q, priv->num_rx_ring[q], GFP_KERNEL);
> >> + if (num_filled != priv->num_rx_ring[q])
> >> + goto error;
> >> +
> >
> > Here you also change the order, but it make sense here as you first deal
> > with all TX and then all RX ;-)
>
> The placement here is because we can't call ravb_rx_ring_refill() until
> priv->dirty_rx[q] has been zero'd.
>
> The init order right now is actually:
> RX page pool
> RX buffers
> TX SKBs
> RX descriptors
> RX ring buffer
> TX descriptors
>
> So maybe this should be re-ordered.
>
> I considered breaking this all apart, so ravb_ring_init() would call
> ravb_rx_ring_init()/ravb_tx_ring_init() and ravb_ring_format() would
> call ravb_rx_ring_format()/ravb_tx_ring_format(). There are several
> steps happening for TX & RX in both init and format stages. Does that
> sound cleaner to you?
I think that is a good idea. If you wish I think this can be done on-top
or break this first part of the series out to a cleanup set, this series
is quiet large already.
>
> Thanks,
>
> --
> Paul Barker
--
Kind Regards,
Niklas Söderlund