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

[Hemant Agrawal <hemant.agrawal@oss.nxp.com>]

On 13-11-2025 15:19, Maxime Leroy wrote:
> [Some people who received this message don't often get email from maxime@leroys.fr. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> 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.
ok, it looks like best approach is to drop this patch.
>
> 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()?

dpaa2_tx_sg_pool is a global resource across devices; which is being 
tracked with dpaa2_valid_dev

we will be revamping this logic in next major release.

>
> Maxime Leroy