Call napi_complete_done() in accordance with the documentation in
`Documentation/networking/napi.rst`.
Signed-off-by: Paul Barker <[email protected]>
---
drivers/net/ethernet/renesas/ravb_main.c | 26 ++++++++++--------------
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 33f8043143c1..1ac599a044b2 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1340,23 +1340,19 @@ 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 (work_done == budget)
- goto out;
-
- napi_complete(napi);
-
- /* Re-enable RX/TX interrupts */
- spin_lock_irqsave(&priv->lock, flags);
- if (!info->irq_en_dis) {
- ravb_modify(ndev, RIC0, mask, mask);
- ravb_modify(ndev, TIC, mask, mask);
- } else {
- ravb_write(ndev, mask, RIE0);
- ravb_write(ndev, mask, TIE);
+ if (work_done < budget && napi_complete_done(napi, work_done)) {
+ /* Re-enable RX/TX interrupts */
+ spin_lock_irqsave(&priv->lock, flags);
+ if (!info->irq_en_dis) {
+ ravb_modify(ndev, RIC0, mask, mask);
+ ravb_modify(ndev, TIC, mask, mask);
+ } else {
+ ravb_write(ndev, mask, RIE0);
+ ravb_write(ndev, mask, TIE);
+ }
+ spin_unlock_irqrestore(&priv->lock, flags);
}
- spin_unlock_irqrestore(&priv->lock, flags);
-out:
return work_done;
}
--
2.39.2
Hi Paul,
Thanks for your work.
On 2024-04-15 10:47:59 +0100, Paul Barker wrote:
> Call napi_complete_done() in accordance with the documentation in
> `Documentation/networking/napi.rst`.
The way I understand it napi_complete() is still OK to use, it's just a
wrapper for napi_complete_done(napi, 0). But of course using
napi_complete_done() is better if you want to use the busypolling status
returned. Maybe the commit message can be updated to reflect this
change, how about?
net: ravb: Consider busypolling status when re-enabling interrupts
Make use of the busypolling status returned from NAPI complete to decide
if interrupts shall be re-enabled or not. This is useful to reduce the
interrupt overhead.
While at it switch to using napi_complete_done() as it take into account
the work done when providing the busypolling status.
>
> Signed-off-by: Paul Barker <[email protected]>
> ---
> drivers/net/ethernet/renesas/ravb_main.c | 26 ++++++++++--------------
> 1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 33f8043143c1..1ac599a044b2 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1340,23 +1340,19 @@ 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 (work_done == budget)
> - goto out;
> -
> - napi_complete(napi);
> -
> - /* Re-enable RX/TX interrupts */
> - spin_lock_irqsave(&priv->lock, flags);
> - if (!info->irq_en_dis) {
> - ravb_modify(ndev, RIC0, mask, mask);
> - ravb_modify(ndev, TIC, mask, mask);
> - } else {
> - ravb_write(ndev, mask, RIE0);
> - ravb_write(ndev, mask, TIE);
> + if (work_done < budget && napi_complete_done(napi, work_done)) {
> + /* Re-enable RX/TX interrupts */
> + spin_lock_irqsave(&priv->lock, flags);
> + if (!info->irq_en_dis) {
> + ravb_modify(ndev, RIC0, mask, mask);
> + ravb_modify(ndev, TIC, mask, mask);
> + } else {
> + ravb_write(ndev, mask, RIE0);
> + ravb_write(ndev, mask, TIE);
> + }
> + spin_unlock_irqrestore(&priv->lock, flags);
> }
> - spin_unlock_irqrestore(&priv->lock, flags);
>
> -out:
> return work_done;
> }
>
> --
> 2.39.2
>
--
Kind Regards,
Niklas Söderlund
On 15/04/2024 12:44, Niklas Söderlund wrote:
> Hi Paul,
>
> Thanks for your work.
>
> On 2024-04-15 10:47:59 +0100, Paul Barker wrote:
>> Call napi_complete_done() in accordance with the documentation in
>> `Documentation/networking/napi.rst`.
>
> The way I understand it napi_complete() is still OK to use, it's just a
> wrapper for napi_complete_done(napi, 0). But of course using
> napi_complete_done() is better if you want to use the busypolling status
> returned. Maybe the commit message can be updated to reflect this
> change, how about?
>
> net: ravb: Consider busypolling status when re-enabling interrupts
>
> Make use of the busypolling status returned from NAPI complete to decide
> if interrupts shall be re-enabled or not. This is useful to reduce the
> interrupt overhead.
>
> While at it switch to using napi_complete_done() as it take into account
> the work done when providing the busypolling status.
That sounds good to me, especially as the motivation for this change was
to support busy polling/software IRQ coalescing. I'll update the commit
message in the next version of the series.
Thanks,
--
Paul Barker