| Message ID | 1539567282-1326-1-git-send-email-frowand.list@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | of: overlay: user space synchronization | expand |
Hi Frank, On Mon, Oct 15, 2018 at 3:36 AM <frowand.list@gmail.com> wrote: > From: Frank Rowand <frank.rowand@sony.com> > > 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. Thanks for your patch! > 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. What does this mean exactly, for users? I am used to applying and removing overlays at run time (still carrying Pantelis' overlay configfs patches), but when would I use non-overlay dynamic modifications? > --- a/Documentation/ABI/testing/sysfs-firmware-ofw > +++ b/Documentation/ABI/testing/sysfs-firmware-ofw > > +What: /sys/firmware/devicetree/tree_version > +Date: October 2018 > +KernelVersion: 4.20 > +Contact: Frank Rowand <frowand.list@gmail.com>, devicetree@vger.kernel.org > +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. > + > + 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: > + > + done=1 > + > + while [ $done = 1 ] ; do > + > + pre_version=$(cat /sys/firmware/devicetree/tree_version) > + version=$pre_version > + while [ $(( ${version} & 1 )) != 0 ] ; do > + # echo is optional, sleep value can be tuned > + echo "${version} sleeping" > + sleep 2; > + pre_version=$(cat /sys/firmware/devicetree/tree_version) > + version=${pre_version} > + done > + > + # 'critical region' > + # access /proc/device-tree/ one or more times > + > + post_version=$(cat /sys/firmware/devicetree/tree_version) > + > + if [ ${post_version} = ${pre_version} ] ; then > + done=0 > + fi > + > + done Please say explicitly that tree_version contains a 32-bit unsigned decimal number, which is incremented before and after every change. I had to deduce that from the code. IMHO that is more important than having the sample script here. Gr{oetje,eeting}s, Geert
On 10/15/18 01:24, Geert Uytterhoeven wrote: > Hi Frank, > > On Mon, Oct 15, 2018 at 3:36 AM <frowand.list@gmail.com> wrote: >> From: Frank Rowand <frank.rowand@sony.com> >> >> 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. > > Thanks for your patch! > >> 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. > > What does this mean exactly, for users? > I am used to applying and removing overlays at run time (still > carrying Pantelis' > overlay configfs patches), but when would I use non-overlay dynamic > modifications? To find some examples, 'git grep of_add_property'. (This is not a comprehensive list, there are other similar functions.) Some Powerpc systems use dynamic modifications of device trees, for example adding and removing cpus. Kexec uses dynamic modifications just before booting a new kernel (so no interference with overlays). Devicetree unittest uses it, with no interference with overlays. There are also a few places platform code or a driver uses dynamic modification. Possible conflicts with overlays are: arch/arm/mach-mvebu/coherency.c arch/arm/mach-omap2/timer.c rcar_du_of.c is a known use that is grandfathered in. It currently is not compatible with overlays. drivers/macintosh/smu.c should not be an issue because I don't expect any macintosh overlay. Some of the reasons for not supporting both overlays and other dynamic modifications on the same system might be possible to resolve with additional code, but some might be difficult or impossible to resolve. So that restriction might be loosened or removed in the future. Some of the reasons are: - dynamic modifications do not use the same locking mechanism as overlay apply and removal, thus the devicetree could be corrupted - dynamic modifications of portions of the devicetree that are the result of applying an overlay will (may?) cause removal of the overlay to fail (or devicetree corruption?) - (future concern: ) static validation of an overlay (or set of overlays) against a specific base devicetree would not be valid if the base devicetree is further modified by dynamic modifications - this is theoretical since validation is a currently under development feature and we do not know what the final feature will look like The plan for overlays has been to add specific use models (or functionality or features) in a limited fashion initially, to ensure that each feature is implemented to a sufficient degree (in other words is not a hack that only works in limited circumstances, such as the correct phase of the moon), works robustly and is maintainable. Then as each feature or set of features is found to be good enough, add more features. I suspect that dynamic modification in general will remain not compatible with overlays, with limited exceptions. Possible exceptions would require stringent review and auditing, and could incude devicetree unittest (even this one makes me nervous) and some platform code (especially early boot code). >> --- a/Documentation/ABI/testing/sysfs-firmware-ofw >> +++ b/Documentation/ABI/testing/sysfs-firmware-ofw >> >> +What: /sys/firmware/devicetree/tree_version >> +Date: October 2018 >> +KernelVersion: 4.20 >> +Contact: Frank Rowand <frowand.list@gmail.com>, devicetree@vger.kernel.org >> +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. >> + >> + 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: >> + >> + done=1 >> + >> + while [ $done = 1 ] ; do >> + >> + pre_version=$(cat /sys/firmware/devicetree/tree_version) >> + version=$pre_version >> + while [ $(( ${version} & 1 )) != 0 ] ; do >> + # echo is optional, sleep value can be tuned >> + echo "${version} sleeping" >> + sleep 2; >> + pre_version=$(cat /sys/firmware/devicetree/tree_version) >> + version=${pre_version} >> + done >> + >> + # 'critical region' >> + # access /proc/device-tree/ one or more times >> + >> + post_version=$(cat /sys/firmware/devicetree/tree_version) >> + >> + if [ ${post_version} = ${pre_version} ] ; then >> + done=0 >> + fi >> + >> + done > > Please say explicitly that tree_version contains a 32-bit unsigned > decimal number, which is incremented before and after every change. > I had to deduce that from the code. Good point. I'll add that. > > IMHO that is more important than having the sample script here. > > Gr{oetje,eeting}s, > > Geert >
On Mon, Oct 15, 2018 at 1:09 PM Frank Rowand <frowand.list@gmail.com> wrote: > > On 10/15/18 01:24, Geert Uytterhoeven wrote: > > > > Please say explicitly that tree_version contains a 32-bit unsigned > > decimal number, which is incremented before and after every change. > > I had to deduce that from the code. > > Good point. I'll add that. Looking at the code, tree_version being odd or even means something. tree_version is incremented every time the of_mutex is locked or unlocked, such that: * tree_version is odd == of_mutex is locked and the tree is currently be in the process of being changed * tree_version is even == of_version is unlocked again and the changes are finished. How about making this explicit in the interface by breaking it up into two attributes: /sys/firmware/devicetree/tree_lock == "locked" or "unlocked". If the tree is locked, you know that the tree may still be changing and the sysfs can't be trusted to be stable yet. Or maybe even more specifically tree_overlay_lock? /sys/firmware/devicetree/tree_version = a u32 that is incremented once for every overlay added or removed. Alan > > > > > > IMHO that is more important than having the sample script here. > > > > Gr{oetje,eeting}s, > > > > Geert > > >
On 10/15/18 13:38, Alan Tull wrote: > On Mon, Oct 15, 2018 at 1:09 PM Frank Rowand <frowand.list@gmail.com> wrote: >> >> On 10/15/18 01:24, Geert Uytterhoeven wrote: >>> >>> Please say explicitly that tree_version contains a 32-bit unsigned >>> decimal number, which is incremented before and after every change. >>> I had to deduce that from the code. >> >> Good point. I'll add that. > > Looking at the code, tree_version being odd or even means something. > tree_version is incremented every time the of_mutex is locked or > unlocked, such that: > * tree_version is odd == of_mutex is locked and the tree is > currently be in the process of being changed > * tree_version is even == of_version is unlocked again and the > changes are finished. > > How about making this explicit in the interface by breaking it up into > two attributes: > > /sys/firmware/devicetree/tree_lock == "locked" or "unlocked". If the > tree is locked, you know that the tree may still be changing and the > sysfs can't be trusted to be stable yet. Or maybe even more > specifically tree_overlay_lock? > > /sys/firmware/devicetree/tree_version = a u32 that is incremented once > for every overlay added or removed. I like the extra clarity of purpose that having two attributes provides, but it makes the user space dance more difficult. With a single attribute, the shell code is (updated from the patch to remove the variable "version"): done=1 while [ $done = 1 ] ; do 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" sleep 2; pre_version=$(cat /sys/firmware/devicetree/tree_version) done # 'critical region' # access /proc/device-tree/ one or more times post_version=$(cat /sys/firmware/devicetree/tree_version) if [ ${post_version} = ${pre_version} ] ; then done=0 fi done With two attributes, the shell code is: done=1 while [ $done = 1 ] ; do # the order of the next three lines must not change version=$(cat /sys/firmware/devicetree/tree_version) pre_version=${version} tree_lock=$(cat /sys/firmware/devicetree/tree_lock) while [ tree_lock != "unlocked" ] || [ ${version} != ${pre_version} ] ; do # echo is optional, sleep value can be tuned echo "locked, sleeping" sleep 2; # the order of the next two lines must not change pre_version=${version} tree_lock=$(cat /sys/firmware/devicetree/tree_lock) version=$(cat /sys/firmware/devicetree/tree_version) done # 'critical region' # access /proc/device-tree/ one or more times # the order of the next two lines must not change post_version=$(cat /sys/firmware/devicetree/tree_version) tree_lock=$(cat /sys/firmware/devicetree/tree_lock) if [ ${tree_lock} = "unlocked" ] && [ ${post_version} = ${pre_version} ] ; then done=0 fi done The two attribute example is untested, could have syntax errors, etc. I'm also not sure that the logic is correct. My opinion is that the extra shell complexity makes the two attribute solution worse. -Frank > > Alan > > >> >> >>> >>> IMHO that is more important than having the sample script here. >>> >>> Gr{oetje,eeting}s, >>> >>> Geert >>> >> >
On Mon, Oct 15, 2018 at 7:04 PM Frank Rowand <frowand.list@gmail.com> wrote: > > On 10/15/18 13:38, Alan Tull wrote: > > On Mon, Oct 15, 2018 at 1:09 PM Frank Rowand <frowand.list@gmail.com> wrote: > >> > >> On 10/15/18 01:24, Geert Uytterhoeven wrote: > >>> > >>> Please say explicitly that tree_version contains a 32-bit unsigned > >>> decimal number, which is incremented before and after every change. > >>> I had to deduce that from the code. > >> > >> Good point. I'll add that. > > > > Looking at the code, tree_version being odd or even means something. > > tree_version is incremented every time the of_mutex is locked or > > unlocked, such that: > > * tree_version is odd == of_mutex is locked and the tree is > > currently be in the process of being changed > > * tree_version is even == of_version is unlocked again and the > > changes are finished. > > > > How about making this explicit in the interface by breaking it up into > > two attributes: > > > > /sys/firmware/devicetree/tree_lock == "locked" or "unlocked". If the > > tree is locked, you know that the tree may still be changing and the > > sysfs can't be trusted to be stable yet. Or maybe even more > > specifically tree_overlay_lock? > > > > /sys/firmware/devicetree/tree_version = a u32 that is incremented once > > for every overlay added or removed. > > I like the extra clarity of purpose that having two attributes provides, > but it makes the user space dance more difficult. > > With a single attribute, the shell code is (updated from the patch > to remove the variable "version"): > > done=1 > > while [ $done = 1 ] ; do > > 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" > sleep 2; > pre_version=$(cat /sys/firmware/devicetree/tree_version) > done > > # 'critical region' > # access /proc/device-tree/ one or more times > > post_version=$(cat /sys/firmware/devicetree/tree_version) > > if [ ${post_version} = ${pre_version} ] ; then > done=0 > fi > > done > > With two attributes, the shell code is: > > > done=1 > > while [ $done = 1 ] ; do > > # the order of the next three lines must not change > version=$(cat /sys/firmware/devicetree/tree_version) > pre_version=${version} > tree_lock=$(cat /sys/firmware/devicetree/tree_lock) > while [ tree_lock != "unlocked" ] || > [ ${version} != ${pre_version} ] ; do > # echo is optional, sleep value can be tuned > echo "locked, sleeping" > sleep 2; > # the order of the next two lines must not change > pre_version=${version} > tree_lock=$(cat /sys/firmware/devicetree/tree_lock) > version=$(cat /sys/firmware/devicetree/tree_version) > done > > # 'critical region' > # access /proc/device-tree/ one or more times > > # the order of the next two lines must not change > post_version=$(cat /sys/firmware/devicetree/tree_version) > tree_lock=$(cat /sys/firmware/devicetree/tree_lock) > > if [ ${tree_lock} = "unlocked" ] && > [ ${post_version} = ${pre_version} ] ; then > done=0 > fi > > done > > > The two attribute example is untested, could have syntax errors, etc. > I'm also not sure that the logic is correct. > > My opinion is that the extra shell complexity makes the two attribute > solution worse. Yes, I can see that now and agree with you here. Thanks for giving the idea consideration. I'll review your v2 . Alan > > -Frank > > > > > > Alan > > > > > >> > >> > >>> > >>> IMHO that is more important than having the sample script here. > >>> > >>> Gr{oetje,eeting}s, > >>> > >>> Geert > >>> > >> > >
diff --git a/Documentation/ABI/testing/sysfs-firmware-ofw b/Documentation/ABI/testing/sysfs-firmware-ofw index edcab3ccfcc0..4111797e8ed5 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 <frowand.list@gmail.com>, devicetree@vger.kernel.org +Description: + Top level Open Firmware, aka devicetree, sysfs directory. + +What: /sys/firmware/devicetree/base Date: November 2013 Contact: Grant Likely <grant.likely@arm.com>, devicetree@vger.kernel.org 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,62 @@ 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 <frowand.list@gmail.com>, devicetree@vger.kernel.org +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. + + 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: + + done=1 + + while [ $done = 1 ] ; do + + pre_version=$(cat /sys/firmware/devicetree/tree_version) + version=$pre_version + while [ $(( ${version} & 1 )) != 0 ] ; do + # echo is optional, sleep value can be tuned + echo "${version} sleeping" + sleep 2; + pre_version=$(cat /sys/firmware/devicetree/tree_version) + version=${pre_version} + done + + # 'critical region' + # access /proc/device-tree/ one or more times + + post_version=$(cat /sys/firmware/devicetree/tree_version) + + if [ ${post_version} = ${pre_version} ] ; then + 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: