On Thu, Jun 6, 2019 at 12:03 PM Ilya Maximets <i.maxim...@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.march...@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
