On Thursday 14 July 2016 09:27 PM, Jan Viktorin wrote:
> On Thu, 14 Jul 2016 10:57:55 +0530
> Shreyansh jain <shreyansh.jain at nxp.com> wrote:
>
>> 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?
>
> Please. The time is almost gone. 18/7/2016 is the release (according
> to the roadmap)... I have to fix it in my patchset, otherwise it
> does not build (after moving the .name from rte_pci_driver to
> rte_driver).
>
I didn't consider 18/Jul.
Please go ahead. I will continue to send v7 _without_ the above change so that
your patchset doesn't break. This way you will not get blocked because of me.
>>
>>>
>>> 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.
>
> In this case, I'd prefer to see DRIVER_REGISTER_PCI_ETH.
>
> At first, this was also my way of thinking. But I've changed my mind. I
> find it to be a bit overdesigned.
There is:
DRIVER_REGISTER_PCI(...)
DRIVER_REGISTER_PCI_TABLE(...)
Wouldn't DRIVER_REGISTER_PCI_ETH look out-of-place?
>
>>
>> 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.
>
> Hide? I am afraid, I don't understand clearly what you mean.
DRIVER_REGISTER_PCI(eth_driver)
DRIVER_REGISTER_SOC(eth_driver)
DRIVER_REGISTER_XXX(eth_driver)
...
In either case, the caller always creates the eth_driver and populates internal
specific driver structure (pci_drv) as a sub-part of eth_driver specification.
Macro 'hides' the internal structure name (pci_drv, soc_drv...).
But again, nothing critical. Just a way of usage. We might not even have a
'XXX' in near future.
>
>>
>> But, I am ok switching back to David's way as well - I don't have any strong
>> argument against that.
>
> I'd like to preserve the clear semantics. That is DRIVER_REGISTER_PCI
> -> give a pci device.
>
> Has anybody a different opinion? David? Thomas?
Yes please.
Or else, if nothing comes up soon, I will simply go ahead and change to
DRIVER_REGISTER_PCI(eth_driver.pci_drv) as this trivial issue shouldn't hold
back this series.
>
>>
>>>
>>> Jan
>>>
>>>>
>>>>> + rte_eal_pci_register(&drv.pci_drv); \
>>>>> +}
>>>>> +
>>>>> /**
>>>>> * Unregister a PCI driver.
>>>>> *
>> [...]
>>
>> -
>> Shreyansh
>>
>
>
>
