Re: [dpdk-dev] [EXT] Re: [PATCH v4 3/7] eal/interrupts: avoid direct access to interrupt handle

[Harman Kalra <hkalra@marvell.com>]

> -----Original Message-----
> From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Sent: Wednesday, October 20, 2021 2:58 AM
> To: Harman Kalra <hkalra@marvell.com>
> Cc: dev@dpdk.org; Bruce Richardson <bruce.richardson@intel.com>;
> david.marchand@redhat.com; mdr@ashroe.eu; thomas@monjalon.net
> Subject: [EXT] Re: [PATCH v4 3/7] eal/interrupts: avoid direct access to
> interrupt handle
> 
> External Email
> 
> ----------------------------------------------------------------------
> 2021-10-20 00:05 (UTC+0530), Harman Kalra:
> > Making changes to the interrupt framework to use interrupt handle APIs
> > to get/set any field. Direct access to any of the fields should be
> > avoided to avoid any ABI breakage in future.
> 
> I get and accept the point why EAL also should use the API.
> However, mentioning ABI is still a wrong wording.
> There is no ABI between EAL structures and EAL functions by definition of
> ABI.

Sure, I will reword the commit message without ABI inclusion.

> 
> >
> > Signed-off-by: Harman Kalra <hkalra@marvell.com>
> > ---
> >  lib/eal/freebsd/eal_interrupts.c |  92 ++++++----
> >  lib/eal/linux/eal_interrupts.c   | 287 +++++++++++++++++++------------
> >  2 files changed, 234 insertions(+), 145 deletions(-)
> >
> > diff --git a/lib/eal/freebsd/eal_interrupts.c
> > b/lib/eal/freebsd/eal_interrupts.c
> [...]
> > @@ -135,9 +137,18 @@ rte_intr_callback_register(const struct
> rte_intr_handle *intr_handle,
> >  				ret = -ENOMEM;
> >  				goto fail;
> >  			} else {
> > -				src->intr_handle = *intr_handle;
> > -				TAILQ_INIT(&src->callbacks);
> > -				TAILQ_INSERT_TAIL(&intr_sources, src, next);
> > +				src->intr_handle = rte_intr_instance_alloc();
> > +				if (src->intr_handle == NULL) {
> > +					RTE_LOG(ERR, EAL, "Can not create
> intr instance\n");
> > +					free(callback);
> > +					ret = -ENOMEM;
> 
> goto fail?

I think goto not required, as we not setting wake_thread = 1 here,
API will just return error after unlocking the spinlock and trace.

> 
> > +				} else {
> > +					rte_intr_instance_copy(src-
> >intr_handle,
> > +							       intr_handle);
> > +					TAILQ_INIT(&src->callbacks);
> > +					TAILQ_INSERT_TAIL(&intr_sources,
> src,
> > +							  next);
> > +				}
> >  			}
> >  		}
> >
> [...]
> > @@ -213,7 +226,7 @@ rte_intr_callback_unregister_pending(const struct
> rte_intr_handle *intr_handle,
> >  	struct rte_intr_callback *cb, *next;
> >
> >  	/* do parameter checking first */
> > -	if (intr_handle == NULL || intr_handle->fd < 0) {
> > +	if (intr_handle == NULL || rte_intr_fd_get(intr_handle) < 0) {
> 
> The handle is checked for NULL inside the accessor, here and in other places:
> grep -R 'intr_handle == NULL ||' lib/eal

Ack, I will remove these NULL checks.

> 
> >  		RTE_LOG(ERR, EAL,
> >  		"Unregistering with invalid input parameter\n");
> >  		return -EINVAL;
> 
> > diff --git a/lib/eal/linux/eal_interrupts.c
> > b/lib/eal/linux/eal_interrupts.c
> [...]