Re: [dpdk-stable] [dpdk-dev] [PATCH v2 1/4] raw/ifpga/base: fix bug in IRQ functions

["Zhang, Tianfei" <tianfei.zhang@intel.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: 2020年10月16日 2:57
> To: Zhang, Tianfei <tianfei.zhang@intel.com>; dev@dpdk.org; Xu, Rosen
> <rosen.xu@intel.com>; Huang, Wei <wei.huang@intel.com>
> Cc: stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] raw/ifpga/base: fix bug in IRQ
> functions
> 
> On 9/28/2020 2:40 AM, Tianfei zhang wrote:
> > From: Wei Huang <wei.huang@intel.com>
> >
> > Using a pointer instead of using a structure and point to
> > ifpga_irq_handle[] in register and unregister interrupt functions.
> > Treat positive return value of ifpga_unregister_msix_irq() as
> > successful.
> >
> > Fixes: e0a1aafe ("raw/ifpga: introduce IRQ functions")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wei Huang <wei.huang@intel.com>
> > Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> 
> I suggest commit log as following:
> 
>      raw/ifpga/base: fix interrupt handler instance usage
> 
>      Interrupt handler copied to the local 'intr_handle' variable by value
>      before passing it to IRQ functions.
>      This leads IRQ functions update the local variable instead of
>      'ifpga_irq_handle'.
> 
>      Instead, using 'intr_handle' local variable as pointer to
>      'ifpga_irq_handle' as intended.
> 
Thanks Ferruh, this commit sounds better. 

>      Also handle unsupported interrupt type requests properly, on
> unsupported
>      interrupt case:
>      'ifpga_unregister_msix_irq()' returns success
>      'ifpga_register_msix_irq()' return failure.
> 
>      Fixes: e0a1aafe2af9 ("raw/ifpga: introduce IRQ functions")
>      Cc: stable@dpdk.org
> 
> 
> The "Also" part highlights that patch addressed two different issues, for next
> time please split different fixes to the different patches.

I will split in two patches in V3.
> 
> Title "fix bug" doesn't give much information, better to give some context.
> 
> 
> And for the following part in the original commit log:
> "
> Treat positive return value of ifpga_unregister_msix_irq() as successful.
> "
> It is missing in the patch, I see that part is in the next patch :)
> 
> +1 to update, since 'rte_intr_callback_unregister()' can return
> +positive, but
> perhaps better to move the change too into its own patch.