diff mbox series

[v3,27/29] ethdev: remove forcing stopped state upon close

Message ID 20200928231437.414489-28-thomas@monjalon.net (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series cleanup ethdev close operation |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Thomas Monjalon Sept. 28, 2020, 11:14 p.m. UTC 
  When closing a port, it is supposed to be already stopped,
and marked as such with "dev_started" state zeroed.

Resetting "dev_started" before calling the driver close operation
was hiding the case of not properly stopped port being closed.
The flag "dev_started" is not changed anymore in "rte_eth_dev_close()".

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Andrew Rybchenko Sept. 29, 2020, 10:44 a.m. UTC | #1 
On 9/29/20 2:14 AM, Thomas Monjalon wrote:
> When closing a port, it is supposed to be already stopped,
> and marked as such with "dev_started" state zeroed.
> 
> Resetting "dev_started" before calling the driver close operation
> was hiding the case of not properly stopped port being closed.
> The flag "dev_started" is not changed anymore in "rte_eth_dev_close()".
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
Ferruh Yigit Sept. 29, 2020, 4:01 p.m. UTC | #2 
On 9/29/2020 12:14 AM, Thomas Monjalon wrote:
> When closing a port, it is supposed to be already stopped,
> and marked as such with "dev_started" state zeroed.
> 
> Resetting "dev_started" before calling the driver close operation
> was hiding the case of not properly stopped port being closed.
> The flag "dev_started" is not changed anymore in "rte_eth_dev_close()".
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>   lib/librte_ethdev/rte_ethdev.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index d7668114ca..0b8e8e3e8d 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1716,7 +1716,6 @@ rte_eth_dev_close(uint16_t port_id)
>   	dev = &rte_eth_devices[port_id];
>   
>   	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_close);
> -	dev->data->dev_started = 0;
>   	(*dev->dev_ops->dev_close)(dev);
>   
>   	rte_ethdev_trace_close(port_id);
> 

The driver 'remove()' function may be calling the driver 'stop()' dev_ops 
internally, so the device will be stopped properly but the 'dev_started' status 
won't be updated because ethdev API is not called.

This API assumes device stopped and updates the state accordingly, it is not 
good but removing it also won't be good for the case device already stopped.

What do you think calling 'rte_eth_dev_stop()' from 'rte_eth_dev_close()'?
Although not sure how to handle driver 'remove()' case.
Thomas Monjalon Sept. 29, 2020, 4:06 p.m. UTC | #3 
29/09/2020 18:01, Ferruh Yigit:
> On 9/29/2020 12:14 AM, Thomas Monjalon wrote:
> > When closing a port, it is supposed to be already stopped,
> > and marked as such with "dev_started" state zeroed.
> > 
> > Resetting "dev_started" before calling the driver close operation
> > was hiding the case of not properly stopped port being closed.
> > The flag "dev_started" is not changed anymore in "rte_eth_dev_close()".
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> >   lib/librte_ethdev/rte_ethdev.c | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index d7668114ca..0b8e8e3e8d 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -1716,7 +1716,6 @@ rte_eth_dev_close(uint16_t port_id)
> >   	dev = &rte_eth_devices[port_id];
> >   
> >   	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_close);
> > -	dev->data->dev_started = 0;
> >   	(*dev->dev_ops->dev_close)(dev);
> >   
> >   	rte_ethdev_trace_close(port_id);
> > 
> 
> The driver 'remove()' function may be calling the driver 'stop()' dev_ops 
> internally, so the device will be stopped properly but the 'dev_started' status 
> won't be updated because ethdev API is not called.

If the driver is managing it internally, it should reset the state as well.

> This API assumes device stopped and updates the state accordingly, it is not 
> good but removing it also won't be good for the case device already stopped.
> 
> What do you think calling 'rte_eth_dev_stop()' from 'rte_eth_dev_close()'?

I think it would be confusing.
Better to let the application and the driver manage "stop"
at their best.

> Although not sure how to handle driver 'remove()' case.

What are you referring to exactly?
Ferruh Yigit Sept. 29, 2020, 4:39 p.m. UTC | #4 
On 9/29/2020 5:06 PM, Thomas Monjalon wrote:
> 29/09/2020 18:01, Ferruh Yigit:
>> On 9/29/2020 12:14 AM, Thomas Monjalon wrote:
>>> When closing a port, it is supposed to be already stopped,
>>> and marked as such with "dev_started" state zeroed.
>>>
>>> Resetting "dev_started" before calling the driver close operation
>>> was hiding the case of not properly stopped port being closed.
>>> The flag "dev_started" is not changed anymore in "rte_eth_dev_close()".
>>>
>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>> ---
>>>    lib/librte_ethdev/rte_ethdev.c | 1 -
>>>    1 file changed, 1 deletion(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>> index d7668114ca..0b8e8e3e8d 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -1716,7 +1716,6 @@ rte_eth_dev_close(uint16_t port_id)
>>>    	dev = &rte_eth_devices[port_id];
>>>    
>>>    	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_close);
>>> -	dev->data->dev_started = 0;
>>>    	(*dev->dev_ops->dev_close)(dev);
>>>    
>>>    	rte_ethdev_trace_close(port_id);
>>>
>>
>> The driver 'remove()' function may be calling the driver 'stop()' dev_ops
>> internally, so the device will be stopped properly but the 'dev_started' status
>> won't be updated because ethdev API is not called.
> 
> If the driver is managing it internally, it should reset the state as well.
> 

I think many PMD don't update the 'dev_started' right now.

>> This API assumes device stopped and updates the state accordingly, it is not
>> good but removing it also won't be good for the case device already stopped.
>>
>> What do you think calling 'rte_eth_dev_stop()' from 'rte_eth_dev_close()'?
> 
> I think it would be confusing.
> Better to let the application and the driver manage "stop"
> at their best.
> 

OK

>> Although not sure how to handle driver 'remove()' case.
> 
> What are you referring to exactly?
> 

This was questioning how to manage 'rte_eth_dev_stop()' call on driver 
'remove()' path. Not valid after above comment.
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index d7668114ca..0b8e8e3e8d 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1716,7 +1716,6 @@  rte_eth_dev_close(uint16_t port_id)
 	dev = &rte_eth_devices[port_id];
 
 	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_close);
-	dev->data->dev_started = 0;
 	(*dev->dev_ops->dev_close)(dev);
 
 	rte_ethdev_trace_close(port_id);