> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: 2020年10月16日 2:57
> To: Zhang, Tianfei <tianfei.zh...@intel.com>; dev@dpdk.org; Xu, Rosen
> <rosen...@intel.com>; Huang, Wei <wei.hu...@intel.com>
> Cc: sta...@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.hu...@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: sta...@dpdk.org
> >
> > Signed-off-by: Wei Huang <wei.hu...@intel.com>
> > Signed-off-by: Tianfei zhang <tianfei.zh...@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: sta...@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.
