Re: [PATCH v3 2/2] cmd: acpi: fix acpi list command
On Mon, Nov 20, 2023 at 01:34:22PM +0100, Heinrich Schuchardt wrote: > On 11/20/23 12:28, Andy Shevchenko wrote: > > On Sat, Nov 18, 2023 at 11:52:48PM +0100, Heinrich Schuchardt wrote: ... > > (Side question: Do you use --histogram when preparing patches? if no, try > > it.) > > Thanks for reviewing. > > No I don't use --histogram. > > Without --histogram: > 2 files changed, 45 insertions(+), 40 deletions(-) > > With --histogram: > 2 files changed, 54 insertions(+), 49 deletions(-) > > The histogram algorithms finds less commonalities. Why should I prefer this? Readability of the patch. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 2/2] cmd: acpi: fix acpi list command
On 11/20/23 12:28, Andy Shevchenko wrote:
On Sat, Nov 18, 2023 at 11:52:48PM +0100, Heinrich Schuchardt wrote:
ACPI tables may comprise either RSDT, XSDT, or both. The current code fails
to check the presence of the RSDT table before accessing it. This leads to
an exception if the RSDT table is not provided.
The XSDT table takes precedence over the RSDT table.
The return values of list_rsdt() and list_rsdp() are always zero and not
checked. Remove the return values.
Addresses in the XSDT table are 64-bit. Adjust the output accordingly.
As the RSDT table has to be ignored if the XSDT command is present there is
no need to compare the tables in a display command. Anyway the
specification does not require that the sequence of addresses in the RSDT
and XSDT table are the same.
The FACS table header does not provide revision information. Correct the
description of dump_hdr().
Adjust the ACPI test to match the changed output format of the 'acpi list'
command.
(Side question: Do you use --histogram when preparing patches? if no, try it.)
Thanks for reviewing.
No I don't use --histogram.
Without --histogram:
2 files changed, 45 insertions(+), 40 deletions(-)
With --histogram:
2 files changed, 54 insertions(+), 49 deletions(-)
The histogram algorithms finds less commonalities. Why should I prefer this?
...
+ if (rsdp->xsdt_address) {
+ if (!xsdt->entry[i])
+ break;
+ hdr = map_sysmem(xsdt->entry[i], 0);
+ } else {
+ if (!rsdt->entry[i])
+ break;
+ hdr = map_sysmem(rsdt->entry[i], 0);
+ }
With a help of temporary variable this can be rewritten as
tmp = 0; // or NULL, I haven't checked the type.
The type would have to be u64. An assignment would not be needed here as
none of the code paths below uses it.
Best regards
Heinrich
if (rsdp->xsdt_address)
tmp = xsdt->entry[i];
else
tmp = rsdt->entry[i];
if (!tmp)
break;
hdr = map_sysmem(tmp, 0);
Re: [PATCH v3 2/2] cmd: acpi: fix acpi list command
On Sat, Nov 18, 2023 at 11:52:48PM +0100, Heinrich Schuchardt wrote:
> ACPI tables may comprise either RSDT, XSDT, or both. The current code fails
> to check the presence of the RSDT table before accessing it. This leads to
> an exception if the RSDT table is not provided.
>
> The XSDT table takes precedence over the RSDT table.
>
> The return values of list_rsdt() and list_rsdp() are always zero and not
> checked. Remove the return values.
>
> Addresses in the XSDT table are 64-bit. Adjust the output accordingly.
>
> As the RSDT table has to be ignored if the XSDT command is present there is
> no need to compare the tables in a display command. Anyway the
> specification does not require that the sequence of addresses in the RSDT
> and XSDT table are the same.
>
> The FACS table header does not provide revision information. Correct the
> description of dump_hdr().
>
> Adjust the ACPI test to match the changed output format of the 'acpi list'
> command.
(Side question: Do you use --histogram when preparing patches? if no, try it.)
...
> + if (rsdp->xsdt_address) {
> + if (!xsdt->entry[i])
> + break;
> + hdr = map_sysmem(xsdt->entry[i], 0);
> + } else {
> + if (!rsdt->entry[i])
> + break;
> + hdr = map_sysmem(rsdt->entry[i], 0);
> + }
With a help of temporary variable this can be rewritten as
tmp = 0; // or NULL, I haven't checked the type.
if (rsdp->xsdt_address)
tmp = xsdt->entry[i];
else
tmp = rsdt->entry[i];
if (!tmp)
break;
hdr = map_sysmem(tmp, 0);
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 2/2] cmd: acpi: fix acpi list command
On Sat, 18 Nov 2023 at 15:54, Heinrich Schuchardt wrote: > > ACPI tables may comprise either RSDT, XSDT, or both. The current code fails > to check the presence of the RSDT table before accessing it. This leads to > an exception if the RSDT table is not provided. > > The XSDT table takes precedence over the RSDT table. > > The return values of list_rsdt() and list_rsdp() are always zero and not > checked. Remove the return values. > > Addresses in the XSDT table are 64-bit. Adjust the output accordingly. > > As the RSDT table has to be ignored if the XSDT command is present there is > no need to compare the tables in a display command. Anyway the > specification does not require that the sequence of addresses in the RSDT > and XSDT table are the same. > > The FACS table header does not provide revision information. Correct the > description of dump_hdr(). > > Adjust the ACPI test to match the changed output format of the 'acpi list' > command. > > Signed-off-by: Heinrich Schuchardt > --- > v3: > consider map_sysmem(0, 0) != NULL on the sandbox > adjust signature of list_rsdt() and list_rsdp() > v2: > add unit test for offset of field Entry in RSDT, XSDT > merge patch 2 and 3 > remove leading zeros from address output > --- > cmd/acpi.c | 67 +++--- > test/dm/acpi.c | 18 +++--- > 2 files changed, 45 insertions(+), 40 deletions(-) Reviewed-by: Simon Glass
