[PATCH v2 3/3] trace: Fix alignment logic in flyrecord header
Tom Rini
trini at konsulko.com
Wed Oct 25 16:19:34 CEST 2023
On Wed, Oct 25, 2023 at 09:33:03AM +0200, Michal Simek wrote:
>
>
> 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.
It's just some small tweaks to the upstream git hook. I should write
doc/develop/patchwork.rst and mention that hook, and some work flows, as
the most generic answer here. And I'm also not super proud of that set
of hacks to an upstream script :)
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231025/2bd5b63a/attachment.sig>
More information about the U-Boot
mailing list