[RFC] lib/igt_kmod: remove devcoredump before a PCI module unload
Rodrigo Vivi
rodrigo.vivi at intel.com
Fri Mar 8 22:20:10 UTC 2024
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 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.
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
More information about the igt-dev
mailing list