Hello Jianbo,
On Monday 12 December 2016 08:05 PM, Jianbo Liu wrote:
Hi Shreyansh,
On 7 December 2016 at 21:10, Shreyansh Jain <shreyansh.j...@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.j...@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).
Thank you. I was almost done with v2 and in that I had changed to what
David had suggested. My preference too is (3). Now, I will prefer
sticking with it - until someone comes with technical issue (like
compiler compatibility etc) which I am unaware of.
@David: Can you re-think if you still prefer (1)? If so, I will change
it in v3 (I will send v2 in a day or two max).
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?
What I had in mind was something on the lines of:
1) Add a new linked list 'bus_list' in rte_bus
2) OR, embed rte_device in rte_bus
(1) is for maintaining buses as independent entity; (2) is for treating
buses like devices (very similar to what Ferruh once suggested [2]). I
prefer (1), but I think programmatically (2) is much more symmetrical. I
am assuming (1) below.
If we have: (taking hint from [1])
CPU
|
====,============`============= PCI Bus 0
|
PCI-PCI
Bridge
|
=,='=======,====== PCI Bus 1
| |
SCSI Ethernet
PCI Bus 0 (rte_bus)pci_bus_0
`.-> scan(): this calls knows it is a PCI-PCI bridge. It would allocate
| a new pci_bus_1 (rte_bus object) and attach to bus_list.
| Then, assign generic SCSI scan functions to pci_bus_1->scan
| and pci_bus_1->match.
`-> eal/probe()
- bus->match() is called with rte_device/driver. In this
case, it would move over all the buses in bus_list pivoted
on pci_bus_0 and call rte_bus->match.
- (#) For each matched entry, subsequently call the
rte_bus->probe()
- (*) Cascading calls to rte_driver->probe() for pci_bus_1
(#) there is still an open discussion about whether bus->probe() should
exist or not. (I am not convinced buses should probe, but DPDK model
doesn't bode well without it)
(*) pci_bus_0->probe() would get rte_device/rte_driver as NULL and
rotate over each device/driver scanned in pci_bus_1 calling bus->probe.
[1] http://www.tldp.org/LDP/tlk/dd/pci.html
[2] http://dpdk.org/ml/archives/dev/2016-August/045947.html
(Note: I agree that there are minor holes in above theory, specifically
from implementation point. But, I am confident that with minor changes
this is achievable).
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.
Thanks for your comments.
-
Shreyansh
