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

[Lucas De Marchi]

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 at linux.intel.com>
>Cc: José Roberto de Souza <jose.souza at intel.com>
>Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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.

Lucas De Marchi