The rte_eth_dev_owner_unset function is unusable because it always returns -EINVAL. This is because the magic (unowned) value is flagged as not valid. Move the validation of owner into set and unset as separate calls. Fixes: 5b7ba31148a8 ("ethdev: add port ownership") Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- lib/librte_ethdev/rte_ethdev.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 4c320250589a..9398550a1189 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -443,10 +443,6 @@ _rte_eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id, return -ENODEV; } - if (!rte_eth_is_valid_owner_id(new_owner->id) && - !rte_eth_is_valid_owner_id(old_owner_id)) - return -EINVAL; - port_owner = &rte_eth_devices[port_id].data->owner; if (port_owner->id != old_owner_id) { RTE_ETHDEV_LOG(ERR, @@ -475,6 +471,9 @@ rte_eth_dev_owner_set(const uint16_t port_id, { int ret; + if (!rte_eth_is_valid_owner_id(owner->id)) + return -EINVAL; + rte_eth_dev_shared_data_prepare(); rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock); @@ -492,6 +491,9 @@ rte_eth_dev_owner_unset(const uint16_t port_id, const uint64_t owner_id) {.id = RTE_ETH_DEV_NO_OWNER, .name = ""}; int ret; + if (!rte_eth_is_valid_owner_id(owner_id)) + return -EINVAL; + rte_eth_dev_shared_data_prepare(); rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock); -- 2.18.0
Hi Stephen From: Stephen Hemminger > The rte_eth_dev_owner_unset function is unusable because it always > returns -EINVAL. This is because the magic (unowned) value is flagged as not > valid. > It's OK to raise an error when you do unset for unowned device. It means that unset owner should be called for owned device. > Move the validation of owner into set and unset as separate calls. > > Fixes: 5b7ba31148a8 ("ethdev: add port ownership") > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > --- > lib/librte_ethdev/rte_ethdev.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 4c320250589a..9398550a1189 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -443,10 +443,6 @@ _rte_eth_dev_owner_set(const uint16_t port_id, > const uint64_t old_owner_id, > return -ENODEV; > } > > - if (!rte_eth_is_valid_owner_id(new_owner->id) && > - !rte_eth_is_valid_owner_id(old_owner_id)) > - return -EINVAL; > - > port_owner = &rte_eth_devices[port_id].data->owner; > if (port_owner->id != old_owner_id) { > RTE_ETHDEV_LOG(ERR, > @@ -475,6 +471,9 @@ rte_eth_dev_owner_set(const uint16_t port_id, { > int ret; > > + if (!rte_eth_is_valid_owner_id(owner->id)) > + return -EINVAL; > + > rte_eth_dev_shared_data_prepare(); > > rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock); > @@ -492,6 +491,9 @@ rte_eth_dev_owner_unset(const uint16_t port_id, > const uint64_t owner_id) > {.id = RTE_ETH_DEV_NO_OWNER, .name = ""}; > int ret; > > + if (!rte_eth_is_valid_owner_id(owner_id)) > + return -EINVAL; > + > rte_eth_dev_shared_data_prepare(); > > rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock); > -- > 2.18.0
On Tue, 14 Aug 2018 05:52:20 +0000
Matan Azrad <matan@mellanox.com> wrote:
> Hi Stephen
>
> From: Stephen Hemminger
> > The rte_eth_dev_owner_unset function is unusable because it always
> > returns -EINVAL. This is because the magic (unowned) value is flagged as not
> > valid.
> >
>
> It's OK to raise an error when you do unset for unowned device.
> It means that unset owner should be called for owned device.
>
Original code was broken. The following would always fail.
rte_eth_dev_owner_new(&owner.id);
sprintf(owner.name, "example");
rte_eth_dev_owner_set(port_id, &owner);
rte_eth_dev_owner_unset(port_id, owner.id);
That is because of:
rte_eth_dev_owner_unset(port_id, owner_id)
_rte_eth_dev_owner_set(port_id, owner_id, &new_owner) << new_owner.id == RTE_ETH_DEV_NO_OWNER (0)
if (!rte_eth_is_valid_owner_id(new_owner->id) && << new_owner->id == RTE_ETH_DEV_NO_OWNER (which is flagged as invalid)
!rte_eth_is_valid_owner_id(old_owner_id))
return -EINVAL;
The failsafe driver never checks the return value, and therefore doesn't see that it never clears ownership.
Hi Stephen From: Stephen Hemminger > On Tue, 14 Aug 2018 05:52:20 +0000 > Matan Azrad <matan@mellanox.com> wrote: > > > Hi Stephen > > > > From: Stephen Hemminger > > > The rte_eth_dev_owner_unset function is unusable because it always > > > returns -EINVAL. This is because the magic (unowned) value is > > > flagged as not valid. > > > > > > > It's OK to raise an error when you do unset for unowned device. > > It means that unset owner should be called for owned device. > > > > Original code was broken. The following would always fail. > > rte_eth_dev_owner_new(&owner.id); > sprintf(owner.name, "example"); > rte_eth_dev_owner_set(port_id, &owner); > rte_eth_dev_owner_unset(port_id, owner.id); > > That is because of: > rte_eth_dev_owner_unset(port_id, owner_id) > _rte_eth_dev_owner_set(port_id, owner_id, &new_owner) > << new_owner.id == RTE_ETH_DEV_NO_OWNER (0) > > > if (!rte_eth_is_valid_owner_id(new_owner->id) && << > new_owner->id == RTE_ETH_DEV_NO_OWNER (which is flagged as invalid) > !rte_eth_is_valid_owner_id(old_owner_id)) > return -EINVAL; > But both should be invalid the new owner and the old owner(&&) to raise an EINVAL error. In the aforementioned check above the old owner should be valid. > The failsafe driver never checks the return value, and therefore doesn't see > that it never clears ownership.
While debugging something else noticed that unset always logged an error. Stephen Hemminger (2): ethdev: silence error message on rte_eth_dev_owner_unset ethdev: make rte_eth_is_valid_owner_id return bool lib/librte_ethdev/rte_ethdev.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) -- 2.18.0
The rte_eth_dev_owner_unset function always generates a log message because the unset value for owner id is 0. Fixes: 5b7ba31148a8 ("ethdev: add port ownership") Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- lib/librte_ethdev/rte_ethdev.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 4c320250589a..f09bf8bc8b01 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -393,11 +393,8 @@ static int rte_eth_is_valid_owner_id(uint64_t owner_id) { if (owner_id == RTE_ETH_DEV_NO_OWNER || - rte_eth_dev_shared_data->next_owner_id <= owner_id) { - RTE_ETHDEV_LOG(ERR, "Invalid owner_id=%016"PRIx64"\n", - owner_id); + rte_eth_dev_shared_data->next_owner_id <= owner_id) return 0; - } return 1; } @@ -444,8 +441,12 @@ _rte_eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id, } if (!rte_eth_is_valid_owner_id(new_owner->id) && - !rte_eth_is_valid_owner_id(old_owner_id)) + !rte_eth_is_valid_owner_id(old_owner_id)) { + RTE_ETHDEV_LOG(ERR, + "Invalid owner old_id=%016"PRIx64" new_id=%016"PRIx64"\n", + old_owner_id, new_owner->id); return -EINVAL; + } port_owner = &rte_eth_devices[port_id].data->owner; if (port_owner->id != old_owner_id) { -- 2.18.0
Function is boolean so use that. Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> --- lib/librte_ethdev/rte_ethdev.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index f09bf8bc8b01..f0336736b7c1 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -389,13 +389,11 @@ rte_eth_dev_is_valid_port(uint16_t port_id) return 1; } -static int +static bool rte_eth_is_valid_owner_id(uint64_t owner_id) { - if (owner_id == RTE_ETH_DEV_NO_OWNER || - rte_eth_dev_shared_data->next_owner_id <= owner_id) - return 0; - return 1; + return !(owner_id == RTE_ETH_DEV_NO_OWNER || + rte_eth_dev_shared_data->next_owner_id <= owner_id); } uint64_t -- 2.18.0
Hi Stephen From: Stephen Hemminger > The rte_eth_dev_owner_unset function always generates a log message > because the unset value for owner id is 0. > > Fixes: 5b7ba31148a8 ("ethdev: add port ownership") > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > --- > lib/librte_ethdev/rte_ethdev.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 4c320250589a..f09bf8bc8b01 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -393,11 +393,8 @@ static int > rte_eth_is_valid_owner_id(uint64_t owner_id) { > if (owner_id == RTE_ETH_DEV_NO_OWNER || > - rte_eth_dev_shared_data->next_owner_id <= owner_id) { > - RTE_ETHDEV_LOG(ERR, "Invalid owner_id=%016"PRIx64"\n", > - owner_id); > + rte_eth_dev_shared_data->next_owner_id <= owner_id) > return 0; > - } > return 1; > } When you remove the log from this function you remove the error log also from rte_eth_dev_owner_delete, so please add the missing error print there too. > > @@ -444,8 +441,12 @@ _rte_eth_dev_owner_set(const uint16_t port_id, > const uint64_t old_owner_id, > } > > if (!rte_eth_is_valid_owner_id(new_owner->id) && > - !rte_eth_is_valid_owner_id(old_owner_id)) > + !rte_eth_is_valid_owner_id(old_owner_id)) { > + RTE_ETHDEV_LOG(ERR, > + "Invalid owner old_id=%016"PRIx64" > new_id=%016"PRIx64"\n", > + old_owner_id, new_owner->id); > return -EINVAL; > + } > > port_owner = &rte_eth_devices[port_id].data->owner; > if (port_owner->id != old_owner_id) { > -- > 2.18.0
From: Stephen Hemminger > Function is boolean so use that. Ethdev is not using bool type, see also: rte_eth_dev_is_valid_port rte_eth_dev_is_removed rte_eth_dev_pool_ops_supported I think it should be a full solution to all. > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> > --- > lib/librte_ethdev/rte_ethdev.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index f09bf8bc8b01..f0336736b7c1 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -389,13 +389,11 @@ rte_eth_dev_is_valid_port(uint16_t port_id) > return 1; > } > > -static int > +static bool > rte_eth_is_valid_owner_id(uint64_t owner_id) { > - if (owner_id == RTE_ETH_DEV_NO_OWNER || > - rte_eth_dev_shared_data->next_owner_id <= owner_id) > - return 0; > - return 1; > + return !(owner_id == RTE_ETH_DEV_NO_OWNER || > + rte_eth_dev_shared_data->next_owner_id <= owner_id); > } > > uint64_t > -- > 2.18.0
On Tue, 21 Aug 2018 10:20:43 +0000
Matan Azrad <matan@mellanox.com> wrote:
> From: Stephen Hemminger
> > Function is boolean so use that.
>
> Ethdev is not using bool type, see also:
> rte_eth_dev_is_valid_port
> rte_eth_dev_is_removed
> rte_eth_dev_pool_ops_supported
>
> I think it should be a full solution to all.
>
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
I didn't want change type of visible (exported by ABI) functions.
From 74ad4c60262b1451a5a2fabf79a2df89c6c5373d Mon Sep 17 00:00:00 2001 From: Stephen Hemminger <stephen@networkplumber.org> Date: Thu, 16 Aug 2018 15:37:14 -0700 Subject: [PATCH 1/5] ethdev: silence error message on rte_eth_dev_owner_unset The rte_eth_dev_owner_unset function always generates a log message because the unset value for owner id is 0. Also, when rte_eth_dev_owner_delete is called with a valid owner id, the log message should be at NOTICE not ERROR severity. Fixes: 5b7ba31148a8 ("ethdev: add port ownership") Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- v3 - handle rte_eth_dev_owner_delete leave other cleanups to later patch series lib/librte_ethdev/rte_ethdev.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 4c320250589a..56dd9c2e83d9 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -393,11 +393,8 @@ static int rte_eth_is_valid_owner_id(uint64_t owner_id) { if (owner_id == RTE_ETH_DEV_NO_OWNER || - rte_eth_dev_shared_data->next_owner_id <= owner_id) { - RTE_ETHDEV_LOG(ERR, "Invalid owner_id=%016"PRIx64"\n", - owner_id); + rte_eth_dev_shared_data->next_owner_id <= owner_id) return 0; - } return 1; } @@ -444,8 +441,12 @@ _rte_eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id, } if (!rte_eth_is_valid_owner_id(new_owner->id) && - !rte_eth_is_valid_owner_id(old_owner_id)) + !rte_eth_is_valid_owner_id(old_owner_id)) { + RTE_ETHDEV_LOG(ERR, + "Invalid owner old_id=%016"PRIx64" new_id=%016"PRIx64"\n", + old_owner_id, new_owner->id); return -EINVAL; + } port_owner = &rte_eth_devices[port_id].data->owner; if (port_owner->id != old_owner_id) { @@ -516,9 +517,13 @@ rte_eth_dev_owner_delete(const uint64_t owner_id) if (rte_eth_devices[port_id].data->owner.id == owner_id) memset(&rte_eth_devices[port_id].data->owner, 0, sizeof(struct rte_eth_dev_owner)); - RTE_ETHDEV_LOG(ERR, + RTE_ETHDEV_LOG(NOTICE, "All port owners owned by %016"PRIx64" identifier have removed\n", owner_id); + } else { + RTE_ETHDEV_LOG(ERR, + "Invalid owner id=%016"PRIx64"\n", + owner_id); } rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock); -- 2.18.0
Hi
From: Stephen Hemminger
> On Tue, 21 Aug 2018 10:20:43 +0000
> Matan Azrad <matan@mellanox.com> wrote:
>
> > From: Stephen Hemminger
> > > Function is boolean so use that.
> >
> > Ethdev is not using bool type, see also:
> > rte_eth_dev_is_valid_port
> > rte_eth_dev_is_removed
> > rte_eth_dev_pool_ops_supported
> >
> > I think it should be a full solution to all.
> >
> > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>
> I didn't want change type of visible (exported by ABI) functions.
>
Since ethdev now is not using bool type I think it's better not to change it only for this API.
Hi
From: Stephen Hemminger
> From 74ad4c60262b1451a5a2fabf79a2df89c6c5373d Mon Sep 17 00:00:00 2001
> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Thu, 16 Aug 2018 15:37:14 -0700
> Subject: [PATCH 1/5] ethdev: silence error message on
> rte_eth_dev_owner_unset
>
> The rte_eth_dev_owner_unset function always generates a log message
> because the unset value for owner id is 0.
>
> Also, when rte_eth_dev_owner_delete is called with a valid owner id, the
> log message should be at NOTICE not ERROR severity.
>
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v3 - handle rte_eth_dev_owner_delete
> leave other cleanups to later patch series
>
> lib/librte_ethdev/rte_ethdev.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 4c320250589a..56dd9c2e83d9 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -393,11 +393,8 @@ static int
> rte_eth_is_valid_owner_id(uint64_t owner_id) {
> if (owner_id == RTE_ETH_DEV_NO_OWNER ||
> - rte_eth_dev_shared_data->next_owner_id <= owner_id) {
> - RTE_ETHDEV_LOG(ERR, "Invalid owner_id=%016"PRIx64"\n",
> - owner_id);
> + rte_eth_dev_shared_data->next_owner_id <= owner_id)
> return 0;
> - }
> return 1;
> }
>
> @@ -444,8 +441,12 @@ _rte_eth_dev_owner_set(const uint16_t port_id,
> const uint64_t old_owner_id,
> }
>
> if (!rte_eth_is_valid_owner_id(new_owner->id) &&
> - !rte_eth_is_valid_owner_id(old_owner_id))
> + !rte_eth_is_valid_owner_id(old_owner_id)) {
> + RTE_ETHDEV_LOG(ERR,
> + "Invalid owner old_id=%016"PRIx64"
> new_id=%016"PRIx64"\n",
> + old_owner_id, new_owner->id);
> return -EINVAL;
> + }
>
> port_owner = &rte_eth_devices[port_id].data->owner;
> if (port_owner->id != old_owner_id) {
> @@ -516,9 +517,13 @@ rte_eth_dev_owner_delete(const uint64_t
> owner_id)
> if (rte_eth_devices[port_id].data->owner.id ==
> owner_id)
> memset(&rte_eth_devices[port_id].data-
> >owner, 0,
> sizeof(struct rte_eth_dev_owner));
> - RTE_ETHDEV_LOG(ERR,
> + RTE_ETHDEV_LOG(NOTICE,
> "All port owners owned by %016"PRIx64" identifier
> have removed\n",
> owner_id);
> + } else {
> + RTE_ETHDEV_LOG(ERR,
> + "Invalid owner id=%016"PRIx64"\n",
> + owner_id);
> }
>
> rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
> --
> 2.18.0
>
I think the title should be:
ethdev: fix port ownership logs
while adding the fixes commits (at list 2 because of the NOTICE change)
and updating stable.
Besides that,
Acked-by: Matan Azrad <matan@mellanox.com>
On Tue, 21 Aug 2018 15:48:19 +0000
Matan Azrad <matan@mellanox.com> wrote:
> Hi
>
> From: Stephen Hemminger
> > On Tue, 21 Aug 2018 10:20:43 +0000
> > Matan Azrad <matan@mellanox.com> wrote:
> >
> > > From: Stephen Hemminger
> > > > Function is boolean so use that.
> > >
> > > Ethdev is not using bool type, see also:
> > > rte_eth_dev_is_valid_port
> > > rte_eth_dev_is_removed
> > > rte_eth_dev_pool_ops_supported
> > >
> > > I think it should be a full solution to all.
> > >
> > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> >
> > I didn't want change type of visible (exported by ABI) functions.
> >
> Since ethdev now is not using bool type I think it's better not to change it only for this API.
I hate to pick nits but there is already a bool usage in internal
function (static) in ethdev.
static bool
is_allocated(const struct rte_eth_dev *ethdev)
{
return ethdev->data->name[0] != '\0';
}
Using bool functions doesn't really generate different code. It is is more
about using modern C conventions.
On 8/14/2018 8:46 PM, Matan Azrad wrote:
> Hi Stephen
>
> From: Stephen Hemminger
>> On Tue, 14 Aug 2018 05:52:20 +0000
>> Matan Azrad <matan@mellanox.com> wrote:
>>
>>> Hi Stephen
>>>
>>> From: Stephen Hemminger
>>>> The rte_eth_dev_owner_unset function is unusable because it always
>>>> returns -EINVAL. This is because the magic (unowned) value is
>>>> flagged as not valid.
>>>>
>>>
>>> It's OK to raise an error when you do unset for unowned device.
>>> It means that unset owner should be called for owned device.
>>>
>>
>> Original code was broken. The following would always fail.
>>
>> rte_eth_dev_owner_new(&owner.id);
>> sprintf(owner.name, "example");
>> rte_eth_dev_owner_set(port_id, &owner);
>> rte_eth_dev_owner_unset(port_id, owner.id);
>>
>> That is because of:
>> rte_eth_dev_owner_unset(port_id, owner_id)
>> _rte_eth_dev_owner_set(port_id, owner_id, &new_owner)
>> << new_owner.id == RTE_ETH_DEV_NO_OWNER (0)
>>
>>
>> if (!rte_eth_is_valid_owner_id(new_owner->id) && <<
>> new_owner->id == RTE_ETH_DEV_NO_OWNER (which is flagged as invalid)
>> !rte_eth_is_valid_owner_id(old_owner_id))
>> return -EINVAL;
>>
>
> But both should be invalid the new owner and the old owner(&&) to raise an EINVAL error.
>
> In the aforementioned check above the old owner should be valid.
It looks rte_eth_dev_owner_unset() works, updating patch as rejected.
On Fri, 24 Aug 2018 14:55:56 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 8/14/2018 8:46 PM, Matan Azrad wrote:
> > Hi Stephen
> >
> > From: Stephen Hemminger
> >> On Tue, 14 Aug 2018 05:52:20 +0000
> >> Matan Azrad <matan@mellanox.com> wrote:
> >>
> >>> Hi Stephen
> >>>
> >>> From: Stephen Hemminger
> >>>> The rte_eth_dev_owner_unset function is unusable because it always
> >>>> returns -EINVAL. This is because the magic (unowned) value is
> >>>> flagged as not valid.
> >>>>
> >>>
> >>> It's OK to raise an error when you do unset for unowned device.
> >>> It means that unset owner should be called for owned device.
> >>>
> >>
> >> Original code was broken. The following would always fail.
> >>
> >> rte_eth_dev_owner_new(&owner.id);
> >> sprintf(owner.name, "example");
> >> rte_eth_dev_owner_set(port_id, &owner);
> >> rte_eth_dev_owner_unset(port_id, owner.id);
> >>
> >> That is because of:
> >> rte_eth_dev_owner_unset(port_id, owner_id)
> >> _rte_eth_dev_owner_set(port_id, owner_id, &new_owner)
> >> << new_owner.id == RTE_ETH_DEV_NO_OWNER (0)
> >>
> >>
> >> if (!rte_eth_is_valid_owner_id(new_owner->id) && <<
> >> new_owner->id == RTE_ETH_DEV_NO_OWNER (which is flagged as invalid)
> >> !rte_eth_is_valid_owner_id(old_owner_id))
> >> return -EINVAL;
> >>
> >
> > But both should be invalid the new owner and the old owner(&&) to raise an EINVAL error.
> >
> > In the aforementioned check above the old owner should be valid.
>
> It looks rte_eth_dev_owner_unset() works, updating patch as rejected.
>
The issue was the incorrect log message, addressed in later patches.
From: Stephen Hemminger
> On Tue, 21 Aug 2018 15:48:19 +0000
> Matan Azrad <matan@mellanox.com> wrote:
>
> > Hi
> >
> > From: Stephen Hemminger
> > > On Tue, 21 Aug 2018 10:20:43 +0000
> > > Matan Azrad <matan@mellanox.com> wrote:
> > >
> > > > From: Stephen Hemminger
> > > > > Function is boolean so use that.
> > > >
> > > > Ethdev is not using bool type, see also:
> > > > rte_eth_dev_is_valid_port
> > > > rte_eth_dev_is_removed
> > > > rte_eth_dev_pool_ops_supported
> > > >
> > > > I think it should be a full solution to all.
> > > >
> > > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > >
> > > I didn't want change type of visible (exported by ABI) functions.
> > >
> > Since ethdev now is not using bool type I think it's better not to change it
> only for this API.
>
> I hate to pick nits but there is already a bool usage in internal function (static)
> in ethdev.
>
>
> static bool
> is_allocated(const struct rte_eth_dev *ethdev) {
> return ethdev->data->name[0] != '\0';
> }
>
> Using bool functions doesn't really generate different code. It is is more
> about using modern C conventions.
Agree, but I think it should be the same API at least as rte_eth_dev_is_valid_port, just for ethdev convention.
Let's give to the maintainer the decision.
On 8/21/2018 4:53 PM, Matan Azrad wrote: > Hi > > From: Stephen Hemminger >> From 74ad4c60262b1451a5a2fabf79a2df89c6c5373d Mon Sep 17 00:00:00 2001 >> From: Stephen Hemminger <stephen@networkplumber.org> >> Date: Thu, 16 Aug 2018 15:37:14 -0700 >> Subject: [PATCH 1/5] ethdev: silence error message on >> rte_eth_dev_owner_unset >> >> The rte_eth_dev_owner_unset function always generates a log message >> because the unset value for owner id is 0. >> >> Also, when rte_eth_dev_owner_delete is called with a valid owner id, the >> log message should be at NOTICE not ERROR severity. >> >> Fixes: 5b7ba31148a8 ("ethdev: add port ownership") >> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> <...> > I think the title should be: > ethdev: fix port ownership logs > > while adding the fixes commits (at list 2 because of the NOTICE change) > and updating stable. > > Besides that, > Acked-by: Matan Azrad <matan@mellanox.com> Applied to dpdk-next-net/master, thanks. (Used suggested title.)