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

[Andrew Rybchenko]

On 11.07.2018 15:30, Zhang, Qi Z wrote:
>
>> -----Original Message-----
>> From: Andrew Rybchenko [mailto:arybchenko at solarflare.com]
>> Sent: Wednesday, July 11, 2018 5:27 PM
>> To: Zhang, Qi Z <qi.z.zhang at intel.com>; thomas at monjalon.net; Burakov,
>> Anatoly <anatoly.burakov at intel.com>
>> Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org;
>> Richardson, Bruce <bruce.richardson at intel.com>; Yigit, Ferruh
>> <ferruh.yigit at intel.com>; Shelton, Benjamin H
>> <benjamin.h.shelton at intel.com>; Vangati, Narender
>> <narender.vangati at 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.zhang at intel.com>
>>> Reviewed-by: Andrew Rybchenko <arybchenko at solarflare.com>
>>> Acked-by: Remy Horton <remy.horton at 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)