Re: [dpdk-dev] [PATCH v9 04/10] ethdev: add simple power management API

[Thomas Monjalon <thomas@monjalon.net>]

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.ma@intel.com>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@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.