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

[Jianbo Liu]

Hi Shreyansh,

On 7 December 2016 at 21:10, Shreyansh Jain <shreyansh.jain at nxp.com> wrote:
> On Wednesday 07 December 2016 05:47 PM, David Marchand wrote:
>>
>> 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.
>
>
> So, in eal_common_bus.c
>
> --->8---
>
> struct rte_bus *global_ptr_to_pci_bus = NULL;
>
> struct rte_bus pci_bus = { ... };
>
> rte_eal_pci_register() {
>     if (global_ptr_to_pci_bus == NULL)
>         rte_eal_bus_register(&pci_bus)
>     else
>        // continue as if PCI bus is registered
> }
>
> --->8---
>
> so, no RTE_REGISTER_BUS()?
>
> If yes, then RTE_REGISTER_BUS() should also check for an existing
> registration for duplication.
>
> I was banking on a model where bus handlers (or bus drivers) are independent
> entities, just like PMDs. So, we have a bus XYZ without any drivers
> necessarily based on it.
>
> By making registration dependent on driver registration, it becomes implicit
> that buses don't exist without drivers.
> I am not in favor of this - or maybe I lack enough reason for this (about
> how it will make framework/PMD life better).
>
>>
>> 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 ;-).
>
>
> :)
> This now is linked to the above issue of constructor priority and having a
> global bus reference. I don't personally prefer it.
> I will still give this a serious thought, though.
>

I'm also in favor of (3).

>>
>>
>>>> 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.
>>

How to express the hierarchical bus layout as the bus in your design
is more like independent objects to hold drivers and their devices?

>>
>> Well, if you have no usecase at the moment, let's keep it simple, please.
>>
>
> Ok.
>
>>
>>>>
>>>>> 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.
>
>
> Is there anything specific that you are looking for in patchset v2?
> I was thinking of:
> 0. fixing BSD compilation issue reported by CI
> 1. improving the test_pci.c
> 2. hotplugging
> 3. trying to move PCI to drives/bus/pci/linux/* and resolving how drivers
> link to it, and how EAL resources like devargs are consumed.
>
> Anything else?
>
>>
>>
>>>>
>>>>> 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).
>
>
> When eth_driver disappears, i was thinking of accomodating the eth_dev_init
> into the rte_pci_driver->probe/init.
> But, this is still a nascent thought.
> I am yet to start working on eth_driver.
>
>>
>>
>>
>