[PATCH v7 1/4] lib/igt_sysfs: Add engine list helpers

[Cavitt, Jonathan]

-----Original Message-----
From: Kamil Konieczny <kamil.konieczny at linux.intel.com> 
Sent: Tuesday, November 26, 2024 9:07 AM
To: igt-dev at lists.freedesktop.org
Cc: Cavitt, Jonathan <jonathan.cavitt at intel.com>; Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; Belgaumkar, Vinay <vinay.belgaumkar at intel.com>
Subject: Re: [PATCH v7 1/4] lib/igt_sysfs: Add engine list helpers
> 
> Hi Jonathan,
> On 2024-11-18 at 18:05:08 +0000, Jonathan Cavitt wrote:
> > Create two new helper functions, igt_sysfs_get_engine_list and
> > igt_sysfs_free_engine_list, that create and destroy lists of open
> > engines, respectively.  The list created by igt_sysfs_get_engine_list
> > can be used to iterate over the set of engines in sysfs/engines and must
> > be freed by igt_sysfs_free_engine_list after use.
> > 
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> 
> Please check all patches in series with checkpatch.pl and correct
> findings.
> 
> > ---
> >  lib/igt_sysfs.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/igt_sysfs.h |  3 +++
> >  2 files changed, 57 insertions(+)
> > 
> > diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> > index 00d5822fd3..3d2dd364cf 100644
> > --- a/lib/igt_sysfs.c
> > +++ b/lib/igt_sysfs.c
> > @@ -1307,6 +1307,60 @@ static uint16_t xe_get_engine_class(char *name)
> >  	return class;
> >  }
> >  
> > +/**
> > + * igt_sysfs_get_engine_list:
> > + * @engines: fd of the directory engine
> > + * @size: pointer to store the size of the returned char pointer
> > + *
> > + * Iterates over sysfs/engines and returns an array of
> > + * opened engines.  The user will be in charge of closing
> > + * the opened engines.
> > + *
> > + * The number of opened engines will be saved to size.
> > + */
> > +int *igt_sysfs_get_engine_list(int engines, int *size)
> > +{
> > +	struct dirent *de;
> > +	DIR *dir;
> > +	int* ret = calloc(1, sizeof(int));
> 
> 	int *ret = ...
> 
> Assert for NULL here: igt_assert(ret)
> 
> Why not 16? So you should avoid realloc?
> This would require to keep its size but imho would be better.
> Up to you.

Wouldn't using 16 here be considered a "magic number" and thus
necessitate some defined macro?  I guess I could just call it ARRAY_MAX
and undefine it after the function...

> 
> > +
> > +	lseek(engines, 0, SEEK_SET);
> > +
> > +	dir = fdopendir(engines);
> > +	if (!dir)
> > +		close(engines);
> 
> Should be:
> 
> 	if (!dir)
> 		return NULL;
> 
> Btw why do you close engines here?

This mirrors igt_sysfs_engines.  If it's wrong here, then it's probably wrong there as well.

> 
> > +
> > +	*size = 0;
> 
> Move this to begin of function. Btw do we need size? You could assume
> a guard at end with -1 so one param less.

That would require some rework on how we iterate over the lists in the
tests that will use this helper function, but otherwise that should be doable.

> 
> 
> > +	while ((de = readdir(dir))) {
> > +		if (*de->d_name == '.')
> > +			continue;
> > +		ret[*size] = openat(engines, de->d_name, O_RDONLY);
> > +		if (ret[*size] < 0) {
> 
> Why ignore? imho this is something unexpected?

We also ignore in igt_sysfs_engines.

> 
> Overall looks good, thx for fixing this.
> 
> Regards,
> Kamil
> 
> > +			ret[*size] = 0;
> > +			continue;
> > +		}
> > +		*size += 1;
> > +		reallocarray(ret, *size + 1, sizeof(int));
> > +	}
> > +	return ret;
> > +}
> > +
> > +/**
> > + * igt_sysfs_free_engine_list:
> > + * @list: list of opened engines
> > + * @size: number of engines in list
> > + *
> > + * Helper for cleaning up after igt_sysfs_get_engine_list.
> > + * Closes all engines in list before freeing the list.
> > + */
> > +void igt_sysfs_free_engine_list(int *list, int size)
> > +{
> > +	int i;
> > +	for (i = 0; i < size; i++)
> > +		close(list[i]);
> > +	free(list);
> > +}
> > +
> >  /**
> >   * igt_sysfs_engines:
> >   * @xe: fd of the device
> > diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> > index 54a4087918..ba0a74755c 100644
> > --- a/lib/igt_sysfs.h
> > +++ b/lib/igt_sysfs.h
> > @@ -168,6 +168,9 @@ typedef struct igt_sysfs_rw_attr {
> >  
> >  void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw);
> >  
> > +int *igt_sysfs_get_engine_list(int engines, int *size);
> > +void igt_sysfs_free_engine_list(int *list, int size);
> > +
> >  void igt_sysfs_engines(int xe, int engines, int gt, bool all, const char **property,
> >  		       void (*test)(int, int, const char **, uint16_t, int));
> >  
> > -- 
> > 2.43.0
> > 
>