[PATCH] ethdev: fix get_reg_info

[PATCH] ethdev: fix get_reg_info

[Thierry Herbelot]

'width' and 'offset' are input parameters when dumping the register
info of an Ethernet device. They should be copied in the new request
before calling the device callback function.

Fixes: 083db2ed9e9 ('ethdev: add report of register names and filter')
Cc: stable@dpdk.org

Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
---
 lib/ethdev/rte_ethdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 6413c54e3b39..073a3bcf5c0b 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -6511,6 +6511,8 @@ rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info)
 	}
 
 	reg_info.length = info->length;
+	reg_info.width = info->width;
+	reg_info.offset = info->offset;
 	reg_info.data = info->data;
 
 	ret = rte_eth_dev_get_reg_info_ext(port_id, &reg_info);
-- 
2.39.2

Re: [PATCH] ethdev: fix get_reg_info

[Stephen Hemminger]

On Tue, 18 Feb 2025 12:58:28 +0100
Thierry Herbelot <thierry.herbelot@6wind.com> wrote:

> 'width' and 'offset' are input parameters when dumping the register
> info of an Ethernet device. They should be copied in the new request
> before calling the device callback function.
> 
> Fixes: 083db2ed9e9 ('ethdev: add report of register names and filter')
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>

Why does the ethdev code create an on stack temporary variable.
Looks like it only wants to make sure that names element is NULL.

Really should be one function and when extended fields were added
should have used API versioning.
Probably too late now, although rte_eth_dev_get_reg_info_ext()
is an experimental API.

Re: [PATCH] ethdev: fix get_reg_info

[Thomas Monjalon]

19/02/2025 19:45, Stephen Hemminger:
> On Tue, 18 Feb 2025 12:58:28 +0100
> Thierry Herbelot <thierry.herbelot@6wind.com> wrote:
> 
> > 'width' and 'offset' are input parameters when dumping the register
> > info of an Ethernet device. They should be copied in the new request
> > before calling the device callback function.
> > 
> > Fixes: 083db2ed9e9 ('ethdev: add report of register names and filter')
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
> 
> Why does the ethdev code create an on stack temporary variable.
> Looks like it only wants to make sure that names element is NULL.
> 
> Really should be one function and when extended fields were added
> should have used API versioning.
> Probably too late now, although rte_eth_dev_get_reg_info_ext()
> is an experimental API.

If it is experimental, the function can be dropped in favour of a better versioned function.

Re: [PATCH] ethdev: fix get_reg_info

[fengchengwen]

On 2025/2/20 2:45, Stephen Hemminger wrote:
> On Tue, 18 Feb 2025 12:58:28 +0100
> Thierry Herbelot <thierry.herbelot@6wind.com> wrote:
> 
>> 'width' and 'offset' are input parameters when dumping the register
>> info of an Ethernet device. They should be copied in the new request
>> before calling the device callback function.
>>
>> Fixes: 083db2ed9e9 ('ethdev: add report of register names and filter')
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
> 
> Why does the ethdev code create an on stack temporary variable.
> Looks like it only wants to make sure that names element is NULL.

It mainly for ABI compatibility.

The original solution is to add an ext API (rte_eth_dev_get_reg_info_ext) and deprecate the original API (rte_eth_dev_get_reg_info).

> 
> Really should be one function and when extended fields were added
> should have used API versioning.
> Probably too late now, although rte_eth_dev_get_reg_info_ext()
> is an experimental API.

Re: [PATCH] ethdev: fix get_reg_info

[fengchengwen]

On 2025/2/18 19:58, Thierry Herbelot wrote:
> 'width' and 'offset' are input parameters when dumping the register
> info of an Ethernet device. They should be copied in the new request
> before calling the device callback function.
> 
> Fixes: 083db2ed9e9 ('ethdev: add report of register names and filter')
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thierry Herbelot <thierry.herbelot@6wind.com>
> ---
>  lib/ethdev/rte_ethdev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 6413c54e3b39..073a3bcf5c0b 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -6511,6 +6511,8 @@ rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info)
>  	}
>  
>  	reg_info.length = info->length;
> +	reg_info.width = info->width;
> +	reg_info.offset = info->offset;

I think there are ambiguities in the original API definition:
1) the width was used as an output parameter in current all PMD impl.
2) the offset was unused in current all PMD impl.

But maybe other non-opensource PMD will use these two field.
So I think it's OK to copy the two fields (maybe another field "version") before invoke _ext API.

>  	reg_info.data = info->data;
>  
>  	ret = rte_eth_dev_get_reg_info_ext(port_id, &reg_info);