[dpdk-dev] [PATCH 00/13] Introducing EAL Bus-Device-Driver Model

[David Marchand]

"Big patchset and a lot of things to look at.

Here is a first look at it.

On Sun, Dec 4, 2016 at 11:11 AM, Shreyansh Jain <shreyansh.jain at nxp.com> wrote:
> In continuation to the RFC posted on 17/Nov [9],
> A series of patches is being posted which attempts to create:
>  1. A basic bus model
>     `- define rte_bus and associated methods/helpers
>     `- test infrastructure to test the Bus infra
>  2. Changes in EAL to support PCI as a bus
>     `- a "pci" bus is registered
>     `- existing scan/match/probe are modified to allow for bus integration
>     `- PCI Device and Driver list, which were global entities, have been
>        moved to rte_bus->[device/driver]_list
>
> I have sanity tested this patch over a XeonD X552 available with me, as
> well as part of PoC for verifying NXP's DPAA2 PMD (being pushed out in a
> separate series). Exhaustive testing is still pending.

I saw some checkpatch issues for patch 1 (I would ignore this one) and
4 (please check).


> :: Brief about Patch Layout ::
>
> 0001:      Container_of patch from [3]

> 0002~0003: Introducing the basic Bus model and associated test case
> 0005:      Support insertion of device rather than addition to tail

Patch 2 and 5 could be squashed.
The constructor priority stuff seems unneeded as long as we use
explicit reference to a global (or local, did not check) bus symbol
rather than a runtime lookup.


> 0004:      Add scan and match callbacks for the Bus and updated test case

Why do you push back the bus object in the 'scan' method ?
This method is bus specific which means that the code "knows" the
object registered with the callback.
Is is that you want to have a single scan method used by multiple buses ?


> 0006:      Integrate bus scan/match with EAL, without any effective driver

Hard to find a right balance in patch splittng, but patch 4 and 6 are
linked, I would squash them into one.


> 0007:      rte_pci_driver->probe replaced with rte_driver->probe

This patch is too big, please separate in two patches: eal changes
then ethdev/driver changes.
I almost missed that mlx4 has been broken : you moved the drv_flags
from the mlx4 pci driver to rte_driver.

Why do you push back the driver object in the 'probe' method ? (idem
rte_bus->scan).


> 0008:      Integrate probe of drivers with EAL

This patch does nothing about "remove" while its title talks about it.

+ What about hotplug code ? I suppose this is for later.


> 0009:      Split the existing PCI probe into match and probe

You don't need to expose rte_eal_pci_match_default() (and I am not
sure what the "default" means here).
This method should be internal to the pci bus object ?


> 0010:      Make PCI probe/match work on rte_driver/device rather than
>            rte_pci_device/rte_pci_driver
> 0011:      Patch from Ben [8], part of series [2]


> 0012~0013: Enable Scan/Match/probe on Bus from EAL and remove unused
>            functions and lists

Same thing as earlier in the series, you don't need to check for the bus object.
The pci bus code can't be called if the bus was not registered and
consequently, you are sure that the pci bus object is the pci_bus
symbol.



Regards,
-- 
David Marchand