On 11.07.2018 15:30, Zhang, Qi Z wrote:

-----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.

The patch makes impossible dev_uninit to be executed for secondary process
for all cases if rte_eth_dev_pci_generic_remove() is used. However, many drivers still check for process type. Yes, sfc does cleanup, but some drivers return -EPERM, some return 0. In fact it does not matter. It leaves dead code which is really confusing.


                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)

Reply via email to