ethdev: stop the device before close
Checks
Commit Message
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.
lib/ethdev/rte_ethdev.c | 11 +++++++++++
1 file changed, 11 insertions(+)
Comments
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.
> @@ -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;
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;
>
>
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;
>>
>>
@@ -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;