These patches fix bugs found during recent work on the ravb driver.
Patches 1 & 2 affect the R-Car code paths so have been tested on an
R-Car M3N Salvator-XS board - this is the only R-Car board I currently
have access to.
Patches 2, 3 & 4 affect the GbEth code paths so have been tested on
RZ/G2L and RZ/G2UL SMARC EVK boards.
Paul Barker (4):
net: ravb: Count packets instead of descriptors in R-Car RX path
net: ravb: Allow RX loop to move past DMA mapping errors
net: ravb: Fix GbEth jumbo packet RX checksum handling
net: ravb: Fix RX byte accounting for jumbo packets
drivers/net/ethernet/renesas/ravb_main.c | 67 +++++++++++-------------
1 file changed, 32 insertions(+), 35 deletions(-)
base-commit: 47d8ac011fe1c9251070e1bd64cb10b48193ec51
--
2.39.2
The units of "work done" in the RX path should be packets instead of
descriptors.
Descriptors which are used by the hardware to record error conditions or
are empty in the case of a DMA mapping error should not count towards
our RX work budget.
Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
Signed-off-by: Paul Barker <[email protected]>
---
drivers/net/ethernet/renesas/ravb_main.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ba01c8cc3c90..70f2900648d4 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -892,29 +892,25 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
struct ravb_private *priv = netdev_priv(ndev);
const struct ravb_hw_info *info = priv->info;
int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
- int boguscnt = (priv->dirty_rx[q] + priv->num_rx_ring[q]) -
- priv->cur_rx[q];
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;
u16 pkt_len;
int limit;
+ int i;
- boguscnt = min(boguscnt, *quota);
- limit = boguscnt;
+ limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
desc = &priv->rx_ring[q].ex_desc[entry];
- while (desc->die_dt != DT_FEMPTY) {
+ for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
/* Descriptor type must be checked before all other reads */
dma_rmb();
desc_status = desc->msc;
pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
- if (--boguscnt < 0)
- break;
-
/* We use 0-byte descriptors to mark the DMA mapping errors */
if (!pkt_len)
continue;
@@ -960,7 +956,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
if (ndev->features & NETIF_F_RXCSUM)
ravb_rx_csum(skb);
napi_gro_receive(&priv->napi[q], skb);
- stats->rx_packets++;
+ rx_packets++;
stats->rx_bytes += pkt_len;
}
@@ -995,9 +991,9 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
desc->die_dt = DT_FEMPTY;
}
- *quota -= limit - (++boguscnt);
-
- return boguscnt <= 0;
+ stats->rx_packets += rx_packets;
+ *quota -= rx_packets;
+ return *quota == 0;
}
/* Packet receive function for Ethernet AVB */
--
2.39.2
Sending a 7kB ping packet to the RZ/G2L in Linux v6.9-rc2 causes the
following backtrace:
WARNING: CPU: 0 PID: 0 at include/linux/skbuff.h:3127 skb_trim+0x30/0x38
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.9.0-rc1-00222-gde11614025b1 #3
Hardware name: Renesas SMARC EVK based on r9a07g044l2 (DT)
pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : skb_trim+0x30/0x38
lr : ravb_rx_csum_gbeth+0x40/0x90
sp : ffff800080003d40
x29: ffff800080003d40 x28: 0000000000000400 x27: ffff00000d0f0000
x26: 0000000000000001 x25: ffff800080003e84 x24: 0000000000000e52
x23: 0000000000000000 x22: ffff00000d0f0960 x21: 000000000000ffff
x20: 000000000000f6ff x19: ffff00000cf4cb00 x18: 0000000000000000
x17: ffff7ffffdd4f000 x16: ffff800080000000 x15: e7e6e5e4e3e2e1e0
x14: dfdedddcdbdad9d8 x13: 0000131211100f0e x12: 0d0c0b0a09080706
x11: 0000000013121110 x10: 0000000000000000 x9 : 0000000000000001
x8 : ffff800080003cf0 x7 : 0000000000000000 x6 : ffff00007faf4590
x5 : 000000000010000b x4 : a1a8362deecb53ea x3 : 0000000000000080
x2 : 00000000ffff0000 x1 : 000000000cf4ccfc x0 : ffff00000cf4cb00
Call trace:
skb_trim+0x30/0x38
ravb_rx_gbeth+0x56c/0x5cc
ravb_poll+0xa0/0x204
__napi_poll+0x38/0x17c
net_rx_action+0x124/0x268
__do_softirq+0x100/0x26c
____do_softirq+0x10/0x1c
call_on_irq_stack+0x24/0x4c
do_softirq_own_stack+0x1c/0x2c
irq_exit_rcu+0xbc/0xd8
el1_interrupt+0x38/0x68
el1h_64_irq_handler+0x18/0x24
el1h_64_irq+0x64/0x68
default_idle_call+0x28/0x3c
do_idle+0x204/0x25c
cpu_startup_entry+0x38/0x3c
kernel_init+0x0/0x1d8
start_kernel+0x504/0x5f0
__primary_switched+0x80/0x88
---[ end trace 0000000000000000 ]---
This is caused by ravb_rx_gbeth() calling ravb_rx_csum_gbeth() with the
wrong skb for a packet which spans multiple descriptors. To fix this,
use the correct skb.
Fixes: c2da9408579d ("ravb: Add Rx checksum offload support for GbEth")
Signed-off-by: Paul Barker <[email protected]>
---
drivers/net/ethernet/renesas/ravb_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 028ab5c6aaf7..e1e39f65224c 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -843,7 +843,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
priv->rx_1st_skb->protocol =
eth_type_trans(priv->rx_1st_skb, ndev);
if (ndev->features & NETIF_F_RXCSUM)
- ravb_rx_csum_gbeth(skb);
+ ravb_rx_csum_gbeth(priv->rx_1st_skb);
napi_gro_receive(&priv->napi[q],
priv->rx_1st_skb);
rx_packets++;
--
2.39.2
The RX loops in ravb_rx_gbeth() and ravb_rx_rcar() skip to the next loop
interation if a zero-length descriptor is seen (indicating a DMA mapping
error). However, the current rx descriptor index `priv->cur_rx[q]` was
incremented at the end of the loop and so would not be incremented when
we skip to the next loop iteration. This would cause the loop to keep
seeing the same zero-length descriptor instead of moving on to the next
descriptor.
As the loop counter `i` still increments, the loop would eventually
terminate so there is no risk of being stuck here forever - but we
should still fix this to avoid wasting cycles.
To fix this, the rx descriptor index is incremented at the top of the
loop, in the for statement itself. The assignments of `entry` and `desc`
are brought into the loop to avoid the need for duplication.
Fixes: d8b48911fd24 ("ravb: fix ring memory allocation")
Signed-off-by: Paul Barker <[email protected]>
---
drivers/net/ethernet/renesas/ravb_main.c | 25 ++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 70f2900648d4..028ab5c6aaf7 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -775,12 +775,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
int limit;
int i;
- entry = priv->cur_rx[q] % priv->num_rx_ring[q];
limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
stats = &priv->stats[q];
- desc = &priv->rx_ring[q].desc[entry];
- for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
+ for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
+ entry = priv->cur_rx[q] % priv->num_rx_ring[q];
+ desc = &priv->rx_ring[q].desc[entry];
+ if (desc->die_dt == DT_FEMPTY)
+ break;
+
/* Descriptor type must be checked before all other reads */
dma_rmb();
desc_status = desc->msc;
@@ -848,9 +851,6 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
break;
}
}
-
- entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
- desc = &priv->rx_ring[q].desc[entry];
}
/* Refill the RX ring buffers. */
@@ -891,7 +891,6 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
{
struct ravb_private *priv = netdev_priv(ndev);
const struct ravb_hw_info *info = priv->info;
- int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
struct net_device_stats *stats = &priv->stats[q];
struct ravb_ex_rx_desc *desc;
struct sk_buff *skb;
@@ -900,12 +899,17 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
int rx_packets = 0;
u8 desc_status;
u16 pkt_len;
+ int entry;
int limit;
int i;
limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
- desc = &priv->rx_ring[q].ex_desc[entry];
- for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
+ for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
+ entry = priv->cur_rx[q] % priv->num_rx_ring[q];
+ desc = &priv->rx_ring[q].ex_desc[entry];
+ if (desc->die_dt == DT_FEMPTY)
+ break;
+
/* Descriptor type must be checked before all other reads */
dma_rmb();
desc_status = desc->msc;
@@ -959,9 +963,6 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
rx_packets++;
stats->rx_bytes += pkt_len;
}
-
- entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
- desc = &priv->rx_ring[q].ex_desc[entry];
}
/* Refill the RX ring buffers. */
--
2.39.2
The RX byte accounting for jumbo packets was changed to fix a potential
use-after-free bug. However, that fix used the wrong variable and so
only accounted for the number of bytes in the final descriptor, not the
number of bytes in the whole packet.
To fix this, we can simply update our stats with the correct number of
bytes before calling napi_gro_receive().
Also rename pkt_len to desc_len in ravb_rx_gbeth() to avoid any future
confusion. The variable name pkt_len is correct in ravb_rx_rcar() as
that function does not handle packets spanning multiple descriptors.
Fixes: 5a5a3e564de6 ("ravb: Fix potential use-after-free in ravb_rx_gbeth()")
Signed-off-by: Paul Barker <[email protected]>
---
drivers/net/ethernet/renesas/ravb_main.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index e1e39f65224c..c4ac9fbe0af4 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -769,7 +769,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
dma_addr_t dma_addr;
int rx_packets = 0;
u8 desc_status;
- u16 pkt_len;
+ u16 desc_len;
u8 die_dt;
int entry;
int limit;
@@ -787,10 +787,10 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
/* Descriptor type must be checked before all other reads */
dma_rmb();
desc_status = desc->msc;
- pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
+ desc_len = le16_to_cpu(desc->ds_cc) & RX_DS;
/* We use 0-byte descriptors to mark the DMA mapping errors */
- if (!pkt_len)
+ if (!desc_len)
continue;
if (desc_status & MSC_MC)
@@ -811,25 +811,25 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
switch (die_dt) {
case DT_FSINGLE:
skb = ravb_get_skb_gbeth(ndev, entry, desc);
- skb_put(skb, pkt_len);
+ skb_put(skb, desc_len);
skb->protocol = eth_type_trans(skb, ndev);
if (ndev->features & NETIF_F_RXCSUM)
ravb_rx_csum_gbeth(skb);
napi_gro_receive(&priv->napi[q], skb);
rx_packets++;
- stats->rx_bytes += pkt_len;
+ stats->rx_bytes += desc_len;
break;
case DT_FSTART:
priv->rx_1st_skb = ravb_get_skb_gbeth(ndev, entry, desc);
- skb_put(priv->rx_1st_skb, pkt_len);
+ skb_put(priv->rx_1st_skb, desc_len);
break;
case DT_FMID:
skb = ravb_get_skb_gbeth(ndev, entry, desc);
skb_copy_to_linear_data_offset(priv->rx_1st_skb,
priv->rx_1st_skb->len,
skb->data,
- pkt_len);
- skb_put(priv->rx_1st_skb, pkt_len);
+ desc_len);
+ skb_put(priv->rx_1st_skb, desc_len);
dev_kfree_skb(skb);
break;
case DT_FEND:
@@ -837,17 +837,17 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
skb_copy_to_linear_data_offset(priv->rx_1st_skb,
priv->rx_1st_skb->len,
skb->data,
- pkt_len);
- skb_put(priv->rx_1st_skb, pkt_len);
+ desc_len);
+ skb_put(priv->rx_1st_skb, desc_len);
dev_kfree_skb(skb);
priv->rx_1st_skb->protocol =
eth_type_trans(priv->rx_1st_skb, ndev);
if (ndev->features & NETIF_F_RXCSUM)
ravb_rx_csum_gbeth(priv->rx_1st_skb);
+ stats->rx_bytes += priv->rx_1st_skb->len;
napi_gro_receive(&priv->napi[q],
priv->rx_1st_skb);
rx_packets++;
- stats->rx_bytes += pkt_len;
break;
}
}
--
2.39.2
On 4/11/24 2:44 PM, Paul Barker wrote:
> The RX loops in ravb_rx_gbeth() and ravb_rx_rcar() skip to the next loop
> interation if a zero-length descriptor is seen (indicating a DMA mapping
Iteration. :-)
> error). However, the current rx descriptor index `priv->cur_rx[q]` was
RX?
> incremented at the end of the loop and so would not be incremented when
> we skip to the next loop iteration. This would cause the loop to keep
> seeing the same zero-length descriptor instead of moving on to the next
> descriptor.
>
> As the loop counter `i` still increments, the loop would eventually
> terminate so there is no risk of being stuck here forever - but we
> should still fix this to avoid wasting cycles.
>
> To fix this, the rx descriptor index is incremented at the top of the
RX?
> loop, in the for statement itself. The assignments of `entry` and `desc`
> are brought into the loop to avoid the need for duplication.
>
> Fixes: d8b48911fd24 ("ravb: fix ring memory allocation")
> Signed-off-by: Paul Barker <[email protected]>
Reviewed-by: Sergey Shtylyov <[email protected]>
[...]
MBR, Sergey
On 4/11/24 2:44 PM, Paul Barker wrote:
> The units of "work done" in the RX path should be packets instead of
> descriptors.
>
> Descriptors which are used by the hardware to record error conditions or
> are empty in the case of a DMA mapping error should not count towards
> our RX work budget.
>
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Paul Barker <[email protected]>
Reviewed-by: Sergey Shtylyov <[email protected]>
[...]
MBR, Sergey
On 4/11/24 2:44 PM, Paul Barker wrote:
> Sending a 7kB ping packet to the RZ/G2L in Linux v6.9-rc2 causes the
> following backtrace:
>
> WARNING: CPU: 0 PID: 0 at include/linux/skbuff.h:3127 skb_trim+0x30/0x38
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.9.0-rc1-00222-gde11614025b1 #3
> Hardware name: Renesas SMARC EVK based on r9a07g044l2 (DT)
> pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : skb_trim+0x30/0x38
> lr : ravb_rx_csum_gbeth+0x40/0x90
> sp : ffff800080003d40
> x29: ffff800080003d40 x28: 0000000000000400 x27: ffff00000d0f0000
> x26: 0000000000000001 x25: ffff800080003e84 x24: 0000000000000e52
> x23: 0000000000000000 x22: ffff00000d0f0960 x21: 000000000000ffff
> x20: 000000000000f6ff x19: ffff00000cf4cb00 x18: 0000000000000000
> x17: ffff7ffffdd4f000 x16: ffff800080000000 x15: e7e6e5e4e3e2e1e0
> x14: dfdedddcdbdad9d8 x13: 0000131211100f0e x12: 0d0c0b0a09080706
> x11: 0000000013121110 x10: 0000000000000000 x9 : 0000000000000001
> x8 : ffff800080003cf0 x7 : 0000000000000000 x6 : ffff00007faf4590
> x5 : 000000000010000b x4 : a1a8362deecb53ea x3 : 0000000000000080
> x2 : 00000000ffff0000 x1 : 000000000cf4ccfc x0 : ffff00000cf4cb00
> Call trace:
> skb_trim+0x30/0x38
> ravb_rx_gbeth+0x56c/0x5cc
> ravb_poll+0xa0/0x204
> __napi_poll+0x38/0x17c
> net_rx_action+0x124/0x268
> __do_softirq+0x100/0x26c
> ____do_softirq+0x10/0x1c
> call_on_irq_stack+0x24/0x4c
> do_softirq_own_stack+0x1c/0x2c
> irq_exit_rcu+0xbc/0xd8
> el1_interrupt+0x38/0x68
> el1h_64_irq_handler+0x18/0x24
> el1h_64_irq+0x64/0x68
> default_idle_call+0x28/0x3c
> do_idle+0x204/0x25c
> cpu_startup_entry+0x38/0x3c
> kernel_init+0x0/0x1d8
> start_kernel+0x504/0x5f0
> __primary_switched+0x80/0x88
> ---[ end trace 0000000000000000 ]---
>
> This is caused by ravb_rx_gbeth() calling ravb_rx_csum_gbeth() with the
> wrong skb for a packet which spans multiple descriptors. To fix this,
> use the correct skb.
>
> Fixes: c2da9408579d ("ravb: Add Rx checksum offload support for GbEth")
> Signed-off-by: Paul Barker <[email protected]>
Reviewed-by: Sergey Shtylyov <[email protected]>
[...]
MBR, Sergey
On 4/11/24 2:44 PM, Paul Barker wrote:
> The RX byte accounting for jumbo packets was changed to fix a potential
> use-after-free bug. However, that fix used the wrong variable and so
> only accounted for the number of bytes in the final descriptor, not the
> number of bytes in the whole packet.
>
> To fix this, we can simply update our stats with the correct number of
> bytes before calling napi_gro_receive().
>
> Also rename pkt_len to desc_len in ravb_rx_gbeth() to avoid any future
> confusion. The variable name pkt_len is correct in ravb_rx_rcar() as
> that function does not handle packets spanning multiple descriptors.
>
> Fixes: 5a5a3e564de6 ("ravb: Fix potential use-after-free in ravb_rx_gbeth()")
> Signed-off-by: Paul Barker <[email protected]>
Reviewed-by: Sergey Shtylyov <[email protected]>
[...]
MBR, Sergey
Hi Paul,
Thanks for your patch.
On 2024-04-11 12:44:30 +0100, Paul Barker wrote:
> The units of "work done" in the RX path should be packets instead of
> descriptors.
>
> Descriptors which are used by the hardware to record error conditions or
> are empty in the case of a DMA mapping error should not count towards
> our RX work budget.
>
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Paul Barker <[email protected]>
> ---
> drivers/net/ethernet/renesas/ravb_main.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ba01c8cc3c90..70f2900648d4 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -892,29 +892,25 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> struct ravb_private *priv = netdev_priv(ndev);
> const struct ravb_hw_info *info = priv->info;
> int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> - int boguscnt = (priv->dirty_rx[q] + priv->num_rx_ring[q]) -
> - priv->cur_rx[q];
> 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;
> u16 pkt_len;
> int limit;
> + int i;
The loop variable can never be negative, use unsigned int.
>
> - boguscnt = min(boguscnt, *quota);
> - limit = boguscnt;
> + limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> desc = &priv->rx_ring[q].ex_desc[entry];
> - while (desc->die_dt != DT_FEMPTY) {
> + for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> /* Descriptor type must be checked before all other reads */
> dma_rmb();
> desc_status = desc->msc;
> pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
>
> - if (--boguscnt < 0)
> - break;
> -
nit: It's a matter of taste, but I like this break condition in the code
instead of modifying the loop as it's much clearer what's going on. But
feel free to keep it as is as Sergey likes it.
> /* We use 0-byte descriptors to mark the DMA mapping errors */
> if (!pkt_len)
> continue;
> @@ -960,7 +956,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> if (ndev->features & NETIF_F_RXCSUM)
> ravb_rx_csum(skb);
> napi_gro_receive(&priv->napi[q], skb);
> - stats->rx_packets++;
> + rx_packets++;
Why do you add this intermediary variable? Is it not confusing to treat
rx_packets and rx_bytes differently? Why not instead decrement *quota
here?
> stats->rx_bytes += pkt_len;
> }
>
> @@ -995,9 +991,9 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> desc->die_dt = DT_FEMPTY;
> }
>
> - *quota -= limit - (++boguscnt);
> -
> - return boguscnt <= 0;
> + stats->rx_packets += rx_packets;
> + *quota -= rx_packets;
> + return *quota == 0;
> }
>
> /* Packet receive function for Ethernet AVB */
> --
> 2.39.2
>
--
Kind Regards,
Niklas Söderlund
Hi Paul,
Thanks for your patch.
On 2024-04-11 12:44:31 +0100, Paul Barker wrote:
> The RX loops in ravb_rx_gbeth() and ravb_rx_rcar() skip to the next loop
> interation if a zero-length descriptor is seen (indicating a DMA mapping
> error). However, the current rx descriptor index `priv->cur_rx[q]` was
> incremented at the end of the loop and so would not be incremented when
> we skip to the next loop iteration. This would cause the loop to keep
> seeing the same zero-length descriptor instead of moving on to the next
> descriptor.
>
> As the loop counter `i` still increments, the loop would eventually
> terminate so there is no risk of being stuck here forever - but we
> should still fix this to avoid wasting cycles.
>
> To fix this, the rx descriptor index is incremented at the top of the
> loop, in the for statement itself. The assignments of `entry` and `desc`
> are brought into the loop to avoid the need for duplication.
>
> Fixes: d8b48911fd24 ("ravb: fix ring memory allocation")
> Signed-off-by: Paul Barker <[email protected]>
> ---
> drivers/net/ethernet/renesas/ravb_main.c | 25 ++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 70f2900648d4..028ab5c6aaf7 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -775,12 +775,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> int limit;
> int i;
>
> - entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> stats = &priv->stats[q];
>
> - desc = &priv->rx_ring[q].desc[entry];
> - for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> + for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> + entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> + desc = &priv->rx_ring[q].desc[entry];
> + if (desc->die_dt == DT_FEMPTY)
> + break;
> +
> /* Descriptor type must be checked before all other reads */
> dma_rmb();
> desc_status = desc->msc;
> @@ -848,9 +851,6 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> break;
> }
> }
> -
> - entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
> - desc = &priv->rx_ring[q].desc[entry];
> }
>
> /* Refill the RX ring buffers. */
> @@ -891,7 +891,6 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> {
> struct ravb_private *priv = netdev_priv(ndev);
> const struct ravb_hw_info *info = priv->info;
> - int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> struct net_device_stats *stats = &priv->stats[q];
> struct ravb_ex_rx_desc *desc;
> struct sk_buff *skb;
> @@ -900,12 +899,17 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> int rx_packets = 0;
> u8 desc_status;
> u16 pkt_len;
> + int entry;
> int limit;
> int i;
>
> limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> - desc = &priv->rx_ring[q].ex_desc[entry];
> - for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> + for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> + entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> + desc = &priv->rx_ring[q].ex_desc[entry];
> + if (desc->die_dt == DT_FEMPTY)
> + break;
I really like moving the assignment of entry and desc to the top of the
loop. But I don't like the loop limits as it's hard, at least for me, to
immediately see what's going on. How about,
limit = min(priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q], *quota);
for (i = 0; i < limit; i++) {
entry = priv->cur_rx[q] % priv->num_rx_ring[q];
desc = &priv->rx_ring[q].ex_desc[entry];
/* There are no more valid descriptors after an empty one. */
if (desc->die_dt == DT_FEMPTY)
break;
...
}
> +
> /* Descriptor type must be checked before all other reads */
> dma_rmb();
> desc_status = desc->msc;
> @@ -959,9 +963,6 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> rx_packets++;
> stats->rx_bytes += pkt_len;
> }
> -
> - entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
> - desc = &priv->rx_ring[q].ex_desc[entry];
> }
>
> /* Refill the RX ring buffers. */
> --
> 2.39.2
>
--
Kind Regards,
Niklas Söderlund
On 14/04/2024 13:08, Niklas Söderlund wrote:
> Hi Paul,
>
> Thanks for your patch.
>
> On 2024-04-11 12:44:30 +0100, Paul Barker wrote:
>> The units of "work done" in the RX path should be packets instead of
>> descriptors.
>>
>> Descriptors which are used by the hardware to record error conditions or
>> are empty in the case of a DMA mapping error should not count towards
>> our RX work budget.
>>
>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>> Signed-off-by: Paul Barker <[email protected]>
>> ---
>> drivers/net/ethernet/renesas/ravb_main.c | 20 ++++++++------------
>> 1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index ba01c8cc3c90..70f2900648d4 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -892,29 +892,25 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>> struct ravb_private *priv = netdev_priv(ndev);
>> const struct ravb_hw_info *info = priv->info;
>> int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>> - int boguscnt = (priv->dirty_rx[q] + priv->num_rx_ring[q]) -
>> - priv->cur_rx[q];
>> 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;
>> u16 pkt_len;
>> int limit;
>> + int i;
>
> The loop variable can never be negative, use unsigned int.
I matched the type we're comparing against - should we also convert
limit to an unsigned int?
>
>>
>> - boguscnt = min(boguscnt, *quota);
>> - limit = boguscnt;
>> + limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>> desc = &priv->rx_ring[q].ex_desc[entry];
>> - while (desc->die_dt != DT_FEMPTY) {
>> + for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
>> /* Descriptor type must be checked before all other reads */
>> dma_rmb();
>> desc_status = desc->msc;
>> pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
>>
>> - if (--boguscnt < 0)
>> - break;
>> -
>
> nit: It's a matter of taste, but I like this break condition in the code
> instead of modifying the loop as it's much clearer what's going on. But
> feel free to keep it as is as Sergey likes it.
>
>> /* We use 0-byte descriptors to mark the DMA mapping errors */
>> if (!pkt_len)
>> continue;
>> @@ -960,7 +956,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>> if (ndev->features & NETIF_F_RXCSUM)
>> ravb_rx_csum(skb);
>> napi_gro_receive(&priv->napi[q], skb);
>> - stats->rx_packets++;
>> + rx_packets++;
>
> Why do you add this intermediary variable? Is it not confusing to treat
> rx_packets and rx_bytes differently? Why not instead decrement *quota
> here?
To me, it's simpler to count received packets once instead of twice
inside the loop (once by incrementing stats->rx_packets, a second time
by decrementing *quota). This also makes future refactoring simpler as
we already have the rx_packets count which we will need to be able to
return so that we can properly track work done in ravb_poll().
Thanks,
--
Paul Barker
On 14/04/2024 13:17, Niklas Söderlund wrote:
> Hi Paul,
>
> Thanks for your patch.
>
> On 2024-04-11 12:44:31 +0100, Paul Barker wrote:
>> The RX loops in ravb_rx_gbeth() and ravb_rx_rcar() skip to the next loop
>> interation if a zero-length descriptor is seen (indicating a DMA mapping
>> error). However, the current rx descriptor index `priv->cur_rx[q]` was
>> incremented at the end of the loop and so would not be incremented when
>> we skip to the next loop iteration. This would cause the loop to keep
>> seeing the same zero-length descriptor instead of moving on to the next
>> descriptor.
>>
>> As the loop counter `i` still increments, the loop would eventually
>> terminate so there is no risk of being stuck here forever - but we
>> should still fix this to avoid wasting cycles.
>>
>> To fix this, the rx descriptor index is incremented at the top of the
>> loop, in the for statement itself. The assignments of `entry` and `desc`
>> are brought into the loop to avoid the need for duplication.
>>
>> Fixes: d8b48911fd24 ("ravb: fix ring memory allocation")
>> Signed-off-by: Paul Barker <[email protected]>
>> ---
>> drivers/net/ethernet/renesas/ravb_main.c | 25 ++++++++++++------------
>> 1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 70f2900648d4..028ab5c6aaf7 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -775,12 +775,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>> int limit;
>> int i;
>>
>> - entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>> limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>> stats = &priv->stats[q];
>>
>> - desc = &priv->rx_ring[q].desc[entry];
>> - for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
>> + for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
>> + entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>> + desc = &priv->rx_ring[q].desc[entry];
>> + if (desc->die_dt == DT_FEMPTY)
>> + break;
>> +
>> /* Descriptor type must be checked before all other reads */
>> dma_rmb();
>> desc_status = desc->msc;
>> @@ -848,9 +851,6 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>> break;
>> }
>> }
>> -
>> - entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
>> - desc = &priv->rx_ring[q].desc[entry];
>> }
>>
>> /* Refill the RX ring buffers. */
>> @@ -891,7 +891,6 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>> {
>> struct ravb_private *priv = netdev_priv(ndev);
>> const struct ravb_hw_info *info = priv->info;
>> - int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>> struct net_device_stats *stats = &priv->stats[q];
>> struct ravb_ex_rx_desc *desc;
>> struct sk_buff *skb;
>> @@ -900,12 +899,17 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>> int rx_packets = 0;
>> u8 desc_status;
>> u16 pkt_len;
>> + int entry;
>> int limit;
>> int i;
>>
>> limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>> - desc = &priv->rx_ring[q].ex_desc[entry];
>> - for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
>> + for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
>> + entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>> + desc = &priv->rx_ring[q].ex_desc[entry];
>> + if (desc->die_dt == DT_FEMPTY)
>> + break;
>
> I really like moving the assignment of entry and desc to the top of the
> loop. But I don't like the loop limits as it's hard, at least for me, to
> immediately see what's going on. How about,
>
> limit = min(priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q], *quota);
>
> for (i = 0; i < limit; i++) {
> entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> desc = &priv->rx_ring[q].ex_desc[entry];
>
> /* There are no more valid descriptors after an empty one. */
> if (desc->die_dt == DT_FEMPTY)
> break;
>
> ...
> }
We need to count received packets separately from the number of
descriptors processed, as done in the previous commit in this series,
so we can't just have a single check against limit.
We also need to increment priv->cur_rx[q]. If we put `priv->cur_rx[q]++`
at the end of the loop then we're back to having to worry about it when
we have a continue statement.
We could move the `rx_packets < *quota` check inside the loop itself,
but I don't see that as any clearer myself.
Thanks,
--
Paul Barker
Hi Paul,
On 2024-04-15 08:04:05 +0100, Paul Barker wrote:
> On 14/04/2024 13:08, Niklas Söderlund wrote:
> > Hi Paul,
> >
> > Thanks for your patch.
> >
> > On 2024-04-11 12:44:30 +0100, Paul Barker wrote:
> >> The units of "work done" in the RX path should be packets instead of
> >> descriptors.
> >>
> >> Descriptors which are used by the hardware to record error conditions or
> >> are empty in the case of a DMA mapping error should not count towards
> >> our RX work budget.
> >>
> >> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> >> Signed-off-by: Paul Barker <[email protected]>
> >> ---
> >> drivers/net/ethernet/renesas/ravb_main.c | 20 ++++++++------------
> >> 1 file changed, 8 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >> index ba01c8cc3c90..70f2900648d4 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -892,29 +892,25 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> >> struct ravb_private *priv = netdev_priv(ndev);
> >> const struct ravb_hw_info *info = priv->info;
> >> int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >> - int boguscnt = (priv->dirty_rx[q] + priv->num_rx_ring[q]) -
> >> - priv->cur_rx[q];
> >> 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;
> >> u16 pkt_len;
> >> int limit;
> >> + int i;
> >
> > The loop variable can never be negative, use unsigned int.
>
> I matched the type we're comparing against - should we also convert
> limit to an unsigned int?
If it can't be negative I think that is a good idea.
>
> >
> >>
> >> - boguscnt = min(boguscnt, *quota);
> >> - limit = boguscnt;
> >> + limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> >> desc = &priv->rx_ring[q].ex_desc[entry];
> >> - while (desc->die_dt != DT_FEMPTY) {
> >> + for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> >> /* Descriptor type must be checked before all other reads */
> >> dma_rmb();
> >> desc_status = desc->msc;
> >> pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
> >>
> >> - if (--boguscnt < 0)
> >> - break;
> >> -
> >
> > nit: It's a matter of taste, but I like this break condition in the code
> > instead of modifying the loop as it's much clearer what's going on. But
> > feel free to keep it as is as Sergey likes it.
> >
> >> /* We use 0-byte descriptors to mark the DMA mapping errors */
> >> if (!pkt_len)
> >> continue;
> >> @@ -960,7 +956,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> >> if (ndev->features & NETIF_F_RXCSUM)
> >> ravb_rx_csum(skb);
> >> napi_gro_receive(&priv->napi[q], skb);
> >> - stats->rx_packets++;
> >> + rx_packets++;
> >
> > Why do you add this intermediary variable? Is it not confusing to treat
> > rx_packets and rx_bytes differently? Why not instead decrement *quota
> > here?
>
> To me, it's simpler to count received packets once instead of twice
> inside the loop (once by incrementing stats->rx_packets, a second time
> by decrementing *quota). This also makes future refactoring simpler as
> we already have the rx_packets count which we will need to be able to
> return so that we can properly track work done in ravb_poll().
I see your point, I think my point was made with the R-Car code path in
mind as it do not yet support splitting a packet over multiple
descriptors. And I agree there is value in trying to keep the two code
paths as close together as possible so we eventually can merge them.
With the unsigned issue above fixed,
Reviewed-by: Niklas Söderlund <[email protected]>
>
> Thanks,
>
> --
> Paul Barker
--
Kind Regards,
Niklas Söderlund
Hello Paul,
On 2024-04-15 08:12:06 +0100, Paul Barker wrote:
> On 14/04/2024 13:17, Niklas Söderlund wrote:
> > Hi Paul,
> >
> > Thanks for your patch.
> >
> > On 2024-04-11 12:44:31 +0100, Paul Barker wrote:
> >> The RX loops in ravb_rx_gbeth() and ravb_rx_rcar() skip to the next loop
> >> interation if a zero-length descriptor is seen (indicating a DMA mapping
> >> error). However, the current rx descriptor index `priv->cur_rx[q]` was
> >> incremented at the end of the loop and so would not be incremented when
> >> we skip to the next loop iteration. This would cause the loop to keep
> >> seeing the same zero-length descriptor instead of moving on to the next
> >> descriptor.
> >>
> >> As the loop counter `i` still increments, the loop would eventually
> >> terminate so there is no risk of being stuck here forever - but we
> >> should still fix this to avoid wasting cycles.
> >>
> >> To fix this, the rx descriptor index is incremented at the top of the
> >> loop, in the for statement itself. The assignments of `entry` and `desc`
> >> are brought into the loop to avoid the need for duplication.
> >>
> >> Fixes: d8b48911fd24 ("ravb: fix ring memory allocation")
> >> Signed-off-by: Paul Barker <[email protected]>
> >> ---
> >> drivers/net/ethernet/renesas/ravb_main.c | 25 ++++++++++++------------
> >> 1 file changed, 13 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >> index 70f2900648d4..028ab5c6aaf7 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -775,12 +775,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> >> int limit;
> >> int i;
> >>
> >> - entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >> limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> >> stats = &priv->stats[q];
> >>
> >> - desc = &priv->rx_ring[q].desc[entry];
> >> - for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> >> + for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> >> + entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >> + desc = &priv->rx_ring[q].desc[entry];
> >> + if (desc->die_dt == DT_FEMPTY)
> >> + break;
> >> +
> >> /* Descriptor type must be checked before all other reads */
> >> dma_rmb();
> >> desc_status = desc->msc;
> >> @@ -848,9 +851,6 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> >> break;
> >> }
> >> }
> >> -
> >> - entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
> >> - desc = &priv->rx_ring[q].desc[entry];
> >> }
> >>
> >> /* Refill the RX ring buffers. */
> >> @@ -891,7 +891,6 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> >> {
> >> struct ravb_private *priv = netdev_priv(ndev);
> >> const struct ravb_hw_info *info = priv->info;
> >> - int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >> struct net_device_stats *stats = &priv->stats[q];
> >> struct ravb_ex_rx_desc *desc;
> >> struct sk_buff *skb;
> >> @@ -900,12 +899,17 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> >> int rx_packets = 0;
> >> u8 desc_status;
> >> u16 pkt_len;
> >> + int entry;
> >> int limit;
> >> int i;
> >>
> >> limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> >> - desc = &priv->rx_ring[q].ex_desc[entry];
> >> - for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> >> + for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> >> + entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >> + desc = &priv->rx_ring[q].ex_desc[entry];
> >> + if (desc->die_dt == DT_FEMPTY)
> >> + break;
> >
> > I really like moving the assignment of entry and desc to the top of the
> > loop. But I don't like the loop limits as it's hard, at least for me, to
> > immediately see what's going on. How about,
> >
> > limit = min(priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q], *quota);
> >
> > for (i = 0; i < limit; i++) {
> > entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> > desc = &priv->rx_ring[q].ex_desc[entry];
> >
> > /* There are no more valid descriptors after an empty one. */
> > if (desc->die_dt == DT_FEMPTY)
> > break;
> >
> > ...
> > }
>
> We need to count received packets separately from the number of
> descriptors processed, as done in the previous commit in this series,
> so we can't just have a single check against limit.
As noted in 1/4 I was only considering the R-Car code path where split
descriptors are not supported. I agree it's good to keep the two code
paths in sync and with that in mind I'm OK with this approach.
>
> We also need to increment priv->cur_rx[q]. If we put `priv->cur_rx[q]++`
> at the end of the loop then we're back to having to worry about it when
> we have a continue statement.
>
> We could move the `rx_packets < *quota` check inside the loop itself,
> but I don't see that as any clearer myself.
I do think this is a good idea however. As this would split the logic in
two distinct sets. The loop would only deal with descriptors and the
stop conditions based on number of packets / work done would be a stop
condition inside the loop.
Thinking a head a bit I think it would be nice if in future the private
data variable rx_1st_skb could be reworked as this will not play nice
with multiple queues. And with a split of the loop to only consider
descriptors we could try and look a head and only process a packet if
all descriptors for it are available to us. Lets cross that bridge when
we get to it. But I think having the loop only consider descriptors
would make this easier.
>
> Thanks,
>
> --
> Paul Barker
--
Kind Regards,
Niklas Söderlund