Re: [PATCH v2 4/5] net/dpaa2: bifurcate close into close and deinit

[Maxime Leroy <maxime@leroys.fr>]

Hi Hemant,

Le jeu. 13 nov. 2025 à 06:30, Hemant Agrawal <hemant.agrawal@nxp.com> a écrit :
>
> The close function was overloaded to be also used as deinit function.
> This can cause issues when using functions like hotplug.
>
> This patch cleans up the code and implement separate close and deinit
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
>  drivers/net/dpaa2/dpaa2_ethdev.c | 92 ++++++++++++++++++--------------
>  1 file changed, 51 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
> index f82c50341d..0cd875d715 100644
> --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> @@ -1524,53 +1524,14 @@ dpaa2_dev_stop(struct rte_eth_dev *dev)
>  static int
>  dpaa2_dev_close(struct rte_eth_dev *dev)
>  {
> -       struct dpaa2_dev_priv *priv = dev->data->dev_private;
> -       struct fsl_mc_io *dpni = dev->process_private;
> -       int i, ret;
> -       struct rte_eth_link link;
> -
>         PMD_INIT_FUNC_TRACE();
>
>         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>                 return 0;
>
> -       if (!dpni) {
> -               DPAA2_PMD_WARN("Already closed or not started");
> -               return -EINVAL;
> -       }
> -
>         dpaa2_tm_deinit(dev);
>         dpaa2_flow_clean(dev);
> -       /* Clean the device first */
> -       ret = dpni_reset(dpni, CMD_PRI_LOW, priv->token);
> -       if (ret) {
> -               DPAA2_PMD_ERR("Failure cleaning dpni device: err=%d", ret);
> -               return ret;
> -       }
> -
> -       memset(&link, 0, sizeof(link));
> -       rte_eth_linkstatus_set(dev, &link);
> -
> -       /* Free private queues memory */
> -       dpaa2_free_rx_tx_queues(dev);
> -       /* Close the device at underlying layer*/
> -       ret = dpni_close(dpni, CMD_PRI_LOW, priv->token);
> -       if (ret) {
> -               DPAA2_PMD_ERR("Failure closing dpni device with err code %d",
> -                       ret);
> -       }
> -
> -       /* Free the allocated memory for ethernet private data and dpni*/
> -       priv->hw = NULL;
> -       dev->process_private = NULL;
> -       rte_free(dpni);
>
> -       for (i = 0; i < MAX_TCS; i++)
> -               rte_free(priv->extract.tc_extract_param[i]);
> -
> -       rte_free(priv->extract.qos_extract_param);
> -
> -       DPAA2_PMD_INFO("%s: netdev deleted", dev->data->name);
>         return 0;
>  }
>
> @@ -2891,6 +2852,55 @@ dpaa2_get_devargs(struct rte_devargs *devargs, const char *key)
>         return 1;
>  }
>
> +static int
> +dpaa2_dev_deinit(struct rte_eth_dev *dev)
> +{
> +       struct dpaa2_dev_priv *priv = dev->data->dev_private;
> +       struct fsl_mc_io *dpni = dev->process_private;
> +       int i, ret;
> +       struct rte_eth_link link;
> +
> +       PMD_INIT_FUNC_TRACE();
> +
> +       if (!dpni) {
> +               DPAA2_PMD_WARN("Already closed or not started");
> +               return -EINVAL;
> +       }
> +
> +       /* Clean the device first */
> +       ret = dpni_reset(dpni, CMD_PRI_LOW, priv->token);
> +       if (ret) {
> +               DPAA2_PMD_ERR("Failure cleaning dpni device: err=%d", ret);
> +               return ret;
> +       }
> +
> +       memset(&link, 0, sizeof(link));
> +       rte_eth_linkstatus_set(dev, &link);
> +
> +       /* Free private queues memory */
> +       dpaa2_free_rx_tx_queues(dev);
> +       /* Close the device at underlying layer*/
> +       ret = dpni_close(dpni, CMD_PRI_LOW, priv->token);
> +       if (ret) {
> +               DPAA2_PMD_ERR("Failure closing dpni device with err code %d",
> +                       ret);
> +       }
> +
> +       /* Free the allocated memory for ethernet private data and dpni*/
> +       priv->hw = NULL;
> +       dev->process_private = NULL;
> +       rte_free(dpni);
> +
> +       for (i = 0; i < MAX_TCS; i++)
> +               rte_free(priv->extract.tc_extract_param[i]);
> +
> +       rte_free(priv->extract.qos_extract_param);
> +
> +       DPAA2_PMD_INFO("%s: netdev deleted", dev->data->name);
> +       return 0;
> +}
> +
> +
>  static int
>  dpaa2_dev_init(struct rte_eth_dev *eth_dev)
>  {
> @@ -3177,7 +3187,7 @@ dpaa2_dev_init(struct rte_eth_dev *eth_dev)
>
>         return 0;
>  init_err:
> -       dpaa2_dev_close(eth_dev);
> +       dpaa2_dev_deinit(eth_dev);
>
>         return ret;
>  }
> @@ -3381,7 +3391,7 @@ rte_dpaa2_remove(struct rte_dpaa2_device *dpaa2_dev)
>
>         eth_dev = rte_eth_dev_allocated(dpaa2_dev->device.name);
>         if (eth_dev) {
> -               dpaa2_dev_close(eth_dev);
> +               dpaa2_dev_deinit(eth_dev);
>                 ret = rte_eth_dev_release_port(eth_dev);
>         }

When a DPDK application stops, dpaa2_dev_close is no longer called.
This happens because eal_cleanup() invokes rte_fslmc_close(), which
then calls the remove function (rte_dpaa2_remove) for each DPAA2
Ethernet driver. As a result, dpaa2_tm_deinit() and dpaa2_flow_clean()
are no longer executed.

Another issue is that dpaa2_dev_deinit is now called in secondary
process, which was not the case before this commit.

For comparison, other drivers like ixgbe call their close method from
the bus’s remove callback. I don’t see any issue doing the same
here—calling close during remove if the Ethernet device is still
allocated.

As far as I know, there is no counterpart to rte_eth_dev_close() in
the DPDK API (there could have been an rte_eth_dev_open()). The open
path is always triggered by the bus’s probe callback.
Because of this, it is expected that dpaa2_dev_init is called from the
plug/probe path, while the corresponding deinit logic is handled in
the close callback.

One more point: why isn’t dpaa2_tx_sg_pool_init() done directly in
dpaa2_dev_init(), with its corresponding cleanup performed in
dpaa2_dev_close()?

Maxime Leroy