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

[Liang, Ma]

<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
> 
>