Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] of: overlay: user space synchronization
To:     Alan Tull <atull@kernel.org>
Cc:     Geert Uytterhoeven <geert@linux-m68k.org>,
        Rob Herring <robh+dt@kernel.org>,
        Pantelis Antoniou <pantelis.antoniou@konsulko.com>,
        Pantelis Antoniou <panto@antoniou-consulting.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
References: <1539567282-1326-1-git-send-email-frowand.list@gmail.com>
 <CAMuHMdXad2g5zEZ8=XzzVgfijcatKYj5AjwVQg=W-by_rHfcAQ@mail.gmail.com>
 <e69685d5-e09f-5f7d-2ff0-7ead12500371@gmail.com>
 <CANk1AXQU=3Ap=wbO5b8c2pMq61hzuRkeWYNv2Esq+c9d+0e_BA@mail.gmail.com>
From:   Frank Rowand <frowand.list@gmail.com>
Message-ID: <3635be33-7301-ecd5-5966-24b0494771ef@gmail.com>
Date:   Mon, 15 Oct 2018 17:04:22 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <CANk1AXQU=3Ap=wbO5b8c2pMq61hzuRkeWYNv2Esq+c9d+0e_BA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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