Patchwork [dpdk-dev,1/1] doc: announce change in power API

login
register
mail settings
Submitter Hajkowski
Date March 13, 2019, 7:21 p.m.
Message ID <20190313192107.696-1-marcinx.hajkowski@intel.com>
Download mbox | patch
Permalink /patch/748889/
State New
Headers show

Comments

Hajkowski - March 13, 2019, 7:21 p.m.
From: Marcin Hajkowski <marcinx.hajkowski@intel.com>

Function rte_power_set_env will no longer return
success on attempt to set env in initialized state.

Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
---
 doc/guides/rel_notes/deprecation.rst | 5 +++++
 1 file changed, 5 insertions(+)
Rami Rosen - March 14, 2019, 6:50 p.m.
Hajkowski ‏<marcinx.hajkowski@intel.com>:

> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
>
> Function rte_power_set_env will no longer return
> success on attempt to set env in initialized state.
>
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>

For further patches, please remove the legal disclaimer at the bottom.

Reviewed-by: Rami Rosen <ramirose@gmail.com>
David Hunt - April 18, 2019, 2:33 p.m.
On 13/3/2019 7:21 PM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
>
> Function rte_power_set_env will no longer return
> success on attempt to set env in initialized state.
>
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---


Acked-by: David Hunt <david.hunt@intel.com>
Thomas Monjalon - May 9, 2019, 11:28 p.m.
> > From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> >
> > Function rte_power_set_env will no longer return
> > success on attempt to set env in initialized state.
> >
> > Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Acked-by: David Hunt <david.hunt@intel.com>

Any other comment about this deprecation notice?
Burakov, Anatoly - May 10, 2019, 9:26 a.m.
On 13-Mar-19 7:21 PM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Function rte_power_set_env will no longer return
> success on attempt to set env in initialized state.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
Bruce Richardson - May 10, 2019, 9:28 a.m.
On Fri, May 10, 2019 at 01:28:09AM +0200, Thomas Monjalon wrote:
> > > From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> > >
> > > Function rte_power_set_env will no longer return
> > > success on attempt to set env in initialized state.
> > >
> > > Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> > 
> > Acked-by: David Hunt <david.hunt@intel.com>
> 
> Any other comment about this deprecation notice?
> 
Seems ok to me, though the actual text is maybe a little unclear. It
implies that the function will always return -1 unless the variable is
unset when the function terminates (which seems to imply a failure case).
What I presume is meant is that we have three possibilities:

* The variable is set by the function -> return 0
* The varaible is already set, so no action needed -> return -1 (and set
  rte_errno to EEXIST or EALREADY??)
* Setting the variable failed -> return -1 (and set rte_errno to ??)

Is my understanding correct? Can the deprecation notice be improved to make
it clear that only the middle case is the one being changed, e.g. by adding
"in this case" to the second sentence. It might also be worthwhile calling
out what the errno value will be to identify this failure vs regular
failures.

/Bruce

PS: For this case, is there a reason to make it an error? Would a +1 value
not also do, so anything non-zero implies no work done, and anything >=0
means that the value is set? Call set on something already set doesn't
really seem like an error case to me.
Bruce Richardson - May 10, 2019, 9:33 a.m.
On Fri, May 10, 2019 at 10:28:14AM +0100, Bruce Richardson wrote:
> On Fri, May 10, 2019 at 01:28:09AM +0200, Thomas Monjalon wrote:
> > > > From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> > > >
> > > > Function rte_power_set_env will no longer return
> > > > success on attempt to set env in initialized state.
> > > >
> > > > Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> > > 
> > > Acked-by: David Hunt <david.hunt@intel.com>
> > 
> > Any other comment about this deprecation notice?
> > 
> Seems ok to me, though the actual text is maybe a little unclear. It
> implies that the function will always return -1 unless the variable is
> unset when the function terminates (which seems to imply a failure case).
> What I presume is meant is that we have three possibilities:
> 
> * The variable is set by the function -> return 0
> * The varaible is already set, so no action needed -> return -1 (and set
>   rte_errno to EEXIST or EALREADY??)
> * Setting the variable failed -> return -1 (and set rte_errno to ??)
> 
> Is my understanding correct? Can the deprecation notice be improved to make
> it clear that only the middle case is the one being changed, e.g. by adding
> "in this case" to the second sentence. It might also be worthwhile calling
> out what the errno value will be to identify this failure vs regular
> failures.
> 
> /Bruce
> 
> PS: For this case, is there a reason to make it an error? Would a +1 value
> not also do, so anything non-zero implies no work done, and anything >=0
> means that the value is set? Call set on something already set doesn't
> really seem like an error case to me.

Sorry, forgot to put:

For changing the ABI itself, no issues, so with the small rewording asked
for, please add my ack.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 1b4fcb7e6..7899e6874 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -75,3 +75,8 @@  Deprecation Notices
 
 * crypto/aesni_mb: the minimum supported intel-ipsec-mb library version will be
   changed from 0.49.0 to 0.52.0.
+
+* power: ``rte_power_set_env`` function will no longer return 0 on attempt
+  to set new power environment if power environment was already initialized.
+  Instead, the function will return -1 unless the environment is unset first
+  (using ``rte_power_unset_env``).