From: Frank Rowand <[email protected]>
When an overlay is applied or removed, the live devicetree visible in
/proc/device-tree/, aka /sys/firmware/devicetree/base/, reflects the
changes. There is no method for user space to determine whether the
live devicetree was modified by overlay actions.
Provide a sysfs file, /sys/firmware/devicetree/tree_version, to allow
user space to determine if the live devicetree has remained unchanged
while a series of one or more accesses of /proc/device-tree/ occur.
The use of both (1) dynamic devicetree modifications and (2) overlay
apply and removal are not supported during the same boot cycle. Thus
non-overlay dynamic modifications are not reflected in the value of
tree_version.
Example shell use:
# set done false (1)
done=1
# keep trying until we can make it through the loop without
# live tree being changed by an overlay changeset during the
# 'critical region'
while [ $done = 1 ] ; do
# loop while live tree is locked for overlay changes
pre_version=$(cat /sys/firmware/devicetree/tree_version)
while [ $(( ${pre_version} & 1 )) != 0 ] ; do
# echo is optional, sleep value can be tuned
echo "${pre_version} tree locked, sleeping 2 seconds"
sleep 2;
pre_version=$(cat /sys/firmware/devicetree/tree_version)
done
# 'critical region'
# access /proc/device-tree/ one or more times
# check that overlay did not change DT during critical region
post_version=$(cat /sys/firmware/devicetree/tree_version)
if [ ${post_version} = ${pre_version} ] ; then
# set done true (0)
done=0
fi
done
Signed-off-by: Frank Rowand <[email protected]>
---
Changes since version 2:
- sysfs-firmware-ofw: add comments to shell script example
- sysfs-firmware-ofw: add more information about the behavior of
tree_version and how its value changes
- correct error in patch comment shell script by copying the shell
script from the patch body
Documentation/ABI/testing/sysfs-firmware-ofw | 98 ++++++++++++++++++++++++++--
drivers/of/base.c | 66 +++++++++++++++++++
drivers/of/of_private.h | 2 +
drivers/of/overlay.c | 12 ++++
4 files changed, 171 insertions(+), 7 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-firmware-ofw b/Documentation/ABI/testing/sysfs-firmware-ofw
index edcab3ccfcc0..20860de3ca4c 100644
--- a/Documentation/ABI/testing/sysfs-firmware-ofw
+++ b/Documentation/ABI/testing/sysfs-firmware-ofw
@@ -1,4 +1,10 @@
-What: /sys/firmware/devicetree/*
+What: /sys/firmware/devicetree/
+Date: November 2013
+Contact: Frank Rowand <[email protected]>, [email protected]
+Description:
+ Top level Open Firmware, aka devicetree, sysfs directory.
+
+What: /sys/firmware/devicetree/base
Date: November 2013
Contact: Grant Likely <[email protected]>, [email protected]
Description:
@@ -6,11 +12,6 @@ Description:
hardware, the device tree structure will be exposed in this
directory.
- It is possible for multiple device-tree directories to exist.
- Some device drivers use a separate detached device tree which
- have no attachment to the system tree and will appear in a
- different subdirectory under /sys/firmware/devicetree.
-
Userspace must not use the /sys/firmware/devicetree/base
path directly, but instead should follow /proc/device-tree
symlink. It is possible that the absolute path will change
@@ -20,13 +21,96 @@ Description:
filesystem support, and has largely the same semantics and
should be compatible with existing userspace.
- The contents of /sys/firmware/devicetree/ is a
+ The /sys/firmware/devicetree/base directory is the root
+ node of the devicetree.
+
+ The contents of /sys/firmware/devicetree/base is a
hierarchy of directories, one per device tree node. The
directory name is the resolved path component name (node
name plus address). Properties are represented as files
in the directory. The contents of each file is the exact
binary data from the device tree.
+What: /sys/firmware/devicetree/tree_version
+Date: October 2018
+KernelVersion: 4.20
+Contact: Frank Rowand <[email protected]>, [email protected]
+Description:
+ When an overlay is applied or removed, the live devicetree
+ visible in /proc/device-tree/, aka
+ /sys/firmware/devicetree/base/, reflects the changes.
+
+ tree_version provides a way for user space to determine if the
+ live devicetree has remained unchanged while a series of one
+ or more accesses of /proc/device-tree/ occur. If tree_version
+ is odd then the devicetree is locked for an overlay update.
+
+ Details about the value of tree_version:
+
+ - tree_version is never decremented
+
+ - tree_version is incremented twice for each overlay
+ changeset that is applied or removed
+
+ - When the tree is locked to apply or remove an overlay
+ changeset, tree_version is incremented once and its
+ value becomes odd
+
+ - When the overlay changeset apply or remove is completed
+ (or an error aborts the overlay changeset apply or
+ remove), the tree is unlocked and tree_version is
+ incremented a second time, and its value becomes even
+
+ - the granularity of tree_version is based on overlay
+ changesets; it will never be smaller than a changeset,
+ but may in the future be extended to cover apply and
+ remove of non-overlay changesets
+
+ - the value of tree_version is initially 0 at the start
+ of kernel boot, but may increase during the boot process,
+ for example due to devicetree unittest activity
+
+ - tree_version is the ascii representation of a kernel
+ unsigned 32 bit int variable, thus when incremented
+ from 4294967295 it wraps around to 0
+
+ The use of both dynamic devicetree modifications and overlay
+ apply and removal are not supported during the same boot
+ cycle. Thus non-overlay dynamic modifications are not
+ reflected in the value of tree_version.
+
+ Example shell use of tree_version:
+
+ # set done false (1)
+ done=1
+
+ # keep trying until we can make it through the loop without
+ # live tree being changed by an overlay changeset during the
+ # 'critical region'
+ while [ $done = 1 ] ; do
+
+ # loop while live tree is locked for overlay changes
+ pre_version=$(cat /sys/firmware/devicetree/tree_version)
+ while [ $(( ${pre_version} & 1 )) != 0 ] ; do
+ # echo is optional, sleep value can be tuned
+ echo "${pre_version} tree locked, sleeping 2 seconds"
+ sleep 2;
+ pre_version=$(cat /sys/firmware/devicetree/tree_version)
+ done
+
+ # 'critical region'
+ # access /proc/device-tree/ one or more times
+
+ # check that overlay did not change DT during critical region
+ post_version=$(cat /sys/firmware/devicetree/tree_version)
+ if [ ${post_version} = ${pre_version} ] ; then
+ # set done true (0)
+ done=0
+ fi
+
+ done
+
+
What: /sys/firmware/fdt
Date: February 2015
KernelVersion: 3.19
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 466e3c8582f0..ec0428e47577 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -151,9 +151,71 @@ int of_free_phandle_cache(void)
late_initcall_sync(of_free_phandle_cache);
#endif
+#define show_one(object) \
+ static ssize_t show_##object \
+ (struct kobject *kobj, struct attribute *attr, char *buf) \
+ { \
+ return sprintf(buf, "%u\n", object); \
+ }
+
+struct global_attr {
+ struct attribute attr;
+ ssize_t (*show)(struct kobject *kobj,
+ struct attribute *attr, char *buf);
+ ssize_t (*store)(struct kobject *a, struct attribute *b,
+ const char *c, size_t count);
+};
+
+#define define_one_global_ro(_name) \
+static struct global_attr attr_##_name = \
+__ATTR(_name, 0444, show_##_name, NULL)
+
+/*
+ * tree_version must be unsigned so that at maximum value it wraps
+ * from an odd value (4294967295) to an even value (0).
+ */
+static u32 tree_version;
+
+show_one(tree_version);
+
+define_one_global_ro(tree_version);
+
+static struct attribute *of_attributes[] = {
+ &attr_tree_version.attr,
+ NULL
+};
+
+static const struct attribute_group of_attr_group = {
+ .attrs = of_attributes,
+};
+
+/*
+ * internal documentation
+ * tree_version_increment() - increment base version
+ *
+ * Before starting overlay apply or overlay remove, call this function.
+ * After completing overlay apply or overlay remove, call this function.
+ *
+ * caller must hold of_mutex
+ *
+ * Userspace can use the value of this variable to determine whether the
+ * devicetree has changed while accessing the devicetree. An odd value
+ * means a modification is underway. An even value means no modification
+ * is underway.
+ *
+ * The use of both (1) dynamic devicetree modifications and (2) overlay apply
+ * and removal are not supported during the same boot cycle. Thus non-overlay
+ * dynamic modifications are not reflected in the value of tree_version.
+ */
+void tree_version_increment(void)
+{
+ tree_version++;
+}
+
void __init of_core_init(void)
{
struct device_node *np;
+ int ret;
of_populate_phandle_cache();
@@ -172,6 +234,10 @@ void __init of_core_init(void)
/* Symlink in /proc as required by userspace ABI */
if (of_root)
proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
+
+ ret = sysfs_create_group(&of_kset->kobj, &of_attr_group);
+ if (ret)
+ pr_err("sysfs_create_group of_attr_group failed: %d\n", ret);
}
static struct property *__of_find_property(const struct device_node *np,
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 216175d11d3d..adf89f6bad81 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -31,6 +31,8 @@ struct alias_prop {
extern struct list_head aliases_lookup;
extern struct kset *of_kset;
+void tree_version_increment(void);
+
#if defined(CONFIG_OF_DYNAMIC)
extern int of_property_notify(int action, struct device_node *np,
struct property *prop, struct property *old_prop);
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index eda57ef12fd0..53b1810b1e03 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -770,6 +770,9 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
of_overlay_mutex_lock();
mutex_lock(&of_mutex);
+ /* live tree may change after this point, user space synchronization */
+ tree_version_increment();
+
ret = of_resolve_phandles(tree);
if (ret)
goto err_free_tree;
@@ -832,6 +835,9 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
free_overlay_changeset(ovcs);
out_unlock:
+ /* live tree change complete, user space synchronization */
+ tree_version_increment();
+
mutex_unlock(&of_mutex);
of_overlay_mutex_unlock();
@@ -1028,6 +1034,9 @@ int of_overlay_remove(int *ovcs_id)
mutex_lock(&of_mutex);
+ /* live tree may change after this point, user space synchronization */
+ tree_version_increment();
+
ovcs = idr_find(&ovcs_idr, *ovcs_id);
if (!ovcs) {
ret = -ENODEV;
@@ -1083,6 +1092,9 @@ int of_overlay_remove(int *ovcs_id)
free_overlay_changeset(ovcs);
out_unlock:
+ /* live tree change complete, user space synchronization */
+ tree_version_increment();
+
mutex_unlock(&of_mutex);
out:
--
Frank Rowand <[email protected]>
On Tue, Oct 16, 2018 at 05:34:26PM -0700, [email protected] wrote:
> From: Frank Rowand <[email protected]>
>
> When an overlay is applied or removed, the live devicetree visible in
> /proc/device-tree/, aka /sys/firmware/devicetree/base/, reflects the
> changes. There is no method for user space to determine whether the
> live devicetree was modified by overlay actions.
Because userspace has no way to modify the DT and the ways the kernel
can do modifications is limited.
Do you have an actually need for this outside of testing/development?
> Provide a sysfs file, /sys/firmware/devicetree/tree_version, to allow
> user space to determine if the live devicetree has remained unchanged
> while a series of one or more accesses of /proc/device-tree/ occur.
>
> The use of both (1) dynamic devicetree modifications and (2) overlay
> apply and removal are not supported during the same boot cycle. Thus
> non-overlay dynamic modifications are not reflected in the value of
> tree_version.
I'd prefer to see some sort of information on overlays exported and user
space can check if that changed. IIRC, Pantelis had a series to do that
along with a kill switch to prevent further modifications. At least some
of that series only had minor issues to fix.
Also, shouldn't we get uevents if the tree changes? Maybe that's not
guaranteed, but I'd bet we can't handle cases where we don't get events.
A property added to an existing node comes to mind.
Rob
On 10/18/18 12:32, Rob Herring wrote:
> On Tue, Oct 16, 2018 at 05:34:26PM -0700, [email protected] wrote:
>> From: Frank Rowand <[email protected]>
>>
>> When an overlay is applied or removed, the live devicetree visible in
>> /proc/device-tree/, aka /sys/firmware/devicetree/base/, reflects the
>> changes. There is no method for user space to determine whether the
>> live devicetree was modified by overlay actions.
>
> Because userspace has no way to modify the DT and the ways the kernel
> can do modifications is limited.
>
> Do you have an actually need for this outside of testing/development?
I do not know if anyone uses /proc/device-tree for anything outside of
testing/development. If we believe that there is no other use of
/proc/device-tree we can simply document that there is no expectation
that accessors will see a consistent, unchanging /proc/device-tree.
That would be a much smaller patch.
>> Provide a sysfs file, /sys/firmware/devicetree/tree_version, to allow
>> user space to determine if the live devicetree has remained unchanged
>> while a series of one or more accesses of /proc/device-tree/ occur.
>>
>> The use of both (1) dynamic devicetree modifications and (2) overlay
>> apply and removal are not supported during the same boot cycle. Thus
>> non-overlay dynamic modifications are not reflected in the value of
>> tree_version.
>
> I'd prefer to see some sort of information on overlays exported and user
> space can check if that changed. IIRC, Pantelis had a series to do that
> along with a kill switch to prevent further modifications. At least some
> of that series only had minor issues to fix.
The kill switch addresses a different concern, which was from the security
community. The kill switch is on my todo list.
I don't remember exactly what info the overlay information export patch
provided. I'll have to go find it and re-read it.
> Also, shouldn't we get uevents if the tree changes? Maybe that's not
Yes (off the top of my head). But a shell script accessing /proc/device-tree
is not going to get uevents.
> guaranteed, but I'd bet we can't handle cases where we don't get events.
> A property added to an existing node comes to mind.>
> Rob
>
Hi Frank,
On Fri, Oct 19, 2018 at 2:06 AM Frank Rowand <[email protected]> wrote:
> On 10/18/18 12:32, Rob Herring wrote:
> > On Tue, Oct 16, 2018 at 05:34:26PM -0700, [email protected] wrote:
> >> Provide a sysfs file, /sys/firmware/devicetree/tree_version, to allow
> >> user space to determine if the live devicetree has remained unchanged
> >> while a series of one or more accesses of /proc/device-tree/ occur.
> >>
> >> The use of both (1) dynamic devicetree modifications and (2) overlay
> >> apply and removal are not supported during the same boot cycle. Thus
> >> non-overlay dynamic modifications are not reflected in the value of
> >> tree_version.
> >
> > I'd prefer to see some sort of information on overlays exported and user
> > space can check if that changed. IIRC, Pantelis had a series to do that
> > along with a kill switch to prevent further modifications. At least some
> > of that series only had minor issues to fix.
>
> The kill switch addresses a different concern, which was from the security
> community. The kill switch is on my todo list.
>
> I don't remember exactly what info the overlay information export patch
> provided. I'll have to go find it and re-read it.
I'm still forward porting the overlay configfs interface, cfr.
https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/renesas-overlays
E.g. after loading r8a7791-koelsch-exio-b-scifa3.dtbo, it changes like:
--- tree /sys/firmware/devicetree old 2018-10-19 08:49:24.073441000 +0200
+++ tree /sys/firmware/devicetree new 2018-10-19 08:49:33.173397000 +0200
@@ -1237,6 +1237,11 @@
│ │ │ │ ├── groups
│ │ │ │ ├── name
│ │ │ │ └── phandle
+│ │ │ ├── scifa3
+│ │ │ │ ├── function
+│ │ │ │ ├── groups
+│ │ │ │ ├── name
+│ │ │ │ └── phandle
│ │ │ ├── scif_clk
│ │ │ │ ├── function
│ │ │ │ ├── groups
@@ -1510,6 +1515,8 @@
│ │ │ ├── interrupts
│ │ │ ├── name
│ │ │ ├── phandle
+│ │ │ ├── pinctrl-0
+│ │ │ ├── pinctrl-names
│ │ │ ├── power-domains
│ │ │ ├── reg
│ │ │ ├── resets
@@ -2277,6 +2284,7 @@
│ │ ├── scifa1
│ │ ├── scifa2
│ │ ├── scifa3
+│ │ ├── scifa3_pins
│ │ ├── scifa4
│ │ ├── scifa5
│ │ ├── scifb0
The above hunks are for /sys/firmware/devicetree/base/.
@@ -2778,6 +2786,14 @@
│ │ └── target
│ └── __symbols__
│ └── target
+ ├── 2
+ │ ├── can_remove
+ │ ├── fragment@0
+ │ │ └── target
+ │ ├── fragment@1
+ │ │ └── target
+ │ └── __symbols__
+ │ └── target
└── enable
The above hunk is for /sys/firmware/devicetree/overlays/
# ls -l /sys/firmware/devicetree/overlays/
total 0
drwxr-xr-x 6 root root 0 okt 19 08:49 1
drwxr-xr-x 5 root root 0 okt 19 08:49 2
-rw-r--r-- 1 root root 4096 okt 19 08:49 enable
1 is from the DT unit tests,
2 is from r8a7791-koelsch-exio-b-scifa3.dtbo,
enable is the kill switch.
Note that after removing overlay 2, and loading a new overlay, the new one is
again called number 2. But the subdir date is newer.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Oct 18, 2018 at 7:06 PM Frank Rowand <[email protected]> wrote:
>
> On 10/18/18 12:32, Rob Herring wrote:
> > On Tue, Oct 16, 2018 at 05:34:26PM -0700, [email protected] wrote:
> >> From: Frank Rowand <[email protected]>
> >>
> >> When an overlay is applied or removed, the live devicetree visible in
> >> /proc/device-tree/, aka /sys/firmware/devicetree/base/, reflects the
> >> changes. There is no method for user space to determine whether the
> >> live devicetree was modified by overlay actions.
> >
> > Because userspace has no way to modify the DT and the ways the kernel
> > can do modifications is limited.
> >
> > Do you have an actually need for this outside of testing/development?
>
> I do not know if anyone uses /proc/device-tree for anything outside of
> testing/development. If we believe that there is no other use of
> /proc/device-tree we can simply document that there is no expectation
> that accessors will see a consistent, unchanging /proc/device-tree.
I didn't mean whether /proc/device-tree is used outside of testing. It
is. The question is whether any users care if there are changes
happening. If so what is the use case?
kexec used to be one of the main users, but I think it has switched
over to the exported FDT which matches what the kernel was originally
passed.
>
> That would be a much smaller patch.
>
>
> >> Provide a sysfs file, /sys/firmware/devicetree/tree_version, to allow
> >> user space to determine if the live devicetree has remained unchanged
> >> while a series of one or more accesses of /proc/device-tree/ occur.
> >>
> >> The use of both (1) dynamic devicetree modifications and (2) overlay
> >> apply and removal are not supported during the same boot cycle. Thus
> >> non-overlay dynamic modifications are not reflected in the value of
> >> tree_version.
> >
> > I'd prefer to see some sort of information on overlays exported and user
> > space can check if that changed. IIRC, Pantelis had a series to do that
> > along with a kill switch to prevent further modifications. At least some
> > of that series only had minor issues to fix.
>
> The kill switch addresses a different concern, which was from the security
> community. The kill switch is on my todo list.
Yes, but there could be other uses. It's not a big step from wanting
to know if the DT has changed to wanting to control it changing or
not.
Perhaps the kill switch needs 2 levels: a temporary freeze and a
permanent freeze. In any case, they don't seem completely unrelated
and I don't really want to see userspace ABI added bit by bit.
> I don't remember exactly what info the overlay information export patch
> provided. I'll have to go find it and re-read it.
>
>
> > Also, shouldn't we get uevents if the tree changes? Maybe that's not
>
> Yes (off the top of my head). But a shell script accessing /proc/device-tree
> is not going to get uevents.
No, but userspace can get them. Accessible from a shell script is not
a requirement of kernel interfaces.
Rob
On 10/19/18 9:06 AM, Rob Herring wrote:
> On Thu, Oct 18, 2018 at 7:06 PM Frank Rowand <[email protected]> wrote:
>>
>> On 10/18/18 12:32, Rob Herring wrote:
>>> On Tue, Oct 16, 2018 at 05:34:26PM -0700, [email protected] wrote:
>>>> From: Frank Rowand <[email protected]>
>>>>
>>>> When an overlay is applied or removed, the live devicetree visible in
>>>> /proc/device-tree/, aka /sys/firmware/devicetree/base/, reflects the
>>>> changes. There is no method for user space to determine whether the
>>>> live devicetree was modified by overlay actions.
>>>
>>> Because userspace has no way to modify the DT and the ways the kernel
>>> can do modifications is limited.
>>>
>>> Do you have an actually need for this outside of testing/development?
>>
>> I do not know if anyone uses /proc/device-tree for anything outside of
>> testing/development. If we believe that there is no other use of
>> /proc/device-tree we can simply document that there is no expectation
>> that accessors will see a consistent, unchanging /proc/device-tree.
>
> I didn't mean whether /proc/device-tree is used outside of testing. It
> is. The question is whether any users care if there are changes
> happening. If so what is the use case?
What is the point of looking at a devicetree if you don't know if it
is in a consistent state or part way through a change?
> kexec used to be one of the main users, but I think it has switched
> over to the exported FDT which matches what the kernel was originally
> passed.
Yes, last I checked kexec was using FDT from /sys/firmware/fdt.
>>
>> That would be a much smaller patch.
>>
>>
>>>> Provide a sysfs file, /sys/firmware/devicetree/tree_version, to allow
>>>> user space to determine if the live devicetree has remained unchanged
>>>> while a series of one or more accesses of /proc/device-tree/ occur.
>>>>
>>>> The use of both (1) dynamic devicetree modifications and (2) overlay
>>>> apply and removal are not supported during the same boot cycle. Thus
>>>> non-overlay dynamic modifications are not reflected in the value of
>>>> tree_version.
>>>
>>> I'd prefer to see some sort of information on overlays exported and user
>>> space can check if that changed. IIRC, Pantelis had a series to do that
>>> along with a kill switch to prevent further modifications. At least some
>>> of that series only had minor issues to fix.
>>
>> The kill switch addresses a different concern, which was from the security
>> community. The kill switch is on my todo list.
>
> Yes, but there could be other uses. It's not a big step from wanting
> to know if the DT has changed to wanting to control it changing or
> not.
>
> Perhaps the kill switch needs 2 levels: a temporary freeze and a
> permanent freeze. In any case, they don't seem completely unrelated
> and I don't really want to see userspace ABI added bit by bit.
I can add a kill switch patch.
>> I don't remember exactly what info the overlay information export patch
>> provided. I'll have to go find it and re-read it.
>>
>>
>>> Also, shouldn't we get uevents if the tree changes? Maybe that's not
>>
>> Yes (off the top of my head). But a shell script accessing /proc/device-tree
>> is not going to get uevents.
>
> No, but userspace can get them. Accessible from a shell script is not
> a requirement of kernel interfaces.
OK for now. I haven't thought that concept through, but it is not key to
whether this feature is useful. The same functionality is also needed
by programs.
I'll have to dig into the uevent implementation and architecture to see
whether uevents are a possible solution. This patch can wait for me to
finish this.
If the current patch ends up being the best method, I still need to
re-work to add memory barriers (or somehow convince myself that they
are not needed).
-Frank
>
> Rob
>
Hi Rob,
First, the point of this patch was to provide a way for userspace (program,
command line interface, whatever -- that is orthogonal) to ensure that its
view of the devicetree via /proc/device-tree/ is consistent since an overlay
apply or remove can alter the devicetree.
For in-kernel use, typically some sort of lock or rcu would be used to
provide this type of functionality.
On 10/22/18 12:30 AM, Frank Rowand wrote:
> On 10/19/18 9:06 AM, Rob Herring wrote:
>> On Thu, Oct 18, 2018 at 7:06 PM Frank Rowand <[email protected]> wrote:
>>>
>>> On 10/18/18 12:32, Rob Herring wrote:
>>>> On Tue, Oct 16, 2018 at 05:34:26PM -0700, [email protected] wrote:
>>>>> From: Frank Rowand <[email protected]>
>>>>>
>>>>> When an overlay is applied or removed, the live devicetree visible in
>>>>> /proc/device-tree/, aka /sys/firmware/devicetree/base/, reflects the
>>>>> changes. There is no method for user space to determine whether the
>>>>> live devicetree was modified by overlay actions.
>>>>
>>>> Because userspace has no way to modify the DT and the ways the kernel
>>>> can do modifications is limited.
>>>>
>>>> Do you have an actually need for this outside of testing/development?
>>>
>>> I do not know if anyone uses /proc/device-tree for anything outside of
>>> testing/development. If we believe that there is no other use of
>>> /proc/device-tree we can simply document that there is no expectation
>>> that accessors will see a consistent, unchanging /proc/device-tree.
>>
>> I didn't mean whether /proc/device-tree is used outside of testing. It
>> is. The question is whether any users care if there are changes
>> happening. If so what is the use case?
>
> What is the point of looking at a devicetree if you don't know if it
> is in a consistent state or part way through a change?
>
>
>> kexec used to be one of the main users, but I think it has switched
>> over to the exported FDT which matches what the kernel was originally
>> passed.
>
> Yes, last I checked kexec was using FDT from /sys/firmware/fdt.
>
>
>>>
>>> That would be a much smaller patch.
>>>
>>>
>>>>> Provide a sysfs file, /sys/firmware/devicetree/tree_version, to allow
>>>>> user space to determine if the live devicetree has remained unchanged
>>>>> while a series of one or more accesses of /proc/device-tree/ occur.
>>>>>
>>>>> The use of both (1) dynamic devicetree modifications and (2) overlay
>>>>> apply and removal are not supported during the same boot cycle. Thus
>>>>> non-overlay dynamic modifications are not reflected in the value of
>>>>> tree_version.
>>>>
>>>> I'd prefer to see some sort of information on overlays exported and user
>>>> space can check if that changed. IIRC, Pantelis had a series to do that
>>>> along with a kill switch to prevent further modifications. At least some
>>>> of that series only had minor issues to fix.
>>>
>>> The kill switch addresses a different concern, which was from the security
>>> community. The kill switch is on my todo list.
>>
>> Yes, but there could be other uses. It's not a big step from wanting
>> to know if the DT has changed to wanting to control it changing or
>> not.
>>
>> Perhaps the kill switch needs 2 levels: a temporary freeze and a
>> permanent freeze. In any case, they don't seem completely unrelated
>> and I don't really want to see userspace ABI added bit by bit.
>
> I can add a kill switch patch.
The point behind the kill switch is to allow a way to disable modification
of the devicetree from userspace via an overlay manager. Since there is
no userspace overlay manager, there is no need for a kill switch. The
kill switch (or equivalent functionality) should be added as part of
adding the overlay manager, when that occurs.
Addressing adding userspace ABI bit by bit, any discussion of what the userspace
overlay manager interface will look like is purely conjecture. I do not want
to wait till the overlay manager to be added before the current problem of
user space synchronization is addressed.
>>> I don't remember exactly what info the overlay information export patch
>>> provided. I'll have to go find it and re-read it.
>>>
>>>
>>>> Also, shouldn't we get uevents if the tree changes? Maybe that's not
>>>
>>> Yes (off the top of my head). But a shell script accessing /proc/device-tree
>>> is not going to get uevents.
>>
>> No, but userspace can get them. Accessible from a shell script is not
>> a requirement of kernel interfaces.
>
> OK for now. I haven't thought that concept through, but it is not key to
> whether this feature is useful. The same functionality is also needed
> by programs.
>
> I'll have to dig into the uevent implementation and architecture to see
> whether uevents are a possible solution. This patch can wait for me to
> finish this.
Getting a uevent does not provide the information needed to ensure that
the devicetree is in a consistent state over a set of accesses to
/proc/device-tree (that is, a "critical section").
>
> If the current patch ends up being the best method, I still need to
> re-work to add memory barriers (or somehow convince myself that they
> are not needed).
In the current version of the patch, I was reluctant to provide the
synchronization via a lock in the sysfs show function because I did
not find any documentation or discussion that assured me that a lock
was legal in that context. I have since asked Greg KH if using a
lock for synchronization in the show function is ok and he assured
me that it is. Based on that, I have a new version of the patch that
is conceptually cleaner, easier to understand, and easier to use.
-Frank
>
> -Frank
>
>
>>
>> Rob
>>
>
>