Re: [PATCH 1/3] net/dpaa2: fix duplicate calling of dpaa2 dev close

[David Marchand <david.marchand@redhat.com>]

Hello,

On Thu, 6 Nov 2025 at 17:38, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
>
> When rte_eth_dev_close() is called, it performs the following actions:
>
> Calls dev->dev_ops->dev_close(), which in this case is dpaa2_dev_close().
> Then calls rte_eth_dev_release_port(), which releases all device data
> and sets dev->data to NULL.
>
> Later, when rte_dev_remove() is called, the FSLMC bus invokes
> dev->remove() — that is, rte_dpaa2_remove().
> However, rte_dpaa2_remove() calls dpaa2_dev_close() again. Since dev->data
> was already set to NULL by the previous call, this second invocation
> causes a crash.
>
> Fixes: 5964d36a2904 ("net/dpaa2: release port upon close")
> Cc: sachin.saxena@nxp.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
>  drivers/net/dpaa2/dpaa2_ethdev.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
> index 7da32ce856..f3db7982a4 100644
> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> @@ -3347,14 +3347,17 @@ static int
>  rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
>  {
>         struct rte_eth_dev *eth_dev;
> -       int ret;
> +       int ret = 0;
>
>         eth_dev = dpaa2_dev->eth_dev;

Having a back reference of the "class" object in a "device" object
seems wrong to me (and there is a dev_priv->eth_dev too...).
It breaks the separation that was introduced with rte_device years ago.

I did not look in detail, but it seems strange that after closing a
first time, there would still remain a reference of the eth_dev object
in the dpaa2 device object.
At least, it would be worth double checking that the
dpaa2_dev->eth_dev is cleared in dpaa2_dev_close.


> -       dpaa2_dev_close(eth_dev);
> +       if (eth_dev->data) {
> +               ret = dpaa2_dev_close(eth_dev);
> +               if (!ret)
> +                       ret = rte_eth_dev_release_port(eth_dev);

I don't see why you would need to decrement dpaa2_valid_dev again below.
Maybe a missing return here?


> +       }
>         dpaa2_valid_dev--;
>         if (!dpaa2_valid_dev)
>                 rte_mempool_free(dpaa2_tx_sg_pool);
> -       ret = rte_eth_dev_release_port(eth_dev);
>
>         return ret;
>  }


Taking a step back, the issue this patch wants to fix is a pattern
that is resolved by other drivers by checking if a eth_dev is
allocated for a rte_device.
A simpler (untested) fix seems to be:

diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
index 7da32ce856..6682a72341 100644
--- a/drivers/net/dpaa2/dpaa2_ethdev.c
+++ b/drivers/net/dpaa2/dpaa2_ethdev.c
@@ -3349,7 +3349,10 @@ rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
        struct rte_eth_dev *eth_dev;
        int ret;

-       eth_dev = dpaa2_dev->eth_dev;
+       eth_dev = rte_eth_dev_allocated(dpaa2_dev->device.name);
+       if (!eth_dev)
+               return 0;
+
        dpaa2_dev_close(eth_dev);
        dpaa2_valid_dev--;
        if (!dpaa2_valid_dev)


-- 
David Marchand