On 10/20/21 3:17 PM, Thomas Monjalon wrote:
> 20/10/2021 12:47, Andrew Rybchenko:
>> From: Ivan Ilchenko <ivan.ilche...@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.ilche...@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybche...@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;
>
>
