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

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

[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.

 lib/ethdev/rte_ethdev.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index a151c05849..b9f0938f20 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -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;
-- 
2.30.2

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

[Thomas Monjalon]

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;

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

[Andrew Rybchenko]

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

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

[Andrew Rybchenko]

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

[dpdk-dev] [PATCH v2] ethdev: do not allow to close started device

[Andrew Rybchenko]

Ethernet device must be stopped first before close in accordance
with the documentation.

Fixes: 980995f8cc56 ("ethdev: improve API comments of close and detach functions")
Cc: stable@dpdk.org

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.

 lib/ethdev/rte_ethdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 3b8ef9ef22..f981e0226a 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -1893,6 +1893,12 @@ rte_eth_dev_close(uint16_t port_id)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
+	if (dev->data->dev_started) {
+		RTE_ETHDEV_LOG(ERR, "Cannot close started device (port %u)\n",
+			       port_id);
+		return -EINVAL;
+	}
+
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_close, -ENOTSUP);
 	*lasterr = (*dev->dev_ops->dev_close)(dev);
 	if (*lasterr != 0)
-- 
2.30.2

Re: [dpdk-dev] [PATCH v2] ethdev: do not allow to close started device

[Thomas Monjalon]

20/10/2021 15:15, Andrew Rybchenko:
> Ethernet device must be stopped first before close in accordance
> with the documentation.
> 
> Fixes: 980995f8cc56 ("ethdev: improve API comments of close and detach functions")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Acked-by: Thomas Monjalon <thomas@monjalon.net>

Re: [dpdk-dev] [PATCH v2] ethdev: do not allow to close started device

[Ajit Khaparde]

On Wed, Oct 20, 2021 at 6:37 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 20/10/2021 15:15, Andrew Rybchenko:
> > Ethernet device must be stopped first before close in accordance
> > with the documentation.
> >
> > Fixes: 980995f8cc56 ("ethdev: improve API comments of close and detach functions")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

>
>

Re: [dpdk-dev] [PATCH v2] ethdev: do not allow to close started device

[Ferruh Yigit]

On 10/20/2021 5:22 PM, Ajit Khaparde wrote:
> On Wed, Oct 20, 2021 at 6:37 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>> 20/10/2021 15:15, Andrew Rybchenko:
>>> Ethernet device must be stopped first before close in accordance
>>> with the documentation.
>>>
>>> Fixes: 980995f8cc56 ("ethdev: improve API comments of close and detach functions")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>
>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> 

Applied to dpdk-next-net/main, thanks.