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
