Re: [PATCH] of: overlay: user space synchronization

[Frank Rowand <frowand.list@gmail.com>]

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