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

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

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?

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