<snip> thanks for your information. Sorry for that. All related maintainer(include other NIC PMD) will be Cced in v10.
> 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. agree > > > + > > + * ``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. agree > > > --- 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. agree > > + * > > + * @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? This function only get the target wakeup address. that does not monitor this address. > > > + * @param expected > > + * The pointer to value to be expected when descriptor is set. > > Not sure we should restrict it to a "descriptor". actully that is not limited to a descriptor, any writeback content should work. > > 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. this parameter doesn not limited user how to use it. In fact, current design can support any bits change within 64 bits content. > > > + * @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. agree will do > >