[PATCH v2 0/3] trace: Fix flyrecord alignment issue

[PATCH v2 0/3] trace: Fix flyrecord alignment issue

[Michal Simek]

Hi,

sandbox is getting bigger and bigger and I have reached the case that
adding some more functions ends up in CI loop failure. After some
investigation I found that flyrecord header have incorrect information
about data offset which is caused by incorrect alignment calculation.
That's why this series is fixing alignment calculation.
I have done it via 3 patches where the first two are just preparing code
for actual fixup.

Thanks,
Michal

Changes in v2:
- s/start_addr/start_ofs/g'

Michal Simek (3):
  trace: Use 64bit variable for start and len
  trace: Move trace_clocks description above record offset calculation
  trace: Fix alignment logic in flyrecord header

 tools/proftool.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

-- 
2.36.1

[PATCH v2 1/3] trace: Use 64bit variable for start and len

[Michal Simek]

tputq() requires variables to have 64bit width that's why make them 64bit
to clean alignment requirement.

Signed-off-by: Michal Simek <michal.simek@amd.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 tools/proftool.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/proftool.c b/tools/proftool.c
index 101bcb63334e..869a2a32c510 100644
--- a/tools/proftool.c
+++ b/tools/proftool.c
@@ -1493,7 +1493,8 @@ static int write_pages(struct twriter *tw, enum out_format_t out_format,
 static int write_flyrecord(struct twriter *tw, enum out_format_t out_format,
 			   int *missing_countp, int *skip_countp)
 {
-	int start, ret, len;
+	unsigned long long start, len;
+	int ret;
 	FILE *fout = tw->fout;
 	char str[200];
 
-- 
2.36.1

[PATCH v2 2/3] trace: Move trace_clocks description above record offset calculation

[Michal Simek]

Flyrecord tracing data are page aligned that's why it is necessary to
calculate alignment properly. Because trace_clocks description is the part
of record length it is necessary to have information about length earlier.

Signed-off-by: Michal Simek <michal.simek@amd.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 tools/proftool.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/proftool.c b/tools/proftool.c
index 869a2a32c510..7c95a94482fc 100644
--- a/tools/proftool.c
+++ b/tools/proftool.c
@@ -1500,6 +1500,10 @@ static int write_flyrecord(struct twriter *tw, enum out_format_t out_format,
 
 	tw->ptr += fprintf(fout, "flyrecord%c", 0);
 
+	snprintf(str, sizeof(str),
+		 "[local] global counter uptime perf mono mono_raw boot x86-tsc\n");
+	len = strlen(str);
+
 	/* trace data */
 	start = ALIGN(tw->ptr + 16, TRACE_PAGE_SIZE);
 	tw->ptr += tputq(fout, start);
@@ -1510,9 +1514,6 @@ static int write_flyrecord(struct twriter *tw, enum out_format_t out_format,
 		return -1;
 	tw->ptr += ret;
 
-	snprintf(str, sizeof(str),
-		 "[local] global counter uptime perf mono mono_raw boot x86-tsc\n");
-	len = strlen(str);
 	tw->ptr += tputq(fout, len);
 	tw->ptr += tputs(fout, str);
 
-- 
2.36.1

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

[Michal Simek]

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@amd.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- s/start_addr/start_ofs/g'

 tools/proftool.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/tools/proftool.c b/tools/proftool.c
index 7c95a94482fc..fca45e4a5af6 100644
--- a/tools/proftool.c
+++ b/tools/proftool.c
@@ -1493,19 +1493,43 @@ static int write_pages(struct twriter *tw, enum out_format_t out_format,
 static int write_flyrecord(struct twriter *tw, enum out_format_t out_format,
 			   int *missing_countp, int *skip_countp)
 {
-	unsigned long long start, len;
+	unsigned long long start, start_ofs, len;
 	int ret;
 	FILE *fout = tw->fout;
 	char str[200];
 
+	/* Record start pointer */
+	start_ofs = tw->ptr;
+	debug("Start of flyrecord header at: 0x%llx\n", start_ofs);
+
 	tw->ptr += fprintf(fout, "flyrecord%c", 0);
 
+	/* flyrecord\0 - allocated 10 bytes */
+	start_ofs += 10;
+
+	/*
+	 * 8 bytes that are a 64-bit word containing the offset into the file
+	 * that holds the data for the CPU.
+	 *
+	 * 8 bytes that are a 64-bit word containing the size of the CPU
+	 * data at that offset.
+	 */
+	start_ofs += 16;
+
 	snprintf(str, sizeof(str),
 		 "[local] global counter uptime perf mono mono_raw boot x86-tsc\n");
 	len = strlen(str);
 
+	/* trace clock length - 8 bytes */
+	start_ofs += 8;
+	/* trace clock data */
+	start_ofs += len;
+
+	debug("Calculated flyrecord header end at: 0x%llx, trace clock len: 0x%llx\n",
+	      start_ofs, len);
+
 	/* trace data */
-	start = ALIGN(tw->ptr + 16, TRACE_PAGE_SIZE);
+	start = ALIGN(start_ofs, TRACE_PAGE_SIZE);
 	tw->ptr += tputq(fout, start);
 
 	/* use a placeholder for the size */
@@ -1517,6 +1541,9 @@ static int write_flyrecord(struct twriter *tw, enum out_format_t out_format,
 	tw->ptr += tputq(fout, len);
 	tw->ptr += tputs(fout, str);
 
+	debug("End of flyrecord header at: 0x%x, offset: 0x%llx\n",
+	      tw->ptr, start);
+
 	debug("trace text base %lx, map file %lx\n", text_base, text_offset);
 
 	ret = write_pages(tw, out_format, missing_countp, skip_countp);
-- 
2.36.1

Re: [PATCH v2 0/3] trace: Fix flyrecord alignment issue

[Michal Simek]

On 9/15/23 14:12, Michal Simek wrote:
> Hi,
> 
> sandbox is getting bigger and bigger and I have reached the case that
> adding some more functions ends up in CI loop failure. After some
> investigation I found that flyrecord header have incorrect information
> about data offset which is caused by incorrect alignment calculation.
> That's why this series is fixing alignment calculation.
> I have done it via 3 patches where the first two are just preparing code
> for actual fixup.
> 
> Thanks,
> Michal
> 
> Changes in v2:
> - s/start_addr/start_ofs/g'
> 
> Michal Simek (3):
>    trace: Use 64bit variable for start and len
>    trace: Move trace_clocks description above record offset calculation
>    trace: Fix alignment logic in flyrecord header
> 
>   tools/proftool.c | 39 ++++++++++++++++++++++++++++++++++-----
>   1 file changed, 34 insertions(+), 5 deletions(-)
> 

Applied.
M

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

[Simon Glass]

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@amd.com>
Reviewed-by: Simon Glass <sjg@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!

Re: [PATCH v2 2/3] trace: Move trace_clocks description above record offset calculation

[Simon Glass]

Flyrecord tracing data are page aligned that's why it is necessary to
calculate alignment properly. Because trace_clocks description is the part
of record length it is necessary to have information about length earlier.

Signed-off-by: Michal Simek <michal.simek@amd.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 tools/proftool.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Applied to u-boot-dm, thanks!

Re: [PATCH v2 1/3] trace: Use 64bit variable for start and len

[Simon Glass]

tputq() requires variables to have 64bit width that's why make them 64bit
to clean alignment requirement.

Signed-off-by: Michal Simek <michal.simek@amd.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 tools/proftool.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Applied to u-boot-dm, thanks!

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

[Michal Simek]

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@amd.com>
> Reviewed-by: Simon Glass <sjg@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/

Thanks,
Michal

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

[Simon Glass]

Hi Michal,

On Mon, 25 Sept 2023 at 00:06, Michal Simek <michal.simek@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@amd.com>
> > Reviewed-by: Simon Glass <sjg@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'?

Regards,
Simon

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

[Michal Simek]

On 9/25/23 15:10, Simon Glass wrote:
> Hi Michal,
> 
> On Mon, 25 Sept 2023 at 00:06, Michal Simek <michal.simek@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@amd.com>
>>> Reviewed-by: Simon Glass <sjg@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.

Thanks,
Michal

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

[Simon Glass]

Hi Michal,

On Mon, 25 Sept 2023 at 07:38, Michal Simek <michal.simek@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@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@amd.com>
> >>> Reviewed-by: Simon Glass <sjg@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.

If you are not using patchwork, how come you are a custodian? Is
someone else dealing with patchwork for you?

Regards,
Simon

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

[Michal Simek]

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@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@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@amd.com>
>>>>> Reviewed-by: Simon Glass <sjg@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.

Thanks,
Michal

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]

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@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@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@amd.com>
> > > > > > Reviewed-by: Simon Glass <sjg@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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Michal Simek]

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@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@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@amd.com>
>>>>>>> Reviewed-by: Simon Glass <sjg@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.

Thanks,
Michal

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1954 bytes --]

On Mon, Sep 25, 2023 at 08:01:41AM -0600, Simon Glass wrote:
> Hi Michal,
> 
> On Mon, 25 Sept 2023 at 07:38, Michal Simek <michal.simek@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@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@amd.com>
> > >>> Reviewed-by: Simon Glass <sjg@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.

Patchwork doesn't, but b4 does, handle tags sent to the cover letter
rather than individually.  Patchwork does have a link to the cover
letter, on individual patch links under the "expand" link on the
"Series" line.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6454 bytes --]

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@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@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@amd.com>
> > > > > > > > Reviewed-by: Simon Glass <sjg@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@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

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Michal Simek]

On 9/25/23 16:28, Tom Rini wrote:
> On Mon, Sep 25, 2023 at 08:01:41AM -0600, Simon Glass wrote:
>> Hi Michal,
>>
>> On Mon, 25 Sept 2023 at 07:38, Michal Simek <michal.simek@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@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@amd.com>
>>>>>> Reviewed-by: Simon Glass <sjg@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.
> 
> Patchwork doesn't, but b4 does, handle tags sent to the cover letter
> rather than individually.

For tags yes it works nicely with b4. Pretty much just reply to cover letter and 
b4 copy it to all patches.
But this was more about that you can't see my Applied reply there.
I don't think that b4 will be able to catch it but I would love to be wrong.

M

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

[Michal Simek]

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@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@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@amd.com>
>>>>>>>>> Reviewed-by: Simon Glass <sjg@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@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.

Thanks,
Michal

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 7887 bytes --]

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@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@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@amd.com>
> > > > > > > > > > Reviewed-by: Simon Glass <sjg@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@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?).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [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@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@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@amd.com>
>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@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@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

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 9375 bytes --]

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@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@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@amd.com>
> > > > > > > > > > > > Reviewed-by: Simon Glass <sjg@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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]