24/10/2020 01:06, Liang Ma:
> Add a simple API to allow getting address of next RX descriptor from the
> PMD, as well as release notes information.
>
> Signed-off-by: Liang Ma <liang.j...@intel.com>
> Signed-off-by: Anatoly Burakov <anatoly.bura...@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.anan...@intel.com>
You keep forgetting Cc ethdev maintainers
(it is automatic when using --cc-cmd devtools/get-maintainer.sh).
As a result we still don't have any feedback from Ferruh and Andrew.
And I believe such API requires having feedback from maintainers
of other NIC drivers. I tried to explain this concern already
in email and community meetings, but I see no progress.
Below are my comments.
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -139,6 +139,11 @@ New Features
> Hairpin Tx part flow rules can be inserted explicitly.
> New API is added to get the hairpin peer ports list.
>
> +* **ethdev: add 1 new EXPERIMENTAL API for PMD power management.**
The guidelines at the top of the file say using past tense.
No need to mention it is experimental as any new API.
> +
> + * ``rte_eth_get_wake_addr()``
> + * add new eth_dev_ops ``get_wake_addr``
No need to mention the dev_ops in the release notes.
Better to explain what it brings from an user perspective.
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> +/**
> + * Retrieve the wake up address for the receive queue.
I guess how this function should be used,
but a bit more explanations would not hurt here.
> + *
> + * @param port_id
> + * The port identifier of the Ethernet device.
> + * @param queue_id
> + * The Rx queue on the Ethernet device for which information will be
> + * retrieved.
> + * @param wake_addr
> + * The pointer to the address which will be monitored.
This function does not make the address monitored, right?
> + * @param expected
> + * The pointer to value to be expected when descriptor is set.
Not sure we should restrict it to a "descriptor".
Expecting a value or some bits looks too much restrictive.
I understand it probably fits well for Intel NICs,
but in the general case, we can imagine that any change
in a byte array could be a wake up signal.
> + * @param mask
> + * The pointer to comparison bitmask for the expected value.
> + * @param data_sz
> + * The pointer to data size for the expected value and comparison bitmask.
It is not clear that above 4 parameters are filled by the driver.
> + *
> + * @return
> + * - 0: Success.
> + * -ENOTSUP: Operation not supported.
> + * -EINVAL: Invalid parameters.
> + * -ENODEV: Invalid port ID.
> + */
> +__rte_experimental
> +int rte_eth_get_wake_addr(uint16_t port_id, uint16_t queue_id,
> + volatile void **wake_addr, uint64_t *expected, uint64_t *mask,
> + uint8_t *data_sz);
[...]
> --- a/lib/librte_ethdev/version.map
> +++ b/lib/librte_ethdev/version.map
> @@ -244,6 +244,7 @@ EXPERIMENTAL {
> rte_flow_get_restore_info;
> rte_flow_tunnel_action_decap_release;
> rte_flow_tunnel_item_release;
> + rte_eth_get_wake_addr;
> };
Please sort in alphabetical order.
