From: "lihuisong (C)" <lihuisong@huawei.com>
To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>,
Dongdong Liu <liudongdong3@huawei.com>, <dev@dpdk.org>,
<ferruh.yigit@xilinx.com>, <thomas@monjalon.net>
Cc: <stable@dpdk.org>
Subject: Re: [PATCH v2 3/3] ethdev: add the check for the valitity of timestamp offload
Date: Thu, 7 Jul 2022 10:05:00 +0800 [thread overview]
Message-ID: <db3f310c-28c1-e28e-cc19-3ba727386054@huawei.com> (raw)
In-Reply-To: <ace694c4-a56b-a650-8ee6-6cae1f7dc2f1@oktetlabs.ru>
在 2022/7/6 22:57, Andrew Rybchenko 写道:
> On 7/2/22 11:17, Dongdong Liu wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> This patch adds the check for the valitity of timestamp offload.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> ---
>> lib/ethdev/rte_ethdev.c | 65 ++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index 1979dc0850..9b8ba3a348 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -5167,15 +5167,48 @@ rte_eth_dev_set_mc_addr_list(uint16_t port_id,
>> mc_addr_set, nb_mc_addr));
>> }
>> +static int
>> +rte_eth_timestamp_offload_valid(struct rte_eth_dev *dev)
>> +{
>> + struct rte_eth_dev_info dev_info;
>> + struct rte_eth_rxmode *rxmode;
>> + int ret;
>> +
>> + ret = rte_eth_dev_info_get(dev->data->port_id, &dev_info);
>> + if (ret != 0) {
>> + RTE_ETHDEV_LOG(ERR, "Cannot get port (%u) device
>> information.\n",
>> + dev->data->port_id);
>> + return ret;
>> + }
>> +
>> + if ((dev_info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_TIMESTAMP) ==
>> 0) {
>
> As I understand strictly speaking the Rx offload is not the same as PTP
> support. May be it is a corner case, but I can imagine case when HW
> cannot provide timestamp for each packet (lack of space in extra
> information associated with a packet), but can return timestamps
> out-of-band using timestamp get API.
>
Generally speaking, Rx offload is a data plane thing and done in hardware.
If hardware cannot provide timestamp for each packet, and can implement PTP
only by using timestamp APIs. Can this corner case still be classified as
Rx offload? If so, It is more of a control plane thing, like MTU.
On the other hand, this offload is a capability obtained from driver.
If driver doesn't support PTP, the ethdev layer should block them in
timestamp APIs.
> I have no strong opinion. May be we are not interested in the corner
> case, but I think it requires acks from maintainers of all drivers
> which support PTP.
>
>> + RTE_ETHDEV_LOG(ERR, "Driver does not support PTP.\n");
>> + return -ENOTSUP;
>> + }
>> +
>> + rxmode = &dev->data->dev_conf.rxmode;
>> + if ((rxmode->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) == 0) {
>> + RTE_ETHDEV_LOG(ERR, "Please enable
>> 'RTE_ETH_RX_OFFLOAD_TIMESTAMP' offload.\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> int
>> rte_eth_timesync_enable(uint16_t port_id)
>> {
>> struct rte_eth_dev *dev;
>> + int ret;
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> dev = &rte_eth_devices[port_id];
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_enable, -ENOTSUP);
>> + ret = rte_eth_timestamp_offload_valid(dev);
>> + if (ret != 0)
>> + return ret;
>> +
>
> Typically ops pointer check is done just before usage. So, it is
> better to avoid adding code between check and usage.
> Same in all cases below.
> if there is a good reason to do so, please, explain it.
A coin has two sides.
Both styles exist in the ethdev layer API. There is no need to check input
configuration if driver doesn't support the API. I am ok for them.
>
>> return eth_err(port_id, (*dev->dev_ops->timesync_enable)(dev));
>> }
>
> [snip]
> .
prev parent reply other threads:[~2022-07-07 2:05 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-28 13:39 [PATCH 0/3] some bugfixes for PTP Dongdong Liu
2022-06-28 13:39 ` [PATCH 1/3] examples/ptpclient: add the check for PTP capability Dongdong Liu
2022-06-28 13:39 ` [PATCH 2/3] net/hns3: fix fail to receive PTP packet Dongdong Liu
2022-06-28 13:39 ` [PATCH 3/3] ethdev: add the check for the valitity of timestamp offload Dongdong Liu
2022-07-02 8:17 ` [PATCH v2 0/3] some bugfixes for PTP Dongdong Liu
2022-07-02 8:17 ` [PATCH v2 1/3] examples/ptpclient: add the check for PTP capability Dongdong Liu
2022-07-02 8:17 ` [PATCH v2 2/3] net/hns3: fix fail to receive PTP packet Dongdong Liu
2022-07-02 8:17 ` [PATCH v2 3/3] ethdev: add the check for the valitity of timestamp offload Dongdong Liu
2022-07-06 14:57 ` Andrew Rybchenko
2022-07-07 2:05 ` lihuisong (C) [this message]
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=db3f310c-28c1-e28e-cc19-3ba727386054@huawei.com \
--to=lihuisong@huawei.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@xilinx.com \
--cc=liudongdong3@huawei.com \
--cc=stable@dpdk.org \
--cc=thomas@monjalon.net \
/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).