[PATCH v2 3/3] trace: Fix alignment logic in flyrecord header

[Michal Simek]

On 10/24/23 20:03, Tom Rini wrote:
> On Tue, Oct 24, 2023 at 02:33:26PM +0200, Michal Simek wrote:
>> Hi Tom,
>>
>> On 9/25/23 16:33, Tom Rini wrote:
>>> On Mon, Sep 25, 2023 at 04:21:17PM +0200, Michal Simek wrote:
>>>>
>>>>
>>>> On 9/25/23 16:19, Tom Rini wrote:
>>>>> On Mon, Sep 25, 2023 at 04:10:38PM +0200, Michal Simek wrote:
>>>>>> Hi Simon,
>>>>>>
>>>>>> On 9/25/23 16:01, Simon Glass wrote:
>>>>>>> Hi Michal,
>>>>>>>
>>>>>>> On Mon, 25 Sept 2023 at 07:38, Michal Simek <michal.simek at amd.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/25/23 15:10, Simon Glass wrote:
>>>>>>>>> Hi Michal,
>>>>>>>>>
>>>>>>>>> On Mon, 25 Sept 2023 at 00:06, Michal Simek <michal.simek at amd.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Simon,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 9/23/23 20:13, Simon Glass wrote:
>>>>>>>>>>> Current alignment which is using 16 bytes is not correct in connection to
>>>>>>>>>>> trace_clocks description and it's length.
>>>>>>>>>>> That's why use start_addr variable and record proper size based on used
>>>>>>>>>>> entries.
>>>>>>>>>>>
>>>>>>>>>>> Fixes: be16fc81b2ed ("trace: Update proftool to use new binary format").
>>>>>>>>>>> Signed-off-by: Michal Simek <michal.simek at amd.com>
>>>>>>>>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> Changes in v2:
>>>>>>>>>>> - s/start_addr/start_ofs/g'
>>>>>>>>>>>
>>>>>>>>>>>        tools/proftool.c | 31 +++++++++++++++++++++++++++++--
>>>>>>>>>>>        1 file changed, 29 insertions(+), 2 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> Applied to u-boot-dm, thanks!
>>>>>>>>>>
>>>>>>>>>> FYI: I have merged it to my tree and already sent pull request to Tom.
>>>>>>>>>> Without it I couldn't pass CI loop to get all reviewed features in.
>>>>>>>>>>
>>>>>>>>>> https://lore.kernel.org/all/ab72c480-e9f8-416e-adf5-726f7d40c4f5@amd.com/
>>>>>>>>>
>>>>>>>>> Ah OK, well that's fine. It was in my patchwork queue still, which
>>>>>>>>> suggests that the patches were not set to 'applied'?
>>>>>>>>
>>>>>>>> I am not using patchwork. But I expect my reply to cover letter was recorded there.
>>>>>>>
>>>>>>> Probably. If you reply to each patch, it shows up in the patch, but
>>>>>>> the cover letter is hidden somewhere else.
>>>>>>
>>>>>> I have never started to like patchwork. I installed that client long time
>>>>>> ago, I also have account for quite a long time.
>>>>>>
>>>>>>> If you are not using patchwork, how come you are a custodian? Is
>>>>>>> someone else dealing with patchwork for you?
>>>>>>
>>>>>> Not really. I am just keep track on it via emails.
>>>>>>
>>>>>> DT folks did wire CI loop on every patch which they get. I am not aware
>>>>>> about any feature like this which would bring me something. That's why I am
>>>>>> considering patchwork as unneeded layer. And I also don't think that I have
>>>>>> read anywhere that all custodians should be using patchwork.
>>>>>
>>>>> Right, patchwork isn't required, but can be helpful.  Part of how
>>>>> patchwork is maintained for everyone (in U-Boot) is that I have a script
>>>>> that will update the status of patches to accepted and add the githash,
>>>>> based on the "patchwork hash" of a given commit.  There's a number of
>>>>> automated tooling things that other projects use which could be helpful
>>>>> here, but due to lack of time/resources, we haven't tried them here.
>>>>
>>>> Can you share that script? I am happy to run it and pretty much close my list.
>>>> I am using b4 for applying patches that's why all message-ids are listed in
>>>> the history which will uniquely identify that patches.
>>>
>>> If you like, yes, you can run the following.  Note that when I run it
>>> myself between tags, it will still re-update things.  This requires
>>> having patchwork cloned from git as well and is a slight modification of
>>> both tools/patchwork-update-commits and tools/post-receive.hook:
>>>
>>> #!/bin/bash
>>>
>>> # Patchwork - automated patch tracking system
>>> # Copyright (C) 2010 martin f. krafft <madduck at madduck.net>
>>> #
>>> # SPDX-License-Identifier: GPL-2.0-or-later
>>>
>>> # Git post-receive hook to update Patchwork patches after Git pushes
>>> set -u
>>>
>>> PW_DIR=/home/trini/work/u-boot/patchwork/patchwork
>>>
>>> #TODO: the state map should really live in the repo's git-config
>>> STATE_MAP=".git/refs/heads/master:Accepted"
>>>
>>> # ignore all commits already present in these refs
>>> # e.g.,
>>> #   EXCLUDE="refs/heads/upstream refs/heads/other-project"
>>> EXCLUDE=""
>>>
>>> do_exit=0
>>> trap "do_exit=1" INT
>>>
>>> get_patchwork_hash() {
>>>       local hash
>>>       hash=$(git diff "$1~..$1" | python3 $PW_DIR/hasher.py)
>>>       echo "$hash"
>>>       test -n "$hash"
>>> }
>>>
>>> get_patchwork_hash_harder() {
>>>       local hash
>>>       hash=$(git diff "$1~..$1" | sed -e 's/^ $//g' | python3 $PW_DIR/hasher.py)
>>>       echo "$hash"
>>>       test -n "$hash"
>>> }
>>>
>>> get_patch_id() {
>>>       local id
>>>       id=$(curl -s "http://patchwork.ozlabs.org/api/patches/?project=uboot&hash=$1" | \
>>>            jq '.[-1] | .id')
>>>       echo "$id"
>>> }
>>>
>>> set_patch_state() {
>>>       pwclient update -s "$2" -c "$3" "$1" 2>&1
>>> }
>>>
>>> update_patches() {
>>>       local cnt; cnt=0
>>>       for rev in $(git rev-parse --not ${EXCLUDE} |
>>>                    git rev-list --stdin --no-merges --reverse "${1}".."${2}"); do
>>>           if [ "$do_exit" = 1 ]; then
>>>               echo "I: exiting..." >&2
>>>               break
>>>           fi
>>>           hash=$(get_patchwork_hash "$rev")
>>>           if [ -z "$hash" ]; then
>>>               echo "E: failed to hash rev $rev." >&2
>>>               continue
>>>           fi
>>>           id=$(get_patch_id "$hash")
>>>           if [ "$id" = "null" ]; then
>>>               hash=$(get_patchwork_hash_harder "$rev")
>>> 	    id=$(get_patch_id "$hash")
>>> 	fi
>>> 	if [ "$id" = "null" ]; then
>>>               echo "E: failed to find patch for rev $rev." >&2
>>>               continue
>>>           fi
>>>           reason="$(set_patch_state "$id" "$3" "$rev")"
>>>           if [ -n "$reason" ]; then
>>>               echo "E: failed to update patch #$id${reason:+: $reason}." >&2
>>>               continue
>>>           fi
>>>           echo "I: patch #$id updated using rev $rev." >&2
>>>           cnt=$((cnt + 1))
>>>       done
>>>
>>>       echo "I: $cnt patch(es) updated to state $3." >&2
>>> }
>>>
>>> oldrev=$1
>>> newrev=$2
>>> refname=".git/refs/heads/master"
>>> found=0
>>> for i in $STATE_MAP; do
>>>       key="${i%:*}"
>>>       if [ "$key" = "$refname" ]; then
>>>           update_patches "$oldrev" "$newrev" ${i#*:}
>>>           found=1
>>>           break
>>>       fi
>>> done
>>> if [ $found -eq 0 ]; then
>>>       echo "E: STATE_MAP has no mapping for branch $refname" >&2
>>> fi
>>>
>>
>> Sorry for delay on this. I played with your script and also look at git-pw
>> client and cleaned my queue.
>> Pretty much incorrect series, rfcs, etc should be only patches listed.
>>
>> If you are running it between tags there is no need for me to run it.
> 
> I do this after each tag so yes, you don't have to run it as well if
> it's not helpful to you (it may be helpful to custodians that do use
> patchwork, perhaps with the upstream commit they're basing their PR on
> and then their own tag for me, to then clean up their queue?).

Actually I have updated that git filter above just to list sha1 from git for me 
as committer and that could serve purpose for custodians for cleaning up the queue.
Would be maybe worth to commit this script to u-boot repository that other 
custodians can start to use it.

Thanks,
Michal