[PATCH 0/9] Dynamic DT device nodes

[PATCH 0/9] Dynamic DT device nodes

[Zev Weiss]

Hello,

This patch series is in some ways kind of a v2 for the "Dynamic
aspeed-smc flash chips via 'reserved' DT status" series I posted
previously [0], but takes a fairly different approach suggested by Rob
Herring [1] and doesn't actually touch the aspeed-smc driver or
anything in the MTD subsystem, so I haven't marked it as such.

To recap a bit of the context from that series, in OpenBMC there's a
need for certain devices (described by device-tree nodes) to be able
to be attached and detached at runtime (for example the SPI flash for
the host's firmware, which is shared between the BMC and the host but
can only be accessed by one or the other at a time).  To provide that
ability, this series adds support for a new common device-tree
property, a boolean "dynamic" that indicates that the device may come
and go at runtime.  When present on a node, the sysfs file for that
node's "status" property is made writable, allowing userspace to do
things like:

  $ echo okay > /sys/firmware/devicetree/.../status
  $ echo reserved > /sys/firmware/devicetree/.../status

to activate and deactivate a dynamic device.

Because it leans on the OF_DYNAMIC machinery internally, this
functionality will only work on busses that register for OF reconfig
notifications and handle them appropriately (presently platform, i2c,
and spi).  This series does not attempt to solve the "dynamic devices
further down the tree" problem [2]; my hope is that handling for OF
reconfig notifications can be extended to other families of devices
(e.g. individual MTD spi-nor flash chips) in the future.

The central change of the series is patch 6; patches 1-5 are various
small infrastructure bits and plumbing tweaks in preparation for #6;
patches 7-9 are Kconfig, documentation, and an inaugural use of the
new property in the ASRock e3c246d4i BMC device tree.

Note that this series requires the duplicate-declaration removal patch
that was recently merged in Rob's tree [3]; it changes one of the
duplicated declarations and hence will not compile without that patch
(because the declarations no longer match).

[0] https://lore.kernel.org/openbmc/20210929115409.21254-1-zev@bewilderbeest.net/
[1] https://lore.kernel.org/openbmc/CAL_JsqJH+b5oFuSP+KBLBsN5QTA6xASuqXJWXUaDkHhugXPpnQ@mail.gmail.com/
[2] https://lore.kernel.org/openbmc/20210929220038.GS17315@packtop/
[3] https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?id=6663ae07d995f5fbe2988a19858b2f87e68cf929

Zev Weiss (9):
  sysfs: add sysfs_remove_bin_file_self() function
  sysfs: add growable flag to struct bin_attribute
  lib/string: add sysfs_buf_streq()
  of: add self parameter to __of_sysfs_remove_bin_file()
  of: add self parameter to of_update_property()
  of: add support for 'dynamic' DT property
  of: make OF_DYNAMIC selectable independently of OF_UNITTEST
  dt-bindings: document new 'dynamic' common property
  ARM: dts: aspeed: Add e3c246d4i BIOS flash device

 .../devicetree/bindings/common-properties.txt | 18 ++++
 .../boot/dts/aspeed-bmc-asrock-e3c246d4i.dts  | 23 ++++++
 drivers/of/Kconfig                            |  8 +-
 drivers/of/base.c                             |  7 +-
 drivers/of/dynamic.c                          |  2 +-
 drivers/of/kobj.c                             | 82 +++++++++++++++++--
 drivers/of/of_private.h                       |  6 +-
 fs/sysfs/file.c                               | 15 +++-
 include/linux/of.h                            |  7 +-
 include/linux/string.h                        |  1 +
 include/linux/sysfs.h                         |  2 +
 lib/string.c                                  | 34 ++++++++
 12 files changed, 187 insertions(+), 18 deletions(-)

-- 
2.33.0

[PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function

[Zev Weiss]

This is simply the bin_attribute analog to sysfs_remove_file_self().

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 fs/sysfs/file.c       | 13 +++++++++++++
 include/linux/sysfs.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index d019d6ac6ad0..b2b85be95adf 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -572,6 +572,19 @@ void sysfs_remove_bin_file(struct kobject *kobj,
 }
 EXPORT_SYMBOL_GPL(sysfs_remove_bin_file);
 
+/**
+ * sysfs_remove_file_self - remove binary object attribute from its own method
+ * @kobj: object to operate on
+ * @attr: binary attribute descriptor
+ *
+ * See kernfs_remove_self() for details.
+ */
+bool sysfs_remove_bin_file_self(struct kobject *kobj, const struct bin_attribute *attr)
+{
+	return sysfs_remove_file_self(kobj, &attr->attr);
+}
+EXPORT_SYMBOL_GPL(sysfs_remove_bin_file_self);
+
 static int internal_change_owner(struct kernfs_node *kn, kuid_t kuid,
 				 kgid_t kgid)
 {
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index e3f1e8ac1f85..49de5189cf88 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -273,6 +273,7 @@ int __must_check sysfs_create_bin_file(struct kobject *kobj,
 				       const struct bin_attribute *attr);
 void sysfs_remove_bin_file(struct kobject *kobj,
 			   const struct bin_attribute *attr);
+bool sysfs_remove_bin_file_self(struct kobject *kobj, const struct bin_attribute *attr);
 
 int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
 				   const char *name);
-- 
2.33.0

Re: [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function

[Greg Kroah-Hartman]

On Wed, Oct 06, 2021 at 05:09:46PM -0700, Zev Weiss wrote:
> This is simply the bin_attribute analog to sysfs_remove_file_self().

No, no binary sysfs file should be triggering a remove.

binary sysfs files are "pass-through-only" from userspace to hardware,
the kernel should not be even knowing what is read/written to them.

What do you think this is needed for?

thanks,

greg k-h

Re: [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function

[Zev Weiss]

On Wed, Oct 06, 2021 at 10:23:33PM PDT, Greg Kroah-Hartman wrote:
>On Wed, Oct 06, 2021 at 05:09:46PM -0700, Zev Weiss wrote:
>> This is simply the bin_attribute analog to sysfs_remove_file_self().
>
>No, no binary sysfs file should be triggering a remove.
>
>binary sysfs files are "pass-through-only" from userspace to hardware,
>the kernel should not be even knowing what is read/written to them.
>
>What do you think this is needed for?
>

So, I initially set out to be able to activate/deactivate specific DT 
nodes at runtime by using the device-tree "reserved" status as defined 
in the spec (but not currently used anywhere in the kernel) to mean 
essentially "create a device for this but don't bind a driver to it" 
(leaving it to userspace to invoke bind/unbind or similar), and added 
initial support for the specific driver I'm concerned with at the moment 
(aspeed-smc) -- that was the previous patch series linked in the cover 
letter of this one.

In the discussion of that series, Rob suggested as an alternate 
approach:

> Another possibility is making 'status' writeable from userspace. It is
> just a sysfs file.

That seemed sort of appealing to me, and this seemed like the most 
obvious way to go about implementing it.  Given that DT properties are 
binary attributes, I gather you'd consider that a non-starter though?


Zev

Re: [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function

[Greg Kroah-Hartman]

On Wed, Oct 06, 2021 at 10:58:59PM -0700, Zev Weiss wrote:
> On Wed, Oct 06, 2021 at 10:23:33PM PDT, Greg Kroah-Hartman wrote:
> > On Wed, Oct 06, 2021 at 05:09:46PM -0700, Zev Weiss wrote:
> > > This is simply the bin_attribute analog to sysfs_remove_file_self().
> > 
> > No, no binary sysfs file should be triggering a remove.
> > 
> > binary sysfs files are "pass-through-only" from userspace to hardware,
> > the kernel should not be even knowing what is read/written to them.
> > 
> > What do you think this is needed for?
> > 
> 
> So, I initially set out to be able to activate/deactivate specific DT nodes
> at runtime by using the device-tree "reserved" status as defined in the spec
> (but not currently used anywhere in the kernel) to mean essentially "create
> a device for this but don't bind a driver to it" (leaving it to userspace to
> invoke bind/unbind or similar), and added initial support for the specific
> driver I'm concerned with at the moment (aspeed-smc) -- that was the
> previous patch series linked in the cover letter of this one.
> 
> In the discussion of that series, Rob suggested as an alternate approach:
> 
> > Another possibility is making 'status' writeable from userspace. It is
> > just a sysfs file.
> 
> That seemed sort of appealing to me, and this seemed like the most obvious
> way to go about implementing it.  Given that DT properties are binary
> attributes, I gather you'd consider that a non-starter though?

Why would a text attribute of "status" be a binary sysfs file?  That
feels really wrong as again, binary sysfs files are not supposed to be
parsed or handled by the kernel at all, they are only a pass-through.

thanks,

greg k-h

Re: [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function

[Zev Weiss]

On Wed, Oct 06, 2021 at 11:12:37PM PDT, Greg Kroah-Hartman wrote:
>On Wed, Oct 06, 2021 at 10:58:59PM -0700, Zev Weiss wrote:
>> On Wed, Oct 06, 2021 at 10:23:33PM PDT, Greg Kroah-Hartman wrote:
>> > On Wed, Oct 06, 2021 at 05:09:46PM -0700, Zev Weiss wrote:
>> > > This is simply the bin_attribute analog to sysfs_remove_file_self().
>> >
>> > No, no binary sysfs file should be triggering a remove.
>> >
>> > binary sysfs files are "pass-through-only" from userspace to hardware,
>> > the kernel should not be even knowing what is read/written to them.
>> >
>> > What do you think this is needed for?
>> >
>>
>> So, I initially set out to be able to activate/deactivate specific DT nodes
>> at runtime by using the device-tree "reserved" status as defined in the spec
>> (but not currently used anywhere in the kernel) to mean essentially "create
>> a device for this but don't bind a driver to it" (leaving it to userspace to
>> invoke bind/unbind or similar), and added initial support for the specific
>> driver I'm concerned with at the moment (aspeed-smc) -- that was the
>> previous patch series linked in the cover letter of this one.
>>
>> In the discussion of that series, Rob suggested as an alternate approach:
>>
>> > Another possibility is making 'status' writeable from userspace. It is
>> > just a sysfs file.
>>
>> That seemed sort of appealing to me, and this seemed like the most obvious
>> way to go about implementing it.  Given that DT properties are binary
>> attributes, I gather you'd consider that a non-starter though?
>
>Why would a text attribute of "status" be a binary sysfs file?  That
>feels really wrong as again, binary sysfs files are not supposed to be
>parsed or handled by the kernel at all, they are only a pass-through.
>

Well, at present all device tree properties are binary sysfs files 
regardless of type, and from a brief git history check it appears 
they've been that way since DT sysfs support was introduced in commit 
75b57ecf9d1d ("of: Make device nodes kobjects so they show up in 
sysfs").

On the surface it seems like it would make sense for string properties 
like status to be text files instead of binary, but (a) looking at some 
of the discussion that preceded that patch, it sounds like there may be 
some ambiguity regarding what the "true" types of different properties 
actually are [0], and (b) changing the contents of those files from e.g.  
"okay\0" to "okay\n" seems likely to lead to broken userspace, so I'd 
guess it's probably moot anyway.

[0] https://lore.kernel.org/lkml/1363801579.17680.3.camel@pasglop/


Zev

[PATCH 2/9] sysfs: add growable flag to struct bin_attribute

[Zev Weiss]

Previously, sysfs_kf_bin_write() unconditionally disallowed writing
past the existing size of the file.  In order to support mutable
device-tree status properties (which are bin_attributes), we need to
be able to write a longer value over a shorter existing one
(e.g. writing "reserved\n" over "okay\0").  bin_attributes that
require this can now set the growable flag to disable that checking
and allow arbitrary amounts of data to be written at arbitrary
offsets.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 fs/sysfs/file.c       | 2 +-
 include/linux/sysfs.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index b2b85be95adf..156df003ea8f 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -147,7 +147,7 @@ static ssize_t sysfs_kf_bin_write(struct kernfs_open_file *of, char *buf,
 	struct kobject *kobj = of->kn->parent->priv;
 	loff_t size = file_inode(of->file)->i_size;
 
-	if (size) {
+	if (!battr->growable && size) {
 		if (size <= pos)
 			return -EFBIG;
 		count = min_t(ssize_t, count, size - pos);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 49de5189cf88..f8a56094c6c9 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -175,6 +175,7 @@ struct address_space;
 struct bin_attribute {
 	struct attribute	attr;
 	size_t			size;
+	bool			growable:1;
 	void			*private;
 	struct address_space *(*f_mapping)(void);
 	ssize_t (*read)(struct file *, struct kobject *, struct bin_attribute *,
-- 
2.33.0

[PATCH 3/9] lib/string: add sysfs_buf_streq()

[Zev Weiss]

This is intended for use with mutable device-tree string properties.
As with sysfs_streq(), it accepts a trailing linefeed for convenient
shell access (e.g. 'echo foo > /sys/firmware/devicetree/...'), but
also accepts a trailing NUL terminator to match how DT string
properties are presented when read (so that we don't reject userspace
writing back exactly what it had previously read from the file).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 include/linux/string.h |  1 +
 lib/string.c           | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 5e96d656be7a..d066ff82d1ec 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -183,6 +183,7 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
 extern void argv_free(char **argv);
 
 extern bool sysfs_streq(const char *s1, const char *s2);
+extern bool sysfs_buf_streq(const void *buf, size_t blen, const char *str);
 int match_string(const char * const *array, size_t n, const char *string);
 int __sysfs_match_string(const char * const *array, size_t n, const char *s);
 
diff --git a/lib/string.c b/lib/string.c
index b2de45a581f4..aab5cadb6b98 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -715,6 +715,40 @@ bool sysfs_streq(const char *s1, const char *s2)
 }
 EXPORT_SYMBOL(sysfs_streq);
 
+/**
+ * sysfs_buf_streq - check if a buffer matches a string, modulo trailing \n or \0
+ * @buf: pointer to the buffer to check
+ * @blen: length of the buffer to check
+ * @str: string to compare against
+ *
+ * This routine returns true iff the buffer equals the string, treating a
+ * trailing \n or \0 on the buffer as an accepted terminator, the length of
+ * which (aside from the optional terminator) must match the length of the
+ * string.  It's geared for use with sysfs bin_attribute inputs, which may
+ * terminate with newlines or NULs (the latter to match how device-tree string
+ * properties in particular are presented on read).
+ */
+bool sysfs_buf_streq(const void *buf, size_t blen, const char *str)
+{
+	const char *p = buf;
+	size_t slen = strlen(str);
+	if (blen < slen)
+		return false;
+
+	if (memcmp(p, str, slen))
+		return false;
+
+	switch (blen - slen) {
+	case 0:
+		return true;
+	case 1:
+		return p[slen] == '\n' || p[slen] == '\0';
+	default:
+		return false;
+	}
+}
+EXPORT_SYMBOL(sysfs_buf_streq);
+
 /**
  * match_string - matches given string in an array
  * @array:	array of strings
-- 
2.33.0

[PATCH 4/9] of: add self parameter to __of_sysfs_remove_bin_file()

[Zev Weiss]

This allows using the function to remove a bin_attribute from that
attribute's own methods (by calling sysfs_remove_bin_file_self()
instead of sysfs_remove_bin_file()).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/of/kobj.c       | 13 ++++++++-----
 drivers/of/of_private.h |  2 +-
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
index 6675b5e56960..06d6c90f7aa1 100644
--- a/drivers/of/kobj.c
+++ b/drivers/of/kobj.c
@@ -84,12 +84,15 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
 	return rc;
 }
 
-void __of_sysfs_remove_bin_file(struct device_node *np, struct property *prop)
+void __of_sysfs_remove_bin_file(struct device_node *np, struct property *prop, bool self)
 {
 	if (!IS_ENABLED(CONFIG_SYSFS))
 		return;
 
-	sysfs_remove_bin_file(&np->kobj, &prop->attr);
+	if (self)
+		sysfs_remove_bin_file_self(&np->kobj, &prop->attr);
+	else
+		sysfs_remove_bin_file(&np->kobj, &prop->attr);
 	kfree(prop->attr.attr.name);
 }
 
@@ -97,7 +100,7 @@ void __of_remove_property_sysfs(struct device_node *np, struct property *prop)
 {
 	/* at early boot, bail here and defer setup to of_init() */
 	if (of_kset && of_node_is_attached(np))
-		__of_sysfs_remove_bin_file(np, prop);
+		__of_sysfs_remove_bin_file(np, prop, false);
 }
 
 void __of_update_property_sysfs(struct device_node *np, struct property *newprop,
@@ -108,7 +111,7 @@ void __of_update_property_sysfs(struct device_node *np, struct property *newprop
 		return;
 
 	if (oldprop)
-		__of_sysfs_remove_bin_file(np, oldprop);
+		__of_sysfs_remove_bin_file(np, oldprop, false);
 	__of_add_property_sysfs(np, newprop);
 }
 
@@ -157,7 +160,7 @@ void __of_detach_node_sysfs(struct device_node *np)
 	/* only remove properties if on sysfs */
 	if (of_node_is_attached(np)) {
 		for_each_property_of_node(np, pp)
-			__of_sysfs_remove_bin_file(np, pp);
+			__of_sysfs_remove_bin_file(np, pp, false);
 		kobject_del(&np->kobj);
 	}
 
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 75e67b8bb826..fff157c63907 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -134,7 +134,7 @@ extern int __of_update_property(struct device_node *np,
 extern void __of_detach_node(struct device_node *np);
 
 extern void __of_sysfs_remove_bin_file(struct device_node *np,
-				       struct property *prop);
+                                       struct property *prop, bool selfremove);
 
 /* illegal phandle value (set when unresolved) */
 #define OF_PHANDLE_ILLEGAL	0xdeadbeef
-- 
2.33.0

Re: [PATCH 4/9] of: add self parameter to __of_sysfs_remove_bin_file()

[Greg Kroah-Hartman]

On Wed, Oct 06, 2021 at 05:09:49PM -0700, Zev Weiss wrote:
> This allows using the function to remove a bin_attribute from that
> attribute's own methods (by calling sysfs_remove_bin_file_self()
> instead of sysfs_remove_bin_file()).
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  drivers/of/kobj.c       | 13 ++++++++-----
>  drivers/of/of_private.h |  2 +-
>  2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
> index 6675b5e56960..06d6c90f7aa1 100644
> --- a/drivers/of/kobj.c
> +++ b/drivers/of/kobj.c
> @@ -84,12 +84,15 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
>  	return rc;
>  }
>  
> -void __of_sysfs_remove_bin_file(struct device_node *np, struct property *prop)
> +void __of_sysfs_remove_bin_file(struct device_node *np, struct property *prop, bool self)
>  {
>  	if (!IS_ENABLED(CONFIG_SYSFS))
>  		return;
>  
> -	sysfs_remove_bin_file(&np->kobj, &prop->attr);
> +	if (self)
> +		sysfs_remove_bin_file_self(&np->kobj, &prop->attr);
> +	else
> +		sysfs_remove_bin_file(&np->kobj, &prop->attr);
>  	kfree(prop->attr.attr.name);
>  }
>  
> @@ -97,7 +100,7 @@ void __of_remove_property_sysfs(struct device_node *np, struct property *prop)
>  {
>  	/* at early boot, bail here and defer setup to of_init() */
>  	if (of_kset && of_node_is_attached(np))
> -		__of_sysfs_remove_bin_file(np, prop);
> +		__of_sysfs_remove_bin_file(np, prop, false);
>  }
>  
>  void __of_update_property_sysfs(struct device_node *np, struct property *newprop,
> @@ -108,7 +111,7 @@ void __of_update_property_sysfs(struct device_node *np, struct property *newprop
>  		return;
>  
>  	if (oldprop)
> -		__of_sysfs_remove_bin_file(np, oldprop);
> +		__of_sysfs_remove_bin_file(np, oldprop, false);
>  	__of_add_property_sysfs(np, newprop);
>  }
>  
> @@ -157,7 +160,7 @@ void __of_detach_node_sysfs(struct device_node *np)
>  	/* only remove properties if on sysfs */
>  	if (of_node_is_attached(np)) {
>  		for_each_property_of_node(np, pp)
> -			__of_sysfs_remove_bin_file(np, pp);
> +			__of_sysfs_remove_bin_file(np, pp, false);
>  		kobject_del(&np->kobj);
>  	}
>  
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 75e67b8bb826..fff157c63907 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -134,7 +134,7 @@ extern int __of_update_property(struct device_node *np,
>  extern void __of_detach_node(struct device_node *np);
>  
>  extern void __of_sysfs_remove_bin_file(struct device_node *np,
> -				       struct property *prop);
> +                                       struct property *prop, bool selfremove);

Ick, no, now you have to go and find the function declaration every time
you see this called.  Adding random boolean flags to functions is not
how to make an api that works well, sorry.

greg k-h

[PATCH 5/9] of: add self parameter to of_update_property()

[Zev Weiss]

This is to indicate that the property is being updated via its own
sysfs method so that we ultimately call into kernfs_remove_self() and
avoid the deadlock that would occur otherwise.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/of/base.c       | 7 ++++---
 drivers/of/dynamic.c    | 2 +-
 drivers/of/kobj.c       | 4 ++--
 drivers/of/of_private.h | 4 ++--
 include/linux/of.h      | 7 ++++++-
 5 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index f720c0d246f2..ce4d3bc2f8a6 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1902,8 +1902,9 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 	return 0;
 }
 
+
 /*
- * of_update_property - Update a property in a node, if the property does
+ * of_update_property_self - Update a property in a node, if the property does
  * not exist, add it.
  *
  * Note that we don't actually remove it, since we have given out
@@ -1911,7 +1912,7 @@ int __of_update_property(struct device_node *np, struct property *newprop,
  * Instead we just move the property to the "dead properties" list,
  * and add the new property to the property list
  */
-int of_update_property(struct device_node *np, struct property *newprop)
+int of_update_property_self(struct device_node *np, struct property *newprop, bool self)
 {
 	struct property *oldprop;
 	unsigned long flags;
@@ -1927,7 +1928,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	if (!rc)
-		__of_update_property_sysfs(np, newprop, oldprop);
+		__of_update_property_sysfs(np, newprop, oldprop, self);
 
 	mutex_unlock(&of_mutex);
 
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index cd3821a6444f..8a67f3e1b223 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -652,7 +652,7 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		__of_remove_property_sysfs(ce->np, ce->prop);
 		break;
 	case OF_RECONFIG_UPDATE_PROPERTY:
-		__of_update_property_sysfs(ce->np, ce->prop, ce->old_prop);
+		__of_update_property_sysfs(ce->np, ce->prop, ce->old_prop, false);
 		break;
 	}
 
diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
index 06d6c90f7aa1..378cb421aae1 100644
--- a/drivers/of/kobj.c
+++ b/drivers/of/kobj.c
@@ -104,14 +104,14 @@ void __of_remove_property_sysfs(struct device_node *np, struct property *prop)
 }
 
 void __of_update_property_sysfs(struct device_node *np, struct property *newprop,
-		struct property *oldprop)
+		struct property *oldprop, bool self)
 {
 	/* At early boot, bail out and defer setup to of_init() */
 	if (!of_kset)
 		return;
 
 	if (oldprop)
-		__of_sysfs_remove_bin_file(np, oldprop, false);
+		__of_sysfs_remove_bin_file(np, oldprop, self);
 	__of_add_property_sysfs(np, newprop);
 }
 
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index fff157c63907..3c6816237278 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -64,7 +64,7 @@ int of_node_is_attached(struct device_node *node);
 int __of_add_property_sysfs(struct device_node *np, struct property *pp);
 void __of_remove_property_sysfs(struct device_node *np, struct property *prop);
 void __of_update_property_sysfs(struct device_node *np, struct property *newprop,
-		struct property *oldprop);
+		struct property *oldprop, bool self);
 int __of_attach_node_sysfs(struct device_node *np);
 void __of_detach_node_sysfs(struct device_node *np);
 #else
@@ -74,7 +74,7 @@ static inline int __of_add_property_sysfs(struct device_node *np, struct propert
 }
 static inline void __of_remove_property_sysfs(struct device_node *np, struct property *prop) {}
 static inline void __of_update_property_sysfs(struct device_node *np,
-		struct property *newprop, struct property *oldprop) {}
+		struct property *newprop, struct property *oldprop, bool self) {}
 static inline int __of_attach_node_sysfs(struct device_node *np)
 {
 	return 0;
diff --git a/include/linux/of.h b/include/linux/of.h
index 6f1c41f109bb..0e6479a884eb 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -401,7 +401,12 @@ extern int of_machine_is_compatible(const char *compat);
 
 extern int of_add_property(struct device_node *np, struct property *prop);
 extern int of_remove_property(struct device_node *np, struct property *prop);
-extern int of_update_property(struct device_node *np, struct property *newprop);
+extern int of_update_property_self(struct device_node *np, struct property *newprop, bool self);
+
+static inline int of_update_property(struct device_node *np, struct property *newprop)
+{
+	return of_update_property_self(np, newprop, false);
+}
 
 /* For updating the device tree at runtime */
 #define OF_RECONFIG_ATTACH_NODE		0x0001
-- 
2.33.0

Re: [PATCH 5/9] of: add self parameter to of_update_property()

[Greg Kroah-Hartman]

On Wed, Oct 06, 2021 at 05:09:50PM -0700, Zev Weiss wrote:
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -64,7 +64,7 @@ int of_node_is_attached(struct device_node *node);
>  int __of_add_property_sysfs(struct device_node *np, struct property *pp);
>  void __of_remove_property_sysfs(struct device_node *np, struct property *prop);
>  void __of_update_property_sysfs(struct device_node *np, struct property *newprop,
> -		struct property *oldprop);
> +		struct property *oldprop, bool self);

Again, not a good api decision at all

thanks,

greg k-h

[PATCH 6/9] of: add support for 'dynamic' DT property

[Zev Weiss]

Nodes marked with this (boolean) property will have a writable status
sysfs file, which can be used to toggle them between "okay" and
"reserved", effectively hot-plugging them.  Note that this will only
be effective for devices on busses that register for OF reconfig
notifications (currently spi, i2c, and platform), and only if
CONFIG_OF_DYNAMIC is enabled.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/of/kobj.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
index 378cb421aae1..141ae23f3130 100644
--- a/drivers/of/kobj.c
+++ b/drivers/of/kobj.c
@@ -36,6 +36,69 @@ static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
 	return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
 }
 
+static ssize_t of_node_status_write(struct file *filp, struct kobject *kobj,
+                                    struct bin_attribute *bin_attr, char *buf,
+                                    loff_t offset, size_t count)
+{
+	int rc;
+	char *newstatus;
+	struct property **deadprev;
+	struct property *newprop = NULL;
+	struct property *oldprop = container_of(bin_attr, struct property, attr);
+	struct device_node *np = container_of(kobj, struct device_node, kobj);
+
+	if (WARN_ON_ONCE(strcmp(oldprop->name, "status")))
+		return -EIO;
+
+	if (offset)
+		return -EINVAL;
+
+	if (sysfs_buf_streq(buf, count, "okay") || sysfs_buf_streq(buf, count, "ok"))
+		newstatus = "okay";
+	else if (sysfs_buf_streq(buf, count, "reserved"))
+		newstatus = "reserved";
+	else if (sysfs_buf_streq(buf, count, "disabled"))
+		newstatus = "disabled";
+	else
+		return -EINVAL;
+
+	if (!strcmp(newstatus, oldprop->value))
+		return count;
+
+	/*
+	 * of_update_property_self() doesn't free replaced properties, so
+	 * rifle through deadprops first to see if there's an equivalent old
+	 * status property we can reuse instead of allocating a new one.
+	 */
+	mutex_lock(&of_mutex);
+	for (deadprev = &np->deadprops; *deadprev; deadprev = &(*deadprev)->next) {
+		struct property *deadprop = *deadprev;
+		if (!strcmp(deadprop->name, "status") &&
+		    deadprop->length == strlen(newstatus) + 1 &&
+		    !strcmp(deadprop->value, newstatus)) {
+			*deadprev = deadprop->next;
+			deadprop->next = NULL;
+			newprop = deadprop;
+			break;
+		}
+	}
+	mutex_unlock(&of_mutex);
+
+	if (!newprop) {
+		newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
+		if (!newprop)
+			return -ENOMEM;
+
+		newprop->name = oldprop->name;
+		newprop->value = newstatus;
+		newprop->length = strlen(newstatus) + 1;
+	}
+
+	rc = of_update_property_self(np, newprop, true);
+
+	return rc ? rc : count;
+}
+
 /* always return newly allocated name, caller must free after use */
 static const char *safe_name(struct kobject *kobj, const char *orig_name)
 {
@@ -79,6 +142,12 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
 	pp->attr.size = secure ? 0 : pp->length;
 	pp->attr.read = of_node_property_read;
 
+	if (!strcmp(pp->name, "status") && of_property_read_bool(np, "dynamic")) {
+		pp->attr.attr.mode |= 0200;
+		pp->attr.write = of_node_status_write;
+		pp->attr.growable = true;
+	}
+
 	rc = sysfs_create_bin_file(&np->kobj, &pp->attr);
 	WARN(rc, "error adding attribute %s to node %pOF\n", pp->name, np);
 	return rc;
-- 
2.33.0

Re: [PATCH 6/9] of: add support for 'dynamic' DT property

[Frank Rowand]

On 10/6/21 7:09 PM, Zev Weiss wrote:
> Nodes marked with this (boolean) property will have a writable status
> sysfs file, which can be used to toggle them between "okay" and
> "reserved", effectively hot-plugging them.  Note that this will only
> be effective for devices on busses that register for OF reconfig
> notifications (currently spi, i2c, and platform), and only if
> CONFIG_OF_DYNAMIC is enabled.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  drivers/of/kobj.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
> index 378cb421aae1..141ae23f3130 100644
> --- a/drivers/of/kobj.c
> +++ b/drivers/of/kobj.c
> @@ -36,6 +36,69 @@ static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
>  	return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
>  }
>  
> +static ssize_t of_node_status_write(struct file *filp, struct kobject *kobj,
> +                                    struct bin_attribute *bin_attr, char *buf,
> +                                    loff_t offset, size_t count)
> +{
> +	int rc;
> +	char *newstatus;
> +	struct property **deadprev;
> +	struct property *newprop = NULL;
> +	struct property *oldprop = container_of(bin_attr, struct property, attr);
> +	struct device_node *np = container_of(kobj, struct device_node, kobj);
> +
> +	if (WARN_ON_ONCE(strcmp(oldprop->name, "status")))
> +		return -EIO;
> +
> +	if (offset)
> +		return -EINVAL;
> +
> +	if (sysfs_buf_streq(buf, count, "okay") || sysfs_buf_streq(buf, count, "ok"))
> +		newstatus = "okay";
> +	else if (sysfs_buf_streq(buf, count, "reserved"))
> +		newstatus = "reserved";
> +	else if (sysfs_buf_streq(buf, count, "disabled"))
> +		newstatus = "disabled";
> +	else
> +		return -EINVAL;
> +
> +	if (!strcmp(newstatus, oldprop->value))
> +		return count;
> +

If the general approach of this patch set is the correct way to provide the desired
functionality (I'm still pondering that), then a version of the following code
probably belongs in drivers/of/dynamic.c so that it is easier to maintain and keep
consistent with other dynamic devicetree updates.  If you look at the code there
that touches deadprops (eg __of_changeset_entry_apply()) you will notice that the
locking issues are more extensive than what is implemented here.

I'm still thinking about how this interacts with other forms of dynamic devicetree
changes (eg drivers/of/dynamic.c and also overlays).

> +	/*
> +	 * of_update_property_self() doesn't free replaced properties, so
> +	 * rifle through deadprops first to see if there's an equivalent old
> +	 * status property we can reuse instead of allocating a new one.
> +	 */
> +	mutex_lock(&of_mutex);
> +	for (deadprev = &np->deadprops; *deadprev; deadprev = &(*deadprev)->next) {
> +		struct property *deadprop = *deadprev;
> +		if (!strcmp(deadprop->name, "status") &&
> +		    deadprop->length == strlen(newstatus) + 1 &&
> +		    !strcmp(deadprop->value, newstatus)) {
> +			*deadprev = deadprop->next;
> +			deadprop->next = NULL;
> +			newprop = deadprop;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&of_mutex);
> +
> +	if (!newprop) {
> +		newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> +		if (!newprop)
> +			return -ENOMEM;
> +
> +		newprop->name = oldprop->name;
> +		newprop->value = newstatus;
> +		newprop->length = strlen(newstatus) + 1;
> +	}
> +
> +	rc = of_update_property_self(np, newprop, true);

-Frank

> +
> +	return rc ? rc : count;
> +}
> +
>  /* always return newly allocated name, caller must free after use */
>  static const char *safe_name(struct kobject *kobj, const char *orig_name)
>  {
> @@ -79,6 +142,12 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
>  	pp->attr.size = secure ? 0 : pp->length;
>  	pp->attr.read = of_node_property_read;
>  
> +	if (!strcmp(pp->name, "status") && of_property_read_bool(np, "dynamic")) {
> +		pp->attr.attr.mode |= 0200;
> +		pp->attr.write = of_node_status_write;
> +		pp->attr.growable = true;
> +	}
> +
>  	rc = sysfs_create_bin_file(&np->kobj, &pp->attr);
>  	WARN(rc, "error adding attribute %s to node %pOF\n", pp->name, np);
>  	return rc;
> 

Re: [PATCH 6/9] of: add support for 'dynamic' DT property

[Frank Rowand]

On 10/8/21 1:51 PM, Frank Rowand wrote:
> On 10/6/21 7:09 PM, Zev Weiss wrote:
>> Nodes marked with this (boolean) property will have a writable status
>> sysfs file, which can be used to toggle them between "okay" and
>> "reserved", effectively hot-plugging them.  Note that this will only
>> be effective for devices on busses that register for OF reconfig
>> notifications (currently spi, i2c, and platform), and only if
>> CONFIG_OF_DYNAMIC is enabled.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>  drivers/of/kobj.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 69 insertions(+)
>>
>> diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
>> index 378cb421aae1..141ae23f3130 100644
>> --- a/drivers/of/kobj.c
>> +++ b/drivers/of/kobj.c
>> @@ -36,6 +36,69 @@ static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
>>  	return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
>>  }
>>  
>> +static ssize_t of_node_status_write(struct file *filp, struct kobject *kobj,
>> +                                    struct bin_attribute *bin_attr, char *buf,
>> +                                    loff_t offset, size_t count)
>> +{
>> +	int rc;
>> +	char *newstatus;
>> +	struct property **deadprev;
>> +	struct property *newprop = NULL;
>> +	struct property *oldprop = container_of(bin_attr, struct property, attr);
>> +	struct device_node *np = container_of(kobj, struct device_node, kobj);
>> +
>> +	if (WARN_ON_ONCE(strcmp(oldprop->name, "status")))
>> +		return -EIO;
>> +
>> +	if (offset)
>> +		return -EINVAL;
>> +
>> +	if (sysfs_buf_streq(buf, count, "okay") || sysfs_buf_streq(buf, count, "ok"))
>> +		newstatus = "okay";
>> +	else if (sysfs_buf_streq(buf, count, "reserved"))
>> +		newstatus = "reserved";
>> +	else if (sysfs_buf_streq(buf, count, "disabled"))
>> +		newstatus = "disabled";
>> +	else
>> +		return -EINVAL;
>> +
>> +	if (!strcmp(newstatus, oldprop->value))
>> +		return count;
>> +
> 
> If the general approach of this patch set is the correct way to provide the desired
> functionality (I'm still pondering that), then a version of the following code
> probably belongs in drivers/of/dynamic.c so that it is easier to maintain and keep
> consistent with other dynamic devicetree updates.  If you look at the code there
> that touches deadprops (eg __of_changeset_entry_apply()) you will notice that the
> locking issues are more extensive than what is implemented here.
> 
> I'm still thinking about how this interacts with other forms of dynamic devicetree
> changes (eg drivers/of/dynamic.c and also overlays).
> 
>> +	/*
>> +	 * of_update_property_self() doesn't free replaced properties, so
>> +	 * rifle through deadprops first to see if there's an equivalent old
>> +	 * status property we can reuse instead of allocating a new one.
>> +	 */
>> +	mutex_lock(&of_mutex);
>> +	for (deadprev = &np->deadprops; *deadprev; deadprev = &(*deadprev)->next) {
>> +		struct property *deadprop = *deadprev;
>> +		if (!strcmp(deadprop->name, "status") &&
>> +		    deadprop->length == strlen(newstatus) + 1 &&
>> +		    !strcmp(deadprop->value, newstatus)) {
>> +			*deadprev = deadprop->next;
>> +			deadprop->next = NULL;
>> +			newprop = deadprop;
>> +			break;
>> +		}
>> +	}
>> +	mutex_unlock(&of_mutex);
>> +
>> +	if (!newprop) {
>> +		newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
>> +		if (!newprop)
>> +			return -ENOMEM;
>> +
>> +		newprop->name = oldprop->name;
>> +		newprop->value = newstatus;
>> +		newprop->length = strlen(newstatus) + 1;
>> +	}
>> +
>> +	rc = of_update_property_self(np, newprop, true);
> 
> -Frank
> 
>> +
>> +	return rc ? rc : count;
>> +}
>> +
>>  /* always return newly allocated name, caller must free after use */
>>  static const char *safe_name(struct kobject *kobj, const char *orig_name)
>>  {
>> @@ -79,6 +142,12 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
>>  	pp->attr.size = secure ? 0 : pp->length;
>>  	pp->attr.read = of_node_property_read;
>>  
>> +	if (!strcmp(pp->name, "status") && of_property_read_bool(np, "dynamic")) {
>> +		pp->attr.attr.mode |= 0200;
>> +		pp->attr.write = of_node_status_write;
>> +		pp->attr.growable = true;
>> +	}

This isn't (yet) a request for a change to the patch, but more exposing a
potential issue of interaction with overlays.

The current in tree instances of updating a property value are fairly
limited.  Adding a userspace interface to update a property value
(although limited to only a node's status value) has me thinking about
the interaction with dynamic devicetree updates (of/overlay/dynamic.c)
and also overlays.

If a node exists in a base devicetree, containing only the property
"status", then an overlay subsequently populates the node with a
"dynamic" property then the sysfs status file will not be updated
to be writable.  If we do not allow an overlay to add a property
to an existing node, then not an issue.  But we have not created
such overlay rules yet.

Again, not a request for a change to this patch yet, just leaving
a breadcrumb for myself.

-Frank

>> +
>>  	rc = sysfs_create_bin_file(&np->kobj, &pp->attr);
>>  	WARN(rc, "error adding attribute %s to node %pOF\n", pp->name, np);
>>  	return rc;
>>
> 

Re: [PATCH 6/9] of: add support for 'dynamic' DT property

[Frank Rowand]

Hi Matt, Greg,

On 10/8/21 1:51 PM, Frank Rowand wrote:
> On 10/6/21 7:09 PM, Zev Weiss wrote:
>> Nodes marked with this (boolean) property will have a writable status
>> sysfs file, which can be used to toggle them between "okay" and
>> "reserved", effectively hot-plugging them.  Note that this will only
>> be effective for devices on busses that register for OF reconfig
>> notifications (currently spi, i2c, and platform), and only if
>> CONFIG_OF_DYNAMIC is enabled.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>  drivers/of/kobj.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 69 insertions(+)
>>
>> diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
>> index 378cb421aae1..141ae23f3130 100644
>> --- a/drivers/of/kobj.c
>> +++ b/drivers/of/kobj.c
>> @@ -36,6 +36,69 @@ static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
>>      return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
>>  }
>>  
>> +static ssize_t of_node_status_write(struct file *filp, struct kobject *kobj,
>> +                                    struct bin_attribute *bin_attr, char *buf,
>> +                                    loff_t offset, size_t count)
>> +{
>> +    int rc;
>> +    char *newstatus;
>> +    struct property **deadprev;
>> +    struct property *newprop = NULL;
>> +    struct property *oldprop = container_of(bin_attr, struct property, attr);
>> +    struct device_node *np = container_of(kobj, struct device_node, kobj);
>> +
>> +    if (WARN_ON_ONCE(strcmp(oldprop->name, "status")))
>> +            return -EIO;
>> +
>> +    if (offset)
>> +            return -EINVAL;
>> +
>> +    if (sysfs_buf_streq(buf, count, "okay") || sysfs_buf_streq(buf, count, "ok"))
>> +            newstatus = "okay";
>> +    else if (sysfs_buf_streq(buf, count, "reserved"))
>> +            newstatus = "reserved";
>> +    else if (sysfs_buf_streq(buf, count, "disabled"))
>> +            newstatus = "disabled";
>> +    else
>> +            return -EINVAL;
>> +
>> +    if (!strcmp(newstatus, oldprop->value))
>> +            return count;
>> +
> 
> If the general approach of this patch set is the correct way to provide the desired
> functionality (I'm still pondering that), then a version of the following code

After pondering, this approach does not appear workable to me.

If we allow one property to be writable via sysfs we open the door for any property to
be writable from sysfs.  This will likely lead to a desire to modify more than one
related property as a single transaction (so that the changes occur as a single
transaction, under a single lock event, with a single notification after all
of the properties are modified).  This is not meant to be an exhaustive list of
the issues that have already been thought through in the context of overlays
(though not all of the issues have been addressed with overlays, at least many
of them, such as one transaction to apply an entire overlay, have been.)

I don't want to make this a long missive, but will at least note the next
issue that popped up in my pondering, which is complications from modifying
the same items in a devicetree via different methods, such as both writing
to sysfs and applying/removing overlays.  If the problems in the previous
paragraph are not sufficient to prevent the sysfs approach then I can
elaborate further on these additional issues.

So another approach is needed.  I have no yet thought this through, but I
have an alternative.  First, change the new property name from "dynamic"
to something more descriptive like "ownership_shifts_between_os_and_others"
(yes, my suggestions is way too verbose and needs to be word smithed, but
the point is to clearly state the underlying action that occurs), then
define the result of this variable to be driver specific, where the
driver is required upon probe to instantiate the device in a manner
that does not impact the other user(s) of the underlying hardware
and to use a driver specific method to transfer control of the
hardware between the os and the other user(s).  I propose the method
would be via a device specific file (or set of files) in sysfs (Greg's
input invited on the use of sysfs in this manner - if I recall correctly
this is the current preferred mechanism).

-Frank


> probably belongs in drivers/of/dynamic.c so that it is easier to maintain and keep
> consistent with other dynamic devicetree updates.  If you look at the code there
> that touches deadprops (eg __of_changeset_entry_apply()) you will notice that the
> locking issues are more extensive than what is implemented here.
> 
> I'm still thinking about how this interacts with other forms of dynamic devicetree
> changes (eg drivers/of/dynamic.c and also overlays).
> 
>> +    /*
>> +     * of_update_property_self() doesn't free replaced properties, so
>> +     * rifle through deadprops first to see if there's an equivalent old
>> +     * status property we can reuse instead of allocating a new one.
>> +     */
>> +    mutex_lock(&of_mutex);
>> +    for (deadprev = &np->deadprops; *deadprev; deadprev = &(*deadprev)->next) {
>> +            struct property *deadprop = *deadprev;
>> +            if (!strcmp(deadprop->name, "status") &&
>> +                deadprop->length == strlen(newstatus) + 1 &&
>> +                !strcmp(deadprop->value, newstatus)) {
>> +                    *deadprev = deadprop->next;
>> +                    deadprop->next = NULL;
>> +                    newprop = deadprop;
>> +                    break;
>> +            }
>> +    }
>> +    mutex_unlock(&of_mutex);
>> +
>> +    if (!newprop) {
>> +            newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
>> +            if (!newprop)
>> +                    return -ENOMEM;
>> +
>> +            newprop->name = oldprop->name;
>> +            newprop->value = newstatus;
>> +            newprop->length = strlen(newstatus) + 1;
>> +    }
>> +
>> +    rc = of_update_property_self(np, newprop, true);
> 
> -Frank
> 
>> +
>> +    return rc ? rc : count;
>> +}
>> +
>>  /* always return newly allocated name, caller must free after use */
>>  static const char *safe_name(struct kobject *kobj, const char *orig_name)
>>  {
>> @@ -79,6 +142,12 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
>>      pp->attr.size = secure ? 0 : pp->length;
>>      pp->attr.read = of_node_property_read;
>>  
>> +    if (!strcmp(pp->name, "status") && of_property_read_bool(np, "dynamic")) {
>> +            pp->attr.attr.mode |= 0200;
>> +            pp->attr.write = of_node_status_write;
>> +            pp->attr.growable = true;
>> +    }
>> +
>>      rc = sysfs_create_bin_file(&np->kobj, &pp->attr);
>>      WARN(rc, "error adding attribute %s to node %pOF\n", pp->name, np);
>>      return rc;
>>

Re: [PATCH 6/9] of: add support for 'dynamic' DT property

[Frank Rowand]

Hi Rob,

On 10/11/21 8:58 AM, Frank Rowand wrote:
> Hi Matt, Greg,

That was meant to be Rob, not Matt.

-Frank

> 
> On 10/8/21 1:51 PM, Frank Rowand wrote:
>> On 10/6/21 7:09 PM, Zev Weiss wrote:
>>> Nodes marked with this (boolean) property will have a writable status
>>> sysfs file, which can be used to toggle them between "okay" and
>>> "reserved", effectively hot-plugging them.  Note that this will only
>>> be effective for devices on busses that register for OF reconfig
>>> notifications (currently spi, i2c, and platform), and only if
>>> CONFIG_OF_DYNAMIC is enabled.
>>>
>>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>>> ---
>>>  drivers/of/kobj.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 69 insertions(+)
>>>
>>> diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
>>> index 378cb421aae1..141ae23f3130 100644
>>> --- a/drivers/of/kobj.c
>>> +++ b/drivers/of/kobj.c
>>> @@ -36,6 +36,69 @@ static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
>>>      return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
>>>  }
>>>  
>>> +static ssize_t of_node_status_write(struct file *filp, struct kobject *kobj,
>>> +                                    struct bin_attribute *bin_attr, char *buf,
>>> +                                    loff_t offset, size_t count)
>>> +{
>>> +    int rc;
>>> +    char *newstatus;
>>> +    struct property **deadprev;
>>> +    struct property *newprop = NULL;
>>> +    struct property *oldprop = container_of(bin_attr, struct property, attr);
>>> +    struct device_node *np = container_of(kobj, struct device_node, kobj);
>>> +
>>> +    if (WARN_ON_ONCE(strcmp(oldprop->name, "status")))
>>> +            return -EIO;
>>> +
>>> +    if (offset)
>>> +            return -EINVAL;
>>> +
>>> +    if (sysfs_buf_streq(buf, count, "okay") || sysfs_buf_streq(buf, count, "ok"))
>>> +            newstatus = "okay";
>>> +    else if (sysfs_buf_streq(buf, count, "reserved"))
>>> +            newstatus = "reserved";
>>> +    else if (sysfs_buf_streq(buf, count, "disabled"))
>>> +            newstatus = "disabled";
>>> +    else
>>> +            return -EINVAL;
>>> +
>>> +    if (!strcmp(newstatus, oldprop->value))
>>> +            return count;
>>> +
>>
>> If the general approach of this patch set is the correct way to provide the desired
>> functionality (I'm still pondering that), then a version of the following code
> 
> After pondering, this approach does not appear workable to me.
> 
> If we allow one property to be writable via sysfs we open the door for any property to
> be writable from sysfs.  This will likely lead to a desire to modify more than one
> related property as a single transaction (so that the changes occur as a single
> transaction, under a single lock event, with a single notification after all
> of the properties are modified).  This is not meant to be an exhaustive list of
> the issues that have already been thought through in the context of overlays
> (though not all of the issues have been addressed with overlays, at least many
> of them, such as one transaction to apply an entire overlay, have been.)
> 
> I don't want to make this a long missive, but will at least note the next
> issue that popped up in my pondering, which is complications from modifying
> the same items in a devicetree via different methods, such as both writing
> to sysfs and applying/removing overlays.  If the problems in the previous
> paragraph are not sufficient to prevent the sysfs approach then I can
> elaborate further on these additional issues.
> 
> So another approach is needed.  I have no yet thought this through, but I
> have an alternative.  First, change the new property name from "dynamic"
> to something more descriptive like "ownership_shifts_between_os_and_others"
> (yes, my suggestions is way too verbose and needs to be word smithed, but
> the point is to clearly state the underlying action that occurs), then
> define the result of this variable to be driver specific, where the
> driver is required upon probe to instantiate the device in a manner
> that does not impact the other user(s) of the underlying hardware
> and to use a driver specific method to transfer control of the
> hardware between the os and the other user(s).  I propose the method
> would be via a device specific file (or set of files) in sysfs (Greg's
> input invited on the use of sysfs in this manner - if I recall correctly
> this is the current preferred mechanism).
> 
> -Frank
> 
> 
>> probably belongs in drivers/of/dynamic.c so that it is easier to maintain and keep
>> consistent with other dynamic devicetree updates.  If you look at the code there
>> that touches deadprops (eg __of_changeset_entry_apply()) you will notice that the
>> locking issues are more extensive than what is implemented here.
>>
>> I'm still thinking about how this interacts with other forms of dynamic devicetree
>> changes (eg drivers/of/dynamic.c and also overlays).
>>
>>> +    /*
>>> +     * of_update_property_self() doesn't free replaced properties, so
>>> +     * rifle through deadprops first to see if there's an equivalent old
>>> +     * status property we can reuse instead of allocating a new one.
>>> +     */
>>> +    mutex_lock(&of_mutex);
>>> +    for (deadprev = &np->deadprops; *deadprev; deadprev = &(*deadprev)->next) {
>>> +            struct property *deadprop = *deadprev;
>>> +            if (!strcmp(deadprop->name, "status") &&
>>> +                deadprop->length == strlen(newstatus) + 1 &&
>>> +                !strcmp(deadprop->value, newstatus)) {
>>> +                    *deadprev = deadprop->next;
>>> +                    deadprop->next = NULL;
>>> +                    newprop = deadprop;
>>> +                    break;
>>> +            }
>>> +    }
>>> +    mutex_unlock(&of_mutex);
>>> +
>>> +    if (!newprop) {
>>> +            newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
>>> +            if (!newprop)
>>> +                    return -ENOMEM;
>>> +
>>> +            newprop->name = oldprop->name;
>>> +            newprop->value = newstatus;
>>> +            newprop->length = strlen(newstatus) + 1;
>>> +    }
>>> +
>>> +    rc = of_update_property_self(np, newprop, true);
>>
>> -Frank
>>
>>> +
>>> +    return rc ? rc : count;
>>> +}
>>> +
>>>  /* always return newly allocated name, caller must free after use */
>>>  static const char *safe_name(struct kobject *kobj, const char *orig_name)
>>>  {
>>> @@ -79,6 +142,12 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
>>>      pp->attr.size = secure ? 0 : pp->length;
>>>      pp->attr.read = of_node_property_read;
>>>  
>>> +    if (!strcmp(pp->name, "status") && of_property_read_bool(np, "dynamic")) {
>>> +            pp->attr.attr.mode |= 0200;
>>> +            pp->attr.write = of_node_status_write;
>>> +            pp->attr.growable = true;
>>> +    }
>>> +
>>>      rc = sysfs_create_bin_file(&np->kobj, &pp->attr);
>>>      WARN(rc, "error adding attribute %s to node %pOF\n", pp->name, np);
>>>      return rc;
>>>

Re: [PATCH 6/9] of: add support for 'dynamic' DT property

[Zev Weiss]

Hi Frank,

Thanks for the thorough consideration on this.  (More inline below.)

On Mon, Oct 11, 2021 at 06:58:32AM PDT, Frank Rowand wrote:
>Hi Matt, Greg,
>
>On 10/8/21 1:51 PM, Frank Rowand wrote:
>> On 10/6/21 7:09 PM, Zev Weiss wrote:
>>> Nodes marked with this (boolean) property will have a writable status
>>> sysfs file, which can be used to toggle them between "okay" and
>>> "reserved", effectively hot-plugging them.  Note that this will only
>>> be effective for devices on busses that register for OF reconfig
>>> notifications (currently spi, i2c, and platform), and only if
>>> CONFIG_OF_DYNAMIC is enabled.
>>>
>>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>>> ---
>>>  drivers/of/kobj.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 69 insertions(+)
>>>
>>> diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
>>> index 378cb421aae1..141ae23f3130 100644
>>> --- a/drivers/of/kobj.c
>>> +++ b/drivers/of/kobj.c
>>> @@ -36,6 +36,69 @@ static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
>>>      return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
>>>  }
>>>
>>> +static ssize_t of_node_status_write(struct file *filp, struct kobject *kobj,
>>> +                                    struct bin_attribute *bin_attr, char *buf,
>>> +                                    loff_t offset, size_t count)
>>> +{
>>> +    int rc;
>>> +    char *newstatus;
>>> +    struct property **deadprev;
>>> +    struct property *newprop = NULL;
>>> +    struct property *oldprop = container_of(bin_attr, struct property, attr);
>>> +    struct device_node *np = container_of(kobj, struct device_node, kobj);
>>> +
>>> +    if (WARN_ON_ONCE(strcmp(oldprop->name, "status")))
>>> +            return -EIO;
>>> +
>>> +    if (offset)
>>> +            return -EINVAL;
>>> +
>>> +    if (sysfs_buf_streq(buf, count, "okay") || sysfs_buf_streq(buf, count, "ok"))
>>> +            newstatus = "okay";
>>> +    else if (sysfs_buf_streq(buf, count, "reserved"))
>>> +            newstatus = "reserved";
>>> +    else if (sysfs_buf_streq(buf, count, "disabled"))
>>> +            newstatus = "disabled";
>>> +    else
>>> +            return -EINVAL;
>>> +
>>> +    if (!strcmp(newstatus, oldprop->value))
>>> +            return count;
>>> +
>>
>> If the general approach of this patch set is the correct way to provide the desired
>> functionality (I'm still pondering that), then a version of the following code
>
>After pondering, this approach does not appear workable to me.
>

Okay -- I had come to a similar conclusion, if for slightly different 
reasons (basically, just that the sets of "things that would avoid 
binary sysfs attr abuse" and "things that would maintain userspace 
compatibility" seemed pretty disjoint).

> <snip>
>
>So another approach is needed.  I have no yet thought this through, but I
>have an alternative.  First, change the new property name from "dynamic"
>to something more descriptive like "ownership_shifts_between_os_and_others"
>(yes, my suggestions is way too verbose and needs to be word smithed, but
>the point is to clearly state the underlying action that occurs), then
>define the result of this variable to be driver specific, where the
>driver is required upon probe to instantiate the device in a manner
>that does not impact the other user(s) of the underlying hardware
>and to use a driver specific method to transfer control of the
>hardware between the os and the other user(s).  I propose the method
>would be via a device specific file (or set of files) in sysfs (Greg's
>input invited on the use of sysfs in this manner - if I recall correctly
>this is the current preferred mechanism).
>

I'm not sure if you've had a chance to take a look at it already, but 
this sounds fairly similar to the approach I took in the semi-prequel 
series that preceded this one: 
https://lore.kernel.org/openbmc/20210929115409.21254-1-zev@bewilderbeest.net/

The general idea there was to start making use of the "reserved" status 
value (defined in the DT spec but thus far not really implemented 
anywhere that I'm aware of) instead of introducing a new property.

The implementation in that series was very driver-specific (probably 
overly so), but I think could be generalized somewhat in a couple 
directions without an enormous amount of additional work.  First 
(top-down), we could have the driver core avoid automatically binding 
drivers for reserved devices.  Second (bottom-up), the machinery 
implemented in the aspeed-smc driver in that series could be lifted into 
the MTD spi-nor framework as suggested by Dhananjay.

Rob, Greg -- do you think another version of that patch series with 
those changes might be a viable strategy?


Thanks,
Zev

[PATCH 7/9] of: make OF_DYNAMIC selectable independently of OF_UNITTEST

[Zev Weiss]

The writable status sysfs file enabled by the 'dynamic' DT property
requires CONFIG_OF_DYNAMIC to be useful, but that shouldn't require
dragging in CONFIG_OF_UNITTEST as well.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/of/Kconfig | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 3dfeae8912df..8e0ba87db030 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -55,12 +55,14 @@ config OF_KOBJ
 # Hardly any platforms need this.  It is safe to select, but only do so if you
 # need it.
 config OF_DYNAMIC
-	bool "Support for dynamic device trees" if OF_UNITTEST
+	bool "Support for dynamic device trees"
 	select OF_KOBJ
 	help
 	  On some platforms, the device tree can be manipulated at runtime.
-	  While this option is selected automatically on such platforms, you
-	  can enable it manually to improve device tree unit test coverage.
+	  With this option enabled, device tree nodes that are marked with
+	  the "dynamic" property can have their status toggled between
+	  "okay" and "reserved" via sysfs.  This can also be enabled to
+	  increase test coverage with CONFIG_OF_UNITTEST if desired.
 
 config OF_ADDRESS
 	def_bool y
-- 
2.33.0

Re: [PATCH 7/9] of: make OF_DYNAMIC selectable independently of OF_UNITTEST

[Frank Rowand]

On 10/6/21 7:09 PM, Zev Weiss wrote:
> The writable status sysfs file enabled by the 'dynamic' DT property
> requires CONFIG_OF_DYNAMIC to be useful, but that shouldn't require
> dragging in CONFIG_OF_UNITTEST as well.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  drivers/of/Kconfig | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 3dfeae8912df..8e0ba87db030 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -55,12 +55,14 @@ config OF_KOBJ
>  # Hardly any platforms need this.  It is safe to select, but only do so if you
>  # need it.
>  config OF_DYNAMIC
> -	bool "Support for dynamic device trees" if OF_UNITTEST
> +	bool "Support for dynamic device trees"
>  	select OF_KOBJ
>  	help
>  	  On some platforms, the device tree can be manipulated at runtime.
> -	  While this option is selected automatically on such platforms, you
> -	  can enable it manually to improve device tree unit test coverage.
> +	  With this option enabled, device tree nodes that are marked with
> +	  the "dynamic" property can have their status toggled between
> +	  "okay" and "reserved" via sysfs.  This can also be enabled to
> +	  increase test coverage with CONFIG_OF_UNITTEST if desired.
>  
>  config OF_ADDRESS
>  	def_bool y
> 

The help message should be extended to explain the impact of enabling
OF_DYNAMIC manually (as opposed to auto selected) - it will be to
allow writes to a node's "status" property in sysfs if the node contains
a true value for the "dynamic" property.

[PATCH 8/9] dt-bindings: document new 'dynamic' common property

[Zev Weiss]

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 .../devicetree/bindings/common-properties.txt  | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/common-properties.txt b/Documentation/devicetree/bindings/common-properties.txt
index 98a28130e100..db6c54e1071d 100644
--- a/Documentation/devicetree/bindings/common-properties.txt
+++ b/Documentation/devicetree/bindings/common-properties.txt
@@ -83,3 +83,21 @@ gpio@0 {
 	      #gpio-cells = <2>;
 	      #daisy-chained-devices = <3>;
 };
+
+Dynamic devices
+---------------
+
+Certain devices may require support for runtime attachment and
+detachment (hot-plugging, essentially).
+
+Optional properties:
+ - dynamic: Boolean; indicates that the device's status may change at
+   runtime.
+
+Example:
+&spi1 {
+	      compatible = "name";
+	      status = "reserved";
+	      ...
+	      dynamic;
+};
-- 
2.33.0

Re: [PATCH 8/9] dt-bindings: document new 'dynamic' common property

[Greg Kroah-Hartman]

On Wed, Oct 06, 2021 at 05:09:53PM -0700, Zev Weiss wrote:
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>

I know I can not take patches without any changelog text.  Maybe other
maintainers are more "lax" :(

thanks,

greg k-h

Re: [PATCH 8/9] dt-bindings: document new 'dynamic' common property

[Zev Weiss]

On Wed, Oct 06, 2021 at 10:26:42PM PDT, Greg Kroah-Hartman wrote:
>On Wed, Oct 06, 2021 at 05:09:53PM -0700, Zev Weiss wrote:
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>
>I know I can not take patches without any changelog text.  Maybe other
>maintainers are more "lax" :(
>

Okay -- for this one I wasn't sure what to put in the body that wasn't 
basically just duplicating the subject line or the content of the patch, 
but I'll make sure to put something there in the future.


Zev

[PATCH 9/9] ARM: dts: aspeed: Add e3c246d4i BIOS flash device

[Zev Weiss]

This uses a dynamic DT node because the BIOS SPI flash requires
significant coordination with the host system (power state tracking,
GPIOs, IPMI messages) before the BMC can touch it, and needs to be
relinquished back to the host when the BMC is done accessing it.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 .../boot/dts/aspeed-bmc-asrock-e3c246d4i.dts  | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
index 9b4cf5ebe6d5..428198703824 100644
--- a/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
@@ -68,6 +68,29 @@ flash@0 {
 	};
 };
 
+&spi1 {
+	/*
+	 * The BIOS SPI flash is shared with the host via an external mux, and
+	 * is not accessible by the BMC by default (hence reserved/dynamic
+	 * here rather than okay).  This would ideally be done on the flash@0
+	 * node instead of the spi1 controller, but the driver infrastructure
+	 * to support dynamic devices at that level of the device tree isn't
+	 * currently in place, and it's the only flash chip on this
+	 * controller, so we can get away with the coarser granularity here
+	 * until support for making individual flash chips dynamic is
+	 * available.
+	 */
+	status = "reserved";
+	dynamic;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_spi1_default>;
+	flash@0 {
+		status = "okay";
+		label = "bios";
+		m25p,fast-read;
+	};
+};
+
 &uart5 {
 	status = "okay";
 };
-- 
2.33.0

Re: [PATCH 0/9] Dynamic DT device nodes

[Florian Fainelli]

On 10/6/2021 5:09 PM, Zev Weiss wrote:
> Hello,
> 
> This patch series is in some ways kind of a v2 for the "Dynamic
> aspeed-smc flash chips via 'reserved' DT status" series I posted
> previously [0], but takes a fairly different approach suggested by Rob
> Herring [1] and doesn't actually touch the aspeed-smc driver or
> anything in the MTD subsystem, so I haven't marked it as such.
> 
> To recap a bit of the context from that series, in OpenBMC there's a
> need for certain devices (described by device-tree nodes) to be able
> to be attached and detached at runtime (for example the SPI flash for
> the host's firmware, which is shared between the BMC and the host but
> can only be accessed by one or the other at a time).  To provide that
> ability, this series adds support for a new common device-tree
> property, a boolean "dynamic" that indicates that the device may come
> and go at runtime.  When present on a node, the sysfs file for that
> node's "status" property is made writable, allowing userspace to do
> things like:
> 
>    $ echo okay > /sys/firmware/devicetree/.../status
>    $ echo reserved > /sys/firmware/devicetree/.../status
> 
> to activate and deactivate a dynamic device.

This is a completely drive by comment here, but cannot you already 
achieve what you want today by making the SPI-NOR to be loaded as a 
module, probe it when you need it from the BMC, and unbind or rmmod the 
drive when you need it on the server/host attached to the BMC?

Looking at [0] there appears to be enough signaling visible by the BMC's 
user-space that it ought to be possible?

Assuming that there may be multiple flash chips and you somehow need to 
access one in order to complete the BMC device boot, but not the other 
one(s), you could imagine unbinding the spi-nor driver from the ones you 
don't want to drive and wait until you have appropriate signaling made 
available to your or is there a risk of racing with the host in doing so?
-- 
Florian

Re: [PATCH 0/9] Dynamic DT device nodes

[Zev Weiss]

On Wed, Oct 06, 2021 at 07:46:08PM PDT, Florian Fainelli wrote:
>
>
>On 10/6/2021 5:09 PM, Zev Weiss wrote:
>>Hello,
>>
>>This patch series is in some ways kind of a v2 for the "Dynamic
>>aspeed-smc flash chips via 'reserved' DT status" series I posted
>>previously [0], but takes a fairly different approach suggested by Rob
>>Herring [1] and doesn't actually touch the aspeed-smc driver or
>>anything in the MTD subsystem, so I haven't marked it as such.
>>
>>To recap a bit of the context from that series, in OpenBMC there's a
>>need for certain devices (described by device-tree nodes) to be able
>>to be attached and detached at runtime (for example the SPI flash for
>>the host's firmware, which is shared between the BMC and the host but
>>can only be accessed by one or the other at a time).  To provide that
>>ability, this series adds support for a new common device-tree
>>property, a boolean "dynamic" that indicates that the device may come
>>and go at runtime.  When present on a node, the sysfs file for that
>>node's "status" property is made writable, allowing userspace to do
>>things like:
>>
>>   $ echo okay > /sys/firmware/devicetree/.../status
>>   $ echo reserved > /sys/firmware/devicetree/.../status
>>
>>to activate and deactivate a dynamic device.
>
>This is a completely drive by comment here, but cannot you already 
>achieve what you want today by making the SPI-NOR to be loaded as a 
>module, probe it when you need it from the BMC, and unbind or rmmod 
>the drive when you need it on the server/host attached to the BMC?
>
>Looking at [0] there appears to be enough signaling visible by the 
>BMC's user-space that it ought to be possible?
>
>Assuming that there may be multiple flash chips and you somehow need 
>to access one in order to complete the BMC device boot, but not the 
>other one(s), you could imagine unbinding the spi-nor driver from the 
>ones you don't want to drive and wait until you have appropriate 
>signaling made available to your or is there a risk of racing with the 
>host in doing so?

Hi Florian,

I sort of considered similar things at various points (in fact the 
bind/unbind technique is what I've been using as a stopgap so far), but 
I don't think it's ultimately a great solution.

In this particular case it happens that the driver for the host's BIOS 
flash (aspeed-smc) is the same driver used for the BMC's firmware flash 
and hence necessary for it to boot.  (OpenBMC also typically uses a 
non-modular kernel, for what it's worth.)

On BMC startup we don't know the state of the host, and while the mux 
that ultimately determines which processor has access to the BIOS flash 
defaults to connecting it to the host (so if we do attempt to attach it 
we'll simply fail clumsily and spew some errors to dmesg rather than 
causing catastrophic errors host-side), blindly starting to poke the 
BIOS flash without doing the proper coordination with the host first 
seems to me to be pretty squarely in the category of "things we 
shouldn't be doing" -- the desire to avoid that sort of clunkiness is a 
significant portion of what led me to pursue a better solution in the 
first place.

Additionally, while I don't know the details of the specific hardware 
involved, others in the OpenBMC community (or at least Andrew Jeffery) 
have indicated that pluggable DT devices would be useful for the systems 
they're working on as well, I suspect for things other than host 
firmware flashes.


Zev

Re: [PATCH 0/9] Dynamic DT device nodes

[Andy Shevchenko]

On Thu, Oct 7, 2021 at 3:10 AM Zev Weiss <zev@bewilderbeest.net> wrote:
> This patch series is in some ways kind of a v2 for the "Dynamic
> aspeed-smc flash chips via 'reserved' DT status" series I posted
> previously [0], but takes a fairly different approach suggested by Rob
> Herring [1] and doesn't actually touch the aspeed-smc driver or
> anything in the MTD subsystem, so I haven't marked it as such.
>
> To recap a bit of the context from that series, in OpenBMC there's a
> need for certain devices (described by device-tree nodes) to be able
> to be attached and detached at runtime (for example the SPI flash for
> the host's firmware, which is shared between the BMC and the host but
> can only be accessed by one or the other at a time).

This seems quite dangerous. Why do you need that? Why can't device
tree overlays be used?

> To provide that
> ability, this series adds support for a new common device-tree
> property, a boolean "dynamic" that indicates that the device may come
> and go at runtime.  When present on a node, the sysfs file for that
> node's "status" property is made writable, allowing userspace to do
> things like:
>
>   $ echo okay > /sys/firmware/devicetree/.../status
>   $ echo reserved > /sys/firmware/devicetree/.../status
>
> to activate and deactivate a dynamic device.
>
> Because it leans on the OF_DYNAMIC machinery internally, this
> functionality will only work on busses that register for OF reconfig
> notifications and handle them appropriately (presently platform, i2c,
> and spi).  This series does not attempt to solve the "dynamic devices
> further down the tree" problem [2]; my hope is that handling for OF
> reconfig notifications can be extended to other families of devices
> (e.g. individual MTD spi-nor flash chips) in the future.

What about ACPI and software nodes?
How will all this affect the host?

> [0] https://lore.kernel.org/openbmc/20210929115409.21254-1-zev@bewilderbeest.net/
> [1] https://lore.kernel.org/openbmc/CAL_JsqJH+b5oFuSP+KBLBsN5QTA6xASuqXJWXUaDkHhugXPpnQ@mail.gmail.com/
> [2] https://lore.kernel.org/openbmc/20210929220038.GS17315@packtop/
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?id=6663ae07d995f5fbe2988a19858b2f87e68cf929


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/9] Dynamic DT device nodes

[Zev Weiss]

On Thu, Oct 07, 2021 at 12:04:41AM PDT, Andy Shevchenko wrote:
>On Thu, Oct 7, 2021 at 3:10 AM Zev Weiss <zev@bewilderbeest.net> wrote:
>> This patch series is in some ways kind of a v2 for the "Dynamic
>> aspeed-smc flash chips via 'reserved' DT status" series I posted
>> previously [0], but takes a fairly different approach suggested by Rob
>> Herring [1] and doesn't actually touch the aspeed-smc driver or
>> anything in the MTD subsystem, so I haven't marked it as such.
>>
>> To recap a bit of the context from that series, in OpenBMC there's a
>> need for certain devices (described by device-tree nodes) to be able
>> to be attached and detached at runtime (for example the SPI flash for
>> the host's firmware, which is shared between the BMC and the host but
>> can only be accessed by one or the other at a time).
>
>This seems quite dangerous. Why do you need that? 

Sometimes the host needs access to the flash (it's the host's firmware, 
after all), sometimes the BMC needs access to it (e.g. to perform an 
out-of-band update to the host's firmware).  To achieve the latter, the 
flash needs to be attached to the BMC, but that requires some careful 
coordination with the host to arbitrate which one actually has access to 
it (that coordination is handled by userspace, which then tells the 
kernel explicitly when the flash should be attached and detached).

What seems dangerous?

>Why can't device tree overlays be used?

I'm hoping to stay closer to mainline.  The OpenBMC kernel has a 
documented policy strongly encouraging upstream-first development: 
https://github.com/openbmc/docs/blob/master/kernel-development.md

I doubt Joel (the OpenBMC kernel maintainer) would be eager to start 
carrying the DT overlay patches; I'd likewise strongly prefer to avoid 
carrying them myself as additional downstream patches.  Hence the 
attempt at getting a solution to the problem upstream.

>
>> To provide that
>> ability, this series adds support for a new common device-tree
>> property, a boolean "dynamic" that indicates that the device may come
>> and go at runtime.  When present on a node, the sysfs file for that
>> node's "status" property is made writable, allowing userspace to do
>> things like:
>>
>>   $ echo okay > /sys/firmware/devicetree/.../status
>>   $ echo reserved > /sys/firmware/devicetree/.../status
>>
>> to activate and deactivate a dynamic device.
>>
>> Because it leans on the OF_DYNAMIC machinery internally, this
>> functionality will only work on busses that register for OF reconfig
>> notifications and handle them appropriately (presently platform, i2c,
>> and spi).  This series does not attempt to solve the "dynamic devices
>> further down the tree" problem [2]; my hope is that handling for OF
>> reconfig notifications can be extended to other families of devices
>> (e.g. individual MTD spi-nor flash chips) in the future.
>
>What about ACPI and software nodes?

I'm afraid I don't understand the question, can you elaborate on what 
you mean?

>How will all this affect the host?

Assuming the coordination mentioned above is done properly, the host 
will be in a quiesced state whenever the BMC is accessing the flash and 
hence won't notice much of anything at all (the BMC will detach the 
flash and relinquish control of it back to the host before the host is 
reactivated).


Zev

Re: [PATCH 0/9] Dynamic DT device nodes

[Greg Kroah-Hartman]

On Thu, Oct 07, 2021 at 02:05:41AM -0700, Zev Weiss wrote:
> On Thu, Oct 07, 2021 at 12:04:41AM PDT, Andy Shevchenko wrote:
> > On Thu, Oct 7, 2021 at 3:10 AM Zev Weiss <zev@bewilderbeest.net> wrote:
> > > This patch series is in some ways kind of a v2 for the "Dynamic
> > > aspeed-smc flash chips via 'reserved' DT status" series I posted
> > > previously [0], but takes a fairly different approach suggested by Rob
> > > Herring [1] and doesn't actually touch the aspeed-smc driver or
> > > anything in the MTD subsystem, so I haven't marked it as such.
> > > 
> > > To recap a bit of the context from that series, in OpenBMC there's a
> > > need for certain devices (described by device-tree nodes) to be able
> > > to be attached and detached at runtime (for example the SPI flash for
> > > the host's firmware, which is shared between the BMC and the host but
> > > can only be accessed by one or the other at a time).
> > 
> > This seems quite dangerous. Why do you need that?
> 
> Sometimes the host needs access to the flash (it's the host's firmware,
> after all), sometimes the BMC needs access to it (e.g. to perform an
> out-of-band update to the host's firmware).  To achieve the latter, the
> flash needs to be attached to the BMC, but that requires some careful
> coordination with the host to arbitrate which one actually has access to it
> (that coordination is handled by userspace, which then tells the kernel
> explicitly when the flash should be attached and detached).
> 
> What seems dangerous?
> 
> > Why can't device tree overlays be used?
> 
> I'm hoping to stay closer to mainline.  The OpenBMC kernel has a documented
> policy strongly encouraging upstream-first development:
> https://github.com/openbmc/docs/blob/master/kernel-development.md
> 
> I doubt Joel (the OpenBMC kernel maintainer) would be eager to start
> carrying the DT overlay patches; I'd likewise strongly prefer to avoid
> carrying them myself as additional downstream patches.  Hence the attempt at
> getting a solution to the problem upstream.

Then why not work to get device tree overlays to be merged properly?
Don't work on a half-of-a-solution when the real solution is already
here.

thanks,

greg k-h

Re: [PATCH 0/9] Dynamic DT device nodes

[Zev Weiss]

On Thu, Oct 07, 2021 at 03:31:39AM PDT, Greg Kroah-Hartman wrote:
>On Thu, Oct 07, 2021 at 02:05:41AM -0700, Zev Weiss wrote:
>> On Thu, Oct 07, 2021 at 12:04:41AM PDT, Andy Shevchenko wrote:
>> > On Thu, Oct 7, 2021 at 3:10 AM Zev Weiss <zev@bewilderbeest.net> wrote:
>> > > This patch series is in some ways kind of a v2 for the "Dynamic
>> > > aspeed-smc flash chips via 'reserved' DT status" series I posted
>> > > previously [0], but takes a fairly different approach suggested by Rob
>> > > Herring [1] and doesn't actually touch the aspeed-smc driver or
>> > > anything in the MTD subsystem, so I haven't marked it as such.
>> > >
>> > > To recap a bit of the context from that series, in OpenBMC there's a
>> > > need for certain devices (described by device-tree nodes) to be able
>> > > to be attached and detached at runtime (for example the SPI flash for
>> > > the host's firmware, which is shared between the BMC and the host but
>> > > can only be accessed by one or the other at a time).
>> >
>> > This seems quite dangerous. Why do you need that?
>>
>> Sometimes the host needs access to the flash (it's the host's firmware,
>> after all), sometimes the BMC needs access to it (e.g. to perform an
>> out-of-band update to the host's firmware).  To achieve the latter, the
>> flash needs to be attached to the BMC, but that requires some careful
>> coordination with the host to arbitrate which one actually has access to it
>> (that coordination is handled by userspace, which then tells the kernel
>> explicitly when the flash should be attached and detached).
>>
>> What seems dangerous?
>>
>> > Why can't device tree overlays be used?
>>
>> I'm hoping to stay closer to mainline.  The OpenBMC kernel has a documented
>> policy strongly encouraging upstream-first development:
>> https://github.com/openbmc/docs/blob/master/kernel-development.md
>>
>> I doubt Joel (the OpenBMC kernel maintainer) would be eager to start
>> carrying the DT overlay patches; I'd likewise strongly prefer to avoid
>> carrying them myself as additional downstream patches.  Hence the attempt at
>> getting a solution to the problem upstream.
>
>Then why not work to get device tree overlays to be merged properly?
>Don't work on a half-of-a-solution when the real solution is already
>here.
>

I had been under the impression that the overlay patches had very dim 
prospects of ever being accepted and that this might be a more tractable 
alternative, but apparently was mistaken -- I'll look into what the 
outstanding issues were with that and perhaps take a stab at addressing 
them.


Zev

Re: [PATCH 0/9] Dynamic DT device nodes

[Rob Herring]

On Thu, Oct 7, 2021 at 10:41 AM Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Thu, Oct 07, 2021 at 03:31:39AM PDT, Greg Kroah-Hartman wrote:
> >On Thu, Oct 07, 2021 at 02:05:41AM -0700, Zev Weiss wrote:
> >> On Thu, Oct 07, 2021 at 12:04:41AM PDT, Andy Shevchenko wrote:
> >> > On Thu, Oct 7, 2021 at 3:10 AM Zev Weiss <zev@bewilderbeest.net> wrote:
> >> > > This patch series is in some ways kind of a v2 for the "Dynamic
> >> > > aspeed-smc flash chips via 'reserved' DT status" series I posted
> >> > > previously [0], but takes a fairly different approach suggested by Rob
> >> > > Herring [1] and doesn't actually touch the aspeed-smc driver or
> >> > > anything in the MTD subsystem, so I haven't marked it as such.
> >> > >
> >> > > To recap a bit of the context from that series, in OpenBMC there's a
> >> > > need for certain devices (described by device-tree nodes) to be able
> >> > > to be attached and detached at runtime (for example the SPI flash for
> >> > > the host's firmware, which is shared between the BMC and the host but
> >> > > can only be accessed by one or the other at a time).
> >> >
> >> > This seems quite dangerous. Why do you need that?
> >>
> >> Sometimes the host needs access to the flash (it's the host's firmware,
> >> after all), sometimes the BMC needs access to it (e.g. to perform an
> >> out-of-band update to the host's firmware).  To achieve the latter, the
> >> flash needs to be attached to the BMC, but that requires some careful
> >> coordination with the host to arbitrate which one actually has access to it
> >> (that coordination is handled by userspace, which then tells the kernel
> >> explicitly when the flash should be attached and detached).
> >>
> >> What seems dangerous?
> >>
> >> > Why can't device tree overlays be used?
> >>
> >> I'm hoping to stay closer to mainline.  The OpenBMC kernel has a documented
> >> policy strongly encouraging upstream-first development:
> >> https://github.com/openbmc/docs/blob/master/kernel-development.md
> >>
> >> I doubt Joel (the OpenBMC kernel maintainer) would be eager to start
> >> carrying the DT overlay patches; I'd likewise strongly prefer to avoid
> >> carrying them myself as additional downstream patches.  Hence the attempt at
> >> getting a solution to the problem upstream.
> >
> >Then why not work to get device tree overlays to be merged properly?

TBC, it's 'just' the general purpose userspace interface to apply
overlays that's missing.

I did suggest what's done here as overlays are kind of an overkill for
this usecase. Much easier to write to a sysfs file than write an
overlay, compile it with dtc, and provide it to the kernel all just to
enable a device.

Perhaps this could also be supported in the driver model directly.
Given the "what about ACPI question", that is probably what should be
done unless the answer is we don't care. I think we'd just need a flag
to create devices, but not bind automatically. Or maybe abusing
driver_override can accomplish that.

> >Don't work on a half-of-a-solution when the real solution is already
> >here.
> >
>
> I had been under the impression that the overlay patches had very dim
> prospects of ever being accepted and that this might be a more tractable
> alternative, but apparently was mistaken -- I'll look into what the
> outstanding issues were with that and perhaps take a stab at addressing
> them.

What's dim is the patches allowing any modification to any part of the
DT. Any changes to a DT need to work (i.e. have some effect). For
example, randomly changing/adding/removing properties wouldn't have
any effect because they've probably already be read and used.

What I've suggested before is some sort of registration of nodes
allowed to apply child nodes and properties to. That would serve the
add-on board usecases which have been the main driver of this to date.
That also got hung up on defining interface nodes to add-on boards.
Your scope is more limited and can be limited to that scope while
using the same configfs interface.

Rob

Re: [PATCH 0/9] Dynamic DT device nodes

[Greg Kroah-Hartman]

On Thu, Oct 07, 2021 at 03:03:43PM -0500, Rob Herring wrote:
> On Thu, Oct 7, 2021 at 10:41 AM Zev Weiss <zev@bewilderbeest.net> wrote:
> >
> > On Thu, Oct 07, 2021 at 03:31:39AM PDT, Greg Kroah-Hartman wrote:
> > >On Thu, Oct 07, 2021 at 02:05:41AM -0700, Zev Weiss wrote:
> > >> On Thu, Oct 07, 2021 at 12:04:41AM PDT, Andy Shevchenko wrote:
> > >> > On Thu, Oct 7, 2021 at 3:10 AM Zev Weiss <zev@bewilderbeest.net> wrote:
> > >> > > This patch series is in some ways kind of a v2 for the "Dynamic
> > >> > > aspeed-smc flash chips via 'reserved' DT status" series I posted
> > >> > > previously [0], but takes a fairly different approach suggested by Rob
> > >> > > Herring [1] and doesn't actually touch the aspeed-smc driver or
> > >> > > anything in the MTD subsystem, so I haven't marked it as such.
> > >> > >
> > >> > > To recap a bit of the context from that series, in OpenBMC there's a
> > >> > > need for certain devices (described by device-tree nodes) to be able
> > >> > > to be attached and detached at runtime (for example the SPI flash for
> > >> > > the host's firmware, which is shared between the BMC and the host but
> > >> > > can only be accessed by one or the other at a time).
> > >> >
> > >> > This seems quite dangerous. Why do you need that?
> > >>
> > >> Sometimes the host needs access to the flash (it's the host's firmware,
> > >> after all), sometimes the BMC needs access to it (e.g. to perform an
> > >> out-of-band update to the host's firmware).  To achieve the latter, the
> > >> flash needs to be attached to the BMC, but that requires some careful
> > >> coordination with the host to arbitrate which one actually has access to it
> > >> (that coordination is handled by userspace, which then tells the kernel
> > >> explicitly when the flash should be attached and detached).
> > >>
> > >> What seems dangerous?
> > >>
> > >> > Why can't device tree overlays be used?
> > >>
> > >> I'm hoping to stay closer to mainline.  The OpenBMC kernel has a documented
> > >> policy strongly encouraging upstream-first development:
> > >> https://github.com/openbmc/docs/blob/master/kernel-development.md
> > >>
> > >> I doubt Joel (the OpenBMC kernel maintainer) would be eager to start
> > >> carrying the DT overlay patches; I'd likewise strongly prefer to avoid
> > >> carrying them myself as additional downstream patches.  Hence the attempt at
> > >> getting a solution to the problem upstream.
> > >
> > >Then why not work to get device tree overlays to be merged properly?
> 
> TBC, it's 'just' the general purpose userspace interface to apply
> overlays that's missing.
> 
> I did suggest what's done here as overlays are kind of an overkill for
> this usecase. Much easier to write to a sysfs file than write an
> overlay, compile it with dtc, and provide it to the kernel all just to
> enable a device.
> 
> Perhaps this could also be supported in the driver model directly.
> Given the "what about ACPI question", that is probably what should be
> done unless the answer is we don't care. I think we'd just need a flag
> to create devices, but not bind automatically. Or maybe abusing
> driver_override can accomplish that.

The driver model already allows devices to be bound/unbound from
drivers, but no, it does not allow new devices to be "created" from
userspace as that is a very bus-specific thing to have happen.

If this is "just" a platform device, perhaps add that logic to the
platform bus code?

thanks,

greg k-h

Re: [PATCH 0/9] Dynamic DT device nodes

[Frank Rowand]

On 10/7/21 3:03 PM, Rob Herring wrote:
> On Thu, Oct 7, 2021 at 10:41 AM Zev Weiss <zev@bewilderbeest.net> wrote:
>>
>> On Thu, Oct 07, 2021 at 03:31:39AM PDT, Greg Kroah-Hartman wrote:
>>> On Thu, Oct 07, 2021 at 02:05:41AM -0700, Zev Weiss wrote:
>>>> On Thu, Oct 07, 2021 at 12:04:41AM PDT, Andy Shevchenko wrote:
>>>>> On Thu, Oct 7, 2021 at 3:10 AM Zev Weiss <zev@bewilderbeest.net> wrote:
>>>>>> This patch series is in some ways kind of a v2 for the "Dynamic
>>>>>> aspeed-smc flash chips via 'reserved' DT status" series I posted
>>>>>> previously [0], but takes a fairly different approach suggested by Rob
>>>>>> Herring [1] and doesn't actually touch the aspeed-smc driver or
>>>>>> anything in the MTD subsystem, so I haven't marked it as such.
>>>>>>
>>>>>> To recap a bit of the context from that series, in OpenBMC there's a
>>>>>> need for certain devices (described by device-tree nodes) to be able
>>>>>> to be attached and detached at runtime (for example the SPI flash for
>>>>>> the host's firmware, which is shared between the BMC and the host but
>>>>>> can only be accessed by one or the other at a time).
>>>>>
>>>>> This seems quite dangerous. Why do you need that?
>>>>
>>>> Sometimes the host needs access to the flash (it's the host's firmware,
>>>> after all), sometimes the BMC needs access to it (e.g. to perform an
>>>> out-of-band update to the host's firmware).  To achieve the latter, the
>>>> flash needs to be attached to the BMC, but that requires some careful
>>>> coordination with the host to arbitrate which one actually has access to it
>>>> (that coordination is handled by userspace, which then tells the kernel
>>>> explicitly when the flash should be attached and detached).
>>>>
>>>> What seems dangerous?
>>>>
>>>>> Why can't device tree overlays be used?
>>>>
>>>> I'm hoping to stay closer to mainline.  The OpenBMC kernel has a documented
>>>> policy strongly encouraging upstream-first development:
>>>> https://github.com/openbmc/docs/blob/master/kernel-development.md
>>>>
>>>> I doubt Joel (the OpenBMC kernel maintainer) would be eager to start
>>>> carrying the DT overlay patches; I'd likewise strongly prefer to avoid
>>>> carrying them myself as additional downstream patches.  Hence the attempt at
>>>> getting a solution to the problem upstream.
>>>
>>> Then why not work to get device tree overlays to be merged properly?
> 
> TBC, it's 'just' the general purpose userspace interface to apply
> overlays that's missing.

A fuller view of what is missing is at:

https://elinux.org/Frank%27s_Evolving_Overlay_Thoughts#issues_and_what_needs_to_be_completed_--_Not_an_exhaustive_list


> 
> I did suggest what's done here as overlays are kind of an overkill for
> this usecase. Much easier to write to a sysfs file than write an
> overlay, compile it with dtc, and provide it to the kernel all just to
> enable a device.
> 
> Perhaps this could also be supported in the driver model directly.
> Given the "what about ACPI question", that is probably what should be
> done unless the answer is we don't care. I think we'd just need a flag
> to create devices, but not bind automatically. Or maybe abusing
> driver_override can accomplish that.
> 
>>> Don't work on a half-of-a-solution when the real solution is already
>>> here.
>>>
>>
>> I had been under the impression that the overlay patches had very dim
>> prospects of ever being accepted and that this might be a more tractable
>> alternative, but apparently was mistaken -- I'll look into what the
>> outstanding issues were with that and perhaps take a stab at addressing
>> them.
> 

> What's dim is the patches allowing any modification to any part of the
> DT. Any changes to a DT need to work (i.e. have some effect). For
> example, randomly changing/adding/removing properties wouldn't have
> any effect because they've probably already be read and used.

Yes, that is a good description.

> 
> What I've suggested before is some sort of registration of nodes
> allowed to apply child nodes and properties to. That would serve the
> add-on board usecases which have been the main driver of this to date.
> That also got hung up on defining interface nodes to add-on boards.
> Your scope is more limited and can be limited to that scope while
> using the same configfs interface.
> 
> Rob
>