| Message ID | 20200908143025.86976-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | [v1] cmd: acpi: Print revisions in hex format | expand |
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
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?
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
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?
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.
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.
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>
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!
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
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!
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 {
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(-)