Re: [PATCH v2 17/22] drivers/base: Make device_find_child_by_name() compatible with sysfs inputs

[Dan Williams]

On Mon, Jul 13, 2020 at 8:52 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Jul 13, 2020 at 08:39:43AM -0700, Dan Williams wrote:
> > On Sun, Jul 12, 2020 at 10:09 AM Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Sun, Jul 12, 2020 at 09:27:37AM -0700, Dan Williams wrote:
> > > > Use sysfs_streq() in device_find_child_by_name() to allow it to use a
> > > > sysfs input string that might contain a trailing newline.
> > > >
> > > > The other "device by name" interfaces,
> > > > {bus,driver,class}_find_device_by_name(), already account for sysfs
> > > > strings.
> > > >
> > > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > > Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> > > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > > > ---
> > > >  drivers/base/core.c |    2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index 67d39a90b45c..5d31b962c898 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -3078,7 +3078,7 @@ struct device *device_find_child_by_name(struct device *parent,
> > > >
> > > >       klist_iter_init(&parent->p->klist_children, &i);
> > > >       while ((child = next_device(&i)))
> > > > -             if (!strcmp(dev_name(child), name) && get_device(child))
> > > > +             if (sysfs_streq(dev_name(child), name) && get_device(child))
> > >
> > > Who wants to call this function with a name passed from userspace?
> > >
> > > Not objecting to it, just curious...
> > >
> >
> > The series that incorporates this patch adds a partitioning mechanism
> > to "device-dax region" devices with an:
> >     "echo 1 > regionX/create" to create a new partition / sub-instance
> > of a region, and...
> >     "echo $devname > regionX/delete" to delete. Where $devname is
> > searched in the child devices of regionX to trigger device_del().
>
> Shouldn't that be done in configfs, not sysfs?

I see configfs as an awkward fit for this situation. configfs wants to
software define kernel objects whereas this facility wants to augment
existing kernel enumerated device objects. The region device is
created by firmware policy and is optionally partitioned, configfs
objects don't exist at all until created. So for this I see sysfs +
'scheme to trigger child device creation' as just enough mechanism
that does not warrant full blown configfs.

I believe it was debates like this [1] that have led me to the camp of
sysfs being capable of some device creation dynamism and leave
configfs for purely software constructed objects.

[1]: https://lore.kernel.org/lkml/17377.42813.479466.690408@xxxxxxxxxxxxxxx/

> > This arrangement avoids one of the design mistakes of libnvdimm which
> > uses a sysfs attribute of the device to delete itself. Parent-device
> > triggered deletion rather than self-deletion avoids those locking
> > entanglements.
>
> Ugh, yeah, getting rid of that would be great, it's a mess.  I think
> scsi still does that :(

Yeah, both nvdimm and scsi both end up need to delay device deletion
to its own thread, and it has led to bugs in the nvdimm case.