Re: [dpdk-dev] [PATCH v11 01/19] ethdev: add function to release port in local process

[Zhang, Qi Z]

> -----Original Message-----
> From: Andrew Rybchenko [mailto:arybche...@solarflare.com]
> Sent: Wednesday, July 11, 2018 5:27 PM
> To: Zhang, Qi Z <qi.z.zh...@intel.com>; tho...@monjalon.net; Burakov,
> Anatoly <anatoly.bura...@intel.com>
> Cc: Ananyev, Konstantin <konstantin.anan...@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richard...@intel.com>; Yigit, Ferruh
> <ferruh.yi...@intel.com>; Shelton, Benjamin H
> <benjamin.h.shel...@intel.com>; Vangati, Narender
> <narender.vang...@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v11 01/19] ethdev: add function to release port
> in local process
> 
> On 11.07.2018 06:08, Qi Zhang wrote:
> > Add driver API rte_eth_release_port_private to support the case when
> > an ethdev need to be detached on a secondary process.
> > Local state is set to unused and shared data will not be reset so the
> > primary process can still use it.
> >
> > Signed-off-by: Qi Zhang <qi.z.zh...@intel.com>
> > Reviewed-by: Andrew Rybchenko <arybche...@solarflare.com>
> > Acked-by: Remy Horton <remy.hor...@intel.com>
> > ---

<...>
> > +   /**
> > +    * PCI device can only be globally detached directly by a
> > +    * primary process. In secondary process, we only need to
> > +    * release port.
> > +    */
> > +   if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > +           return rte_eth_dev_release_port_private(eth_dev);
> 
> I've realized that some uninit functions which will not be called anymore in
> secondary processes have check for process type and handling of secondary
> process case. It makes code inconsistent and should be fixed.

Good point, I did a scan and check all the places that 
rte_eth_dev_pci_generic_remove be involved.
I found only sfc driver (sfc_eth_dev_unit) will call some cleanup on secondary 
process as below.

                if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
                sfc_eth_dev_secondary_clear_ops(dev);
                return 0;
        }

But in sfc_eth_dev_secondary_clear_ops

static void
sfc_eth_dev_secondary_clear_ops(struct rte_eth_dev *dev)
{
        dev->dev_ops = NULL;
        dev->tx_pkt_burst = NULL;
        dev->rx_pkt_burst = NULL;
}

So my understand is current change is not a problem for all exist drivers.

Please let me know if I missed something

Thanks
Qi

> 
> > +
> >     if (dev_uninit) {
> >             ret = dev_uninit(eth_dev);
> >             if (ret)