[dpdk-dev] [PATCH v1 07/28] eal/soc: add rte_eal_soc_register/unregister logic

[Jan Viktorin]

On Wed, 15 Jun 2016 05:57:33 +0000
Shreyansh Jain <shreyansh.jain at nxp.com> wrote:

> Hi Jan,
> 
> One more comment which I missed in previous reply:
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Shreyansh Jain
> > Sent: Monday, June 13, 2016 7:50 PM
> > To: Jan Viktorin <viktorin at rehivetech.com>
> > Cc: David Marchand <david.marchand at 6wind.com>; Thomas Monjalon
> > <thomas.monjalon at 6wind.com>; Bruce Richardson <bruce.richardson at intel.com>;
> > Declan Doherty <declan.doherty at intel.com>; jianbo.liu at linaro.org;
> > jerin.jacob at caviumnetworks.com; Keith Wiles <keith.wiles at intel.com>; Stephen
> > Hemminger <stephen at networkplumber.org>; dev at dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v1 07/28] eal/soc: add
> > rte_eal_soc_register/unregister logic
> > 

[...]

> > > +
> > > +/**
> > > + * Empty PMD driver based on the SoC infra.
> > > + *
> > > + * The rte_soc_device is usually wrapped in some higher-level struct
> > > + * (eth_driver). We simulate such a wrapper with an anonymous struct here.
> > > + */
> > > +struct test_wrapper {
> > > +	struct rte_soc_driver soc_drv;
> > > +};
> > > +
> > > +struct test_wrapper empty_pmd0 = {
> > > +	.soc_drv = {
> > > +		.name = "empty_pmd0",
> > > +	},
> > > +};
> > > +
> > > +struct test_wrapper empty_pmd1 = {
> > > +	.soc_drv = {
> > > +		.name = "empty_pmd1",
> > > +	},
> > > +};
> > > +
> > > +static int
> > > +count_registered_socdrvs(void)
> > > +{
> > > +	int i;
> > > +	struct rte_soc_driver *drv;
> > > +
> > > +	i = 0;
> > > +	TAILQ_FOREACH(drv, &soc_driver_list, next)
> > > +		i += 1;
> > > +
> > > +	return i;
> > > +}
> > > +
> > > +static int
> > > +test_register_unregister(void)
> > > +{
> > > +	struct rte_soc_driver *drv;
> > > +	int count;
> > > +
> > > +	rte_eal_soc_register(&empty_pmd0.soc_drv);

[...]

> > > +
> > > +extern struct soc_driver_list soc_driver_list; /**< Global list of SoC
> > > drivers. */
> > >
> > >  struct rte_soc_id {
> > >  	const char *compatible; /**< OF compatible specification */
> > > @@ -131,4 +137,21 @@ rte_eal_compare_soc_addr(const struct rte_soc_addr  
> > *a0,  
> > >  	return strcmp(a0->name, a1->name);
> > >  }
> > >
> > > +/**
> > > + * Register a SoC driver.
> > > + */
> > > +void rte_eal_soc_register(struct rte_soc_driver *driver);
> > > +
> > > +#define RTE_EAL_SOC_REGISTER(name) \
> > > +RTE_INIT(socinitfn_ ##name); \
> > > +static void socinitfn_ ##name(void) \
> > > +{ \
> > > +	rte_eal_soc_register(&name.soc_drv); \  
> 
> It should be 'rte_eal_soc_register(&name)'.
> As a user of 'RTE_EAL_SOC_REGISTER', I would pass reference to 'rte_soc_driver' object. It doesn't have any 'soc_drv' member.

But eth_driver would have it. And other upper-level structs would have it as well.

> 
> I am guessing that because you have created a wrapper structure 'test_wrapper' in test_soc.c which contains a 'soc_drv', the macro reflects that usage.

I don't assume to use rte_soc_driver directly (similarly to rte_pci_driver). However, I agree that
it is strange, we should have RTE_ETHDRV_SOC_REGISTER for such purpose (if needed).

I wanted to avoid redundant arguments to the RTE_EAL_SOC_REGISTER because the name is 
used to create the constructor function. But, it seems that other parts of DPDK does not
care of this so I will probably give up and make it:

RTE_EAL_SOC_REGISTER(my_cool_drv, &my_cool_drv.soc_drv);

Thanks for your opinion. I'll fix it in v2.

Jan

> 
[...]
> 
> -
> Shreyansh
> 



-- 
   Jan Viktorin                  E-mail: Viktorin at RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic