[v3,27/29] ethdev: remove forcing stopped state upon close
Checks
Commit Message
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
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>
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.
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?
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.
@@ -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);