Re: [ndctl PATCH 2/2] libdaxctl: fix device reconfiguration with builtin drivers

From: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
To: "Verma,
	Vishal L"
	<vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: "Brice.Goglin-MZpvjPyXg2s@public.gmane.org"
	<Brice.Goglin-MZpvjPyXg2s@public.gmane.org>,
	"dave.hansen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org"
	<dave.hansen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>,
	"linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org"
	<linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org>
Subject: Re: [ndctl PATCH 2/2] libdaxctl: fix device reconfiguration with builtin drivers
Date: Wed, 4 Sep 2019 14:01:31 -0700	[thread overview]
Message-ID: <CAPcyv4inooSbqCaAXJ4KzwVMxcDpfKpMD2QG5aXOP_sKnFy6UQ@mail.gmail.com> (raw)
In-Reply-To: <be47815b3d454ce76a81799ba355b5579713c916.camel-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

On Wed, Sep 4, 2019 at 1:27 PM Verma, Vishal L <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>
> On Tue, 2019-09-03 at 19:20 -0700, Dan Williams wrote:
> >
> > > +static int try_kmod_builtin(struct daxctl_dev *dev, const char *mod_name)
> > > +{
> > > +       const char *devname = daxctl_dev_get_devname(dev);
> > > +       struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
> > > +       struct kmod_module *kmod;
> > > +       int rc = -ENXIO;
> > > +
> > > +       rc = kmod_module_new_from_name(ctx->kmod_ctx, mod_name, &kmod);
> > > +       if (rc < 0) {
> > > +               err(ctx, "%s: failed getting module for: %s: %s\n",
> > > +                       devname, mod_name, strerror(-rc));
> > > +               return rc;
> > > +       }
> > > +
> > > +       if (kmod_module_get_initstate(kmod) != KMOD_MODULE_BUILTIN)
> > > +               return -ENXIO;
> > > +
> > > +       dbg(ctx, "%s inserting module: %s\n", devname,
> > > +               kmod_module_get_name(kmod));
> > > +       rc = kmod_module_probe_insert_module(kmod,
> > > +                       KMOD_PROBE_APPLY_BLACKLIST,
> > > +                       NULL, NULL, NULL, NULL);
> > > +       if (rc < 0) {
> > > +               err(ctx, "%s: insert failure: %d\n", devname, rc);
> > > +               return rc;
> > > +       }
> > > +       dev->module = kmod;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  static int daxctl_insert_kmod_for_mode(struct daxctl_dev *dev,
> > >                 const char *mod_name)
> > >  {
> > > @@ -877,6 +908,8 @@ static int daxctl_insert_kmod_for_mode(struct daxctl_dev *dev,
> > >         int rc = -ENXIO;
> > >
> > >         if (dev->kmod_list == NULL) {
> >
> > Hmm, why wait until now to check if this list is NULL. How about fall
> > back to kmod_module_new_from_name() at to_module_list() time? That
> > would seem to simplify this follow up routine to not need to worry
> > about working around a NULL list.
>
> So we moved the list checking to later in the process around v4 of the
> original series, so that we don't unnecessarily fail add_dax_dev() if
> for some reason a list wasn't created.

Ah true, I forgot that wrinkle, however...

> Also, we use mod_name = dax_modules[mode] during an 'enable' to
> determine the module name to use for the fallback - we wouldn't have
> this at add_dax_dev() time.

Since modalias is already not reliable it seems the implementation
should go ahead never do module lookups and just do everything based
on module names.

In other words the libndctl panacea of not needing to hard code module
names is already lost in libdaxctl land. If the code drops modalias
usage does that clean up some of these flows?