[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250324192116.GK892515@horms.kernel.org>
Date: Mon, 24 Mar 2025 19:21:16 +0000
From: Simon Horman <horms@...nel.org>
To: Mengyuan Lou <mengyuanlou@...-swift.com>
Cc: netdev@...r.kernel.org, kuba@...nel.org, jiawenwu@...stnetic.com,
duanqiangwen@...-swift.com
Subject: Re: [RESEND,PATCH net-next v9 5/6] net: ngbe: add sriov function
support
On Mon, Mar 24, 2025 at 10:00:32AM +0800, Mengyuan Lou wrote:
> Add sriov_configure for driver ops.
> Add mailbox handler wx_msg_task for ngbe in
> the interrupt handler.
> Add the notification flow when the vfs exist.
>
> Signed-off-by: Mengyuan Lou <mengyuanlou@...-swift.com>
...
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
...
> @@ -200,12 +206,10 @@ static irqreturn_t ngbe_intr(int __always_unused irq, void *data)
> return IRQ_HANDLED;
> }
>
> -static irqreturn_t ngbe_msix_other(int __always_unused irq, void *data)
> +static irqreturn_t ngbe_msix_common(struct wx *wx, u32 eicr)
> {
> - struct wx *wx = data;
> - u32 eicr;
> -
> - eicr = wx_misc_isb(wx, WX_ISB_MISC);
> + if (eicr & NGBE_PX_MISC_IC_VF_MBOX)
> + wx_msg_task(wx);
>
> if (unlikely(eicr & NGBE_PX_MISC_IC_TIMESYNC))
> wx_ptp_check_pps_event(wx);
> @@ -217,6 +221,35 @@ static irqreturn_t ngbe_msix_other(int __always_unused irq, void *data)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t ngbe_msix_other(int __always_unused irq, void *data)
> +{
> + struct wx *wx = data;
> + u32 eicr;
> +
> + eicr = wx_misc_isb(wx, WX_ISB_MISC);
> +
> + return ngbe_msix_common(wx, eicr);
> +}
> +
> +static irqreturn_t ngbe_msic_and_queue(int __always_unused irq, void *data)
> +{
> + struct wx_q_vector *q_vector;
> + struct wx *wx = data;
> + u32 eicr;
> +
> + eicr = wx_misc_isb(wx, WX_ISB_MISC);
> + if (!eicr) {
> + /* queue */
> + q_vector = wx->q_vector[0];
> + napi_schedule_irqoff(&q_vector->napi);
> + if (netif_running(wx->netdev))
> + ngbe_irq_enable(wx, true);
> + return IRQ_HANDLED;
> + }
> +
> + return ngbe_msix_common(wx, eicr);
> +}
> +
> /**
> * ngbe_request_msix_irqs - Initialize MSI-X interrupts
> * @wx: board private structure
> @@ -249,8 +282,16 @@ static int ngbe_request_msix_irqs(struct wx *wx)
> }
> }
>
> - err = request_irq(wx->msix_entry->vector,
> - ngbe_msix_other, 0, netdev->name, wx);
> + /* Due to hardware design, when num_vfs < 7, pf can use 0 for misc and 1
> + * for queue. But when num_vfs == 7, vector[1] is assigned to vf6.
> + * Misc and queue should reuse interrupt vector[0].
> + */
> + if (wx->num_vfs == 7)
> + err = request_irq(wx->msix_entry->vector,
> + ngbe_msic_and_queue, 0, netdev->name, wx);
> + else
> + err = request_irq(wx->msix_entry->vector,
> + ngbe_msix_other, 0, netdev->name, wx);
Sorry for the late review. It has been a busy time.
I have been thinking about the IRQ handler registration above in the
context of the feedback from Jakub on v7:
"Do you have proper synchronization in place to make sure IRQs
don't get mis-routed when SR-IOV is enabled?
The goal should be to make sure the right handler is register
for the IRQ, or at least do the muxing earlier in a safe fashion.
Not decide that it was a packet IRQ half way thru a function called
ngbe_msix_other"
Link: https://lore.kernel.org/all/20250211140652.6f1a2aa9@kernel.org/
My understanding is that is that:
* In the case where num_vfs < 7, vector 1 is used by the pf for
"queue". But when num_vfs == 7 (the maximum value), vector 1 is used
by the VF6.
* Correspondingly, when num_vfs < 7 vector 0 is only used for
"misc". While when num_vfs == 7 is used for both "misc" and "queue".
* The code registration above is about vector 0 (while other vectors are
registered in the code just above this hunk).
* ngbe_msix_other only handles "misc" interrupts, while
* ngbe_msic_and_queue demuxes "misc" and "queue" interrupts
(without evaluating num_vfs), handling "queue" interrupts inline
and using a helper function, which is also used by ngbe_msix_other,
to handle "misc" interrupts.
If so, I believe this addresses Jakub's concerns.
And given that we are at v9 and the last feedback of substance was the
above comment from Jakub, I think this looks good.
Reviewed-by: Simon Horman <horms@...nel.org>
But I would like to say that there could be some follow-up to align
the comment and the names of the handlers:
* "other" seems to be used as a synonym for "misc".
Perhaps ngbe_msix_misc() ?
* "common" seems to only process "misc" interrupts.
Perhaps __ngbe_msix_misc() ?
* msic seems to be a misspelling of misc.
>
> if (err) {
> wx_err(wx, "request_irq for msix_other failed: %d\n", err);
...
Powered by blists - more mailing lists