openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/9] Dynamic DT device nodes
@ 2021-10-07  0:09 Zev Weiss
  2021-10-07  0:09 ` [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function Zev Weiss
                   ` (10 more replies)
  0 siblings, 11 replies; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  0:09 UTC (permalink / raw)
  To: openbmc
  Cc: Krzysztof Wilczyński, Zev Weiss, Rafael J. Wysocki,
	Daniel Vetter, Jeremy Kerr, Francis Laniel, linux-aspeed,
	Frank Rowand, Andrey Konovalov, Alexey Dobriyan, devicetree,
	Kees Cook, Rob Herring, Jonathan Cameron, Dan Williams,
	linux-arm-kernel, Daniel Axtens, Andy Shevchenko, Andrew Jeffery,
	Greg Kroah-Hartman, Nick Desaulniers, linux-kernel,
	Andrew Morton, Heiner Kallweit

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function
  2021-10-07  0:09 [PATCH 0/9] Dynamic DT device nodes Zev Weiss
@ 2021-10-07  0:09 ` Zev Weiss
  2021-10-07  5:23   ` Greg Kroah-Hartman
  2021-10-07  0:09 ` [PATCH 2/9] sysfs: add growable flag to struct bin_attribute Zev Weiss
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  0:09 UTC (permalink / raw)
  To: openbmc
  Cc: devicetree, Zev Weiss, Rafael J. Wysocki, Greg Kroah-Hartman,
	linux-kernel, Bjorn Helgaas, Rob Herring, Daniel Vetter,
	Krzysztof Wilczyński, Jeremy Kerr, Heiner Kallweit

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 2/9] sysfs: add growable flag to struct bin_attribute
  2021-10-07  0:09 [PATCH 0/9] Dynamic DT device nodes Zev Weiss
  2021-10-07  0:09 ` [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function Zev Weiss
@ 2021-10-07  0:09 ` Zev Weiss
  2021-10-07  0:09 ` [PATCH 3/9] lib/string: add sysfs_buf_streq() Zev Weiss
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  0:09 UTC (permalink / raw)
  To: openbmc
  Cc: devicetree, Zev Weiss, Rafael J. Wysocki, Greg Kroah-Hartman,
	Krzysztof Wilczyński, linux-kernel, Rob Herring,
	Daniel Vetter, Bjorn Helgaas, Jeremy Kerr, Dan Williams,
	Heiner Kallweit

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 3/9] lib/string: add sysfs_buf_streq()
  2021-10-07  0:09 [PATCH 0/9] Dynamic DT device nodes Zev Weiss
  2021-10-07  0:09 ` [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function Zev Weiss
  2021-10-07  0:09 ` [PATCH 2/9] sysfs: add growable flag to struct bin_attribute Zev Weiss
@ 2021-10-07  0:09 ` Zev Weiss
  2021-10-07  0:09 ` [PATCH 4/9] of: add self parameter to __of_sysfs_remove_bin_file() Zev Weiss
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  0:09 UTC (permalink / raw)
  To: openbmc
  Cc: Andy Shevchenko, devicetree, Kees Cook, Zev Weiss,
	Greg Kroah-Hartman, Nick Desaulniers, linux-kernel, Rob Herring,
	Jonathan Cameron, Jeremy Kerr, Andrew Morton, Andrey Konovalov,
	Francis Laniel, Alexey Dobriyan, Daniel Axtens

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 4/9] of: add self parameter to __of_sysfs_remove_bin_file()
  2021-10-07  0:09 [PATCH 0/9] Dynamic DT device nodes Zev Weiss
                   ` (2 preceding siblings ...)
  2021-10-07  0:09 ` [PATCH 3/9] lib/string: add sysfs_buf_streq() Zev Weiss
@ 2021-10-07  0:09 ` Zev Weiss
  2021-10-07  5:25   ` Greg Kroah-Hartman
  2021-10-07  0:09 ` [PATCH 5/9] of: add self parameter to of_update_property() Zev Weiss
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  0:09 UTC (permalink / raw)
  To: openbmc
  Cc: devicetree, Zev Weiss, Greg Kroah-Hartman, linux-kernel,
	Rob Herring, Jeremy Kerr, Frank Rowand

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 5/9] of: add self parameter to of_update_property()
  2021-10-07  0:09 [PATCH 0/9] Dynamic DT device nodes Zev Weiss
                   ` (3 preceding siblings ...)
  2021-10-07  0:09 ` [PATCH 4/9] of: add self parameter to __of_sysfs_remove_bin_file() Zev Weiss
@ 2021-10-07  0:09 ` Zev Weiss
  2021-10-07  5:26   ` Greg Kroah-Hartman
  2021-10-07  0:09 ` [PATCH 6/9] of: add support for 'dynamic' DT property Zev Weiss
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  0:09 UTC (permalink / raw)
  To: openbmc
  Cc: devicetree, Zev Weiss, Greg Kroah-Hartman, linux-kernel,
	Rob Herring, Jeremy Kerr, Frank Rowand

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 6/9] of: add support for 'dynamic' DT property
  2021-10-07  0:09 [PATCH 0/9] Dynamic DT device nodes Zev Weiss
                   ` (4 preceding siblings ...)
  2021-10-07  0:09 ` [PATCH 5/9] of: add self parameter to of_update_property() Zev Weiss
@ 2021-10-07  0:09 ` Zev Weiss
  2021-10-08 18:51   ` Frank Rowand
  2021-10-07  0:09 ` [PATCH 7/9] of: make OF_DYNAMIC selectable independently of OF_UNITTEST Zev Weiss
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  0:09 UTC (permalink / raw)
  To: openbmc
  Cc: devicetree, Zev Weiss, Greg Kroah-Hartman, linux-kernel,
	Rob Herring, Jeremy Kerr, Frank Rowand

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 7/9] of: make OF_DYNAMIC selectable independently of OF_UNITTEST
  2021-10-07  0:09 [PATCH 0/9] Dynamic DT device nodes Zev Weiss
                   ` (5 preceding siblings ...)
  2021-10-07  0:09 ` [PATCH 6/9] of: add support for 'dynamic' DT property Zev Weiss
@ 2021-10-07  0:09 ` Zev Weiss
  2021-10-08 19:01   ` Frank Rowand
  2021-10-07  0:09 ` [PATCH 8/9] dt-bindings: document new 'dynamic' common property Zev Weiss
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  0:09 UTC (permalink / raw)
  To: openbmc
  Cc: devicetree, Zev Weiss, Greg Kroah-Hartman, linux-kernel,
	Rob Herring, Jeremy Kerr, Frank Rowand

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 8/9] dt-bindings: document new 'dynamic' common property
  2021-10-07  0:09 [PATCH 0/9] Dynamic DT device nodes Zev Weiss
                   ` (6 preceding siblings ...)
  2021-10-07  0:09 ` [PATCH 7/9] of: make OF_DYNAMIC selectable independently of OF_UNITTEST Zev Weiss
@ 2021-10-07  0:09 ` Zev Weiss
  2021-10-07  5:26   ` Greg Kroah-Hartman
  2021-10-07  0:09 ` [PATCH 9/9] ARM: dts: aspeed: Add e3c246d4i BIOS flash device Zev Weiss
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  0:09 UTC (permalink / raw)
  To: openbmc
  Cc: devicetree, Zev Weiss, Greg Kroah-Hartman, linux-kernel,
	Rob Herring, Jeremy Kerr

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [PATCH 9/9] ARM: dts: aspeed: Add e3c246d4i BIOS flash device
  2021-10-07  0:09 [PATCH 0/9] Dynamic DT device nodes Zev Weiss
                   ` (7 preceding siblings ...)
  2021-10-07  0:09 ` [PATCH 8/9] dt-bindings: document new 'dynamic' common property Zev Weiss
@ 2021-10-07  0:09 ` Zev Weiss
  2021-10-07  2:46 ` [PATCH 0/9] Dynamic DT device nodes Florian Fainelli
  2021-10-07  7:04 ` Andy Shevchenko
  10 siblings, 0 replies; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  0:09 UTC (permalink / raw)
  To: openbmc
  Cc: devicetree, linux-aspeed, Zev Weiss, Andrew Jeffery,
	Greg Kroah-Hartman, linux-kernel, Rob Herring, Jeremy Kerr,
	linux-arm-kernel

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/9] Dynamic DT device nodes
  2021-10-07  0:09 [PATCH 0/9] Dynamic DT device nodes Zev Weiss
                   ` (8 preceding siblings ...)
  2021-10-07  0:09 ` [PATCH 9/9] ARM: dts: aspeed: Add e3c246d4i BIOS flash device Zev Weiss
@ 2021-10-07  2:46 ` Florian Fainelli
  2021-10-07  5:44   ` Zev Weiss
  2021-10-07  7:04 ` Andy Shevchenko
  10 siblings, 1 reply; 33+ messages in thread
From: Florian Fainelli @ 2021-10-07  2:46 UTC (permalink / raw)
  To: Zev Weiss, openbmc
  Cc: Krzysztof Wilczyński, linux-aspeed, Rafael J. Wysocki,
	Daniel Vetter, Jeremy Kerr, Francis Laniel, Frank Rowand,
	Andrey Konovalov, Alexey Dobriyan, devicetree, Kees Cook,
	Rob Herring, Jonathan Cameron, Dan Williams, linux-arm-kernel,
	Daniel Axtens, Andy Shevchenko, Andrew Jeffery,
	Greg Kroah-Hartman, Nick Desaulniers, linux-kernel,
	Andrew Morton, Heiner Kallweit



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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function
  2021-10-07  0:09 ` [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function Zev Weiss
@ 2021-10-07  5:23   ` Greg Kroah-Hartman
  2021-10-07  5:58     ` Zev Weiss
  0 siblings, 1 reply; 33+ messages in thread
From: Greg Kroah-Hartman @ 2021-10-07  5:23 UTC (permalink / raw)
  To: Zev Weiss
  Cc: devicetree, Rafael J. Wysocki, Daniel Vetter, openbmc,
	linux-kernel, Bjorn Helgaas, Rob Herring,
	Krzysztof Wilczyński, Jeremy Kerr, Heiner Kallweit

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 4/9] of: add self parameter to __of_sysfs_remove_bin_file()
  2021-10-07  0:09 ` [PATCH 4/9] of: add self parameter to __of_sysfs_remove_bin_file() Zev Weiss
@ 2021-10-07  5:25   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 33+ messages in thread
From: Greg Kroah-Hartman @ 2021-10-07  5:25 UTC (permalink / raw)
  To: Zev Weiss
  Cc: devicetree, openbmc, linux-kernel, Rob Herring, Jeremy Kerr,
	Frank Rowand

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 5/9] of: add self parameter to of_update_property()
  2021-10-07  0:09 ` [PATCH 5/9] of: add self parameter to of_update_property() Zev Weiss
@ 2021-10-07  5:26   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 33+ messages in thread
From: Greg Kroah-Hartman @ 2021-10-07  5:26 UTC (permalink / raw)
  To: Zev Weiss
  Cc: devicetree, openbmc, linux-kernel, Rob Herring, Jeremy Kerr,
	Frank Rowand

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 8/9] dt-bindings: document new 'dynamic' common property
  2021-10-07  0:09 ` [PATCH 8/9] dt-bindings: document new 'dynamic' common property Zev Weiss
@ 2021-10-07  5:26   ` Greg Kroah-Hartman
  2021-10-07  6:03     ` Zev Weiss
  0 siblings, 1 reply; 33+ messages in thread
From: Greg Kroah-Hartman @ 2021-10-07  5:26 UTC (permalink / raw)
  To: Zev Weiss; +Cc: devicetree, openbmc, linux-kernel, Rob Herring, Jeremy Kerr

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/9] Dynamic DT device nodes
  2021-10-07  2:46 ` [PATCH 0/9] Dynamic DT device nodes Florian Fainelli
@ 2021-10-07  5:44   ` Zev Weiss
  0 siblings, 0 replies; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  5:44 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Krzysztof Wilczyński, linux-aspeed, Rafael J. Wysocki,
	Daniel Vetter, Jeremy Kerr, Francis Laniel, Frank Rowand,
	openbmc, Andrey Konovalov, Alexey Dobriyan, devicetree,
	Kees Cook, Rob Herring, Jonathan Cameron, Dan Williams,
	linux-arm-kernel, Daniel Axtens, Andy Shevchenko, Andrew Jeffery,
	Greg Kroah-Hartman, Nick Desaulniers, linux-kernel,
	Andrew Morton, Heiner Kallweit

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function
  2021-10-07  5:23   ` Greg Kroah-Hartman
@ 2021-10-07  5:58     ` Zev Weiss
  2021-10-07  6:12       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  5:58 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: devicetree, Rafael J. Wysocki, Daniel Vetter, openbmc,
	linux-kernel, Bjorn Helgaas, Rob Herring,
	Krzysztof Wilczyński, Jeremy Kerr, Heiner Kallweit

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 8/9] dt-bindings: document new 'dynamic' common property
  2021-10-07  5:26   ` Greg Kroah-Hartman
@ 2021-10-07  6:03     ` Zev Weiss
  0 siblings, 0 replies; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  6:03 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: devicetree, openbmc, linux-kernel, Rob Herring, Jeremy Kerr

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function
  2021-10-07  5:58     ` Zev Weiss
@ 2021-10-07  6:12       ` Greg Kroah-Hartman
  2021-10-07  6:55         ` Zev Weiss
  0 siblings, 1 reply; 33+ messages in thread
From: Greg Kroah-Hartman @ 2021-10-07  6:12 UTC (permalink / raw)
  To: Zev Weiss
  Cc: devicetree, Rafael J. Wysocki, Daniel Vetter, openbmc,
	linux-kernel, Bjorn Helgaas, Rob Herring,
	Krzysztof Wilczyński, Jeremy Kerr, Heiner Kallweit

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function
  2021-10-07  6:12       ` Greg Kroah-Hartman
@ 2021-10-07  6:55         ` Zev Weiss
  0 siblings, 0 replies; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  6:55 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: devicetree, Rafael J. Wysocki, Daniel Vetter, openbmc,
	linux-kernel, Bjorn Helgaas, Rob Herring,
	Krzysztof Wilczyński, Jeremy Kerr, Heiner Kallweit

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/9] Dynamic DT device nodes
  2021-10-07  0:09 [PATCH 0/9] Dynamic DT device nodes Zev Weiss
                   ` (9 preceding siblings ...)
  2021-10-07  2:46 ` [PATCH 0/9] Dynamic DT device nodes Florian Fainelli
@ 2021-10-07  7:04 ` Andy Shevchenko
  2021-10-07  9:05   ` Zev Weiss
  10 siblings, 1 reply; 33+ messages in thread
From: Andy Shevchenko @ 2021-10-07  7:04 UTC (permalink / raw)
  To: Zev Weiss
  Cc: Krzysztof Wilczyński,
	moderated list:ARM/ASPEED MACHINE SUPPORT, Rafael J. Wysocki,
	Daniel Vetter, Jeremy Kerr, Francis Laniel, Frank Rowand,
	OpenBMC Maillist, Andrey Konovalov, Alexey Dobriyan, devicetree,
	Kees Cook, Rob Herring, Jonathan Cameron, Dan Williams,
	linux-arm Mailing List, Daniel Axtens, Andy Shevchenko,
	Andrew Jeffery, Greg Kroah-Hartman, Nick Desaulniers,
	Linux Kernel Mailing List, Andrew Morton, Heiner Kallweit

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/9] Dynamic DT device nodes
  2021-10-07  7:04 ` Andy Shevchenko
@ 2021-10-07  9:05   ` Zev Weiss
  2021-10-07 10:31     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 33+ messages in thread
From: Zev Weiss @ 2021-10-07  9:05 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Krzysztof Wilczyński,
	moderated list:ARM/ASPEED MACHINE SUPPORT, Rafael J. Wysocki,
	Daniel Vetter, Jeremy Kerr, Francis Laniel, Frank Rowand,
	OpenBMC Maillist, Andrey Konovalov, Alexey Dobriyan, devicetree,
	Kees Cook, Rob Herring, Jonathan Cameron, Dan Williams,
	linux-arm Mailing List, Daniel Axtens, Andy Shevchenko,
	Andrew Jeffery, Greg Kroah-Hartman, Nick Desaulniers,
	Linux Kernel Mailing List, Andrew Morton, Heiner Kallweit

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/9] Dynamic DT device nodes
  2021-10-07  9:05   ` Zev Weiss
@ 2021-10-07 10:31     ` Greg Kroah-Hartman
  2021-10-07 15:41       ` Zev Weiss
  0 siblings, 1 reply; 33+ messages in thread
From: Greg Kroah-Hartman @ 2021-10-07 10:31 UTC (permalink / raw)
  To: Zev Weiss
  Cc: Krzysztof Wilczyński,
	moderated list:ARM/ASPEED MACHINE SUPPORT, Rafael J. Wysocki,
	Daniel Vetter, Jeremy Kerr, Francis Laniel, Frank Rowand,
	OpenBMC Maillist, Andy Shevchenko, Andrey Konovalov,
	Alexey Dobriyan, devicetree, Kees Cook, Rob Herring,
	Jonathan Cameron, Dan Williams, linux-arm Mailing List,
	Daniel Axtens, Andy Shevchenko, Andrew Jeffery, Nick Desaulniers,
	Linux Kernel Mailing List, Andrew Morton, Heiner Kallweit

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/9] Dynamic DT device nodes
  2021-10-07 10:31     ` Greg Kroah-Hartman
@ 2021-10-07 15:41       ` Zev Weiss
  2021-10-07 20:03         ` Rob Herring
  0 siblings, 1 reply; 33+ messages in thread
From: Zev Weiss @ 2021-10-07 15:41 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Krzysztof Wilczyński,
	moderated list:ARM/ASPEED MACHINE SUPPORT, Rafael J. Wysocki,
	Daniel Vetter, Jeremy Kerr, Francis Laniel, Frank Rowand,
	OpenBMC Maillist, Andy Shevchenko, Andrey Konovalov,
	Alexey Dobriyan, devicetree, Kees Cook, Rob Herring,
	Jonathan Cameron, Dan Williams, linux-arm Mailing List,
	Daniel Axtens, Andy Shevchenko, Andrew Jeffery, Nick Desaulniers,
	Linux Kernel Mailing List, Andrew Morton, Heiner Kallweit

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/9] Dynamic DT device nodes
  2021-10-07 15:41       ` Zev Weiss
@ 2021-10-07 20:03         ` Rob Herring
  2021-10-08  5:41           ` Greg Kroah-Hartman
  2021-10-08 19:43           ` Frank Rowand
  0 siblings, 2 replies; 33+ messages in thread
From: Rob Herring @ 2021-10-07 20:03 UTC (permalink / raw)
  To: Zev Weiss, Greg Kroah-Hartman
  Cc: Krzysztof Wilczyński,
	moderated list:ARM/ASPEED MACHINE SUPPORT, Rafael J. Wysocki,
	Daniel Vetter, Jeremy Kerr, Francis Laniel, Frank Rowand,
	OpenBMC Maillist, Andy Shevchenko, Andrey Konovalov,
	Alexey Dobriyan, devicetree, Kees Cook, Jonathan Cameron,
	Dan Williams, linux-arm Mailing List, Daniel Axtens,
	Andy Shevchenko, Andrew Jeffery, Nick Desaulniers,
	Linux Kernel Mailing List, Andrew Morton, Heiner Kallweit

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/9] Dynamic DT device nodes
  2021-10-07 20:03         ` Rob Herring
@ 2021-10-08  5:41           ` Greg Kroah-Hartman
  2021-10-08 19:43           ` Frank Rowand
  1 sibling, 0 replies; 33+ messages in thread
From: Greg Kroah-Hartman @ 2021-10-08  5:41 UTC (permalink / raw)
  To: Rob Herring
  Cc: Krzysztof Wilczyński, Zev Weiss, Rafael J. Wysocki,
	Daniel Vetter, Jeremy Kerr, Francis Laniel,
	moderated list:ARM/ASPEED MACHINE SUPPORT, Frank Rowand,
	OpenBMC Maillist, Andy Shevchenko, Andrey Konovalov,
	Alexey Dobriyan, devicetree, Kees Cook, Jonathan Cameron,
	Dan Williams, linux-arm Mailing List, Daniel Axtens,
	Andy Shevchenko, Andrew Jeffery, Nick Desaulniers,
	Linux Kernel Mailing List, Andrew Morton, Heiner Kallweit

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

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 6/9] of: add support for 'dynamic' DT property
  2021-10-07  0:09 ` [PATCH 6/9] of: add support for 'dynamic' DT property Zev Weiss
@ 2021-10-08 18:51   ` Frank Rowand
  2021-10-08 19:19     ` Frank Rowand
  2021-10-11 13:58     ` Frank Rowand
  0 siblings, 2 replies; 33+ messages in thread
From: Frank Rowand @ 2021-10-08 18:51 UTC (permalink / raw)
  To: Zev Weiss, openbmc
  Cc: devicetree, Greg Kroah-Hartman, linux-kernel, Rob Herring, Jeremy Kerr

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;
> 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 7/9] of: make OF_DYNAMIC selectable independently of OF_UNITTEST
  2021-10-07  0:09 ` [PATCH 7/9] of: make OF_DYNAMIC selectable independently of OF_UNITTEST Zev Weiss
@ 2021-10-08 19:01   ` Frank Rowand
  0 siblings, 0 replies; 33+ messages in thread
From: Frank Rowand @ 2021-10-08 19:01 UTC (permalink / raw)
  To: Zev Weiss, openbmc
  Cc: devicetree, Greg Kroah-Hartman, linux-kernel, Rob Herring, Jeremy Kerr

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.

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 6/9] of: add support for 'dynamic' DT property
  2021-10-08 18:51   ` Frank Rowand
@ 2021-10-08 19:19     ` Frank Rowand
  2021-10-11 13:58     ` Frank Rowand
  1 sibling, 0 replies; 33+ messages in thread
From: Frank Rowand @ 2021-10-08 19:19 UTC (permalink / raw)
  To: Zev Weiss, openbmc
  Cc: devicetree, Greg Kroah-Hartman, linux-kernel, Rob Herring, Jeremy Kerr

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;
>>
> 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 0/9] Dynamic DT device nodes
  2021-10-07 20:03         ` Rob Herring
  2021-10-08  5:41           ` Greg Kroah-Hartman
@ 2021-10-08 19:43           ` Frank Rowand
  1 sibling, 0 replies; 33+ messages in thread
From: Frank Rowand @ 2021-10-08 19:43 UTC (permalink / raw)
  To: Rob Herring, Zev Weiss, Greg Kroah-Hartman
  Cc: Krzysztof Wilczyński,
	moderated list:ARM/ASPEED MACHINE SUPPORT, Rafael J. Wysocki,
	Daniel Vetter, Jeremy Kerr, Francis Laniel, OpenBMC Maillist,
	Andy Shevchenko, Andrey Konovalov, Alexey Dobriyan, devicetree,
	Kees Cook, Jonathan Cameron, Dan Williams,
	linux-arm Mailing List, Daniel Axtens, Andy Shevchenko,
	Andrew Jeffery, Nick Desaulniers, Linux Kernel Mailing List,
	Andrew Morton, Heiner Kallweit

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
> 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 6/9] of: add support for 'dynamic' DT property
  2021-10-08 18:51   ` Frank Rowand
  2021-10-08 19:19     ` Frank Rowand
@ 2021-10-11 13:58     ` Frank Rowand
  2021-10-11 14:46       ` Frank Rowand
  2021-10-11 17:35       ` Zev Weiss
  1 sibling, 2 replies; 33+ messages in thread
From: Frank Rowand @ 2021-10-11 13:58 UTC (permalink / raw)
  To: Zev Weiss, openbmc
  Cc: devicetree, Greg Kroah-Hartman, linux-kernel, Rob Herring, Jeremy Kerr

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;
>>

^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 6/9] of: add support for 'dynamic' DT property
  2021-10-11 13:58     ` Frank Rowand
@ 2021-10-11 14:46       ` Frank Rowand
  2021-10-11 17:35       ` Zev Weiss
  1 sibling, 0 replies; 33+ messages in thread
From: Frank Rowand @ 2021-10-11 14:46 UTC (permalink / raw)
  To: Zev Weiss, openbmc
  Cc: devicetree, Greg Kroah-Hartman, linux-kernel, Rob Herring, Jeremy Kerr

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;
>>>


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [PATCH 6/9] of: add support for 'dynamic' DT property
  2021-10-11 13:58     ` Frank Rowand
  2021-10-11 14:46       ` Frank Rowand
@ 2021-10-11 17:35       ` Zev Weiss
  1 sibling, 0 replies; 33+ messages in thread
From: Zev Weiss @ 2021-10-11 17:35 UTC (permalink / raw)
  To: Frank Rowand
  Cc: devicetree, Greg Kroah-Hartman, openbmc, linux-kernel,
	Rob Herring, Jeremy Kerr

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


^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2021-10-11 17:36 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-07  0:09 [PATCH 0/9] Dynamic DT device nodes Zev Weiss
2021-10-07  0:09 ` [PATCH 1/9] sysfs: add sysfs_remove_bin_file_self() function Zev Weiss
2021-10-07  5:23   ` Greg Kroah-Hartman
2021-10-07  5:58     ` Zev Weiss
2021-10-07  6:12       ` Greg Kroah-Hartman
2021-10-07  6:55         ` Zev Weiss
2021-10-07  0:09 ` [PATCH 2/9] sysfs: add growable flag to struct bin_attribute Zev Weiss
2021-10-07  0:09 ` [PATCH 3/9] lib/string: add sysfs_buf_streq() Zev Weiss
2021-10-07  0:09 ` [PATCH 4/9] of: add self parameter to __of_sysfs_remove_bin_file() Zev Weiss
2021-10-07  5:25   ` Greg Kroah-Hartman
2021-10-07  0:09 ` [PATCH 5/9] of: add self parameter to of_update_property() Zev Weiss
2021-10-07  5:26   ` Greg Kroah-Hartman
2021-10-07  0:09 ` [PATCH 6/9] of: add support for 'dynamic' DT property Zev Weiss
2021-10-08 18:51   ` Frank Rowand
2021-10-08 19:19     ` Frank Rowand
2021-10-11 13:58     ` Frank Rowand
2021-10-11 14:46       ` Frank Rowand
2021-10-11 17:35       ` Zev Weiss
2021-10-07  0:09 ` [PATCH 7/9] of: make OF_DYNAMIC selectable independently of OF_UNITTEST Zev Weiss
2021-10-08 19:01   ` Frank Rowand
2021-10-07  0:09 ` [PATCH 8/9] dt-bindings: document new 'dynamic' common property Zev Weiss
2021-10-07  5:26   ` Greg Kroah-Hartman
2021-10-07  6:03     ` Zev Weiss
2021-10-07  0:09 ` [PATCH 9/9] ARM: dts: aspeed: Add e3c246d4i BIOS flash device Zev Weiss
2021-10-07  2:46 ` [PATCH 0/9] Dynamic DT device nodes Florian Fainelli
2021-10-07  5:44   ` Zev Weiss
2021-10-07  7:04 ` Andy Shevchenko
2021-10-07  9:05   ` Zev Weiss
2021-10-07 10:31     ` Greg Kroah-Hartman
2021-10-07 15:41       ` Zev Weiss
2021-10-07 20:03         ` Rob Herring
2021-10-08  5:41           ` Greg Kroah-Hartman
2021-10-08 19:43           ` Frank Rowand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).