Re: [RESEND,PATCH net-next v9 5/6] net: ngbe: add sriov function support

[Simon Horman <horms@kernel.org>]

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@net-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@kernel.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);

...