Message-Id: <D3FEA7AF-1990-40DF-903C-30790ED782E9@net-swift.com>
Date: Tue, 25 Mar 2025 10:36:00 +0800
From: "mengyuanlou@...-swift.com" <mengyuanlou@...-swift.com>
To: Simon Horman <horms@...nel.org>
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



> 2025年3月25日 03:21，Simon Horman <horms@...nel.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@...-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.
> 
That’s right.
> 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.
> 


>> +static irqreturn_t ngbe_msix_misc(int __always_unused irq, void *data)
>> +{
>>  ...
>> + return __ngbe_msix_misc(wx, eicr);
>> +}
>> +
>> +static irqreturn_t ngbe_misc_and_queue(int __always_unused irq, void *data)
>> +{
>>  ...
>> + return __ngbe_msix_misc(wx, eicr);


if (wx->num_vfs == 7)
	err = request_irq(wx->msix_entry->vector,
			  ngbe_misc_and_queue, 0, netdev->name, wx);
else
	err = request_irq(wx->msix_entry->vector,
			  ngbe_msix_misc, 0, netdev->name, wx);

It’s more appropriate.

Thanks！
>> 
>> if (err) {
>> wx_err(wx, "request_irq for msix_other failed: %d\n", err);
> 
> ...
> 
> 

Powered by blists - more mailing lists