Re: [dpdk-dev] [PATCH v2] eal: fix positive error codes from probe/remove

[David Marchand <david.marchand@redhat.com>]

On Thu, Jun 6, 2019 at 12:03 PM Ilya Maximets <i.maximets@samsung.com>
wrote:

> According to API, 'rte_dev_probe()' and 'rte_dev_remove()' must
> return 0 or negative error code. Bus code returns positive values
> if device wasn't recognized by any driver, so the result of
> 'bus->plug/unplug()' must be converted. 'local_dev_probe()' and
> 'local_dev_remove()' also has their internal API, so the conversion
> should be done there.
>
> Positive on remove means that device not found by driver.
>

For backports, it is safer to add the check on > 0.
The patch looks good to me.

Reviewed-by: David Marchand <david.marchand@redhat.com>


But I have some comments on the current state of the code.
After inspecting the eal and buses, this problem is not supposed to happen
on the rte_dev_remove path.
rte_dev_remove() ensures that it calls local_dev_remove() after checking
that the device is attached to a driver (see the check on
!rte_dev_probed()).


Anatoly,

- When handling a detach operation in the primary process
https://git.dpdk.org/dpdk/tree/lib/librte_eal/common/hotplug_mp.c#n124, we
signal all other secondary processes to detach right away.
Then we do a bus/device lookup.
Then we call the bus unplug.

Would not it be better to check the device exists _and_ check if the device
is attached to a driver in the primary process before calling other
secondary processes?


Thomas,

- Calling unplug on a device that is not attached is a bit weird to me, all
the more so that we have rte_dev_probed().
But there might be users calling directly the bus unplug api and not the
official api...
Does this enter the ABI stability perimeter?
If not, I would be for changing unplug api so that we only deal with 0 or <
0 on remove path.

On the plug side, is there a reason why we do not check for
rte_dev_probed() and let the bus replies that the device is already probed?
Does it have something to do with representors ?
Only guessing.

- On the plug side again, can't we have an indication from the buses that
they have a driver that can handle the device rather than this odd (and
historical) > 0 return code?
This should not change the current behavior, just make the code a bit
easier to understand.

I know you are travelling, so this can wait anyway.



-- 
David Marchand