diff mbox series

[v9,1/3] ethdev: add an API to get device configuration info

Message ID 20210926092055.495322-2-jie1x.wang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series testpmd shows incorrect rx_offload configuration |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-testing warning apply patch failure

Commit Message

Jie Wang Sept. 26, 2021, 9:20 a.m. UTC 
  This patch adds a new API "rte_eth_dev_conf_info_get()" to help users get
device configuration info.

Cc: stable@dpdk.org

Signed-off-by: Jie Wang <jie1x.wang@intel.com>
---
 lib/ethdev/rte_ethdev.c | 23 +++++++++++++++++++++++
 lib/ethdev/rte_ethdev.h | 21 +++++++++++++++++++++
 lib/ethdev/version.map  |  3 +++
 3 files changed, 47 insertions(+)

Comments

Thomas Monjalon Sept. 27, 2021, 6:19 a.m. UTC | #1 
26/09/2021 11:20, Jie Wang:
> This patch adds a new API "rte_eth_dev_conf_info_get()" to help users get
> device configuration info.
> 
> Cc: stable@dpdk.org

No we don't backport new features.

> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
[...]
> + * Retrieve the configuration of an Ethernet device.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param dev_conf_info
> + *   A pointer to a structure of type *rte_eth_conf* to be filled with
> + *   the configuration of the Ethernet device.
> + *   And the memory of the structure should be allocated by the caller.
> + * @return
> + *   - (0) if successful.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EINVAL) if bad parameter.
> + */
> +__rte_experimental
> +int rte_eth_dev_conf_info_get(uint16_t port_id,
> +				struct rte_eth_conf *dev_conf_info);

It does not make sense to me.
rte_eth_conf is passed by the app to rte_eth_dev_configure.
Why the app would need to get the same info back?
Jie Wang Sept. 27, 2021, 7:21 a.m. UTC | #2 
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, September 27, 2021 2:19 PM
> To: Wang, Jie1X <jie1x.wang@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>;
> andrew.rybchenko@oktetlabs.ru; Li, Xiaoyun <xiaoyun.li@intel.com>; Yang,
> SteveX <stevex.yang@intel.com>
> Subject: Re: [PATCH v9 1/3] ethdev: add an API to get device configuration info
> 
> 26/09/2021 11:20, Jie Wang:
> > This patch adds a new API "rte_eth_dev_conf_info_get()" to help users
> > get device configuration info.
> >
> > Cc: stable@dpdk.org
> 
> No we don't backport new features.

Ok, I'll remove 'Cc: stable@dpdk.org'.

> 
> > Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> [...]
> > + * Retrieve the configuration of an Ethernet device.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param dev_conf_info
> > + *   A pointer to a structure of type *rte_eth_conf* to be filled with
> > + *   the configuration of the Ethernet device.
> > + *   And the memory of the structure should be allocated by the caller.
> > + * @return
> > + *   - (0) if successful.
> > + *   - (-ENODEV) if *port_id* invalid.
> > + *   - (-EINVAL) if bad parameter.
> > + */
> > +__rte_experimental
> > +int rte_eth_dev_conf_info_get(uint16_t port_id,
> > +				struct rte_eth_conf *dev_conf_info);
> 
> It does not make sense to me.
> rte_eth_conf is passed by the app to rte_eth_dev_configure.
> Why the app would need to get the same info back?
> 
> 

In rte_eth_dev_configure, dev->data->dev_conf copies the info from port->dev_conf, and then the driver updates it. It doesn't same as port->dev_conf.
We need to get the updated device configuration.
Thomas Monjalon Sept. 27, 2021, 7:56 a.m. UTC | #3 
27/09/2021 09:21, Wang, Jie1X:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 26/09/2021 11:20, Jie Wang:
> > > This patch adds a new API "rte_eth_dev_conf_info_get()" to help users
> > > get device configuration info.
> > [...]
> > > + * Retrieve the configuration of an Ethernet device.
> > > + *
> > > + * @param port_id
> > > + *   The port identifier of the Ethernet device.
> > > + * @param dev_conf_info
> > > + *   A pointer to a structure of type *rte_eth_conf* to be filled with
> > > + *   the configuration of the Ethernet device.
> > > + *   And the memory of the structure should be allocated by the caller.
> > > + * @return
> > > + *   - (0) if successful.
> > > + *   - (-ENODEV) if *port_id* invalid.
> > > + *   - (-EINVAL) if bad parameter.
> > > + */
> > > +__rte_experimental
> > > +int rte_eth_dev_conf_info_get(uint16_t port_id,
> > > +				struct rte_eth_conf *dev_conf_info);
> > 
> > It does not make sense to me.
> > rte_eth_conf is passed by the app to rte_eth_dev_configure.
> > Why the app would need to get the same info back?
> > 
> > 
> 
> In rte_eth_dev_configure, dev->data->dev_conf copies the info from port->dev_conf, and then the driver updates it. It doesn't same as port->dev_conf.
> We need to get the updated device configuration.

OK I see.
Please update the commit log to explain this.
Ferruh Yigit Oct. 4, 2021, 11:20 a.m. UTC | #4 
On 9/27/2021 8:56 AM, Thomas Monjalon wrote:
> 27/09/2021 09:21, Wang, Jie1X:
>> From: Thomas Monjalon <thomas@monjalon.net>
>>> 26/09/2021 11:20, Jie Wang:
>>>> This patch adds a new API "rte_eth_dev_conf_info_get()" to help users
>>>> get device configuration info.
>>> [...]
>>>> + * Retrieve the configuration of an Ethernet device.
>>>> + *
>>>> + * @param port_id
>>>> + *   The port identifier of the Ethernet device.
>>>> + * @param dev_conf_info
>>>> + *   A pointer to a structure of type *rte_eth_conf* to be filled with
>>>> + *   the configuration of the Ethernet device.
>>>> + *   And the memory of the structure should be allocated by the caller.
>>>> + * @return
>>>> + *   - (0) if successful.
>>>> + *   - (-ENODEV) if *port_id* invalid.
>>>> + *   - (-EINVAL) if bad parameter.
>>>> + */
>>>> +__rte_experimental
>>>> +int rte_eth_dev_conf_info_get(uint16_t port_id,
>>>> +				struct rte_eth_conf *dev_conf_info);
>>>
>>> It does not make sense to me.
>>> rte_eth_conf is passed by the app to rte_eth_dev_configure.
>>> Why the app would need to get the same info back?
>>>
>>>
>>
>> In rte_eth_dev_configure, dev->data->dev_conf copies the info from port->dev_conf, and then the driver updates it. It doesn't same as port->dev_conf.
>> We need to get the updated device configuration.
> 
> OK I see.
> Please update the commit log to explain this.
> 

Also either an application needs to keep copy of the configuration (like testpmd
does), or won't have any way to know device configuration details.
And for the apps that keeps the configuration, it has a risk that application
copy and device copy of the configuration diverged, as Jie mentioned.

I think it makes sense to have a way to get the configuration from device, small
applications can rely on it without keeping a copy of a config at all.

And for testpmd, we have aligned with Xiaoyun to rely on the device
configuration more, in a way:
- When to display a config, use device copy as much as possible
- Use app copy of config to accumulate user config change requests to apply them
later, sync app config with device config after config applied.
Ferruh Yigit Oct. 4, 2021, 11:22 a.m. UTC | #5 
On 9/26/2021 10:20 AM, Jie Wang wrote:
> This patch adds a new API "rte_eth_dev_conf_info_get()" to help users get
> device configuration info.
> 
> Cc: stable@dpdk.org
> 

Since this is a new API, I think we can request it to be backported.

> Signed-off-by: Jie Wang <jie1x.wang@intel.com>

<...>

> @@ -247,6 +247,9 @@ EXPERIMENTAL {
>  	rte_mtr_meter_policy_delete;
>  	rte_mtr_meter_policy_update;
>  	rte_mtr_meter_policy_validate;
> +
> +	# added in 21.11
> +	rte_eth_dev_conf_info_get;

Not sure about the 'info' part in the API, what about 'rte_eth_dev_conf_get()'?
Thomas Monjalon Oct. 4, 2021, 11:25 a.m. UTC | #6 
04/10/2021 13:20, Ferruh Yigit:
> On 9/27/2021 8:56 AM, Thomas Monjalon wrote:
> > 27/09/2021 09:21, Wang, Jie1X:
> >> From: Thomas Monjalon <thomas@monjalon.net>
> >>> 26/09/2021 11:20, Jie Wang:
> >>>> This patch adds a new API "rte_eth_dev_conf_info_get()" to help users
> >>>> get device configuration info.
> >>> [...]
> >>>> + * Retrieve the configuration of an Ethernet device.
> >>>> + *
> >>>> + * @param port_id
> >>>> + *   The port identifier of the Ethernet device.
> >>>> + * @param dev_conf_info
> >>>> + *   A pointer to a structure of type *rte_eth_conf* to be filled with
> >>>> + *   the configuration of the Ethernet device.
> >>>> + *   And the memory of the structure should be allocated by the caller.
> >>>> + * @return
> >>>> + *   - (0) if successful.
> >>>> + *   - (-ENODEV) if *port_id* invalid.
> >>>> + *   - (-EINVAL) if bad parameter.
> >>>> + */
> >>>> +__rte_experimental
> >>>> +int rte_eth_dev_conf_info_get(uint16_t port_id,
> >>>> +				struct rte_eth_conf *dev_conf_info);
> >>>
> >>> It does not make sense to me.
> >>> rte_eth_conf is passed by the app to rte_eth_dev_configure.
> >>> Why the app would need to get the same info back?
> >>>
> >>>
> >>
> >> In rte_eth_dev_configure, dev->data->dev_conf copies the info from port->dev_conf, and then the driver updates it. It doesn't same as port->dev_conf.
> >> We need to get the updated device configuration.
> > 
> > OK I see.
> > Please update the commit log to explain this.
> > 
> 
> Also either an application needs to keep copy of the configuration (like testpmd
> does), or won't have any way to know device configuration details.
> And for the apps that keeps the configuration, it has a risk that application
> copy and device copy of the configuration diverged, as Jie mentioned.
> 
> I think it makes sense to have a way to get the configuration from device, small
> applications can rely on it without keeping a copy of a config at all.
> 
> And for testpmd, we have aligned with Xiaoyun to rely on the device
> configuration more, in a way:
> - When to display a config, use device copy as much as possible
> - Use app copy of config to accumulate user config change requests to apply them
> later, sync app config with device config after config applied.

Makes sense, thanks.
Thomas Monjalon Oct. 4, 2021, 11:26 a.m. UTC | #7 
04/10/2021 13:22, Ferruh Yigit:
> On 9/26/2021 10:20 AM, Jie Wang wrote:
> > This patch adds a new API "rte_eth_dev_conf_info_get()" to help users get
> > device configuration info.
> > 
> > Cc: stable@dpdk.org
> > 
> 
> Since this is a new API, I think we can request it to be backported.

We cannot.

> > Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> 
> <...>
> 
> > @@ -247,6 +247,9 @@ EXPERIMENTAL {
> >  	rte_mtr_meter_policy_delete;
> >  	rte_mtr_meter_policy_update;
> >  	rte_mtr_meter_policy_validate;
> > +
> > +	# added in 21.11
> > +	rte_eth_dev_conf_info_get;
> 
> Not sure about the 'info' part in the API, what about 'rte_eth_dev_conf_get()'?

+1
Ferruh Yigit Oct. 4, 2021, 11:35 a.m. UTC | #8 
On 10/4/2021 12:26 PM, Thomas Monjalon wrote:
> 04/10/2021 13:22, Ferruh Yigit:
>> On 9/26/2021 10:20 AM, Jie Wang wrote:
>>> This patch adds a new API "rte_eth_dev_conf_info_get()" to help users get
>>> device configuration info.
>>>
>>> Cc: stable@dpdk.org
>>>
>>
>> Since this is a new API, I think we can request it to be backported.
> 
> We cannot.
> 

Of course, it is a typo in my end, I mean "we can NOT request ..."

>>> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
>>
>> <...>
>>
>>> @@ -247,6 +247,9 @@ EXPERIMENTAL {
>>>  	rte_mtr_meter_policy_delete;
>>>  	rte_mtr_meter_policy_update;
>>>  	rte_mtr_meter_policy_validate;
>>> +
>>> +	# added in 21.11
>>> +	rte_eth_dev_conf_info_get;
>>
>> Not sure about the 'info' part in the API, what about 'rte_eth_dev_conf_get()'?
> 
> +1
> 
>
diff mbox series

Patch

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index daf5ca9242..a0f521323a 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -3457,6 +3457,29 @@  rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
 	return 0;
 }
 
+int
+rte_eth_dev_conf_info_get(uint16_t port_id,
+				struct rte_eth_conf *dev_conf_info)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+
+	if (dev_conf_info == NULL) {
+		RTE_ETHDEV_LOG(ERR,
+			"Cannot get ethdev port %u configuration info to NULL\n",
+			port_id);
+		return -EINVAL;
+	}
+
+	/* copy dev->data->dev_conf to dev_conf_info */
+	memcpy(dev_conf_info, &dev->data->dev_conf,
+		sizeof(struct rte_eth_conf));
+
+	return 0;
+}
+
 int
 rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
 				 uint32_t *ptypes, int num)
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 1da37896d8..c21ee6a1fe 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -3068,6 +3068,27 @@  int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
  */
 int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Retrieve the configuration of an Ethernet device.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param dev_conf_info
+ *   A pointer to a structure of type *rte_eth_conf* to be filled with
+ *   the configuration of the Ethernet device.
+ *   And the memory of the structure should be allocated by the caller.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+__rte_experimental
+int rte_eth_dev_conf_info_get(uint16_t port_id,
+				struct rte_eth_conf *dev_conf_info);
+
 /**
  * Retrieve the firmware version of a device.
  *
diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
index 904bce6ea1..4b0a1f0fae 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -247,6 +247,9 @@  EXPERIMENTAL {
 	rte_mtr_meter_policy_delete;
 	rte_mtr_meter_policy_update;
 	rte_mtr_meter_policy_validate;
+
+	# added in 21.11
+	rte_eth_dev_conf_info_get;
 };
 
 INTERNAL {