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@xxxxxxxxxxxxxx>
> >> ---
> >> 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
