> -----Original Message-----
> From: Ferruh Yigit <[email protected]>
> Sent: 2020年10月16日 2:57
> To: Zhang, Tianfei <[email protected]>; [email protected]; Xu, Rosen
> <[email protected]>; Huang, Wei <[email protected]>
> Cc: [email protected]
> 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 <[email protected]>
> >
> > 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: [email protected]
> >
> > Signed-off-by: Wei Huang <[email protected]>
> > Signed-off-by: Tianfei zhang <[email protected]>
>
> 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: [email protected]
>
>
> 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.
