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