DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] ethdev: stop the device before close
@ 2021-10-20 10:47 Andrew Rybchenko
  2021-10-20 12:17 ` Thomas Monjalon
  2021-10-20 13:15 ` [dpdk-dev] [PATCH v2] ethdev: do not allow to close started device Andrew Rybchenko
  0 siblings, 2 replies; 8+ messages in thread
From: Andrew Rybchenko @ 2021-10-20 10:47 UTC (permalink / raw)
  To: Thomas Monjalon, Ferruh Yigit
  Cc: dev, Ivan Ilchenko, Maxime Coquelin, Chenbo Xia

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: stop the device before close
  2021-10-20 10:47 [dpdk-dev] [PATCH] ethdev: stop the device before close Andrew Rybchenko
@ 2021-10-20 12:17 ` Thomas Monjalon
  2021-10-20 12:24   ` Andrew Rybchenko
  2021-10-20 13:15 ` [dpdk-dev] [PATCH v2] ethdev: do not allow to close started device Andrew Rybchenko
  1 sibling, 1 reply; 8+ messages in thread
From: Thomas Monjalon @ 2021-10-20 12:17 UTC (permalink / raw)
  To: Andrew Rybchenko, Ivan Ilchenko
  Cc: Ferruh Yigit, dev, Maxime Coquelin, Chenbo Xia

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;




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: stop the device before close
  2021-10-20 12:17 ` Thomas Monjalon
@ 2021-10-20 12:24   ` Andrew Rybchenko
  2021-10-20 13:06     ` Andrew Rybchenko
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Rybchenko @ 2021-10-20 12:24 UTC (permalink / raw)
  To: Thomas Monjalon, Ivan Ilchenko
  Cc: Ferruh Yigit, dev, Maxime Coquelin, Chenbo Xia

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] ethdev: stop the device before close
  2021-10-20 12:24   ` Andrew Rybchenko
@ 2021-10-20 13:06     ` Andrew Rybchenko
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Rybchenko @ 2021-10-20 13:06 UTC (permalink / raw)
  To: Thomas Monjalon, Ivan Ilchenko
  Cc: Ferruh Yigit, dev, Maxime Coquelin, Chenbo Xia

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v2] ethdev: do not allow to close started device
  2021-10-20 10:47 [dpdk-dev] [PATCH] ethdev: stop the device before close Andrew Rybchenko
  2021-10-20 12:17 ` Thomas Monjalon
@ 2021-10-20 13:15 ` Andrew Rybchenko
  2021-10-20 13:36   ` Thomas Monjalon
  1 sibling, 1 reply; 8+ messages in thread
From: Andrew Rybchenko @ 2021-10-20 13:15 UTC (permalink / raw)
  To: Thomas Monjalon, Ferruh Yigit, Tetsuya Mukawa
  Cc: dev, Maxime Coquelin, Chenbo Xia, stable

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] ethdev: do not allow to close started device
  2021-10-20 13:15 ` [dpdk-dev] [PATCH v2] ethdev: do not allow to close started device Andrew Rybchenko
@ 2021-10-20 13:36   ` Thomas Monjalon
  2021-10-20 16:22     ` Ajit Khaparde
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Monjalon @ 2021-10-20 13:36 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: Ferruh Yigit, Tetsuya Mukawa, dev, Maxime Coquelin, Chenbo Xia, stable

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>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] ethdev: do not allow to close started device
  2021-10-20 13:36   ` Thomas Monjalon
@ 2021-10-20 16:22     ` Ajit Khaparde
  2021-10-20 17:25       ` Ferruh Yigit
  0 siblings, 1 reply; 8+ messages in thread
From: Ajit Khaparde @ 2021-10-20 16:22 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Andrew Rybchenko, Ferruh Yigit, Tetsuya Mukawa, dpdk-dev,
	Maxime Coquelin, Chenbo Xia, dpdk stable

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>

>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] ethdev: do not allow to close started device
  2021-10-20 16:22     ` Ajit Khaparde
@ 2021-10-20 17:25       ` Ferruh Yigit
  0 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2021-10-20 17:25 UTC (permalink / raw)
  To: Ajit Khaparde, Thomas Monjalon
  Cc: Andrew Rybchenko, Tetsuya Mukawa, dpdk-dev, Maxime Coquelin,
	Chenbo Xia, dpdk stable

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-10-20 17:25 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-20 10:47 [dpdk-dev] [PATCH] ethdev: stop the device before close Andrew Rybchenko
2021-10-20 12:17 ` Thomas Monjalon
2021-10-20 12:24   ` Andrew Rybchenko
2021-10-20 13:06     ` Andrew Rybchenko
2021-10-20 13:15 ` [dpdk-dev] [PATCH v2] ethdev: do not allow to close started device Andrew Rybchenko
2021-10-20 13:36   ` Thomas Monjalon
2021-10-20 16:22     ` Ajit Khaparde
2021-10-20 17:25       ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).