| Message ID | 20240411114434.26186-3-paul.barker.ct@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | ravb Ethernet driver bugfixes | expand |
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 <paul.barker.ct@bp.renesas.com> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey
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 <paul.barker.ct@bp.renesas.com> > --- > 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 >
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 <paul.barker.ct@bp.renesas.com> >> --- >> 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,
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 <paul.barker.ct@bp.renesas.com> > >> --- > >> 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
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. */
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 <paul.barker.ct@bp.renesas.com> --- drivers/net/ethernet/renesas/ravb_main.c | 25 ++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)