[RFC] lib/ethdev: add dev configured flag
Checks
Commit Message
Currently, if dev_configure is not invoked or fails to be invoked, users
can still invoke dev_start successfully. This patch adds a "dev_configured"
flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
lib/ethdev/rte_ethdev.c | 11 +++++++++++
lib/ethdev/rte_ethdev_core.h | 6 +++++-
2 files changed, 16 insertions(+), 1 deletion(-)
Comments
Hi, All & Ferruh
What do you think about this patch?
在 2021/5/8 16:00, Huisong Li 写道:
> Currently, if dev_configure is not invoked or fails to be invoked, users
> can still invoke dev_start successfully. This patch adds a "dev_configured"
> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> lib/ethdev/rte_ethdev.c | 11 +++++++++++
> lib/ethdev/rte_ethdev_core.h | 6 +++++-
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index a187976..7d74b17 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -1604,6 +1604,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
> }
>
> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q, dev_conf, 0);
> + dev->data->dev_configured = 1;
> +
> return 0;
> reset_queues:
> eth_dev_rx_queue_config(dev, 0);
> @@ -1614,6 +1616,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
> dev->data->mtu = old_mtu;
>
> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q, dev_conf, ret);
> + dev->data->dev_configured = 0;
> +
> return ret;
> }
>
> @@ -1749,6 +1753,13 @@ rte_eth_dev_start(uint16_t port_id)
>
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_start, -ENOTSUP);
>
> + if (dev->data->dev_configured == 0) {
> + RTE_ETHDEV_LOG(INFO,
> + "Device with port_id=%"PRIu16" is not configured.\n",
> + port_id);
> + return -EINVAL;
> + }
> +
> if (dev->data->dev_started != 0) {
> RTE_ETHDEV_LOG(INFO,
> "Device with port_id=%"PRIu16" already started\n",
> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
> index 4679d94..b508769 100644
> --- a/lib/ethdev/rte_ethdev_core.h
> +++ b/lib/ethdev/rte_ethdev_core.h
> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
> scattered_rx : 1, /**< RX of scattered packets is ON(1) / OFF(0) */
> all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
> dev_started : 1, /**< Device state: STARTED(1) / STOPPED(0). */
> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
> + dev_configured : 1;
> + /**< Device configuration state:
> + * CONFIGURED(1) / NOT CONFIGURED(0).
> + */
> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> /**< Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0). */
> uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];
Summary should start from "ethdev: "
Don't forget to include all maintainers in Cc the next time.
Just use --cc-cmd or --to-cmd options.
Adding Thomas.
On 5/8/21 11:00 AM, Huisong Li wrote:
> Currently, if dev_configure is not invoked or fails to be invoked, users
> can still invoke dev_start successfully. This patch adds a "dev_configured"
> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
In theory there is an indirect condition. If number of configured Tx
*and* Rx queues is 0, device is not configured.
I have no strong opinion on the topic. Extra flag requires
extra housekeeping. Indirect conditions are not always good
and could be a subject to change.
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> lib/ethdev/rte_ethdev.c | 11 +++++++++++
> lib/ethdev/rte_ethdev_core.h | 6 +++++-
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index a187976..7d74b17 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -1604,6 +1604,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
> }
>
> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q, dev_conf, 0);
> + dev->data->dev_configured = 1;
> +
> return 0;
> reset_queues:
> eth_dev_rx_queue_config(dev, 0);
> @@ -1614,6 +1616,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
> dev->data->mtu = old_mtu;
>
> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q, dev_conf, ret);
> + dev->data->dev_configured = 0;
> +
> return ret;
> }
>
> @@ -1749,6 +1753,13 @@ rte_eth_dev_start(uint16_t port_id)
>
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_start, -ENOTSUP);
>
> + if (dev->data->dev_configured == 0) {
> + RTE_ETHDEV_LOG(INFO,
> + "Device with port_id=%"PRIu16" is not configured.\n",
> + port_id);
> + return -EINVAL;
> + }
> +
> if (dev->data->dev_started != 0) {
> RTE_ETHDEV_LOG(INFO,
> "Device with port_id=%"PRIu16" already started\n",
> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
> index 4679d94..b508769 100644
> --- a/lib/ethdev/rte_ethdev_core.h
> +++ b/lib/ethdev/rte_ethdev_core.h
> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
> scattered_rx : 1, /**< RX of scattered packets is ON(1) / OFF(0) */
> all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
> dev_started : 1, /**< Device state: STARTED(1) / STOPPED(0). */
> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
> + dev_configured : 1;
> + /**< Device configuration state:
> + * CONFIGURED(1) / NOT CONFIGURED(0).
> + */
> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> /**< Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0). */
> uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>
在 2021/6/14 23:37, Andrew Rybchenko 写道:
> Summary should start from "ethdev: "
>
> Don't forget to include all maintainers in Cc the next time.
> Just use --cc-cmd or --to-cmd options.
ok, thanks!
>
> Adding Thomas.
>
> On 5/8/21 11:00 AM, Huisong Li wrote:
>> Currently, if dev_configure is not invoked or fails to be invoked, users
>> can still invoke dev_start successfully. This patch adds a
>> "dev_configured"
>> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
>
> In theory there is an indirect condition. If number of configured Tx
> *and* Rx queues is 0, device is not configured.
That's true. If the framework doesn't have this check, each driver needs
to do this.
But it's a common thing, and it's probably more reasonable to put it in
the ethdev layer.
>
> I have no strong opinion on the topic. Extra flag requires
> extra housekeeping. Indirect conditions are not always good
> and could be a subject to change.
>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> lib/ethdev/rte_ethdev.c | 11 +++++++++++
>> lib/ethdev/rte_ethdev_core.h | 6 +++++-
>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index a187976..7d74b17 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -1604,6 +1604,8 @@ rte_eth_dev_configure(uint16_t port_id,
>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>> }
>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
>> dev_conf, 0);
>> + dev->data->dev_configured = 1;
>> +
>> return 0;
>> reset_queues:
>> eth_dev_rx_queue_config(dev, 0);
>> @@ -1614,6 +1616,8 @@ rte_eth_dev_configure(uint16_t port_id,
>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>> dev->data->mtu = old_mtu;
>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
>> dev_conf, ret);
>> + dev->data->dev_configured = 0;
>> +
>> return ret;
>> }
>> @@ -1749,6 +1753,13 @@ rte_eth_dev_start(uint16_t port_id)
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_start, -ENOTSUP);
>> + if (dev->data->dev_configured == 0) {
>> + RTE_ETHDEV_LOG(INFO,
>> + "Device with port_id=%"PRIu16" is not configured.\n",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> if (dev->data->dev_started != 0) {
>> RTE_ETHDEV_LOG(INFO,
>> "Device with port_id=%"PRIu16" already started\n",
>> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
>> index 4679d94..b508769 100644
>> --- a/lib/ethdev/rte_ethdev_core.h
>> +++ b/lib/ethdev/rte_ethdev_core.h
>> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
>> scattered_rx : 1, /**< RX of scattered packets is ON(1) /
>> OFF(0) */
>> all_multicast : 1, /**< RX all multicast mode ON(1) /
>> OFF(0). */
>> dev_started : 1, /**< Device state: STARTED(1) /
>> STOPPED(0). */
>> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
>> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
>> + dev_configured : 1;
>> + /**< Device configuration state:
>> + * CONFIGURED(1) / NOT CONFIGURED(0).
>> + */
>> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>> /**< Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0). */
>> uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>>
>
> .
@Thomas, @Ferruh, I tend to accept it (with minor style fixes),
but I need your opinion on it before doing it.
Thanks,
Andrew.
On 6/29/21 5:27 AM, Huisong Li wrote:
>
> 在 2021/6/14 23:37, Andrew Rybchenko 写道:
>> Summary should start from "ethdev: "
>>
>> Don't forget to include all maintainers in Cc the next time.
>> Just use --cc-cmd or --to-cmd options.
> ok, thanks!
>>
>> Adding Thomas.
>>
>> On 5/8/21 11:00 AM, Huisong Li wrote:
>>> Currently, if dev_configure is not invoked or fails to be invoked, users
>>> can still invoke dev_start successfully. This patch adds a
>>> "dev_configured"
>>> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
>>
>> In theory there is an indirect condition. If number of configured Tx
>> *and* Rx queues is 0, device is not configured.
>
> That's true. If the framework doesn't have this check, each driver needs
> to do this.
>
> But it's a common thing, and it's probably more reasonable to put it in
> the ethdev layer.
>
>>
>> I have no strong opinion on the topic. Extra flag requires
>> extra housekeeping. Indirect conditions are not always good
>> and could be a subject to change.
>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> ---
>>> lib/ethdev/rte_ethdev.c | 11 +++++++++++
>>> lib/ethdev/rte_ethdev_core.h | 6 +++++-
>>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>> index a187976..7d74b17 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -1604,6 +1604,8 @@ rte_eth_dev_configure(uint16_t port_id,
>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>>> }
>>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
>>> dev_conf, 0);
>>> + dev->data->dev_configured = 1;
>>> +
>>> return 0;
>>> reset_queues:
>>> eth_dev_rx_queue_config(dev, 0);
>>> @@ -1614,6 +1616,8 @@ rte_eth_dev_configure(uint16_t port_id,
>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>>> dev->data->mtu = old_mtu;
>>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
>>> dev_conf, ret);
>>> + dev->data->dev_configured = 0;
>>> +
>>> return ret;
>>> }
>>> @@ -1749,6 +1753,13 @@ rte_eth_dev_start(uint16_t port_id)
>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_start, -ENOTSUP);
>>> + if (dev->data->dev_configured == 0) {
>>> + RTE_ETHDEV_LOG(INFO,
>>> + "Device with port_id=%"PRIu16" is not configured.\n",
>>> + port_id);
>>> + return -EINVAL;
>>> + }
>>> +
>>> if (dev->data->dev_started != 0) {
>>> RTE_ETHDEV_LOG(INFO,
>>> "Device with port_id=%"PRIu16" already started\n",
>>> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
>>> index 4679d94..b508769 100644
>>> --- a/lib/ethdev/rte_ethdev_core.h
>>> +++ b/lib/ethdev/rte_ethdev_core.h
>>> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
>>> scattered_rx : 1, /**< RX of scattered packets is ON(1) /
>>> OFF(0) */
>>> all_multicast : 1, /**< RX all multicast mode ON(1) /
>>> OFF(0). */
>>> dev_started : 1, /**< Device state: STARTED(1) /
>>> STOPPED(0). */
>>> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
>>> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
>>> + dev_configured : 1;
>>> + /**< Device configuration state:
>>> + * CONFIGURED(1) / NOT CONFIGURED(0).
>>> + */
>>> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>>> /**< Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0). */
>>> uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>>>
>>
>> .
On 7/2/2021 12:08 PM, Andrew Rybchenko wrote:
> @Thomas, @Ferruh, I tend to accept it (with minor style fixes),
> but I need your opinion on it before doing it.
>
I guess we were relying on the user/application to have correct order up until
now, it can be good to add this into the API. OK to add it for me.
> Thanks,
> Andrew.
>
> On 6/29/21 5:27 AM, Huisong Li wrote:
>>
>> 在 2021/6/14 23:37, Andrew Rybchenko 写道:
>>> Summary should start from "ethdev: "
>>>
>>> Don't forget to include all maintainers in Cc the next time.
>>> Just use --cc-cmd or --to-cmd options.
>> ok, thanks!
>>>
>>> Adding Thomas.
>>>
>>> On 5/8/21 11:00 AM, Huisong Li wrote:
>>>> Currently, if dev_configure is not invoked or fails to be invoked, users
>>>> can still invoke dev_start successfully. This patch adds a
>>>> "dev_configured"
>>>> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
>>>
>>> In theory there is an indirect condition. If number of configured Tx
>>> *and* Rx queues is 0, device is not configured.
>>
>> That's true. If the framework doesn't have this check, each driver needs
>> to do this.
>>
>> But it's a common thing, and it's probably more reasonable to put it in
>> the ethdev layer.
>>
>>>
>>> I have no strong opinion on the topic. Extra flag requires
>>> extra housekeeping. Indirect conditions are not always good
>>> and could be a subject to change.
>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> ---
>>>> lib/ethdev/rte_ethdev.c | 11 +++++++++++
>>>> lib/ethdev/rte_ethdev_core.h | 6 +++++-
>>>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>> index a187976..7d74b17 100644
>>>> --- a/lib/ethdev/rte_ethdev.c
>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>> @@ -1604,6 +1604,8 @@ rte_eth_dev_configure(uint16_t port_id,
>>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>>>> }
>>>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
>>>> dev_conf, 0);
>>>> + dev->data->dev_configured = 1;
>>>> +
>>>> return 0;
>>>> reset_queues:
>>>> eth_dev_rx_queue_config(dev, 0);
>>>> @@ -1614,6 +1616,8 @@ rte_eth_dev_configure(uint16_t port_id,
>>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>>>> dev->data->mtu = old_mtu;
>>>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
>>>> dev_conf, ret);
>>>> + dev->data->dev_configured = 0;
>>>> +
I would move it before trace function.
>>>> return ret;
>>>> }
>>>> @@ -1749,6 +1753,13 @@ rte_eth_dev_start(uint16_t port_id)
>>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_start, -ENOTSUP);
>>>> + if (dev->data->dev_configured == 0) {
>>>> + RTE_ETHDEV_LOG(INFO,
>>>> + "Device with port_id=%"PRIu16" is not configured.\n",
>>>> + port_id);
Should log type be warning/error?
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> if (dev->data->dev_started != 0) {
>>>> RTE_ETHDEV_LOG(INFO,
>>>> "Device with port_id=%"PRIu16" already started\n",
>>>> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
>>>> index 4679d94..b508769 100644
>>>> --- a/lib/ethdev/rte_ethdev_core.h
>>>> +++ b/lib/ethdev/rte_ethdev_core.h
>>>> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
>>>> scattered_rx : 1, /**< RX of scattered packets is ON(1) /
>>>> OFF(0) */
>>>> all_multicast : 1, /**< RX all multicast mode ON(1) /
>>>> OFF(0). */
>>>> dev_started : 1, /**< Device state: STARTED(1) /
>>>> STOPPED(0). */
>>>> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
>>>> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
>>>> + dev_configured : 1;
>>>> + /**< Device configuration state:
>>>> + * CONFIGURED(1) / NOT CONFIGURED(0).
>>>> + */
>>>> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>>>> /**< Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0). */
>>>> uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>>>>
>>>
>>> .
>
>
> On 7/2/2021 12:08 PM, Andrew Rybchenko wrote:
> > @Thomas, @Ferruh, I tend to accept it (with minor style fixes),
> > but I need your opinion on it before doing it.
> >
>
> I guess we were relying on the user/application to have correct order up until
> now, it can be good to add this into the API. OK to add it for me.
I don't know do we really need that flag in dev_data or not,
but if we do - probably better to reset it at dev_confgure() straight before
we start to make any changes in dev_data.
That way SP can also figure out that device is not configured yet, etc.
>
> > Thanks,
> > Andrew.
> >
> > On 6/29/21 5:27 AM, Huisong Li wrote:
> >>
> >> 在 2021/6/14 23:37, Andrew Rybchenko 写道:
> >>> Summary should start from "ethdev: "
> >>>
> >>> Don't forget to include all maintainers in Cc the next time.
> >>> Just use --cc-cmd or --to-cmd options.
> >> ok, thanks!
> >>>
> >>> Adding Thomas.
> >>>
> >>> On 5/8/21 11:00 AM, Huisong Li wrote:
> >>>> Currently, if dev_configure is not invoked or fails to be invoked, users
> >>>> can still invoke dev_start successfully. This patch adds a
> >>>> "dev_configured"
> >>>> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
> >>>
> >>> In theory there is an indirect condition. If number of configured Tx
> >>> *and* Rx queues is 0, device is not configured.
> >>
> >> That's true. If the framework doesn't have this check, each driver needs
> >> to do this.
> >>
> >> But it's a common thing, and it's probably more reasonable to put it in
> >> the ethdev layer.
> >>
> >>>
> >>> I have no strong opinion on the topic. Extra flag requires
> >>> extra housekeeping. Indirect conditions are not always good
> >>> and could be a subject to change.
> >>>
> >>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >>>> ---
> >>>> lib/ethdev/rte_ethdev.c | 11 +++++++++++
> >>>> lib/ethdev/rte_ethdev_core.h | 6 +++++-
> >>>> 2 files changed, 16 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> >>>> index a187976..7d74b17 100644
> >>>> --- a/lib/ethdev/rte_ethdev.c
> >>>> +++ b/lib/ethdev/rte_ethdev.c
> >>>> @@ -1604,6 +1604,8 @@ rte_eth_dev_configure(uint16_t port_id,
> >>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
> >>>> }
> >>>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
> >>>> dev_conf, 0);
> >>>> + dev->data->dev_configured = 1;
> >>>> +
> >>>> return 0;
> >>>> reset_queues:
> >>>> eth_dev_rx_queue_config(dev, 0);
> >>>> @@ -1614,6 +1616,8 @@ rte_eth_dev_configure(uint16_t port_id,
> >>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
> >>>> dev->data->mtu = old_mtu;
> >>>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
> >>>> dev_conf, ret);
> >>>> + dev->data->dev_configured = 0;
> >>>> +
>
> I would move it before trace function.
>
> >>>> return ret;
> >>>> }
> >>>> @@ -1749,6 +1753,13 @@ rte_eth_dev_start(uint16_t port_id)
> >>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_start, -ENOTSUP);
> >>>> + if (dev->data->dev_configured == 0) {
> >>>> + RTE_ETHDEV_LOG(INFO,
> >>>> + "Device with port_id=%"PRIu16" is not configured.\n",
> >>>> + port_id);
>
> Should log type be warning/error?
>
> >>>> + return -EINVAL;
> >>>> + }
> >>>> +
> >>>> if (dev->data->dev_started != 0) {
> >>>> RTE_ETHDEV_LOG(INFO,
> >>>> "Device with port_id=%"PRIu16" already started\n",
> >>>> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
> >>>> index 4679d94..b508769 100644
> >>>> --- a/lib/ethdev/rte_ethdev_core.h
> >>>> +++ b/lib/ethdev/rte_ethdev_core.h
> >>>> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
> >>>> scattered_rx : 1, /**< RX of scattered packets is ON(1) /
> >>>> OFF(0) */
> >>>> all_multicast : 1, /**< RX all multicast mode ON(1) /
> >>>> OFF(0). */
> >>>> dev_started : 1, /**< Device state: STARTED(1) /
> >>>> STOPPED(0). */
> >>>> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
> >>>> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
> >>>> + dev_configured : 1;
> >>>> + /**< Device configuration state:
> >>>> + * CONFIGURED(1) / NOT CONFIGURED(0).
> >>>> + */
> >>>> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> >>>> /**< Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0). */
> >>>> uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> >>>>
> >>>
> >>> .
> >
在 2021/7/2 21:23, Ananyev, Konstantin 写道:
>
>> On 7/2/2021 12:08 PM, Andrew Rybchenko wrote:
>>> @Thomas, @Ferruh, I tend to accept it (with minor style fixes),
>>> but I need your opinion on it before doing it.
>>>
>> I guess we were relying on the user/application to have correct order up until
>> now, it can be good to add this into the API. OK to add it for me.
> I don't know do we really need that flag in dev_data or not,
> but if we do - probably better to reset it at dev_confgure() straight before
> we start to make any changes in dev_data.
Sorry, I don't get you. Some fields in rte_eth_dev_data are initialized
firstly in the probe phase.
Do you mean to add clear this flag at the beginning of dev_configure()?
> That way SP can also figure out that device is not configured yet, etc.
>
>>> Thanks,
>>> Andrew.
>>>
>>> On 6/29/21 5:27 AM, Huisong Li wrote:
>>>> 在 2021/6/14 23:37, Andrew Rybchenko 写道:
>>>>> Summary should start from "ethdev: "
>>>>>
>>>>> Don't forget to include all maintainers in Cc the next time.
>>>>> Just use --cc-cmd or --to-cmd options.
>>>> ok, thanks!
>>>>> Adding Thomas.
>>>>>
>>>>> On 5/8/21 11:00 AM, Huisong Li wrote:
>>>>>> Currently, if dev_configure is not invoked or fails to be invoked, users
>>>>>> can still invoke dev_start successfully. This patch adds a
>>>>>> "dev_configured"
>>>>>> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
>>>>> In theory there is an indirect condition. If number of configured Tx
>>>>> *and* Rx queues is 0, device is not configured.
>>>> That's true. If the framework doesn't have this check, each driver needs
>>>> to do this.
>>>>
>>>> But it's a common thing, and it's probably more reasonable to put it in
>>>> the ethdev layer.
>>>>
>>>>> I have no strong opinion on the topic. Extra flag requires
>>>>> extra housekeeping. Indirect conditions are not always good
>>>>> and could be a subject to change.
>>>>>
>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>> ---
>>>>>> lib/ethdev/rte_ethdev.c | 11 +++++++++++
>>>>>> lib/ethdev/rte_ethdev_core.h | 6 +++++-
>>>>>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>>>> index a187976..7d74b17 100644
>>>>>> --- a/lib/ethdev/rte_ethdev.c
>>>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>>>> @@ -1604,6 +1604,8 @@ rte_eth_dev_configure(uint16_t port_id,
>>>>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>>>>>> }
>>>>>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
>>>>>> dev_conf, 0);
>>>>>> + dev->data->dev_configured = 1;
>>>>>> +
>>>>>> return 0;
>>>>>> reset_queues:
>>>>>> eth_dev_rx_queue_config(dev, 0);
>>>>>> @@ -1614,6 +1616,8 @@ rte_eth_dev_configure(uint16_t port_id,
>>>>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>>>>>> dev->data->mtu = old_mtu;
>>>>>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
>>>>>> dev_conf, ret);
>>>>>> + dev->data->dev_configured = 0;
>>>>>> +
>> I would move it before trace function.
>>
>>>>>> return ret;
>>>>>> }
>>>>>> @@ -1749,6 +1753,13 @@ rte_eth_dev_start(uint16_t port_id)
>>>>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_start, -ENOTSUP);
>>>>>> + if (dev->data->dev_configured == 0) {
>>>>>> + RTE_ETHDEV_LOG(INFO,
>>>>>> + "Device with port_id=%"PRIu16" is not configured.\n",
>>>>>> + port_id);
>> Should log type be warning/error?
>>
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> if (dev->data->dev_started != 0) {
>>>>>> RTE_ETHDEV_LOG(INFO,
>>>>>> "Device with port_id=%"PRIu16" already started\n",
>>>>>> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
>>>>>> index 4679d94..b508769 100644
>>>>>> --- a/lib/ethdev/rte_ethdev_core.h
>>>>>> +++ b/lib/ethdev/rte_ethdev_core.h
>>>>>> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
>>>>>> scattered_rx : 1, /**< RX of scattered packets is ON(1) /
>>>>>> OFF(0) */
>>>>>> all_multicast : 1, /**< RX all multicast mode ON(1) /
>>>>>> OFF(0). */
>>>>>> dev_started : 1, /**< Device state: STARTED(1) /
>>>>>> STOPPED(0). */
>>>>>> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
>>>>>> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
>>>>>> + dev_configured : 1;
>>>>>> + /**< Device configuration state:
>>>>>> + * CONFIGURED(1) / NOT CONFIGURED(0).
>>>>>> + */
>>>>>> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>>>>>> /**< Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0). */
>>>>>> uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>>>>>>
>>>>> .
>
> 在 2021/7/2 21:23, Ananyev, Konstantin 写道:
> >
> >> On 7/2/2021 12:08 PM, Andrew Rybchenko wrote:
> >>> @Thomas, @Ferruh, I tend to accept it (with minor style fixes),
> >>> but I need your opinion on it before doing it.
> >>>
> >> I guess we were relying on the user/application to have correct order up until
> >> now, it can be good to add this into the API. OK to add it for me.
> > I don't know do we really need that flag in dev_data or not,
> > but if we do - probably better to reset it at dev_confgure() straight before
> > we start to make any changes in dev_data.
>
> Sorry, I don't get you. Some fields in rte_eth_dev_data are initialized
> firstly in the probe phase.
>
> Do you mean to add clear this flag at the beginning of dev_configure()?
Yes, just before we start to modify things.
>
> > That way SP can also figure out that device is not configured yet, etc.
> >
> >>> Thanks,
> >>> Andrew.
> >>>
> >>> On 6/29/21 5:27 AM, Huisong Li wrote:
> >>>> 在 2021/6/14 23:37, Andrew Rybchenko 写道:
> >>>>> Summary should start from "ethdev: "
> >>>>>
> >>>>> Don't forget to include all maintainers in Cc the next time.
> >>>>> Just use --cc-cmd or --to-cmd options.
> >>>> ok, thanks!
> >>>>> Adding Thomas.
> >>>>>
> >>>>> On 5/8/21 11:00 AM, Huisong Li wrote:
> >>>>>> Currently, if dev_configure is not invoked or fails to be invoked, users
> >>>>>> can still invoke dev_start successfully. This patch adds a
> >>>>>> "dev_configured"
> >>>>>> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
> >>>>> In theory there is an indirect condition. If number of configured Tx
> >>>>> *and* Rx queues is 0, device is not configured.
> >>>> That's true. If the framework doesn't have this check, each driver needs
> >>>> to do this.
> >>>>
> >>>> But it's a common thing, and it's probably more reasonable to put it in
> >>>> the ethdev layer.
> >>>>
> >>>>> I have no strong opinion on the topic. Extra flag requires
> >>>>> extra housekeeping. Indirect conditions are not always good
> >>>>> and could be a subject to change.
> >>>>>
> >>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >>>>>> ---
> >>>>>> lib/ethdev/rte_ethdev.c | 11 +++++++++++
> >>>>>> lib/ethdev/rte_ethdev_core.h | 6 +++++-
> >>>>>> 2 files changed, 16 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> >>>>>> index a187976..7d74b17 100644
> >>>>>> --- a/lib/ethdev/rte_ethdev.c
> >>>>>> +++ b/lib/ethdev/rte_ethdev.c
> >>>>>> @@ -1604,6 +1604,8 @@ rte_eth_dev_configure(uint16_t port_id,
> >>>>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
> >>>>>> }
> >>>>>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
> >>>>>> dev_conf, 0);
> >>>>>> + dev->data->dev_configured = 1;
> >>>>>> +
> >>>>>> return 0;
> >>>>>> reset_queues:
> >>>>>> eth_dev_rx_queue_config(dev, 0);
> >>>>>> @@ -1614,6 +1616,8 @@ rte_eth_dev_configure(uint16_t port_id,
> >>>>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
> >>>>>> dev->data->mtu = old_mtu;
> >>>>>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
> >>>>>> dev_conf, ret);
> >>>>>> + dev->data->dev_configured = 0;
> >>>>>> +
> >> I would move it before trace function.
> >>
> >>>>>> return ret;
> >>>>>> }
> >>>>>> @@ -1749,6 +1753,13 @@ rte_eth_dev_start(uint16_t port_id)
> >>>>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_start, -ENOTSUP);
> >>>>>> + if (dev->data->dev_configured == 0) {
> >>>>>> + RTE_ETHDEV_LOG(INFO,
> >>>>>> + "Device with port_id=%"PRIu16" is not configured.\n",
> >>>>>> + port_id);
> >> Should log type be warning/error?
> >>
> >>>>>> + return -EINVAL;
> >>>>>> + }
> >>>>>> +
> >>>>>> if (dev->data->dev_started != 0) {
> >>>>>> RTE_ETHDEV_LOG(INFO,
> >>>>>> "Device with port_id=%"PRIu16" already started\n",
> >>>>>> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
> >>>>>> index 4679d94..b508769 100644
> >>>>>> --- a/lib/ethdev/rte_ethdev_core.h
> >>>>>> +++ b/lib/ethdev/rte_ethdev_core.h
> >>>>>> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
> >>>>>> scattered_rx : 1, /**< RX of scattered packets is ON(1) /
> >>>>>> OFF(0) */
> >>>>>> all_multicast : 1, /**< RX all multicast mode ON(1) /
> >>>>>> OFF(0). */
> >>>>>> dev_started : 1, /**< Device state: STARTED(1) /
> >>>>>> STOPPED(0). */
> >>>>>> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
> >>>>>> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
> >>>>>> + dev_configured : 1;
> >>>>>> + /**< Device configuration state:
> >>>>>> + * CONFIGURED(1) / NOT CONFIGURED(0).
> >>>>>> + */
> >>>>>> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> >>>>>> /**< Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0). */
> >>>>>> uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> >>>>>>
> >>>>> .
08/05/2021 10:00, Huisong Li:
> Currently, if dev_configure is not invoked or fails to be invoked, users
> can still invoke dev_start successfully. This patch adds a "dev_configured"
> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
[...]
> --- a/lib/ethdev/rte_ethdev_core.h
> +++ b/lib/ethdev/rte_ethdev_core.h
> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
> scattered_rx : 1, /**< RX of scattered packets is ON(1) / OFF(0) */
> all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
> dev_started : 1, /**< Device state: STARTED(1) / STOPPED(0). */
> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
> + dev_configured : 1;
> + /**< Device configuration state:
> + * CONFIGURED(1) / NOT CONFIGURED(0).
> + */
Why not using "enum rte_eth_dev_state"?
Because rte_eth_dev.state is not shared between processes?
在 2021/7/3 19:04, Ananyev, Konstantin 写道:
>> 在 2021/7/2 21:23, Ananyev, Konstantin 写道:
>>>> On 7/2/2021 12:08 PM, Andrew Rybchenko wrote:
>>>>> @Thomas, @Ferruh, I tend to accept it (with minor style fixes),
>>>>> but I need your opinion on it before doing it.
>>>>>
>>>> I guess we were relying on the user/application to have correct order up until
>>>> now, it can be good to add this into the API. OK to add it for me.
>>> I don't know do we really need that flag in dev_data or not,
>>> but if we do - probably better to reset it at dev_confgure() straight before
>>> we start to make any changes in dev_data.
>> Sorry, I don't get you. Some fields in rte_eth_dev_data are initialized
>> firstly in the probe phase.
>>
>> Do you mean to add clear this flag at the beginning of dev_configure()?
> Yes, just before we start to modify things.
In this patch, this flag has been cleared for all scenarios where the
rte_eth_dev_data modification fails in the dev_configure().
And it is set to 1 when dev_configure() is configured successfully.
Please check the rollback. Thanks😁
>
>>> That way SP can also figure out that device is not configured yet, etc.
>>>
>>>>> Thanks,
>>>>> Andrew.
>>>>>
>>>>> On 6/29/21 5:27 AM, Huisong Li wrote:
>>>>>> 在 2021/6/14 23:37, Andrew Rybchenko 写道:
>>>>>>> Summary should start from "ethdev: "
>>>>>>>
>>>>>>> Don't forget to include all maintainers in Cc the next time.
>>>>>>> Just use --cc-cmd or --to-cmd options.
>>>>>> ok, thanks!
>>>>>>> Adding Thomas.
>>>>>>>
>>>>>>> On 5/8/21 11:00 AM, Huisong Li wrote:
>>>>>>>> Currently, if dev_configure is not invoked or fails to be invoked, users
>>>>>>>> can still invoke dev_start successfully. This patch adds a
>>>>>>>> "dev_configured"
>>>>>>>> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
>>>>>>> In theory there is an indirect condition. If number of configured Tx
>>>>>>> *and* Rx queues is 0, device is not configured.
>>>>>> That's true. If the framework doesn't have this check, each driver needs
>>>>>> to do this.
>>>>>>
>>>>>> But it's a common thing, and it's probably more reasonable to put it in
>>>>>> the ethdev layer.
>>>>>>
>>>>>>> I have no strong opinion on the topic. Extra flag requires
>>>>>>> extra housekeeping. Indirect conditions are not always good
>>>>>>> and could be a subject to change.
>>>>>>>
>>>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>>>> ---
>>>>>>>> lib/ethdev/rte_ethdev.c | 11 +++++++++++
>>>>>>>> lib/ethdev/rte_ethdev_core.h | 6 +++++-
>>>>>>>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>>>>>> index a187976..7d74b17 100644
>>>>>>>> --- a/lib/ethdev/rte_ethdev.c
>>>>>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>>>>>> @@ -1604,6 +1604,8 @@ rte_eth_dev_configure(uint16_t port_id,
>>>>>>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>>>>>>>> }
>>>>>>>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
>>>>>>>> dev_conf, 0);
>>>>>>>> + dev->data->dev_configured = 1;
>>>>>>>> +
>>>>>>>> return 0;
>>>>>>>> reset_queues:
>>>>>>>> eth_dev_rx_queue_config(dev, 0);
>>>>>>>> @@ -1614,6 +1616,8 @@ rte_eth_dev_configure(uint16_t port_id,
>>>>>>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>>>>>>>> dev->data->mtu = old_mtu;
>>>>>>>> rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q,
>>>>>>>> dev_conf, ret);
>>>>>>>> + dev->data->dev_configured = 0;
>>>>>>>> +
>>>> I would move it before trace function.
>>>>
>>>>>>>> return ret;
>>>>>>>> }
>>>>>>>> @@ -1749,6 +1753,13 @@ rte_eth_dev_start(uint16_t port_id)
>>>>>>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_start, -ENOTSUP);
>>>>>>>> + if (dev->data->dev_configured == 0) {
>>>>>>>> + RTE_ETHDEV_LOG(INFO,
>>>>>>>> + "Device with port_id=%"PRIu16" is not configured.\n",
>>>>>>>> + port_id);
>>>> Should log type be warning/error?
>>>>
>>>>>>>> + return -EINVAL;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> if (dev->data->dev_started != 0) {
>>>>>>>> RTE_ETHDEV_LOG(INFO,
>>>>>>>> "Device with port_id=%"PRIu16" already started\n",
>>>>>>>> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
>>>>>>>> index 4679d94..b508769 100644
>>>>>>>> --- a/lib/ethdev/rte_ethdev_core.h
>>>>>>>> +++ b/lib/ethdev/rte_ethdev_core.h
>>>>>>>> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
>>>>>>>> scattered_rx : 1, /**< RX of scattered packets is ON(1) /
>>>>>>>> OFF(0) */
>>>>>>>> all_multicast : 1, /**< RX all multicast mode ON(1) /
>>>>>>>> OFF(0). */
>>>>>>>> dev_started : 1, /**< Device state: STARTED(1) /
>>>>>>>> STOPPED(0). */
>>>>>>>> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
>>>>>>>> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
>>>>>>>> + dev_configured : 1;
>>>>>>>> + /**< Device configuration state:
>>>>>>>> + * CONFIGURED(1) / NOT CONFIGURED(0).
>>>>>>>> + */
>>>>>>>> uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>>>>>>>> /**< Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0). */
>>>>>>>> uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];
>>>>>>>>
>>>>>>> .
在 2021/7/5 4:05, Thomas Monjalon 写道:
> 08/05/2021 10:00, Huisong Li:
>> Currently, if dev_configure is not invoked or fails to be invoked, users
>> can still invoke dev_start successfully. This patch adds a "dev_configured"
>> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
> [...]
>> --- a/lib/ethdev/rte_ethdev_core.h
>> +++ b/lib/ethdev/rte_ethdev_core.h
>> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
>> scattered_rx : 1, /**< RX of scattered packets is ON(1) / OFF(0) */
>> all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
>> dev_started : 1, /**< Device state: STARTED(1) / STOPPED(0). */
>> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
>> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
>> + dev_configured : 1;
>> + /**< Device configuration state:
>> + * CONFIGURED(1) / NOT CONFIGURED(0).
>> + */
> Why not using "enum rte_eth_dev_state"?
> Because rte_eth_dev.state is not shared between processes?
It doesn't feel right. "enum rte_eth_dev_state" is private to the
primary and secondary processes and can be independently controlled.
However, the secondary process does not make resource allocations and
does not call dev_configure().
These are done by the primary process and can be obtained or used by
the secondary process.
Like "dev_started" in struct rte_eth_dev_data.
>
>
>
> .
05/07/2021 05:18, Huisong Li:
> 在 2021/7/5 4:05, Thomas Monjalon 写道:
> > 08/05/2021 10:00, Huisong Li:
> >> Currently, if dev_configure is not invoked or fails to be invoked, users
> >> can still invoke dev_start successfully. This patch adds a "dev_configured"
> >> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
> > [...]
> >> --- a/lib/ethdev/rte_ethdev_core.h
> >> +++ b/lib/ethdev/rte_ethdev_core.h
> >> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
> >> scattered_rx : 1, /**< RX of scattered packets is ON(1) / OFF(0) */
> >> all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
> >> dev_started : 1, /**< Device state: STARTED(1) / STOPPED(0). */
> >> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
> >> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
> >> + dev_configured : 1;
> >> + /**< Device configuration state:
> >> + * CONFIGURED(1) / NOT CONFIGURED(0).
> >> + */
> > Why not using "enum rte_eth_dev_state"?
> > Because rte_eth_dev.state is not shared between processes?
>
> It doesn't feel right. "enum rte_eth_dev_state" is private to the
> primary and secondary processes and can be independently controlled.
>
> However, the secondary process does not make resource allocations and
> does not call dev_configure().
>
> These are done by the primary process and can be obtained or used by
> the secondary process.
>
> Like "dev_started" in struct rte_eth_dev_data.
That's a good reason, thanks.
On 7/5/21 6:03 AM, Huisong Li wrote:
>
> 在 2021/7/3 19:04, Ananyev, Konstantin 写道:
>>> 在 2021/7/2 21:23, Ananyev, Konstantin 写道:
>>>>> On 7/2/2021 12:08 PM, Andrew Rybchenko wrote:
>>>>>> @Thomas, @Ferruh, I tend to accept it (with minor style fixes),
>>>>>> but I need your opinion on it before doing it.
>>>>>>
>>>>> I guess we were relying on the user/application to have correct
>>>>> order up until
>>>>> now, it can be good to add this into the API. OK to add it for me.
>>>> I don't know do we really need that flag in dev_data or not,
>>>> but if we do - probably better to reset it at dev_confgure()
>>>> straight before
>>>> we start to make any changes in dev_data.
>>> Sorry, I don't get you. Some fields in rte_eth_dev_data are initialized
>>> firstly in the probe phase.
>>>
>>> Do you mean to add clear this flag at the beginning of dev_configure()?
>> Yes, just before we start to modify things.
>
> In this patch, this flag has been cleared for all scenarios where the
> rte_eth_dev_data modification fails in the dev_configure().
>
> And it is set to 1 when dev_configure() is configured successfully.
>
> Please check the rollback. Thanks😁
I guess Konstantin means the case when user re-configures
the device which has been configured before and the operation
fails. I'm not 100% what should be the state of the flag when
dev_configure callback is executed. I'd say that it should be
0 when the first configure happens and should be 1 in the
case of reconfigure. I'll try to review it carefully when
non-RFC version of the patch is available.
Hi Huisong,
On 7/5/21 9:07 AM, Thomas Monjalon wrote:
> 05/07/2021 05:18, Huisong Li:
>> 在 2021/7/5 4:05, Thomas Monjalon 写道:
>>> 08/05/2021 10:00, Huisong Li:
>>>> Currently, if dev_configure is not invoked or fails to be invoked, users
>>>> can still invoke dev_start successfully. This patch adds a "dev_configured"
>>>> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
>>> [...]
>>>> --- a/lib/ethdev/rte_ethdev_core.h
>>>> +++ b/lib/ethdev/rte_ethdev_core.h
>>>> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
>>>> scattered_rx : 1, /**< RX of scattered packets is ON(1) / OFF(0) */
>>>> all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
>>>> dev_started : 1, /**< Device state: STARTED(1) / STOPPED(0). */
>>>> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
>>>> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
>>>> + dev_configured : 1;
>>>> + /**< Device configuration state:
>>>> + * CONFIGURED(1) / NOT CONFIGURED(0).
>>>> + */
>>> Why not using "enum rte_eth_dev_state"?
>>> Because rte_eth_dev.state is not shared between processes?
>>
>> It doesn't feel right. "enum rte_eth_dev_state" is private to the
>> primary and secondary processes and can be independently controlled.
>>
>> However, the secondary process does not make resource allocations and
>> does not call dev_configure().
>>
>> These are done by the primary process and can be obtained or used by
>> the secondary process.
>>
>> Like "dev_started" in struct rte_eth_dev_data.
>
> That's a good reason, thanks.
>
Please, send non-RFC version of the patch with fixed summary
(w/o "lib/") and above reason mentioned in the patch
description as well.
> On 7/5/21 6:03 AM, Huisong Li wrote:
> >
> > 在 2021/7/3 19:04, Ananyev, Konstantin 写道:
> >>> 在 2021/7/2 21:23, Ananyev, Konstantin 写道:
> >>>>> On 7/2/2021 12:08 PM, Andrew Rybchenko wrote:
> >>>>>> @Thomas, @Ferruh, I tend to accept it (with minor style fixes),
> >>>>>> but I need your opinion on it before doing it.
> >>>>>>
> >>>>> I guess we were relying on the user/application to have correct
> >>>>> order up until
> >>>>> now, it can be good to add this into the API. OK to add it for me.
> >>>> I don't know do we really need that flag in dev_data or not,
> >>>> but if we do - probably better to reset it at dev_confgure()
> >>>> straight before
> >>>> we start to make any changes in dev_data.
> >>> Sorry, I don't get you. Some fields in rte_eth_dev_data are initialized
> >>> firstly in the probe phase.
> >>>
> >>> Do you mean to add clear this flag at the beginning of dev_configure()?
> >> Yes, just before we start to modify things.
> >
> > In this patch, this flag has been cleared for all scenarios where the
> > rte_eth_dev_data modification fails in the dev_configure().
I understand that.
What I am saying: at first call to dev_confgiure() you execute it with
dev_data->confgiured == 0.
On second and subsequent calls - it could be either 0 or 1,
depending how previous dev_confgiure() had finished.
I think it would be good to keep it always the same,
to avoid any non-anticipated behaviour.
> > And it is set to 1 when dev_configure() is configured successfully.
> >
> > Please check the rollback. Thanks😁
>
> I guess Konstantin means the case when user re-configures
> the device which has been configured before and the operation
> fails. I'm not 100% what should be the state of the flag when
> dev_configure callback is executed. I'd say that it should be
> 0 when the first configure happens and should be 1 in the
> case of reconfigure. I'll try to review it carefully when
> non-RFC version of the patch is available.
在 2021/7/5 19:22, Ananyev, Konstantin 写道:
>
>> On 7/5/21 6:03 AM, Huisong Li wrote:
>>> 在 2021/7/3 19:04, Ananyev, Konstantin 写道:
>>>>> 在 2021/7/2 21:23, Ananyev, Konstantin 写道:
>>>>>>> On 7/2/2021 12:08 PM, Andrew Rybchenko wrote:
>>>>>>>> @Thomas, @Ferruh, I tend to accept it (with minor style fixes),
>>>>>>>> but I need your opinion on it before doing it.
>>>>>>>>
>>>>>>> I guess we were relying on the user/application to have correct
>>>>>>> order up until
>>>>>>> now, it can be good to add this into the API. OK to add it for me.
>>>>>> I don't know do we really need that flag in dev_data or not,
>>>>>> but if we do - probably better to reset it at dev_confgure()
>>>>>> straight before
>>>>>> we start to make any changes in dev_data.
>>>>> Sorry, I don't get you. Some fields in rte_eth_dev_data are initialized
>>>>> firstly in the probe phase.
>>>>>
>>>>> Do you mean to add clear this flag at the beginning of dev_configure()?
>>>> Yes, just before we start to modify things.
>>> In this patch, this flag has been cleared for all scenarios where the
>>> rte_eth_dev_data modification fails in the dev_configure().
> I understand that.
> What I am saying: at first call to dev_confgiure() you execute it with
> dev_data->confgiured == 0.
> On second and subsequent calls - it could be either 0 or 1,
> depending how previous dev_confgiure() had finished.
> I think it would be good to keep it always the same,
> to avoid any non-anticipated behaviour.
I get it.
The current patch can ensure that this flag is 1 when dev_configure() is
called successfully,
and is 0 when dev_configure() isn't called or dev_data fails to be
modified.
But there is a drawback, as you say. I will fix it in non-RFC version.
>>> And it is set to 1 when dev_configure() is configured successfully.
>>>
>>> Please check the rollback. Thanks😁
>
>
>> I guess Konstantin means the case when user re-configures
>> the device which has been configured before and the operation
>> fails. I'm not 100% what should be the state of the flag when
>> dev_configure callback is executed. I'd say that it should be
>> 0 when the first configure happens and should be 1 in the
>> case of reconfigure. I'll try to review it carefully when
>> non-RFC version of the patch is available.
在 2021/7/5 17:50, Andrew Rybchenko 写道:
> Hi Huisong,
>
> On 7/5/21 9:07 AM, Thomas Monjalon wrote:
>> 05/07/2021 05:18, Huisong Li:
>>> 在 2021/7/5 4:05, Thomas Monjalon 写道:
>>>> 08/05/2021 10:00, Huisong Li:
>>>>> Currently, if dev_configure is not invoked or fails to be invoked, users
>>>>> can still invoke dev_start successfully. This patch adds a "dev_configured"
>>>>> flag in "rte_eth_dev_data" to control whether dev_start can be invoked.
>>>> [...]
>>>>> --- a/lib/ethdev/rte_ethdev_core.h
>>>>> +++ b/lib/ethdev/rte_ethdev_core.h
>>>>> @@ -167,7 +167,11 @@ struct rte_eth_dev_data {
>>>>> scattered_rx : 1, /**< RX of scattered packets is ON(1) / OFF(0) */
>>>>> all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
>>>>> dev_started : 1, /**< Device state: STARTED(1) / STOPPED(0). */
>>>>> - lro : 1; /**< RX LRO is ON(1) / OFF(0) */
>>>>> + lro : 1, /**< RX LRO is ON(1) / OFF(0) */
>>>>> + dev_configured : 1;
>>>>> + /**< Device configuration state:
>>>>> + * CONFIGURED(1) / NOT CONFIGURED(0).
>>>>> + */
>>>> Why not using "enum rte_eth_dev_state"?
>>>> Because rte_eth_dev.state is not shared between processes?
>>> It doesn't feel right. "enum rte_eth_dev_state" is private to the
>>> primary and secondary processes and can be independently controlled.
>>>
>>> However, the secondary process does not make resource allocations and
>>> does not call dev_configure().
>>>
>>> These are done by the primary process and can be obtained or used by
>>> the secondary process.
>>>
>>> Like "dev_started" in struct rte_eth_dev_data.
>> That's a good reason, thanks.
>>
> Please, send non-RFC version of the patch with fixed summary
> (w/o "lib/") and above reason mentioned in the patch
> description as well.
ok
> .
@@ -1604,6 +1604,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
}
rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q, dev_conf, 0);
+ dev->data->dev_configured = 1;
+
return 0;
reset_queues:
eth_dev_rx_queue_config(dev, 0);
@@ -1614,6 +1616,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
dev->data->mtu = old_mtu;
rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q, dev_conf, ret);
+ dev->data->dev_configured = 0;
+
return ret;
}
@@ -1749,6 +1753,13 @@ rte_eth_dev_start(uint16_t port_id)
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_start, -ENOTSUP);
+ if (dev->data->dev_configured == 0) {
+ RTE_ETHDEV_LOG(INFO,
+ "Device with port_id=%"PRIu16" is not configured.\n",
+ port_id);
+ return -EINVAL;
+ }
+
if (dev->data->dev_started != 0) {
RTE_ETHDEV_LOG(INFO,
"Device with port_id=%"PRIu16" already started\n",
@@ -167,7 +167,11 @@ struct rte_eth_dev_data {
scattered_rx : 1, /**< RX of scattered packets is ON(1) / OFF(0) */
all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
dev_started : 1, /**< Device state: STARTED(1) / STOPPED(0). */
- lro : 1; /**< RX LRO is ON(1) / OFF(0) */
+ lro : 1, /**< RX LRO is ON(1) / OFF(0) */
+ dev_configured : 1;
+ /**< Device configuration state:
+ * CONFIGURED(1) / NOT CONFIGURED(0).
+ */
uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
/**< Queues state: HAIRPIN(2) / STARTED(1) / STOPPED(0). */
uint8_t tx_queue_state[RTE_MAX_QUEUES_PER_PORT];