Re: [dpdk-dev] [PATCH] ethdev: stop the device before close

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 10/20/21 3:24 PM, Andrew Rybchenko wrote:
> On 10/20/21 3:17 PM, Thomas Monjalon wrote:
>> 20/10/2021 12:47, Andrew Rybchenko:
>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>
>>> In drivers important cleanup could happen on the device stop.
>>> Do stop in the rte_eth_dev_close() function for robustness and
>>> to simplify drivers code.
>>>
>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> ---
>>> In fact the patch is required to fix segfault in the case of
>>> net/virtio on close without stop after Rx interrupts enabled.
>>>
>>> I believe that the right way to address the problem is automated
>>> stop from close, but I guess it cannot not be backported and
>>> may be fix in a different way required in stable branches.
>>
>> It is possible to do this addition.
>> But the right fix (not changing API behaviour) should be to return early
>> if the port is not stopped.
> 
> Isn't returning an error a change of API behaviour?

After looking at rte_eth_dev_close() description I agreethat we
should return an error. Yes, it is a behaviour change, but a
change in accordance with the documentation.
I'll submit v2 which checks that port  is stopped and return
error.

> 
> This way we change behaviour less since some PMDs allow
> to close in started state and do stop itself.
> 
>>
>>> @@ -1894,6 +1894,17 @@ rte_eth_dev_close(uint16_t port_id)
>>>  	dev = &rte_eth_devices[port_id];
>>>  
>>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_close, -ENOTSUP);
>>> +
>>> +	if (dev->data->dev_started) {
>>> +		*lasterr = rte_eth_dev_stop(port_id);
>>> +		if (*lasterr != 0) {
>>> +			RTE_ETHDEV_LOG(ERR,
>>> +				"Failed to stop device (port %u) before close: %s - ignore\n",
>>> +				port_id, rte_strerror(-*lasterr));
>>> +			lasterr = &binerr;
>>> +		}
>>> +	}
>>> +
>>>  	*lasterr = (*dev->dev_ops->dev_close)(dev);
>>>  	if (*lasterr != 0)
>>>  		lasterr = &binerr;
>>
>>