[PATCH v1] cmd: acpi: Print revisions in hex format

[PATCH v1] cmd: acpi: Print revisions in hex format

[Andy Shevchenko]

The revisions are usually dates in hex-decimal format representing
YYYYmmdd. Print them in hex to see this clearly.

Before:
  ...
  FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 538970376 INTL 0)
  DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 65536 INTL 538968870)
  ...
After:
  ...
  FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 20200908 INTL 0)
  DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 10000 INTL 20200326)
  ...

Fixes: 0b885bcfd9b0 ("acpi: Add an acpi command")
Cc: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 cmd/acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/acpi.c b/cmd/acpi.c
index 085a3a650d1a..a3419b42b556 100644
--- a/cmd/acpi.c
+++ b/cmd/acpi.c
@@ -26,7 +26,7 @@ static void dump_hdr(struct acpi_table_header *hdr)
 	printf("%.*s %08lx %06x", ACPI_NAME_LEN, hdr->signature,
 	       (ulong)map_to_sysmem(hdr), hdr->length);
 	if (has_hdr) {
-		printf(" (v%02d %.6s %.8s %u %.4s %d)\n", hdr->revision,
+		printf(" (v%02d %.6s %.8s %x %.4s %x)\n", hdr->revision,
 		       hdr->oem_id, hdr->oem_table_id, hdr->oem_revision,
 		       hdr->aslc_id, hdr->aslc_revision);
 	} else {
-- 
2.28.0

[PATCH v1] cmd: acpi: Print revisions in hex format

[Wolfgang Wallner]

Hi Andy,

-----"Andy Shevchenko" <andriy.shevchenko@linux.intel.com> schrieb: -----
> Betreff: [PATCH v1] cmd: acpi: Print revisions in hex format
> 
> The revisions are usually dates in hex-decimal format representing
> YYYYmmdd. Print them in hex to see this clearly.
> 
> Before:
>   ...
>   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 538970376 INTL 0)
>   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 65536 INTL 538968870)
>   ...
> After:
>   ...
>   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 20200908 INTL 0)
>   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 10000 INTL 20200326)
>   ...
> 
> Fixes: 0b885bcfd9b0 ("acpi: Add an acpi command")
> Cc: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  cmd/acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
Tested-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
Tested on a custom Apollolake board.

Related to "acpi list":
During my recent ACPI debugging I found it very useful to have the checksum
printed for each table with "acpi list". Would there be interest to have that
upstream? If so I would send a patch.

regards, Wolfgang

[PATCH v1] cmd: acpi: Print revisions in hex format

[Andy Shevchenko]

On Tue, Sep 8, 2020 at 5:58 PM Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
> -----"Andy Shevchenko" <andriy.shevchenko@linux.intel.com> schrieb: -----
> > Betreff: [PATCH v1] cmd: acpi: Print revisions in hex format
> >
> > The revisions are usually dates in hex-decimal format representing
> > YYYYmmdd. Print them in hex to see this clearly.
> >
> > Before:
> >   ...
> >   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 538970376 INTL 0)
> >   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 65536 INTL 538968870)
> >   ...
> > After:
> >   ...
> >   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 20200908 INTL 0)
> >   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 10000 INTL 20200326)
> >   ...
> >
> > Fixes: 0b885bcfd9b0 ("acpi: Add an acpi command")
> > Cc: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  cmd/acpi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> Tested-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> Tested on a custom Apollolake board.

Thanks!

> Related to "acpi list":
> During my recent ACPI debugging I found it very useful to have the checksum
> printed for each table with "acpi list". Would there be interest to have that
> upstream? If so I would send a patch.

Can you elaborate what was the problem that checksum helped?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1] cmd: acpi: Print revisions in hex format

[Wolfgang Wallner]

Hi Andy,

-----"Andy Shevchenko" <andy.shevchenko@gmail.com> schrieb: -----
> Betreff: Re: [PATCH v1] cmd: acpi: Print revisions in hex format
> 
> On Tue, Sep 8, 2020 at 5:58 PM Wolfgang Wallner
> <wolfgang.wallner@br-automation.com> wrote:
> > -----"Andy Shevchenko" <andriy.shevchenko@linux.intel.com> schrieb: -----
> > > Betreff: [PATCH v1] cmd: acpi: Print revisions in hex format
> > >
> > > The revisions are usually dates in hex-decimal format representing
> > > YYYYmmdd. Print them in hex to see this clearly.
> > >
> > > Before:
> > >   ...
> > >   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 538970376 INTL 0)
> > >   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 65536 INTL 538968870)
> > >   ...
> > > After:
> > >   ...
> > >   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 20200908 INTL 0)
> > >   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 10000 INTL 20200326)
> > >   ...
> > >
> > > Fixes: 0b885bcfd9b0 ("acpi: Add an acpi command")
> > > Cc: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  cmd/acpi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Reviewed-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> > Tested-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> > Tested on a custom Apollolake board.
> 
> Thanks!

You're welcome.

> 
> > Related to "acpi list":
> > During my recent ACPI debugging I found it very useful to have the checksum
> > printed for each table with "acpi list". Would there be interest to have that
> > upstream? If so I would send a patch.
> 
> Can you elaborate what was the problem that checksum helped?

Sure. I saw two strange things with the ACPI checksums:

1) The DSDT length included uninitialized bytes from alignment. This is
described in the following link:

   https://lists.denx.de/pipermail/u-boot/2020-September/425378.html
   
This was the actual bug I was looking for.
   
2) acpi_create_spcr() is missing a memset(). The other acpi_create_xxxx()
functions perform a memset on their structure, acpi_create_spcr() does not
and as a result the contents of this table are party uninitialized.

I plan to send a patch for both of them.

Regards, Wolfgang

[PATCH v1] cmd: acpi: Print revisions in hex format

[Andy Shevchenko]

On Tue, Sep 08, 2020 at 05:32:08PM +0200, Wolfgang Wallner wrote:
> -----"Andy Shevchenko" <andy.shevchenko@gmail.com> schrieb: -----
> > On Tue, Sep 8, 2020 at 5:58 PM Wolfgang Wallner
> > <wolfgang.wallner@br-automation.com> wrote:
> > > -----"Andy Shevchenko" <andriy.shevchenko@linux.intel.com> schrieb: -----

...

> > > Related to "acpi list":
> > > During my recent ACPI debugging I found it very useful to have the checksum
> > > printed for each table with "acpi list". Would there be interest to have that
> > > upstream? If so I would send a patch.
> > 
> > Can you elaborate what was the problem that checksum helped?
> 
> Sure. I saw two strange things with the ACPI checksums:
> 
> 1) The DSDT length included uninitialized bytes from alignment. This is
> described in the following link:
> 
>    https://lists.denx.de/pipermail/u-boot/2020-September/425378.html
>    
> This was the actual bug I was looking for.
>    
> 2) acpi_create_spcr() is missing a memset(). The other acpi_create_xxxx()
> functions perform a memset on their structure, acpi_create_spcr() does not
> and as a result the contents of this table are party uninitialized.
> 
> I plan to send a patch for both of them.

I'm not sure I understood how checksum pointed to uninitialized data?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1] cmd: acpi: Print revisions in hex format

[Wolfgang Wallner]

Hi Andy,

-----"Andy Shevchenko" <andy.shevchenko@gmail.com> schrieb: -----
> Betreff: Re: [PATCH v1] cmd: acpi: Print revisions in hex format
> 
> On Tue, Sep 08, 2020 at 05:32:08PM +0200, Wolfgang Wallner wrote:
> > -----"Andy Shevchenko" <andy.shevchenko@gmail.com> schrieb: -----
> > > On Tue, Sep 8, 2020 at 5:58 PM Wolfgang Wallner
> > > <wolfgang.wallner@br-automation.com> wrote:
> > > > -----"Andy Shevchenko" <andriy.shevchenko@linux.intel.com> schrieb: -----
> 
> ...
> 
> > > > Related to "acpi list":
> > > > During my recent ACPI debugging I found it very useful to have the checksum
> > > > printed for each table with "acpi list". Would there be interest to have that
> > > > upstream? If so I would send a patch.
> > > 
> > > Can you elaborate what was the problem that checksum helped?
> > 
> > Sure. I saw two strange things with the ACPI checksums:
> > 
> > 1) The DSDT length included uninitialized bytes from alignment. This is
> > described in the following link:
> > 
> >    https://lists.denx.de/pipermail/u-boot/2020-September/425378.html
> >    
> > This was the actual bug I was looking for.
> >    
> > 2) acpi_create_spcr() is missing a memset(). The other acpi_create_xxxx()
> > functions perform a memset on their structure, acpi_create_spcr() does not
> > and as a result the contents of this table are party uninitialized.
> > 
> > I plan to send a patch for both of them.
> 
> I'm not sure I understood how checksum pointed to uninitialized data?

After adding the checksums to "acpi list" I realized that the checksums for
DSDT and SPCR where different after every reset. Looking at the code each
turned out to be somehow related to uninitialized memory.

regards, Wolfgang

PS: My mail client has somehow corrupted your last mail, and I can't open it.
A colleague has forwarded it to me so I could reply. This is why I reply to my
own mail.

[PATCH v1] cmd: acpi: Print revisions in hex format

[Andy Shevchenko]

On Wed, Sep 09, 2020 at 09:15:22AM +0200, Wolfgang Wallner wrote:
> -----"Andy Shevchenko" <andy.shevchenko@gmail.com> schrieb: -----
> > Betreff: Re: [PATCH v1] cmd: acpi: Print revisions in hex format
> > On Tue, Sep 08, 2020 at 05:32:08PM +0200, Wolfgang Wallner wrote:
> > > -----"Andy Shevchenko" <andy.shevchenko@gmail.com> schrieb: -----
> > > > On Tue, Sep 8, 2020 at 5:58 PM Wolfgang Wallner
> > > > <wolfgang.wallner@br-automation.com> wrote:
> > > > > -----"Andy Shevchenko" <andriy.shevchenko@linux.intel.com> schrieb: -----

...

> > > > > Related to "acpi list":
> > > > > During my recent ACPI debugging I found it very useful to have the checksum
> > > > > printed for each table with "acpi list". Would there be interest to have that
> > > > > upstream? If so I would send a patch.
> > > > 
> > > > Can you elaborate what was the problem that checksum helped?
> > > 
> > > Sure. I saw two strange things with the ACPI checksums:
> > > 
> > > 1) The DSDT length included uninitialized bytes from alignment. This is
> > > described in the following link:
> > > 
> > >    https://lists.denx.de/pipermail/u-boot/2020-September/425378.html
> > >    
> > > This was the actual bug I was looking for.
> > >    
> > > 2) acpi_create_spcr() is missing a memset(). The other acpi_create_xxxx()
> > > functions perform a memset on their structure, acpi_create_spcr() does not
> > > and as a result the contents of this table are party uninitialized.
> > > 
> > > I plan to send a patch for both of them.
> > 
> > I'm not sure I understood how checksum pointed to uninitialized data?
> 
> After adding the checksums to "acpi list" I realized that the checksums for
> DSDT and SPCR where different after every reset. Looking at the code each
> turned out to be somehow related to uninitialized memory.

I see. I would rather to have it as a separate (sub-)command. But I leave this
to Simon.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1] cmd: acpi: Print revisions in hex format

[Bin Meng]

On Tue, Sep 8, 2020 at 10:30 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> The revisions are usually dates in hex-decimal format representing
> YYYYmmdd. Print them in hex to see this clearly.
>
> Before:
>   ...
>   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 538970376 INTL 0)
>   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 65536 INTL 538968870)
>   ...
> After:
>   ...
>   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 20200908 INTL 0)
>   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 10000 INTL 20200326)
>   ...
>
> Fixes: 0b885bcfd9b0 ("acpi: Add an acpi command")
> Cc: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  cmd/acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH v1] cmd: acpi: Print revisions in hex format

[Bin Meng]

On Mon, Sep 21, 2020 at 9:31 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Tue, Sep 8, 2020 at 10:30 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > The revisions are usually dates in hex-decimal format representing
> > YYYYmmdd. Print them in hex to see this clearly.
> >
> > Before:
> >   ...
> >   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 538970376 INTL 0)
> >   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 65536 INTL 538968870)
> >   ...
> > After:
> >   ...
> >   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 20200908 INTL 0)
> >   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 10000 INTL 20200326)
> >   ...
> >
> > Fixes: 0b885bcfd9b0 ("acpi: Add an acpi command")
> > Cc: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  cmd/acpi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

applied to u-boot-x86, thanks!

[PATCH v1] cmd: acpi: Print revisions in hex format

[Bin Meng]

Hi Andy,

On Tue, Sep 8, 2020 at 10:30 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> The revisions are usually dates in hex-decimal format representing
> YYYYmmdd. Print them in hex to see this clearly.
>
> Before:
>   ...
>   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 538970376 INTL 0)
>   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 65536 INTL 538968870)
>   ...
> After:
>   ...
>   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 20200908 INTL 0)
>   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 10000 INTL 20200326)
>   ...
>
> Fixes: 0b885bcfd9b0 ("acpi: Add an acpi command")
> Cc: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  cmd/acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cmd/acpi.c b/cmd/acpi.c
> index 085a3a650d1a..a3419b42b556 100644
> --- a/cmd/acpi.c
> +++ b/cmd/acpi.c
> @@ -26,7 +26,7 @@ static void dump_hdr(struct acpi_table_header *hdr)
>         printf("%.*s %08lx %06x", ACPI_NAME_LEN, hdr->signature,
>                (ulong)map_to_sysmem(hdr), hdr->length);
>         if (has_hdr) {
> -               printf(" (v%02d %.6s %.8s %u %.4s %d)\n", hdr->revision,
> +               printf(" (v%02d %.6s %.8s %x %.4s %x)\n", hdr->revision,
>                        hdr->oem_id, hdr->oem_table_id, hdr->oem_revision,
>                        hdr->aslc_id, hdr->aslc_revision);
>         } else {
> --

This unfortunately breaks the unit test.

/u/test/dm/acpi.c:371, dm_test_acpi_cmd_list(): console:
Expected 'RSDT 0570ebf0 000030 (v01 U-BOOT U-BOOTBL 538970401 INTL 0)',
got 'RSDT 0570ebf0 000030 (v01 U-BOOT U-BOOTBL 20200921 INTL 0)'
Failures: 2

Would you please resend v2 to fix this?

Regards,
Bin

[PATCH v1] cmd: acpi: Print revisions in hex format

[Andy Shevchenko]

On Mon, Sep 21, 2020 at 10:20:18AM +0800, Bin Meng wrote:
> On Tue, Sep 8, 2020 at 10:30 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > The revisions are usually dates in hex-decimal format representing
> > YYYYmmdd. Print them in hex to see this clearly.
> >
> > Before:
> >   ...
> >   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 538970376 INTL 0)
> >   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 65536 INTL 538968870)
> >   ...
> > After:
> >   ...
> >   FACP 000e5420 0000f4 (v06 U-BOOT U-BOOTBL 20200908 INTL 0)
> >   DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 10000 INTL 20200326)
> >   ...
> >
> > Fixes: 0b885bcfd9b0 ("acpi: Add an acpi command")
> > Cc: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  cmd/acpi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/cmd/acpi.c b/cmd/acpi.c
> > index 085a3a650d1a..a3419b42b556 100644
> > --- a/cmd/acpi.c
> > +++ b/cmd/acpi.c
> > @@ -26,7 +26,7 @@ static void dump_hdr(struct acpi_table_header *hdr)
> >         printf("%.*s %08lx %06x", ACPI_NAME_LEN, hdr->signature,
> >                (ulong)map_to_sysmem(hdr), hdr->length);
> >         if (has_hdr) {
> > -               printf(" (v%02d %.6s %.8s %u %.4s %d)\n", hdr->revision,
> > +               printf(" (v%02d %.6s %.8s %x %.4s %x)\n", hdr->revision,
> >                        hdr->oem_id, hdr->oem_table_id, hdr->oem_revision,
> >                        hdr->aslc_id, hdr->aslc_revision);
> >         } else {
> > --
> 
> This unfortunately breaks the unit test.
> 
> /u/test/dm/acpi.c:371, dm_test_acpi_cmd_list(): console:
> Expected 'RSDT 0570ebf0 000030 (v01 U-BOOT U-BOOTBL 538970401 INTL 0)',
> got 'RSDT 0570ebf0 000030 (v01 U-BOOT U-BOOTBL 20200921 INTL 0)'
> Failures: 2
> 
> Would you please resend v2 to fix this?

v2 has been just sent. Thanks!

-- 
With Best Regards,
Andy Shevchenko