Hi Paul,
I really like this patch!
One nit below.
On 2024-04-15 10:47:58 +0100, Paul Barker wrote:
> We don't need to pass the work budget to ravb_rx() by reference, it's
> cleaner to pass this by value and return the amount of work done. This
> allows us to simplify the ravb_poll() function and use the common
> `work_done` variable name seen in other network drivers for consistency
> and ease of understanding.
>
> This is a pure refactor and should not affect behaviour.
>
> Signed-off-by: Paul Barker <paul.barker.ct@xxxxxxxxxxxxxx>
> ---
> drivers/net/ethernet/renesas/ravb.h | 2 +-
> drivers/net/ethernet/renesas/ravb_main.c | 29 ++++++++++--------------
> 2 files changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index b48935ec7e28..71de2a7aa27c 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1039,7 +1039,7 @@ struct ravb_ptp {
> };
>
> struct ravb_hw_info {
> - bool (*receive)(struct net_device *ndev, int *quota, int q);
> + int (*receive)(struct net_device *ndev, int budget, int q);
> void (*set_rate)(struct net_device *ndev);
> int (*set_feature)(struct net_device *ndev, netdev_features_t features);
> int (*dmac_init)(struct net_device *ndev);
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index fd2679ce4e3d..33f8043143c1 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -759,7 +759,7 @@ static struct sk_buff *ravb_get_skb_gbeth(struct net_device *ndev, int entry,
> }
>
> /* Packet receive function for Gigabit Ethernet */
> -static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> +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;
> @@ -778,7 +778,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> stats = &priv->stats[q];
>
> - for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> + for (i = 0; i < limit && rx_packets < budget; 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)
> @@ -881,13 +881,11 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> desc->die_dt = DT_FEMPTY;
> }
>
> - stats->rx_packets += rx_packets;
> - *quota -= rx_packets;
> - return *quota == 0;
> + return rx_packets;
> }
>
> /* Packet receive function for Ethernet AVB */
> -static bool ravb_rx_rcar(struct net_device *ndev, int *quota, 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;
> @@ -904,7 +902,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> int i;
>
> limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> - for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> + for (i = 0; i < limit && rx_packets < budget; 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)
> @@ -992,18 +990,16 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> desc->die_dt = DT_FEMPTY;
> }
>
> - stats->rx_packets += rx_packets;
Don't we still need to update the statistics? Same for ravb_rx_gbeth().
> - *quota -= rx_packets;
> - return *quota == 0;
> + return rx_packets;
> }
>
> /* Packet receive function for Ethernet AVB */
> -static bool ravb_rx(struct net_device *ndev, int *quota, int q)
> +static int ravb_rx(struct net_device *ndev, int budget, int q)
> {
> struct ravb_private *priv = netdev_priv(ndev);
> const struct ravb_hw_info *info = priv->info;
>
> - return info->receive(ndev, quota, q);
> + return info->receive(ndev, budget, q);
> }
>
> static void ravb_rcv_snd_disable(struct net_device *ndev)
> @@ -1320,13 +1316,12 @@ static int ravb_poll(struct napi_struct *napi, int budget)
> unsigned long flags;
> int q = napi - priv->napi;
> int mask = BIT(q);
> - int quota = budget;
> - bool unmask;
> + int work_done;
>
> /* Processing RX Descriptor Ring */
> /* Clear RX interrupt */
> ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
> - unmask = !ravb_rx(ndev, "a, q);
> + work_done = ravb_rx(ndev, budget, q);
>
> /* Processing TX Descriptor Ring */
> spin_lock_irqsave(&priv->lock, flags);
> @@ -1345,7 +1340,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
> if (priv->rx_fifo_errors != ndev->stats.rx_fifo_errors)
> ndev->stats.rx_fifo_errors = priv->rx_fifo_errors;
>
> - if (!unmask)
> + if (work_done == budget)
> goto out;
>
> napi_complete(napi);
> @@ -1362,7 +1357,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
> spin_unlock_irqrestore(&priv->lock, flags);
>
> out:
> - return budget - quota;
> + return work_done;
> }
>
> static void ravb_set_duplex_gbeth(struct net_device *ndev)
> --
> 2.39.2
>
--
Kind Regards,
Niklas Söderlund
