[PATCH v3 2/2] cmd: acpi: fix acpi list command

[Heinrich Schuchardt]

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);
> 
>