Re: [PATCH i-g-t 2/3] lib/igt_kmod: drop devcoredump before a PCI module unload

["Souza, Jose" <jose.souza@intel.com>]

On Fri, 2024-04-05 at 12:36 -0500, Lucas De Marchi wrote:
> On Wed, Apr 03, 2024 at 11:14:08AM -0400, Rodrigo Vivi wrote:
> > devcoredump holds a module reference, blocking the module removal.
> > 
> > It is intentional from the devcoredump perspective to keep the
> > log available even after the unbind/unprobe. However it blocks
> > our module removal here.

'devcoredump: Add dev_coredump_put()' was reviewed by devcoredump maintainers, so we can remove devcoredump before unload Xe.
So I don't think we will need this patch.

It is still pending on getting pushed but we could add to the topic branches to unblock CI if needed.

> > 
> > v2: Accepting many suggestions from Lucas.
> > 
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > lib/igt_kmod.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 55 insertions(+)
> > 
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index cc242838f..14d51f4f6 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -323,6 +323,59 @@ static int igt_kmod_unload_r(struct kmod_module *kmod, unsigned int flags)
> > 	return err;
> > }
> > 
> > +static void igt_drop_devcoredump(const char *driver)
> > +{
> > +	char sysfspath[PATH_MAX];
> > +	DIR *dir;
> > +	char *devcoredump;
> > +	FILE *data;
> > +	struct dirent *entry;
> > +	int len, ret;
> > +
> > +	len = snprintf(sysfspath, sizeof(sysfspath),
> > +		       "/sys/bus/pci/drivers/%s", driver);
> > +
> > +	igt_assert(len < sizeof(sysfspath));
> > +
> > +	 /* Not a PCI module */
> > +	if (access(sysfspath, F_OK))
> > +		return;
> > +
> > +	devcoredump = sysfspath + len;
> > +
> > +	dir = opendir(sysfspath);
> > +	igt_assert(dir);
> > +
> > +	while ((entry = readdir(dir)) != NULL) {
> > +		if (entry->d_type != DT_LNK ||
> > +		    strcmp(entry->d_name, ".") == 0 ||
> > +		    strcmp(entry->d_name, "..") == 0)
> > +			continue;
> > +
> > +		ret = snprintf(devcoredump, sizeof(sysfspath) - len,
> > +			       "/%s/devcoredump", entry->d_name);
> 
> I  think this could be simplified a little bit further
> 
> 		ret = snprintf(devcoredump, sizeof(sysfspath) - len,
> 			       "/%s/devcoredump/data", entry->d_name);
> 		igt_assert(ret < sizeof(sysfspath) - len);
> 
> 		data = fopen(sysfspath, "w");
> 		if (data) {
> 			igt_info("Removing devcoredump before module unload: %s\n",
> 				 sysfspath);
> 
> 			/*
> 			 * Write anything to devcoredump/data to
> 			 * force its deletion
> 			 */
> 			fprintf(data, "1\n");
> 			fclose(data);
> 		}
> 
> so it drops the TOCTOU of access()/open() and make it shorter.
> ... but totally optional. And untested).
> 
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Lucas De Marchi