From: Jie Hai <haijie1@huawei.com>
To: Ferruh Yigit <ferruh.yigit@amd.com>, <dev@dpdk.org>
Cc: <lihuisong@huawei.com>, <fengchengwen@huawei.com>,
<liuyonglong@huawei.com>, <huangdengdui@huawei.com>
Subject: Re: [PATCH v2 1/7] ethdev: support report register names and filter
Date: Tue, 20 Feb 2024 16:43:12 +0800 [thread overview]
Message-ID: <5e5e7f10-0227-3b5a-1e82-fdb92b5573b1@huawei.com> (raw)
In-Reply-To: <7884b943-376e-4c04-9b9f-ebf04515899c@amd.com>
Hi, Ferruh,
Thanks for your review.
On 2024/2/8 1:00, Ferruh Yigit wrote:
> On 2/5/2024 10:51 AM, Jie Hai wrote:
>> This patch adds "filter" and "names" fields to "rte_dev_reg_info"
>> structure. Names of registers in data fields can be reported and
>> the registers can be filtered by their names.
>>
>> For compatibility, the original API rte_eth_dev_get_reg_info()
>> does not use the name and filter fields. The new API
>> rte_eth_dev_get_reg_info_ext() is added to support reporting
>> names and filtering by names. If the drivers does not report
>> the names, set them to "offset_XXX".
>>
>> Signed-off-by: Jie Hai <haijie1@huawei.com>
>> ---
>> doc/guides/rel_notes/release_24_03.rst | 8 ++++++
>> lib/ethdev/rte_dev_info.h | 11 ++++++++
>> lib/ethdev/rte_ethdev.c | 36 ++++++++++++++++++++++++++
>> lib/ethdev/rte_ethdev.h | 22 ++++++++++++++++
>> 4 files changed, 77 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/release_24_03.rst b/doc/guides/rel_notes/release_24_03.rst
>> index 84d3144215c6..5d402341223a 100644
>> --- a/doc/guides/rel_notes/release_24_03.rst
>> +++ b/doc/guides/rel_notes/release_24_03.rst
>> @@ -75,6 +75,11 @@ New Features
>> * Added support for Atomic Rules' TK242 packet-capture family of devices
>> with PCI IDs: ``0x1024, 0x1025, 0x1026``.
>>
>> +* **Added support for dumping regiters with names and filter.**
>>
>
> s/regiters/registers/
Will correct in v3.
>
>> +
>> + * Added new API functions ``rte_eth_dev_get_reg_info_ext()`` to and filter
>> + * the registers by their names and get the information of registers(names,
>> + * values and other attributes).
>>
>
> '*' makes a bullet, but above seems one sentences, if so please only
> keep the first '*'.
Will correct in v3.
>
>> Removed Items
>> -------------
>> @@ -124,6 +129,9 @@ ABI Changes
>>
>> * No ABI change that would break compatibility with 23.11.
>>
>> +* ethdev: Added ``filter`` and ``names`` fields to ``rte_dev_reg_info``
>> + structure for reporting names of regiters and filtering them by names.
>> +
>>
>
> This will break the ABI.
>
> Think about a case, an application compiled with an old version of DPDK,
> later same application started to use this version without re-compile,
> application will send old version of 'struct rte_dev_reg_info', but new
> version of DPDK will try to access or update new fields of the 'struct
> rte_dev_reg_info'
>
Actually, we use a local variable "struct rte_dev_reg_info reg_info" in
'rte_eth_dev_get_reg_info()' to pass to the driver, and the new fields
are set to zero. The old drivers do not visit the new fields.
We make constrains that if filter is NULL, do not filter and get info of
all registers.
For an old version APP and new version ethdev, driver will not visit the
new fields. so it does not break the ABI.
> One option is:
> - to add a new 'struct rte_dev_reg_info_ext',
> - 'rte_eth_dev_get_reg_info()' still uses old 'struct rte_dev_reg_info'
> - 'get_reg()' dev_ops will use this new 'struct rte_dev_reg_info_ext'
> - Add deprecation notice to update 'rte_eth_dev_get_reg_info()' to use
> new struct in next LTS release
>
>
>> Known Issues
>> ------------
>> diff --git a/lib/ethdev/rte_dev_info.h b/lib/ethdev/rte_dev_info.h
>> index 67cf0ae52668..2f4541bd46c8 100644
>> --- a/lib/ethdev/rte_dev_info.h
>> +++ b/lib/ethdev/rte_dev_info.h
>> @@ -11,6 +11,11 @@ extern "C" {
>>
>> #include <stdint.h>
>>
>> +#define RTE_ETH_REG_NAME_SIZE 128
>> +struct rte_eth_reg_name {
>> + char name[RTE_ETH_REG_NAME_SIZE];
>> +};
>> +
>> /*
>> * Placeholder for accessing device registers
>> */
>> @@ -20,6 +25,12 @@ struct rte_dev_reg_info {
>> uint32_t length; /**< Number of registers to fetch */
>> uint32_t width; /**< Size of device register */
>> uint32_t version; /**< Device version */
>> + /**
>> + * Filter for target subset of registers.
>> + * This field could affects register selection for data/length/names.
>> + */
>> + char *filter;
>> + struct rte_eth_reg_name *names; /**< Registers name saver */
>> };
>>
>> /*
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index f1c658f49e80..3e0294e49092 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -6388,8 +6388,39 @@ rte_eth_read_clock(uint16_t port_id, uint64_t *clock)
>>
>> int
>> rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info)
>> +{
>> + struct rte_dev_reg_info reg_info;
>> + int ret;
>> +
>> + if (info == NULL) {
>> + RTE_ETHDEV_LOG_LINE(ERR,
>> + "Cannot get ethdev port %u register info to NULL",
>> + port_id);
>> + return -EINVAL;
>> + }
>> +
>> + reg_info.length = info->length;
>> + reg_info.data = info->data;
>> + reg_info.names = NULL;
>> + reg_info.filter = NULL;
>> +
>> + ret = rte_eth_dev_get_reg_info_ext(port_id, ®_info);
>> + if (ret != 0)
>> + return ret;
>> +
>> + info->length = reg_info.length;
>> + info->width = reg_info.width;
>> + info->version = reg_info.version;
>> + info->offset = reg_info.offset;
>> +
>> + return 0;
>> +}
>> +
>> +int
>> +rte_eth_dev_get_reg_info_ext(uint16_t port_id, struct rte_dev_reg_info *info)
>> {
>> struct rte_eth_dev *dev;
>> + uint32_t i;
>> int ret;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> @@ -6408,6 +6439,11 @@ rte_eth_dev_get_reg_info(uint16_t port_id, struct rte_dev_reg_info *info)
>>
>> rte_ethdev_trace_get_reg_info(port_id, info, ret);
>>
>> + /* Report the default names if drivers not report. */
>> + if (info->names != NULL && strlen(info->names[0].name) == 0)
>> + for (i = 0; i < info->length; i++)
>> + sprintf(info->names[i].name, "offset_%x",
>> + info->offset + i * info->width);
>>
>
> Better to use 'snprintf()'
>
Will correct in v3.
>> return ret;
>> }
>>
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index 2687c23fa6fb..3abc2ad3f865 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -5053,6 +5053,28 @@ __rte_experimental
>> int rte_eth_get_monitor_addr(uint16_t port_id, uint16_t queue_id,
>> struct rte_power_monitor_cond *pmc);
>>
>> +/**
>> + * Retrieve the filtered device registers (values and names) and
>> + * register attributes (number of registers and register size)
>> + *
>> + * @param port_id
>> + * The port identifier of the Ethernet device.
>> + * @param info
>> + * Pointer to rte_dev_reg_info structure to fill in. If info->data is
>> + * NULL, the function fills in the width and length fields. If non-NULL,
>> + * the values of registers whose name contains the filter string are put
>> + * into the buffer pointed at by the data field. Do the same for the names
>> + * of registers if info->names is not NULL.
>>
>
> May be good to mention if info->names is not NULL, but driver doesn't
> support names, ehtdev fills the names automatically.
>
> As both 'rte_eth_dev_get_reg_info()' & 'rte_eth_dev_get_reg_info_ext()'
> use same dev_ops ('get_reg()'), it is possible that driver doesn't
> support filtering, so if the user provides info->filter but driver
> doesn't support it, I think API should return error, what do you think?
>
> And can you please make it clear above, if filtering is provided with
> info->data = NULL, are the returned width and length fields for filtered
> number of registers or all registers?
>
>
Will correct in v3.
>> + * @return
>> + * - (0) if successful.
>> + * - (-ENOTSUP) if hardware doesn't support.
>> + * - (-EINVAL) if bad parameter.
>> + * - (-ENODEV) if *port_id* invalid.
>> + * - (-EIO) if device is removed.
>> + * - others depends on the specific operations implementation.
>> + */
>> +int rte_eth_dev_get_reg_info_ext(uint16_t port_id, struct rte_dev_reg_info *info);
>> +
>>
>
> Can you please make the new API as experimental. That is the requirement
> for new APIs.
>
> Also need to add the API to version.map
Will correct in v3.
>
>
>> /**
>> * Retrieve device registers and register attributes (number of registers and
>> * register size)
>
> .
Best regards,
Jie Hai
next prev parent reply other threads:[~2024-02-20 8:43 UTC|newest]
Thread overview: 69+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-14 1:56 [PATCH] ethdev: add dump regs for telemetry Jie Hai
2023-12-14 12:49 ` Ferruh Yigit
2024-01-09 2:19 ` Jie Hai
2024-01-09 2:41 ` Jie Hai
2024-01-09 18:06 ` Ferruh Yigit
2024-01-10 1:38 ` fengchengwen
2024-01-10 12:15 ` Ferruh Yigit
2024-01-10 14:09 ` Thomas Monjalon
2024-01-10 15:48 ` Ferruh Yigit
2024-01-11 1:55 ` fengchengwen
2024-01-11 11:11 ` Ferruh Yigit
2024-01-11 12:43 ` fengchengwen
2024-02-05 10:51 ` [PATCH v2 0/7] support dump reigser names and filter them Jie Hai
2024-02-05 10:51 ` [PATCH v2 1/7] ethdev: support report register names and filter Jie Hai
2024-02-07 17:00 ` Ferruh Yigit
2024-02-20 8:43 ` Jie Hai [this message]
2024-02-05 10:51 ` [PATCH v2 2/7] ethdev: add telemetry cmd for registers Jie Hai
2024-02-07 17:03 ` Ferruh Yigit
2024-02-22 9:01 ` Jie Hai
2024-02-05 10:51 ` [PATCH v2 3/7] net/hns3: fix dump counter of registers Jie Hai
2024-02-05 10:51 ` [PATCH v2 4/7] net/hns3: remove dump format " Jie Hai
2024-02-05 10:51 ` [PATCH v2 5/7] net/hns3: add names for registers Jie Hai
2024-02-05 10:51 ` [PATCH v2 6/7] net/hns3: support filter directly accessed registers Jie Hai
2024-02-05 10:51 ` [PATCH v2 7/7] net/hns3: support filter dump of registers Jie Hai
2024-02-20 10:58 ` [PATCH v3 0/7] support dump reigser names and filter them Jie Hai
2024-02-20 10:58 ` [PATCH v3 1/7] ethdev: support report register names and filter Jie Hai
2024-02-20 15:09 ` Stephen Hemminger
2024-02-26 2:33 ` Jie Hai
2024-02-20 15:13 ` Stephen Hemminger
2024-02-26 2:41 ` Jie Hai
2024-02-20 15:14 ` Stephen Hemminger
2024-02-26 2:57 ` Jie Hai
2024-02-20 15:14 ` Stephen Hemminger
2024-02-26 2:33 ` Jie Hai
2024-02-20 10:58 ` [PATCH v3 2/7] ethdev: add telemetry cmd for registers Jie Hai
2024-02-20 10:58 ` [PATCH v3 3/7] net/hns3: fix dump counter of registers Jie Hai
2024-02-20 10:58 ` [PATCH v3 4/7] net/hns3: remove dump format " Jie Hai
2024-02-20 10:58 ` [PATCH v3 5/7] net/hns3: add names for registers Jie Hai
2024-02-20 10:58 ` [PATCH v3 6/7] net/hns3: support filter directly accessed registers Jie Hai
2024-02-20 10:58 ` [PATCH v3 7/7] net/hns3: support filter dump of registers Jie Hai
2024-02-26 3:07 ` [PATCH v4 0/7] support dump reigser names and filter them Jie Hai
2024-02-26 3:07 ` [PATCH v4 1/7] ethdev: support report register names and filter Jie Hai
2024-02-26 8:01 ` fengchengwen
2024-03-06 7:22 ` Jie Hai
2024-02-29 9:52 ` Thomas Monjalon
2024-03-05 7:45 ` Jie Hai
2024-02-26 3:07 ` [PATCH v4 2/7] ethdev: add telemetry cmd for registers Jie Hai
2024-02-26 9:09 ` fengchengwen
2024-03-06 7:18 ` Jie Hai
2024-02-26 3:07 ` [PATCH v4 3/7] net/hns3: fix dump counter of registers Jie Hai
2024-02-26 3:07 ` [PATCH v4 4/7] net/hns3: remove dump format " Jie Hai
2024-02-26 3:07 ` [PATCH v4 5/7] net/hns3: add names for registers Jie Hai
2024-02-26 3:07 ` [PATCH v4 6/7] net/hns3: support filter directly accessed registers Jie Hai
2024-02-26 3:07 ` [PATCH v4 7/7] net/hns3: support filter dump of registers Jie Hai
2024-03-07 3:02 ` [PATCH v5 0/7] support dump reigser names and filter them Jie Hai
2024-03-07 3:02 ` [PATCH v5 1/7] ethdev: support report register names and filter Jie Hai
2024-03-08 8:09 ` lihuisong (C)
2024-03-07 3:02 ` [PATCH v5 2/7] ethdev: add telemetry cmd for registers Jie Hai
2024-03-08 8:48 ` lihuisong (C)
2024-03-07 3:02 ` [PATCH v5 3/7] net/hns3: fix dump counter of registers Jie Hai
2024-03-08 8:49 ` lihuisong (C)
2024-03-07 3:02 ` [PATCH v5 4/7] net/hns3: remove dump format " Jie Hai
2024-03-08 9:17 ` lihuisong (C)
2024-03-07 3:02 ` [PATCH v5 5/7] net/hns3: add names for registers Jie Hai
2024-03-08 9:41 ` lihuisong (C)
2024-03-08 10:24 ` lihuisong (C)
2024-03-07 3:02 ` [PATCH v5 6/7] net/hns3: support filter directly accessed registers Jie Hai
2024-03-08 9:41 ` lihuisong (C)
2024-03-07 3:02 ` [PATCH v5 7/7] net/hns3: support filter dump of registers Jie Hai
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5e5e7f10-0227-3b5a-1e82-fdb92b5573b1@huawei.com \
--to=haijie1@huawei.com \
--cc=dev@dpdk.org \
--cc=fengchengwen@huawei.com \
--cc=ferruh.yigit@amd.com \
--cc=huangdengdui@huawei.com \
--cc=lihuisong@huawei.com \
--cc=liuyonglong@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).