[PATCH 1/2] net/i40e: fix return value of close operation

[PATCH 1/2] net/i40e: fix return value of close operation

[SidAli CHERRATI]

rte_intr_callback_unregister() returns the number of unregistered
callbacks on success. Since the close operation started returning
the ret variable instead of 0, this positive value leaks as the
return value of i40e_dev_close(), so applications checking
rte_eth_dev_close() != 0 treat a successful close as a failure.

Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
Cc: stable@dpdk.org
Signed-off-by: SidAli CHERRATI <scherrati1@gmail.com>
---
 drivers/net/intel/i40e/i40e_ethdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/intel/i40e/i40e_ethdev.c b/drivers/net/intel/i40e/i40e_ethdev.c
index b6b2d291ee..2fe6b1e118 100644
--- a/drivers/net/intel/i40e/i40e_ethdev.c
+++ b/drivers/net/intel/i40e/i40e_ethdev.c
@@ -2764,6 +2764,7 @@ i40e_dev_close(struct rte_eth_dev *dev)
 		ret = rte_intr_callback_unregister(intr_handle,
 				i40e_dev_interrupt_handler, dev);
 		if (ret >= 0 || ret == -ENOENT) {
+			ret = 0;
 			break;
 		} else if (ret != -EAGAIN) {
 			PMD_INIT_LOG(ERR,
-- 
2.34.1

[PATCH 2/2] net/ixgbe: fix return value of close operation

[SidAli CHERRATI]

rte_intr_callback_unregister() returns the number of unregistered
callbacks on success. Since the close operation started returning
the ret variable instead of 0, this positive value leaks as the
return value of ixgbe_dev_close(), so applications checking
rte_eth_dev_close() != 0 treat a successful close as a failure.

Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
Cc: stable@dpdk.org
Signed-off-by: SidAli CHERRATI <scherrati1@gmail.com>
---
 drivers/net/intel/ixgbe/ixgbe_ethdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/intel/ixgbe/ixgbe_ethdev.c b/drivers/net/intel/ixgbe/ixgbe_ethdev.c
index c5010f623c..6b1799d7a0 100644
--- a/drivers/net/intel/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/intel/ixgbe/ixgbe_ethdev.c
@@ -3121,6 +3121,7 @@ ixgbe_dev_close(struct rte_eth_dev *dev)
 		ret = rte_intr_callback_unregister(intr_handle,
 				ixgbe_dev_interrupt_handler, dev);
 		if (ret >= 0 || ret == -ENOENT) {
+			ret = 0;
 			break;
 		} else if (ret != -EAGAIN) {
 			PMD_INIT_LOG(ERR,
-- 
2.34.1

Re: [PATCH 2/2] net/ixgbe: fix return value of close operation

[Bruce Richardson]

On Fri, Jul 10, 2026 at 03:46:39PM +0200, SidAli CHERRATI wrote:
> rte_intr_callback_unregister() returns the number of unregistered
> callbacks on success. Since the close operation started returning
> the ret variable instead of 0, this positive value leaks as the
> return value of ixgbe_dev_close(), so applications checking
> rte_eth_dev_close() != 0 treat a successful close as a failure.
> 
> Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
> Cc: stable@dpdk.org
> Signed-off-by: SidAli CHERRATI <scherrati1@gmail.com>
> ---
>  drivers/net/intel/ixgbe/ixgbe_ethdev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/intel/ixgbe/ixgbe_ethdev.c b/drivers/net/intel/ixgbe/ixgbe_ethdev.c
> index c5010f623c..6b1799d7a0 100644
> --- a/drivers/net/intel/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/intel/ixgbe/ixgbe_ethdev.c
> @@ -3121,6 +3121,7 @@ ixgbe_dev_close(struct rte_eth_dev *dev)
>  		ret = rte_intr_callback_unregister(intr_handle,
>  				ixgbe_dev_interrupt_handler, dev);
>  		if (ret >= 0 || ret == -ENOENT) {
> +			ret = 0;
>  			break;

This fixes the issue of returning an invalid value from the interrupt
unregister, but it also means losing the previous value of ret from the
stop call above. I'm therefore wondering if a better fix is to use a new
temporary variable inside the do{ }while loop, and assign that to ret in
the case of an actual error, otherwise leaving ret unmodified.
In that case, we probably want to stop retries on an error too, right?

	do {
		int cb_ret = rte_intr_callback_unregister(intr_handle,
				ixgbe_dev_interrupt_handler, dev);
		if (cb_ret >= 0 || cb_ret == -ENOENT) {
			break;
		} else if (cb_ret != -EAGAIN) {
			PMD_INIT_LOG(ERR,
				"intr callback unregister failed: %d",
				cb_ret);
			ret = cb_ret;
			break;  /* on error, don't retry */
		}
		rte_delay_ms(100);
	} while (retries++ < (10 + IXGBE_LINK_UP_TIME));

IF we do need to retry on error, then this approach won't work. Maybe a
modified version of your suggestion might do instead - rather than assigning
0 to ret on success, we assign a saved off version of it from before the
loop?

What do you think?
/Bruce

[PATCH v2 1/2] net/i40e: fix return value of close operation

[Sid Ali Cherrati]

rte_intr_callback_unregister() returns the number of unregistered
callbacks on success. Since the close operation started returning
the ret variable instead of 0, this positive value leaks as the
return value of i40e_dev_close(), so applications checking
rte_eth_dev_close() != 0 treat a successful close as a failure.

Use a separate cb_ret variable for the unregister return value so a
successful unregister never clobbers an earlier error stored in ret,
and stop retrying once unregister fails with anything other than
-EAGAIN, since those errors are not transient.

Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
Cc: stable@dpdk.org

Signed-off-by: Sid Ali Cherrati <scherrati1@gmail.com>
---
v2: use a temporary cb_ret variable instead of unconditionally zeroing
    ret, and stop retrying on a non-transient unregister error, so an
    earlier error is not lost or masked by a successful/failed
    unregister

 drivers/net/intel/i40e/i40e_ethdev.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/intel/i40e/i40e_ethdev.c b/drivers/net/intel/i40e/i40e_ethdev.c
index b6b2d291ee..b5457bcf2e 100644
--- a/drivers/net/intel/i40e/i40e_ethdev.c
+++ b/drivers/net/intel/i40e/i40e_ethdev.c
@@ -2693,6 +2693,7 @@ i40e_dev_close(struct rte_eth_dev *dev)
 	uint32_t reg;
 	int i;
 	int ret;
+	int cb_ret;
 	uint8_t aq_fail = 0;
 	int retries = 0;
 
@@ -2761,14 +2762,17 @@ i40e_dev_close(struct rte_eth_dev *dev)
 	i40e_pf_host_uninit(dev);
 
 	do {
-		ret = rte_intr_callback_unregister(intr_handle,
+		cb_ret = rte_intr_callback_unregister(intr_handle,
 				i40e_dev_interrupt_handler, dev);
-		if (ret >= 0 || ret == -ENOENT) {
+		if (cb_ret >= 0 || cb_ret == -ENOENT) {
 			break;
-		} else if (ret != -EAGAIN) {
+		} else if (cb_ret != -EAGAIN) {
 			PMD_INIT_LOG(ERR,
 				 "intr callback unregister failed: %d",
-				 ret);
+				 cb_ret);
+			if (ret == 0)
+				ret = cb_ret;
+			break;
 		}
 		i40e_msec_delay(500);
 	} while (retries++ < 5);
-- 
2.34.1

[PATCH v2 2/2] net/ixgbe: fix return value of close operation

[Sid Ali Cherrati]

rte_intr_callback_unregister() returns the number of unregistered
callbacks on success. Since the close operation started returning
the ret variable instead of 0, this positive value leaks as the
return value of ixgbe_dev_close(), so applications checking
rte_eth_dev_close() != 0 treat a successful close as a failure.

Use a separate cb_ret variable for the unregister return value so a
successful unregister never clobbers the dev_stop() error already
stored in ret, and stop retrying once unregister fails with anything
other than -EAGAIN, since those errors are not transient.

Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
Cc: stable@dpdk.org

Signed-off-by: Sid Ali Cherrati <scherrati1@gmail.com>
---
v2: use a temporary cb_ret variable instead of unconditionally zeroing
    ret, and stop retrying on a non-transient unregister error, so an
    earlier error is not lost or masked by a successful/failed
    unregister

 drivers/net/intel/ixgbe/ixgbe_ethdev.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/intel/ixgbe/ixgbe_ethdev.c b/drivers/net/intel/ixgbe/ixgbe_ethdev.c
index c5010f623c..9433515b85 100644
--- a/drivers/net/intel/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/intel/ixgbe/ixgbe_ethdev.c
@@ -3095,6 +3095,7 @@ ixgbe_dev_close(struct rte_eth_dev *dev)
 	struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
 	int retries = 0;
 	int ret;
+	int cb_ret;
 
 	PMD_INIT_FUNC_TRACE();
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
@@ -3118,14 +3119,17 @@ ixgbe_dev_close(struct rte_eth_dev *dev)
 	rte_intr_disable(intr_handle);
 
 	do {
-		ret = rte_intr_callback_unregister(intr_handle,
+		cb_ret = rte_intr_callback_unregister(intr_handle,
 				ixgbe_dev_interrupt_handler, dev);
-		if (ret >= 0 || ret == -ENOENT) {
+		if (cb_ret >= 0 || cb_ret == -ENOENT) {
 			break;
-		} else if (ret != -EAGAIN) {
+		} else if (cb_ret != -EAGAIN) {
 			PMD_INIT_LOG(ERR,
 				"intr callback unregister failed: %d",
-				ret);
+				cb_ret);
+			if (ret == 0)
+				ret = cb_ret;
+			break;
 		}
 		rte_delay_ms(100);
 	} while (retries++ < (10 + IXGBE_LINK_UP_TIME));
-- 
2.34.1