On Mon, Feb 10, 2020 at 11:16:38AM +0100, Jean Delvare wrote:
> Hi all,
Hi Jean,
> I'm still working on my customer issue where the wdat_wdt driver
> reboots the server instantly as soon as the watchdog daemon is started.
BTW, you can use "wdat_wdt.dyndbg" to debug this. It should log all the
instructions it runs.
> I looked at all the upstream fixes and we already have all relevant
> ones in our kernel so I start suspecting either a driver bug or a BIOS
> issue.
>
> While reading the driver code I noticed one suspect thing related to
> the register access width, which I'd like a second opinion on.
>
> Both acpi_watchdog.c and wdat_wdt.c contain code like:
>
> res.end = res.start + gas->access_width - 1;
>
> This suggests that gas->access_width is expected to be 4 in case of a
> 32-bit register. However in wdat_wdt_read/wdat_wdt_write we have:
>
> switch (gas->access_width) {
> (...)
> case 3:
> *value = ioread32(instr->reg);
>
> This looks inconsistent to me.
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.
However, this code in wdat_wdt.c:
r.end = r.start + gas->access_width - 1;
is not correct. In this case, I don't think it affects anything but
should still be fixed.
> My reading of the ACPI specification suggests that 3 is the right value
> for 32-bit registers. If so, then shouldn't the resource's end be set
> to:
>
> res.end = res.start + (1 << (gas->access_width - 1)) - 1;
>
> ?
Yes, I agree. It seems that we also have helper macro for this:
ACPI_ACCESS_BIT_WIDTH() that can be used as well but the result needs to
be divided by 8.
I will make a patch that fixes these later this week (quite busy with
something else right now), unless you want to do that.
