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

[David Marchand]

Hello Shreyansh,

On Wed, Dec 7, 2016 at 10:55 AM, Shreyansh Jain <shreyansh.jain at nxp.com> wrote:
> On Wednesday 07 December 2016 02:22 AM, David Marchand wrote:
>>> 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.
>
>
> I deliberately kept them separate. I intent to extend the Patch 5 for
> hotplugging. But, if I don't end up adding support for that in this series,
> I will merge these two.

Fine.


>> 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.
>
>
> I didn't understand your point here.
> IMO, constructor priority (or some other way to handle this) is important. I
> faced this issue while verifying it at my end when the drivers were getting
> registered before the bus.
>
> Can you elaborate more on '..use explicit reference to a global...'?

The drivers register themselves to a bus using this bus specific api.

For pci, this is rte_eal_pci_register().
The pci_bus object must be moved to eal_common_pci.c (we can stil
internally expose for bsd / linux specific implementations).
Then, rte_eal_pci_register() can add the pci driver to the pci_bus
drivers list even if this pci_bus object is not registered yet to the
buses list.

And no constructor order issue ?


>>
>>
>>> 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.
>
>
> This 'knows' is the grey area for me.
> The bus (for example, PCI) after scanning needs to call
> rte_eal_bus_add_device() to link the device in bus's device_list.
>
> Two options:
> 1. Have a global reference to "pci" bus (rte_bus) somewhere in eal_pci.c
> 2. Call rte_eal_get_bus() every time someone needs the reference.
> 3. C++ style, 'this->'.
>
> I have taken the 3rd path. It simplifies my code to not assume a handle as
> well as not allow for reference fetch calls every now and then.
>
> As a disadvantage: it means passing this as argument - and some cases
> maintaining it as __rte_unused.
>
> Taking (1) or (2) is not advantageous than this approach.

1) is the simplest one.

When you write a pci_scan method and embed it in you pci_bus object,
but this pci_scan method still wonders which bus object it is supposed
to work on, this is a bit like Schizophrenia ;-).


>> Is is that you want to have a single scan method used by multiple buses ?
>
>
> Yes, but only as a use case. For example, platform devices are of various
> types - what if we have a south-bound bus over a platform bus. In which
> case, a hierarchical bus layout is possible.
> But, this is far-fetched idea for now.

Well, if you have no usecase at the moment, let's keep it simple, please.


>>
>>> 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.
>
>
> Yes, it is hard and sometimes there is simply no strong rationale for
> splitting or merging. This is one of those cases.
> My idea was that one patch _only_ introduces Bus services (structures,
> functions etc) and another should enable the calls to it from EAL.
> In that sense, I still think 4 and 6 should remain separate, may be
> consecutive, though.

Ok, will see in next version of the patchset.


>>
>>> 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 don't think that can be done. One change is incomplete without the other.
>
> Changes to all files are only for rte_pci_driver->probe to rte_driver->probe
> movement. EAL changes is to allow rte_eth_dev_pci_probe function after such
> a change as rte_driver->probe has different arguments as compared to
> rte_pci_driver->probe. The patches won't compile if I split.
>
> Am I missing something?
>>
>> Why do you push back the driver object in the 'probe' method ? (idem
>> rte_bus->scan).
>
>
> I am assuming you are referring to rte_driver->probe().
> This is being done so that implementations (specific to drivers on a
> particular bus) can start extracting the rte_xxx_driver, if need be.
>
> For example, for e1000/em_ethdev.c, rte_driver->probe() have been set to
> rte_eth_dev_pci_probe() which requires rte_pci_driver to work with. In
> absence of the rte_driver object, this function cannot call
> rte_pci_driver->probe (for example) for driver specific operations.

Sorry, I am thinking a step ahead with eth_driver out of the picture.
But once eth_driver disappears, I can see no reason to keep this
driver in the probe method (Schizophrenia again).



-- 
David Marchand