| Message ID | 159457125753.754248.6000936585361264069.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | device-dax: Support sub-dividing soft-reserved ranges | expand |
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
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.
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
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.
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>
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 --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;
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(-)