Re: [RFC] lib/igt_kmod: remove devcoredump before a PCI module unload

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Feb 01, 2024 at 05:14:58PM -0600, Lucas De Marchi wrote:
> On Tue, Jan 30, 2024 at 05:37:02PM -0500, 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.
> > 
> > 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 | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 54 insertions(+)
> > 
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index 250ab2107..da44a7bca 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -323,6 +323,58 @@ static int igt_kmod_unload_r(struct kmod_module *kmod, unsigned int flags)
> > 	return err;
> > }
> > 
> > +#define MAX_PATH 256
> 
> just use PATH_MAX? it's more than this and should be sufficient for
> these sysfs paths
> 
> > +
> > +static void _igt_remove_devcoredump(const char *driver)
> > +{
> > +	char sysfspath[MAX_PATH];
> > +	DIR *dir;
> > +	char devcoredump[MAX_PATH];
> > +	FILE *data;
> > +	struct dirent *entry;
> > +
> > +	snprintf(sysfspath, sizeof(sysfspath),
> > +		 "/sys/module/%s/drivers/pci:%s/", driver, driver);
> 
> "/sys/bus/pci/drivers/%s", driver
> 
> I´d also save the return value so you can 1) check if it was truncated,
> 2) do something like  `char *entry = sysfspath + len`,
> so you don't have to keep printing the prefix below.
> 
> > +
> > +	/* Not a PCI module */
> > +	if(access(sysfspath, F_OK) == -1)
> > +		return;
> 
> coding style
> 
> > +
> > +	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) {
> 
> invert the comparison and do a "continue;" ? It would drop the indent a
> little bit.
> 
> > +
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Wformat-truncation"
> > +			snprintf(devcoredump, sizeof(devcoredump),
> > +				 "%s/%s/devcoredump", sysfspath, entry->d_name);
> > +#pragma GCC diagnostic pop
> 
> instead of ignoring the truncation, simply check the return? As per
> above you could do:
> 
> 	ret = snprintf(entry, sizeof(sysfspath) - prefixlen,
> 		       "/%s/devcoredump", entry->d_name);
> 
> then check ret <  sizeof(sysfspath) - prefixlen
> 
> if you follow this, then you should use sysfspath everywhere below
> rather than devcoredump.
> 
> > +
> > +			igt_info("%s\n", devcoredump);
> > +
> > +			if (access(devcoredump, F_OK) != -1) {
> > +				igt_info("Removing devcoredump before module unload: %s\n",
> > +					 devcoredump);
> > +
> > +				strcat(devcoredump, "/data");
> 
> could we just do that above and check if we have a devcoredump/data?
> 
> 
> I'd reword the commit message to say "drop" instead of remove. The
> reason I came here to review was that you had
> "igt_kmod: remove devcoredump ..." and I thought you were talking about a
> **module** called devcoredump.
> 
> 
> > +				data = fopen(devcoredump, "w");
> > +				igt_assert(data);
> > +
> > +				/*
> > +				 * Write anything to devcoredump/data to
> > +				 * force its deletion
> > +				 */
> > +				fprintf(data, "1\n");
> > +				fclose(data);
> > +			}
> > +		}
> > +	}
> > +	closedir(dir);
> > +}
> > +
> > /**
> >  * igt_kmod_unload:
> >  * @mod_name: Module name.
> > @@ -341,6 +393,8 @@ igt_kmod_unload(const char *mod_name, unsigned int flags)
> > 	struct kmod_module *kmod;
> > 	int err;
> > 
> > +	_igt_remove_devcoredump(mod_name);
> 
> what's the _ for? Also as I said above please s/remove/drop/
> 
> However I wonder if we shouldn't **dump** and make it available as
> output, just like we save the dmesg.

all the suggestions above accepted, except the dump here.
dmesg dump is at the runner/executor.

We might need to collect that there or add the udev rule, but
I believe it is orthogonal to the goal of this patch that is
to unblock the module reload/unload after a hang has happened.

Thank you so much for all the suggestions.

> 
> Lucas De Marchi