diff mbox series

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

Message ID 159457125753.754248.6000936585361264069.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive)
State New, archived
Headers show
Series device-dax: Support sub-dividing soft-reserved ranges | expand

Commit Message

Dan Williams July 12, 2020, 4:27 p.m. UTC
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@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/base/core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Greg Kroah-Hartman July 12, 2020, 5:09 p.m. UTC | #1
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@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  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...

thanks,

greg k-h
Dan Williams July 13, 2020, 3:39 p.m. UTC | #2
On Sun, Jul 12, 2020 at 10:09 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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@linuxfoundation.org>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  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().

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.
Greg Kroah-Hartman July 13, 2020, 3:52 p.m. UTC | #3
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@linuxfoundation.org> 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@linuxfoundation.org>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > ---
> > >  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?

> 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 :(

thanks,

greg k-h
Dan Williams July 13, 2020, 4:09 p.m. UTC | #4
On Mon, Jul 13, 2020 at 8:52 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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@linuxfoundation.org> 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@linuxfoundation.org>
> > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > > ---
> > > >  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@cse.unsw.edu.au/

> > 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.
Greg Kroah-Hartman July 13, 2020, 4:12 p.m. UTC | #5
On Mon, Jul 13, 2020 at 09:09:18AM -0700, Dan Williams wrote:
> On Mon, Jul 13, 2020 at 8:52 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> 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@linuxfoundation.org> 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@linuxfoundation.org>
> > > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > > > ---
> > > > >  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@cse.unsw.edu.au/

"some" :)

And that was from 2006, ugh, how did you find that...

Ok, that's fine, no objection from me for this patch:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Dan Williams July 13, 2020, 4:36 p.m. UTC | #6
On Mon, Jul 13, 2020 at 9:13 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 13, 2020 at 09:09:18AM -0700, Dan Williams wrote:
> > On Mon, Jul 13, 2020 at 8:52 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> 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@linuxfoundation.org> 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@linuxfoundation.org>
> > > > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > > > > ---
> > > > > >  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@cse.unsw.edu.au/
>
> "some" :)

Yes, lowercase and quoted: "some" :).

> And that was from 2006, ugh, how did you find that...

Oh, public-inbox is a wonderful thing. "I kinda sort of remember Neil
laying out a configfs vs sysfs argument", /me searches for "f:neil
configfs" and voila.

> Ok, that's fine, no objection from me for this patch:
>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks, Greg.
diff mbox series

Patch

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))
 			break;
 	klist_iter_exit(&i);
 	return child;