Re: [dpdk-dev] [PATCH v6 00/28] remove pci driver from vdevs

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Oct 30, 2015 at 02:32:35PM +0100, David Marchand wrote:
> Hello Bernard,
> 
> On Thu, Oct 29, 2015 at 7:36 PM, Bernard Iremonger <
> bernard.iremonger@intel.com> wrote:
> 
> > There is a dummy pci driver in the vdev PMD's at present.
> > This patch set removes the pci driver from the vdev PMD's.
> > Changes have been made to librte_ether to handle vdevs and pdevs in the
> > same way.
> >
> > [snip]
> >
> > Bernard Iremonger (28):
> >   librte_eal: add RTE_KDRV_NONE for vdevs
> >   librte_ether: add fields from rte_pci_driver to rte_eth_dev_data
> >   librte_ether: add function rte_eth_copy_dev_info
> >   ixgbe: copy pci device info to eth_dev data
> >   e1000: copy pci device info to eth_dev data
> >   i40e: copy pci device info to eth_dev data
> >   fm10k: copy pci device info to eth_dev data
> >   bnx2x: copy pci device info to eth_dev data
> >   cxgbe: copy pci device info to eth_dev data
> >   enic: copy pci device info to eth_dev data
> >   mlx4: copy pci device info to eth_dev data
> >   virtio: copy pci device info to eth_dev data
> >   vmxnet3: copy pci device info to eth_dev data
> >   null: copy device info to eth_dev data
> >   ring: copy device info to eth_dev data
> >   pcap: copy device info to eth_dev data
> >   af_packet: copy device info to eth_dev data
> >   xenvirt: copy device info to eth_dev data
> >   mpipe: copy device info to eth_dev data
> >   bonding: copy device info to eth_dev data
> >   librte_ether: remove branches on pci_dev
> >   null: remove pci device
> >   ring: remove pci device
> >   pcap: remove pci device
> >   af_packet: remove pci device
> >   xenvirt: remove pci device
> >   mpipe: remove pci device
> >   bonding: remove pci device
> >
> 
> We end up with kdrv none, while for virtual devices, I can't see a case
> where we would need it.
> 
> And these flags end up in ethdev :
> +/** Device needs PCI BAR mapping (done with either IGB_UIO or VFIO) */
> +#define RTE_ETH_DEV_DRV_NEED_MAPPING   RTE_PCI_DRV_NEED_MAPPING
> +/** Device needs to be unbound even if no module is provided */
> +#define RTE_ETH_DEV_DRV_FORCE_UNBIND   RTE_PCI_DRV_FORCE_UNBIND
> +/** Device supports link state interrupt */
> +#define RTE_ETH_DEV_INTR_LSC   RTE_PCI_DRV_INTR_LSC
> +/** Device  supports detaching capability */
> +#define RTE_ETH_DEV_DETACHABLE RTE_PCI_DRV_DETACHABLE
> 
> I can't see the point of all this, you are just moving pci specific fields
> to ethdev and I don't see how it would ease future cleanups or
> introductions of other bus types.
> 
> A real cleanup would be something like what Neil already proposed before.
>
Hi David,

this is perhaps not a full solution to enable other device types, but it's
certainly a step along the way. The primary goals were to:
* remove all references to the pci_dev structure from ethdev, so that no checks
  need to be explicitly made for the underlying device type. In this, I believe
  it largely succeeds, as the flags above are checked for each device directly,
  and a number of them are completely generic across device types, e.g. whether
  it supports a link state interrupt or is hotpluggable. The others may be of use
  to other device types too, just without any non-pci devices to look at as
  reference in DPDK, it's hard to know for sure.
* Remove the need to have a pci_dev structure within each vdev, and again this
  set does that cleanup, simplifying all the vdevs.

I'm sure there is more work to do be done to get more and different device support
in there, but this patch has been through 6 revisions and multiple reviews and I
believe is worthwhile including in 2.2 for the reasons stated above.

Regards,
/Bruce