diff mbox

[dpdk-dev,v5,05/12] eal: add probe and remove support for rte_driver

Message ID 1482758645-23057-6-git-send-email-shreyansh.jain@nxp.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Shreyansh Jain Dec. 26, 2016, 1:23 p.m. UTC 
  rte_driver now supports probe and remove. These would be used for generic
device type (PCI, etc) probe and remove.

Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
 lib/librte_eal/common/include/rte_dev.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Thomas Monjalon Jan. 3, 2017, 10:05 p.m. UTC | #1 
2016-12-26 18:53, Shreyansh Jain:
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -152,6 +162,8 @@ struct rte_driver {
>  	struct rte_bus *bus;           /**< Bus serviced by this driver */
>  	const char *name;                   /**< Driver name. */
>  	const char *alias;              /**< Driver alias. */
> +	driver_probe_t *probe;         /**< Probe the device */
> +	driver_remove_t *remove;       /**< Remove/hotplugging the device */
>  };

If I understand well, this probe function does neither scan nor match.
So it could be named init.

I think the probe (init) and remove ops must be specific to the bus.
We can have them in rte_bus, and as an example, the pci implementation
would call the pci probe and remove ops of rte_pci_driver.

Please use rte_ prefix in public headers.
Shreyansh Jain Jan. 6, 2017, 11:44 a.m. UTC | #2 
On Wednesday 04 January 2017 03:35 AM, Thomas Monjalon wrote:
> 2016-12-26 18:53, Shreyansh Jain:
>> --- a/lib/librte_eal/common/include/rte_dev.h
>> +++ b/lib/librte_eal/common/include/rte_dev.h
>> @@ -152,6 +162,8 @@ struct rte_driver {
>>  	struct rte_bus *bus;           /**< Bus serviced by this driver */
>>  	const char *name;                   /**< Driver name. */
>>  	const char *alias;              /**< Driver alias. */
>> +	driver_probe_t *probe;         /**< Probe the device */
>> +	driver_remove_t *remove;       /**< Remove/hotplugging the device */
>>  };
>
> If I understand well, this probe function does neither scan nor match.
> So it could be named init.

Current model is:

After scanning for devices and populating bus->device_list,
Bus probe does:
  `-> bus->match()
  `-> rte_driver->probe() for matched driver

For PCI drivers, '.probe = rte_eal_pci_probe'.

For example, igb_ethdev.c:

--->8---
static struct eth_driver rte_igb_pmd = {
         .pci_drv = {
                 .driver = {
                         .probe = rte_eal_pci_probe,
                         .remove = rte_eal_pci_remove,
                 },
...
--->8---

>
> I think the probe (init) and remove ops must be specific to the bus.
> We can have them in rte_bus, and as an example, the pci implementation
> would call the pci probe and remove ops of rte_pci_driver.

So,
---
After scanning for devices (bus->scan()):
Bus probe (rte_eal_bus_probe()):
  `-> bus->match()
  `-> bus->init() - a new fn rte_bus_pci_init()
      -> which calls rte_eal_pci_probe()
      -> and rte_pci_driver->probe()

and remove rte_driver probe and remove callbacks because they are now
redundant. (they were added in bus patches itself)
---

Is the above correct understanding of your statement?

Somehow I don't remember why I didn't do this in first place - it seems
to be better option than introducing a rte_driver->probe()/remove()
layer. I will change it (and think again why I rejected this idea in
first place). Thanks.

>
> Please use rte_ prefix in public headers.
>

I am assuming you are referring to driver_probe_t/driver_remove_t.
Thomas Monjalon Jan. 6, 2017, 3:26 p.m. UTC | #3 
2017-01-06 17:14, Shreyansh Jain:
> On Wednesday 04 January 2017 03:35 AM, Thomas Monjalon wrote:
> > 2016-12-26 18:53, Shreyansh Jain:
> >> --- a/lib/librte_eal/common/include/rte_dev.h
> >> +++ b/lib/librte_eal/common/include/rte_dev.h
> >> @@ -152,6 +162,8 @@ struct rte_driver {
> >>  	struct rte_bus *bus;           /**< Bus serviced by this driver */
> >>  	const char *name;                   /**< Driver name. */
> >>  	const char *alias;              /**< Driver alias. */
> >> +	driver_probe_t *probe;         /**< Probe the device */
> >> +	driver_remove_t *remove;       /**< Remove/hotplugging the device */
> >>  };
> >
> > If I understand well, this probe function does neither scan nor match.
> > So it could be named init.
> 
> Current model is:
> 
> After scanning for devices and populating bus->device_list,
> Bus probe does:
>   `-> bus->match()
>   `-> rte_driver->probe() for matched driver
> 
> For PCI drivers, '.probe = rte_eal_pci_probe'.
> 
> For example, igb_ethdev.c:
> 
> --->8---
> static struct eth_driver rte_igb_pmd = {
>          .pci_drv = {
>                  .driver = {
>                          .probe = rte_eal_pci_probe,
>                          .remove = rte_eal_pci_remove,
>                  },
> ...
> --->8---

Yes
I'm just having some doubts about the naming "probe" compared to "init".
And yes I know I was advocating to unify naming to "probe" recently :)
I would like to be sure it is not confusing for anyone.
Do you agree that "init" refers to global driver initialization and
"probe" refers to instantiating a device?

If yes, the comment could be changed from "Probe the device" to
"Check and instantiate a device".

> > I think the probe (init) and remove ops must be specific to the bus.
> > We can have them in rte_bus, and as an example, the pci implementation
> > would call the pci probe and remove ops of rte_pci_driver.

I do not understand clearly what I was saying here :/

> So,
> ---
> After scanning for devices (bus->scan()):
> Bus probe (rte_eal_bus_probe()):
>   `-> bus->match()
>   `-> bus->init() - a new fn rte_bus_pci_init()

I suggest the naming bus->probe().
It is currently implemented in rte_eal_pci_probe_one_driver().

>       -> which calls rte_eal_pci_probe()

Not needed here, this function is converted into the PCI match function.

>       -> and rte_pci_driver->probe()

Yes, bus->probe() makes some processing and calls rte_pci_driver->probe().


> and remove rte_driver probe and remove callbacks because they are now
> redundant. (they were added in bus patches itself)
> ---
> 
> Is the above correct understanding of your statement?

I think we just need to move probe/remove in rte_pci_driver.

> Somehow I don't remember why I didn't do this in first place - it seems
> to be better option than introducing a rte_driver->probe()/remove()
> layer. I will change it (and think again why I rejected this idea in
> first place). Thanks.

Thanks
Shreyansh Jain Jan. 9, 2017, 6:28 a.m. UTC | #4 
On Friday 06 January 2017 08:56 PM, Thomas Monjalon wrote:
> 2017-01-06 17:14, Shreyansh Jain:
>> On Wednesday 04 January 2017 03:35 AM, Thomas Monjalon wrote:
>>> 2016-12-26 18:53, Shreyansh Jain:
>>>> --- a/lib/librte_eal/common/include/rte_dev.h
>>>> +++ b/lib/librte_eal/common/include/rte_dev.h
>>>> @@ -152,6 +162,8 @@ struct rte_driver {
>>>>  	struct rte_bus *bus;           /**< Bus serviced by this driver */
>>>>  	const char *name;                   /**< Driver name. */
>>>>  	const char *alias;              /**< Driver alias. */
>>>> +	driver_probe_t *probe;         /**< Probe the device */
>>>> +	driver_remove_t *remove;       /**< Remove/hotplugging the device */
>>>>  };
>>>
>>> If I understand well, this probe function does neither scan nor match.
>>> So it could be named init.
>>
>> Current model is:
>>
>> After scanning for devices and populating bus->device_list,
>> Bus probe does:
>>   `-> bus->match()
>>   `-> rte_driver->probe() for matched driver
>>
>> For PCI drivers, '.probe = rte_eal_pci_probe'.
>>
>> For example, igb_ethdev.c:
>>
>> --->8---
>> static struct eth_driver rte_igb_pmd = {
>>          .pci_drv = {
>>                  .driver = {
>>                          .probe = rte_eal_pci_probe,
>>                          .remove = rte_eal_pci_remove,
>>                  },
>> ...
>> --->8---
>
> Yes
> I'm just having some doubts about the naming "probe" compared to "init".
> And yes I know I was advocating to unify naming to "probe" recently :)
> I would like to be sure it is not confusing for anyone.
> Do you agree that "init" refers to global driver initialization and
> "probe" refers to instantiating a device?

Ok. Makes sense as a standardized way of differentiating 'init' from 
'probe'.

>
> If yes, the comment could be changed from "Probe the device" to
> "Check and instantiate a device".

Now that probe if removed from rte_driver, I think this would no longer 
be valid. [1]

[1] http://dpdk.org/ml/archives/dev/2017-January/054140.html

>
>>> I think the probe (init) and remove ops must be specific to the bus.
>>> We can have them in rte_bus, and as an example, the pci implementation
>>> would call the pci probe and remove ops of rte_pci_driver.
>
> I do not understand clearly what I was saying here :/

:)

>
>> So,
>> ---
>> After scanning for devices (bus->scan()):
>> Bus probe (rte_eal_bus_probe()):
>>   `-> bus->match()
>>   `-> bus->init() - a new fn rte_bus_pci_init()
>
> I suggest the naming bus->probe().
> It is currently implemented in rte_eal_pci_probe_one_driver().
>
>>       -> which calls rte_eal_pci_probe()
>
> Not needed here, this function is converted into the PCI match function.
>
>>       -> and rte_pci_driver->probe()
>
> Yes, bus->probe() makes some processing and calls rte_pci_driver->probe().

I have made some changes on similar lines. Will share them soon. Then we 
can discuss again.

>
>
>> and remove rte_driver probe and remove callbacks because they are now
>> redundant. (they were added in bus patches itself)
>> ---
>>
>> Is the above correct understanding of your statement?
>
> I think we just need to move probe/remove in rte_pci_driver.
>
>> Somehow I don't remember why I didn't do this in first place - it seems
>> to be better option than introducing a rte_driver->probe()/remove()
>> layer. I will change it (and think again why I rejected this idea in
>> first place). Thanks.
>
> Thanks
>
diff mbox

Patch

diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index 8ac09e0..3a3dc9b 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -145,6 +145,16 @@  void rte_eal_device_insert(struct rte_device *dev);
 void rte_eal_device_remove(struct rte_device *dev);
 
 /**
+ * Initialisation function for the driver called during probing.
+ */
+typedef int (driver_probe_t)(struct rte_driver *, struct rte_device *);
+
+/**
+ * Uninitialisation function for the driver called during hotplugging.
+ */
+typedef int (driver_remove_t)(struct rte_device *);
+
+/**
  * A structure describing a device driver.
  */
 struct rte_driver {
@@ -152,6 +162,8 @@  struct rte_driver {
 	struct rte_bus *bus;           /**< Bus serviced by this driver */
 	const char *name;                   /**< Driver name. */
 	const char *alias;              /**< Driver alias. */
+	driver_probe_t *probe;         /**< Probe the device */
+	driver_remove_t *remove;       /**< Remove/hotplugging the device */
 };
 
 /**