Hi again Mika,
On Mon, 10 Feb 2020 13:23:26 +0200, Mika Westerberg wrote:
> I think you are right. For the code in acpi_watchdog.c:
>
> if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> res.flags = IORESOURCE_MEM;
> res.end = res.start + ALIGN(gas->access_width, 4) - 1;
> } else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> res.flags = IORESOURCE_IO;
> res.end = res.start + gas->access_width - 1;
> } else {
> ..
>
> I think it does the "correct" thing, although it is bit convoluted. The
> first one aligns it to 4 and the I/O access is either 8- or 16-bits so
> it should be fine, unless I'm missing something.
I'm looking again into this today. What was the rationale for the
ALIGN() in the first place? The WDAT table is supposed to declare the
resources with the appropriate width so it should not set access_width
= 1 or 2 if the register should be accessed with 32-bit memory
reads/writes, right? Could it be that the ALIGN() was added to solve
the bug caused by using access_width directly instead of converting it
to a byte count first?
Or is the ALIGN() a safety guard against broken WDAT tables? I'm not
sure what bad would happen from doing memory-mapped reads/writes of
less than 32 bits, so I'm really wondering why the ALIGN() is there.
Especially when the code in wdat_wdt itself doesn't align anything, so
it's only about the resource size really. Requesting a resource larger
than we need doesn't make a lot of sense to me.
(The underlying question being: can I get rid of that ALIGN()
altogether while fixing the gas->access_width misuse bug?)
--
Jean Delvare
SUSE L3 Support
