Re: [PATCH v2 00/11] bus: attach / detach API

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 31 May 2017 15:17:45 +0200
Gaetan Rivet <gaetan.rivet@6wind.com> wrote:

> Following the work from Jan:
> 
> This patchset introduces the attach / detach API to rte_bus.
> The rte_device structure is used as the generic device representation.
> 
> This API is implemented for the virtual bus.
> The functions rte_eal_dev_attach and rte_eal_dev_detach are updated to
> use this new interface.
> 
> --
> 
> 0. API rework
> -------------
> 
> I would like to propose an evolution on the API developed by Jan.
> 
> The attach / detach rte_bus API is necessary to support the attach/detach
> rte_dev API. Those are two different levels for one similar functionality.
> 
> Attach / detach does not allow true hotplugging, because the attach
> function expects the devices operated upon to already exist within the
> buses / sub-layers. This means that this API expects devices meta-datas
> (bus-internal device representation and associated device information
> read from the system) to be present upon attach. This part of the work
> is done during scanning.
> 
> While it is best to avoid changing the public rte_dev API as it already
> exists, nothing prevents this new rte_bus API from superseeding it.
> It has been said during the previous release cycle that device hotplug
> was a feature that interested users. True hotplug is not allowed by the
> current attach / detach API. Worse, this API hinders the effort to bring
> this new functionality by squatting its semantic field.
> 
> Thus, I propose to rename rte_bus attach / detach; plug / unplug. As it
> is a superset of the attach / detach functionality, it can be used to
> implement rte_dev attach / detach. Now is the right time to pivot to
> this new feature.
> 
> This should help maintainers understanding the aim of this API and the
> differences with the APIs higher-up, clarify the field and allow a new
> functionality to be proposed.
> 
> The vdev bus is inherently supporting the new API, however it has been
> made explicit. My implementation in the PCI bus in further patchset also
> follows the rte_bus hotplug API instead of only attach / detach.
> 
> One remaining problem with the vdev bus is the rte_dev attach
> implementation, which needs the rte_devargs rework to be properly fixed.
> 
> 1. Additional evolutions in the patchset
> ----------------------------------------
> 
> The RTE_VERIFY on the find_device is too stringent I think and forces
> all buses to implement a public device iterator. While it could be
> argued that it would push for quicker support for the functionality, I
> think it's possible that some buses are not interested at all in it and
> should simply be ignored.
> 
> The bus devices iterator has been fixed.
> 
> The internal rte_device handle was not properly setup within the
> net_ring PMD.
> 
> Gaetan Rivet (2):
>   vdev: implement hotplug functionality
>   net/ring: fix dev handle in eth_dev
> 
> Jan Blunck (9):
>   bus: add bus iterator to find a particular bus
>   bus: add device iterator
>   bus: add helper to find bus for a particular device
>   bus: add bus helper iterator to find a particular device
>   bus: introduce hotplug functionality
>   vdev: implement find_device bus operation
>   vdev: implement unplug bus operation
>   eal: make virtual driver probe and remove take rte_vdev_device
>   ethdev: Use embedded rte_device to detach driver
> 
>  drivers/net/ring/rte_eth_ring.c                 |   7 ++
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |   4 +
>  lib/librte_eal/common/eal_common_bus.c          |  65 +++++++++++++++
>  lib/librte_eal/common/eal_common_dev.c          | 100 ++++++++++++++++++------
>  lib/librte_eal/common/eal_common_vdev.c         |  27 +++++++
>  lib/librte_eal/common/include/rte_bus.h         |  87 +++++++++++++++++++++
>  lib/librte_eal/common/include/rte_dev.h         |  26 ++++++
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |   3 +
>  lib/librte_ether/rte_ethdev.c                   |   3 +-
>  9 files changed, 299 insertions(+), 23 deletions(-)
> 

LGTM

Maybe we should evolve it by having both rte_bus and  rte_dev API for one release and mark
the rte_dev API for attach/detach as deprecated?