[dpdk-dev] [PATCH v4 11/17] eal/soc: add default scan for Soc devices

Shreyansh Jain shreyansh.jain at nxp.com
Sun Oct 16 09:12:13 CEST 2016


Hi Jan,

> -----Original Message-----
> From: Jan Viktorin [mailto:viktorin at rehivetech.com]
> Sent: Sunday, October 16, 2016 6:27 AM
> To: Shreyansh Jain <shreyansh.jain at nxp.com>
> Cc: dev at dpdk.org; thomas.monjalon at 6wind.com; david.marchand at 6wind.com
> Subject: Re: [PATCH v4 11/17] eal/soc: add default scan for Soc devices
> 
> On Sat, 15 Oct 2016 19:15:02 +0530
> Shreyansh Jain <shreyansh.jain at nxp.com> wrote:
> 
> > From: Jan Viktorin <viktorin at rehivetech.com>
> >
> > Default implementation which scans the sysfs platform devices hierarchy.
> > For each device, extract the ueven and convert into rte_soc_device.
> >
> > The information populated can then be used in probe to match against
> > the drivers registered.
> >
> > Signed-off-by: Jan Viktorin <viktorin at rehivetech.com>
> > [Shreyansh: restructure commit to be an optional implementation]
> > Signed-off-by: Shreyansh Jain <shreyansh.jain at nxp.com>
> 
> [...]
> 
> > +
> > +int
> > +rte_eal_soc_scan(void)
> 
> What about naming it rte_eal_soc_scan_default? This would underline the
> fact that this function can be replaced.

Yes, that would be in sync with match default. I will do it.

> 
> Second, this is for the 7/17 patch:
> 
> -/* register a driver */
>  void
>  rte_eal_soc_register(struct rte_soc_driver *driver)
>  {
> +	/* For a valid soc driver, match and scan function
> +	 * should be provided.
> +	 */
> +	RTE_VERIFY(driver != NULL);
> +	RTE_VERIFY(driver->match_fn != NULL);
> +	RTE_VERIFY(driver->scan_fn != NULL);
> 
> What about setting the match_fn and scan_fn to default implementations if
> they
> are NULL? This would make the standard/default approach easier to use.
> 
>  	TAILQ_INSERT_TAIL(&soc_driver_list, driver, next);
>  }

I am not in favor of a forced default. What if user never intended it - it would lead to wrong scan being used and only intimation which can provided to user is a log.
Selecting such functions should be a model of PMD - one which is enforced.

> 
> > +{
> > +	struct dirent *e;
> > +	DIR *dir;
> > +	char dirname[PATH_MAX];
> > +
> > +	dir = opendir(soc_get_sysfs_path());
> > +	if (dir == NULL) {
> > +		RTE_LOG(ERR, EAL, "%s(): opendir failed: %s\n",
> > +			__func__, strerror(errno));
> > +		return -1;
> > +	}
> > +
> > +	while ((e = readdir(dir)) != NULL) {
> > +		if (e->d_name[0] == '.')
> > +			continue;
> > +
> > +		snprintf(dirname, sizeof(dirname), "%s/%s",
> > +				soc_get_sysfs_path(), e->d_name);
> > +		if (soc_scan_one(dirname, e->d_name) < 0)
> > +			goto error;
> > +	}
> > +	closedir(dir);
> > +	return 0;
> > +
> > +error:
> > +	closedir(dir);
> > +	return -1;
> > +}
> > +
> >  /* Init the SoC EAL subsystem */
> >  int
> >  rte_eal_soc_init(void)
> 
> 
> 
> --
>   Jan Viktorin                E-mail: Viktorin at RehiveTech.com
>   System Architect            Web:    www.RehiveTech.com
>   RehiveTech
>   Brno, Czech Republic

Thanks for your quick comments.

I have not yet taken all the inputs you had provided in review of v3 - I will be replying to those soon marking out what I have taken and what I have not.
 
-
Shreyansh


More information about the dev mailing list