[dpdk-dev] [PATCH v6 05/17] eal: introduce init macros

Shreyansh jain shreyansh.jain at nxp.com
Thu Jul 14 07:27:55 CEST 2016


Hi Jan,

On Wednesday 13 July 2016 11:04 PM, Jan Viktorin wrote:
> On Wed, 13 Jul 2016 11:20:43 +0200
> Jan Viktorin <viktorin at rehivetech.com> wrote:
> 
>> Hello Shreyansh,
>>
>> On Tue, 12 Jul 2016 11:31:10 +0530
>> Shreyansh Jain <shreyansh.jain at nxp.com> wrote:
>>
>>> Introduce a RTE_INIT macro used to mark an init function as a constructor.
>>> Current eal macros have been converted to use this (no functional impact).
>>> DRIVER_REGISTER_PCI is added as a helper for pci drivers.
>>>
>>> Suggested-by: Jan Viktorin <viktorin at rehivetech.com>
>>> Signed-off-by: David Marchand <david.marchand at 6wind.com>
>>> Signed-off-by: Shreyansh Jain <shreyansh.jain at nxp.com>
>>> ---  
>>
>> [...]
>>
>>> +#define RTE_INIT(func) \
>>> +static void __attribute__((constructor, used)) func(void)
>>> +
>>>  #ifdef __cplusplus
>>>  }
>>>  #endif
>>> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
>>> index fa74962..3027adf 100644
>>> --- a/lib/librte_eal/common/include/rte_pci.h
>>> +++ b/lib/librte_eal/common/include/rte_pci.h
>>> @@ -470,6 +470,14 @@ void rte_eal_pci_dump(FILE *f);
>>>   */
>>>  void rte_eal_pci_register(struct rte_pci_driver *driver);
>>>  
>>> +/** Helper for PCI device registeration from driver (eth, crypto) instance */
>>> +#define DRIVER_REGISTER_PCI(nm, drv) \
>>> +RTE_INIT(pciinitfn_ ##nm); \
>>> +static void pciinitfn_ ##nm(void) \
>>> +{ \  
>>
>> You are missing setting the name here like PMD_REGISTER_DRIVER does
>> now. Or should I include it in my patch set?
>>
>> 	(drv).name = RTE_STR(nm);

That is a miss from my side.
I will publish v7 with this. You want this right away or should I wait a little while (more reviews, or any pending additions as per Thomas's notes) before publishing?

> 
> Moreover, it should accept the rte_pci_driver *, shouldn't it? Here, it
> expects a wrapper around it (eth_driver)... I now, my SoC patches were
> supposing the some... but I think it is wrong.
> 
> The original David's patch set contains calls like this:
> 
>   RTE_EAL_PCI_REGISTER(bnx2xvf, rte_bnx2xvf_pmd.pci_drv);
> 
> So, I think, we should go the original way.

I have a slightly different opinion of the above.
IMO, aim of the helpers is to hide the PCI details and continue to make driver consider itself as a generic ETH driver. In that case, dereferencing pci_drv would be done by macro.

Also, considering that in future pci_drv would also have soc_drv, the helpers can effectively hide the intra-structure naming of these. It would help when more such device types (would there be?) are introduced - in which case, driver framework has a consistent coding convention.

But, I am ok switching back to David's way as well - I don't have any strong argument against that.

> 
> Jan
> 
>>
>>> +	rte_eal_pci_register(&drv.pci_drv); \
>>> +}
>>> +
>>>  /**
>>>   * Unregister a PCI driver.
>>>   *
[...]

-
Shreyansh



More information about the dev mailing list