Re: wdat_wdt: access width inconsistency

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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.