Re: [PATCH 4/5] net/dpaa2: remove ethdev device in bus device

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

Hi Maxime and Hemant,

On Wed, 12 Nov 2025 at 09:44, Maxime Leroy <maxime@leroys.fr> wrote:
> Le sam. 8 nov. 2025 à 16:32, David Marchand
> <david.marchand@redhat.com> a écrit :
> >
> > Calling rte_eth_dev_allocated() is enough to retrieve the
> > ethdev device object.
> >
> > This leaves no user of the ethdev field in the fslmc device object.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  drivers/bus/fslmc/bus_fslmc_driver.h | 1 -
> >  drivers/net/dpaa2/dpaa2_ethdev.c     | 6 ++++--
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/bus/fslmc/bus_fslmc_driver.h b/drivers/bus/fslmc/bus_fslmc_driver.h
> > index efa88754a7..094f885f59 100644
> > --- a/drivers/bus/fslmc/bus_fslmc_driver.h
> > +++ b/drivers/bus/fslmc/bus_fslmc_driver.h
> > @@ -98,7 +98,6 @@ struct rte_dpaa2_device {
> >         TAILQ_ENTRY(rte_dpaa2_device) next; /**< Next probed DPAA2 device. */
> >         struct rte_device device;           /**< Inherit core device */
> >         union {
> > -               struct rte_eth_dev *eth_dev;        /**< ethernet device */
> >                 struct rte_rawdev *rawdev;          /**< Raw Device */
> >         };
> >         enum rte_dpaa2_dev_type dev_type;   /**< Device Type */
> > diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
> > index 7da32ce856..2a26a02cb4 100644
> > --- a/drivers/net/dpaa2/dpaa2_ethdev.c
> > +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
> > @@ -3320,7 +3320,6 @@ rte_dpaa2_probe(struct rte_dpaa2_driver *dpaa2_drv,
> >
> >         eth_dev->device = &dpaa2_dev->device;
> >
> > -       dpaa2_dev->eth_dev = eth_dev;
> >         eth_dev->data->rx_mbuf_alloc_failed = 0;
> >
> >         if (dpaa2_drv->drv_flags & RTE_DPAA2_DRV_INTR_LSC)
> > @@ -3349,7 +3348,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)
>
>
> dpaa2_valid_dev is currently incremented in rte_dpaa2_probe. With your
> modification, dpaa2_valid_dev will never be decremented in
> rte_dpaa2_remove if rte_eth_dev_close has already been called.
> As a consequence, dpaa2_tx_sg_pool will never be freed.
>
> I don’t think dpaa2_tx_sg_pool should be freed in rte_dpaa2_remove; it
> should be freed in dpaa2_dev_close instead.
> For symmetry, dpaa2_tx_sg_pool should also be allocated directly in
> dpaa2_dev_init.

Indeed, moving the dpaa2_tx_sg_pool handling in dev_init/dev_close is
more self contained, and looks cleaner.

Now, taking a step back, I should not try and fix the bug you guys
have been working on.
IOW, the check on !eth_dev in this patch of mine is unrelated to the series.

I'll respin a v2 shortly, that focuses on the layer violation only.
For you guys to fix the double close bug.


-- 
David Marchand