wdat_wdt: access width inconsistency

wdat_wdt: access width inconsistency

[Jean Delvare]

Hi all,

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.
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.

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;

?

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: wdat_wdt: access width inconsistency

[Mika Westerberg]

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.

Re: wdat_wdt: access width inconsistency

[Jean Delvare]

Hi Mika,

On Mon, 10 Feb 2020 13:23:26 +0200, Mika Westerberg wrote:
> On Mon, Feb 10, 2020 at 11:16:38AM +0100, Jean Delvare wrote:
> > 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.

Thanks for the suggestion. That's not going to help much when the
system reboots instantly as soon as the device node is accessed, but
maybe it will bring some light about what happens before that point.

> > 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.

OK, thanks for confirming.

> > 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 looked for that macro but my grep wouldn't find it because I expected
a value in bytes not bits. Thanks for pointing it out.

Maybe we should introduce ACPI_ACCESS_BYTE_WIDTH() for the purpose?
This would make the code less convoluted than ACPI_ACCESS_BIT_WIDTH() /
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.

I'll write the patch, no problem.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: wdat_wdt: access width inconsistency

[Mika Westerberg]

On Tue, Feb 11, 2020 at 02:11:47PM +0100, Jean Delvare wrote:
> Hi Mika,
> 
> On Mon, 10 Feb 2020 13:23:26 +0200, Mika Westerberg wrote:
> > On Mon, Feb 10, 2020 at 11:16:38AM +0100, Jean Delvare wrote:
> > > 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.
> 
> Thanks for the suggestion. That's not going to help much when the
> system reboots instantly as soon as the device node is accessed, but
> maybe it will bring some light about what happens before that point.

If the default timeout is short then that might happen but I think WDAT
spec had some "reasonable" lower limit.

You may set bigger default timeout during the probe by doing something
like below. This should give some 30s time before the system is rebooted
after the device is opened.

diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index b069349b52f5..24351afe2718 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -439,6 +439,10 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, wdat);
 
 	watchdog_set_nowayout(&wdat->wdd, nowayout);
+
+	/* Increase default timeout */
+	wdat_wdt_set_timeout(&wdat->wdd, 30 * 1000);
+
 	return devm_watchdog_register_device(dev, &wdat->wdd);
 }
 
> > > 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.
> 
> OK, thanks for confirming.
> 
> > > 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 looked for that macro but my grep wouldn't find it because I expected
> a value in bytes not bits. Thanks for pointing it out.
> 
> Maybe we should introduce ACPI_ACCESS_BYTE_WIDTH() for the purpose?
> This would make the code less convoluted than ACPI_ACCESS_BIT_WIDTH() /
> 8.

Good idea.

> > I will make a patch that fixes these later this week (quite busy with
> > something else right now), unless you want to do that.
> 
> I'll write the patch, no problem.

Thanks!

Re: wdat_wdt: access width inconsistency

[Jean Delvare]

On Tue, 11 Feb 2020 15:59:44 +0200, Mika Westerberg wrote:
> If the default timeout is short then that might happen but I think WDAT
> spec had some "reasonable" lower limit.

Could you please point me to the WDAT specification? Somehow my web
search failed to spot it.

> You may set bigger default timeout during the probe by doing something
> like below. This should give some 30s time before the system is rebooted
> after the device is opened.
> 
> diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> index b069349b52f5..24351afe2718 100644
> --- a/drivers/watchdog/wdat_wdt.c
> +++ b/drivers/watchdog/wdat_wdt.c
> @@ -439,6 +439,10 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, wdat);
>  
>  	watchdog_set_nowayout(&wdat->wdd, nowayout);
> +
> +	/* Increase default timeout */
> +	wdat_wdt_set_timeout(&wdat->wdd, 30 * 1000);
> +
>  	return devm_watchdog_register_device(dev, &wdat->wdd);
>  }

That is a very valid point. We know that the device works fine when
using the iTCO_wdt driver, and the iTCO_wdt driver *does* set a timeout
value at probe time, it does not rely on the BIOS having set a sane
value beforehand. So maybe that's the problem.

Guenter, what is considered best practice for watchdog drivers in this
respect? Trust the BIOS or set an arbitrary timeout value?

Maybe we should read the timeout value before enabling the device, and
if it is unreasonably low (< 5 seconds), log a warning and reset the
value to a sane default (30 seconds)?

Alternatively, or in addition to that, maybe the wdat_wdt driver should
have a module parameter to set the default timeout value, as the
iTCO_wdt driver has? Or is this deprecated in favor of the
WDIOC_SETTIMEOUT ioctl? Problem with the ioctl is that it requires the
device node to be opened, which starts the count down (I think?)

-- 
Jean Delvare
SUSE L3 Support

Re: wdat_wdt: access width inconsistency

[Mika Westerberg]

On Tue, Feb 11, 2020 at 05:25:33PM +0100, Jean Delvare wrote:
> On Tue, 11 Feb 2020 15:59:44 +0200, Mika Westerberg wrote:
> > If the default timeout is short then that might happen but I think WDAT
> > spec had some "reasonable" lower limit.
> 
> Could you please point me to the WDAT specification? Somehow my web
> search failed to spot it.

You can find it here:

  http://msdn.microsoft.com/en-us/windows/hardware/gg463320.aspx

Most of the ACPI related documents not part of the spec itself are
listed in the following page:

  https://uefi.org/acpi

> 
> > You may set bigger default timeout during the probe by doing something
> > like below. This should give some 30s time before the system is rebooted
> > after the device is opened.
> > 
> > diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> > index b069349b52f5..24351afe2718 100644
> > --- a/drivers/watchdog/wdat_wdt.c
> > +++ b/drivers/watchdog/wdat_wdt.c
> > @@ -439,6 +439,10 @@ static int wdat_wdt_probe(struct platform_device *pdev)
> >  	platform_set_drvdata(pdev, wdat);
> >  
> >  	watchdog_set_nowayout(&wdat->wdd, nowayout);
> > +
> > +	/* Increase default timeout */
> > +	wdat_wdt_set_timeout(&wdat->wdd, 30 * 1000);
> > +
> >  	return devm_watchdog_register_device(dev, &wdat->wdd);
> >  }
> 
> That is a very valid point. We know that the device works fine when
> using the iTCO_wdt driver, and the iTCO_wdt driver *does* set a timeout
> value at probe time, it does not rely on the BIOS having set a sane
> value beforehand. So maybe that's the problem.
> 
> Guenter, what is considered best practice for watchdog drivers in this
> respect? Trust the BIOS or set an arbitrary timeout value?
> 
> Maybe we should read the timeout value before enabling the device, and
> if it is unreasonably low (< 5 seconds), log a warning and reset the
> value to a sane default (30 seconds)?

Yes, that would work.

> Alternatively, or in addition to that, maybe the wdat_wdt driver should
> have a module parameter to set the default timeout value, as the
> iTCO_wdt driver has? Or is this deprecated in favor of the
> WDIOC_SETTIMEOUT ioctl? Problem with the ioctl is that it requires the
> device node to be opened, which starts the count down (I think?)

Indeed it seems to be the case if I understand watchdog core code
correctly.

Re: wdat_wdt: access width inconsistency

[Jean Delvare]

On Tue, 11 Feb 2020 18:37:53 +0200, Mika Westerberg wrote:
> On Tue, Feb 11, 2020 at 05:25:33PM +0100, Jean Delvare wrote:
> > On Tue, 11 Feb 2020 15:59:44 +0200, Mika Westerberg wrote:  
> > > If the default timeout is short then that might happen but I think WDAT
> > > spec had some "reasonable" lower limit.  
> > 
> > Could you please point me to the WDAT specification? Somehow my web
> > search failed to spot it.  
> 
> You can find it here:
> 
>   http://msdn.microsoft.com/en-us/windows/hardware/gg463320.aspx
> 
> Most of the ACPI related documents not part of the spec itself are
> listed in the following page:
> 
>   https://uefi.org/acpi

Great, thanks for the info.

As I read the specification, it is mandatory to have a timeout >= 5
minutes *if* the watchdog is enabled at boot time. Otherwise the 5
minutes is only a recommendation. I wouldn't be surprised if some
hardware vendors do not initialize the timeout value and assume the OS
will do it for them. Odds are that Windows does that.

Thanks again,
-- 
Jean Delvare
SUSE L3 Support

[PATCH 1/3] ACPICA: Introduce ACPI_ACCESS_BIT_WIDTH() macro

[Mika Westerberg]

Sometimes it is useful to find the access_width field value in bytes and
not in bits so add a helper that can be used for this purpose.

Suggested-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 include/acpi/actypes.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index a2583c2bc054..77d40b02f62a 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -537,6 +537,7 @@ typedef u64 acpi_integer;
  * struct acpi_resource_generic_register.
  */
 #define ACPI_ACCESS_BIT_WIDTH(size)     (1 << ((size) + 2))
+#define ACPI_ACCESS_BYTE_WIDTH(size)    (ACPI_ACCESS_BIT_WIDTH(size) / 8)
 
 /*******************************************************************************
  *
-- 
2.25.0

[PATCH 2/3] ACPI / watchdog: Fix gas->access_width usage

[Mika Westerberg]

ACPI Generic Address Structure (GAS) access_width field is not in bytes
as the driver seems to expect in few places so fix this by using the
newly introduced macro ACPI_ACCESS_BIT_WIDTH().

Reported-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Not sure if this is stable material since there is no user visible issues
without the fix. If this needs to go stable then the ACPICA change need to
be taken there as well (or we make separate fix for stable without the
macro).

 drivers/acpi/acpi_watchdog.c | 3 +--
 drivers/watchdog/wdat_wdt.c  | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
index b5516b04ffc0..ef0832999892 100644
--- a/drivers/acpi/acpi_watchdog.c
+++ b/drivers/acpi/acpi_watchdog.c
@@ -126,12 +126,11 @@ void __init acpi_watchdog_init(void)
 		gas = &entries[i].register_region;
 
 		res.start = gas->address;
+		res.end = res.start + ACPI_ACCESS_BIT_WIDTH(gas->access_width) - 1;
 		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 {
 			pr_warn("Unsupported address space: %u\n",
 				gas->space_id);
diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index b069349b52f5..2132018f031d 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -389,7 +389,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 
 		memset(&r, 0, sizeof(r));
 		r.start = gas->address;
-		r.end = r.start + gas->access_width - 1;
+		r.end = r.start + ACPI_ACCESS_BIT_WIDTH(gas->access_width) - 1;
 		if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
 			r.flags = IORESOURCE_MEM;
 		} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
-- 
2.25.0

Re: [PATCH 2/3] ACPI / watchdog: Fix gas->access_width usage

[Jean Delvare]

On Wed, 12 Feb 2020 14:05:39 +0300, Mika Westerberg wrote:
> ACPI Generic Address Structure (GAS) access_width field is not in bytes
> as the driver seems to expect in few places so fix this by using the
> newly introduced macro ACPI_ACCESS_BIT_WIDTH().
> 
> Reported-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Not sure if this is stable material since there is no user visible issues
> without the fix. If this needs to go stable then the ACPICA change need to
> be taken there as well (or we make separate fix for stable without the
> macro).

I thought about it too and I don't think it qualifies for stable
because we have no proof it affects any user in pratcice. You may want
to add a "Fixes:" tag though to track where the bug was introduced and
let distributions backport the fix if they want to.

> 
>  drivers/acpi/acpi_watchdog.c | 3 +--
>  drivers/watchdog/wdat_wdt.c  | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
> index b5516b04ffc0..ef0832999892 100644
> --- a/drivers/acpi/acpi_watchdog.c
> +++ b/drivers/acpi/acpi_watchdog.c
> @@ -126,12 +126,11 @@ void __init acpi_watchdog_init(void)
>  		gas = &entries[i].register_region;
>  
>  		res.start = gas->address;
> +		res.end = res.start + ACPI_ACCESS_BIT_WIDTH(gas->access_width) - 1;
>  		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 {
>  			pr_warn("Unsupported address space: %u\n",
>  				gas->space_id);
> diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> index b069349b52f5..2132018f031d 100644
> --- a/drivers/watchdog/wdat_wdt.c
> +++ b/drivers/watchdog/wdat_wdt.c
> @@ -389,7 +389,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>  
>  		memset(&r, 0, sizeof(r));
>  		r.start = gas->address;
> -		r.end = r.start + gas->access_width - 1;
> +		r.end = r.start + ACPI_ACCESS_BIT_WIDTH(gas->access_width) - 1;
>  		if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>  			r.flags = IORESOURCE_MEM;
>  		} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {

You are using ACPI_ACCESS_BIT_WIDTH() where you clearly meant to use
ACPI_ACCESS_BYTE_WIDTH().

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/3] ACPI / watchdog: Fix gas->access_width usage

[Mika Westerberg]

On Wed, Feb 12, 2020 at 12:56:05PM +0100, Jean Delvare wrote:
> On Wed, 12 Feb 2020 14:05:39 +0300, Mika Westerberg wrote:
> > ACPI Generic Address Structure (GAS) access_width field is not in bytes
> > as the driver seems to expect in few places so fix this by using the
> > newly introduced macro ACPI_ACCESS_BIT_WIDTH().
> > 
> > Reported-by: Jean Delvare <jdelvare@suse.de>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> > Not sure if this is stable material since there is no user visible issues
> > without the fix. If this needs to go stable then the ACPICA change need to
> > be taken there as well (or we make separate fix for stable without the
> > macro).
> 
> I thought about it too and I don't think it qualifies for stable
> because we have no proof it affects any user in pratcice. You may want
> to add a "Fixes:" tag though to track where the bug was introduced and
> let distributions backport the fix if they want to.

OK.

> >  drivers/acpi/acpi_watchdog.c | 3 +--
> >  drivers/watchdog/wdat_wdt.c  | 2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
> > index b5516b04ffc0..ef0832999892 100644
> > --- a/drivers/acpi/acpi_watchdog.c
> > +++ b/drivers/acpi/acpi_watchdog.c
> > @@ -126,12 +126,11 @@ void __init acpi_watchdog_init(void)
> >  		gas = &entries[i].register_region;
> >  
> >  		res.start = gas->address;
> > +		res.end = res.start + ACPI_ACCESS_BIT_WIDTH(gas->access_width) - 1;
> >  		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 {
> >  			pr_warn("Unsupported address space: %u\n",
> >  				gas->space_id);
> > diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> > index b069349b52f5..2132018f031d 100644
> > --- a/drivers/watchdog/wdat_wdt.c
> > +++ b/drivers/watchdog/wdat_wdt.c
> > @@ -389,7 +389,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
> >  
> >  		memset(&r, 0, sizeof(r));
> >  		r.start = gas->address;
> > -		r.end = r.start + gas->access_width - 1;
> > +		r.end = r.start + ACPI_ACCESS_BIT_WIDTH(gas->access_width) - 1;
> >  		if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> >  			r.flags = IORESOURCE_MEM;
> >  		} else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> 
> You are using ACPI_ACCESS_BIT_WIDTH() where you clearly meant to use
> ACPI_ACCESS_BYTE_WIDTH().

Oops! Indeed it was supposed to be ACPI_ACCESS_BYTE_WIDTH(). I'll fix
this and the other comments in v2.

[PATCH 3/3] ACPI / watchdog: Set default timeout in probe

[Mika Westerberg]

If the BIOS default timeout for the watchdog is too small userspace may
not have enough time to configure new timeout after opening the device
before the system is already reset. For this reason program default
timeout of 30 seconds in the driver probe and allow userspace to change
this from command line or through module parameter (wdat_wdt.timeout).

Reported-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/watchdog/wdat_wdt.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index 2132018f031d..7b0257163522 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -54,6 +54,13 @@ module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
+#define WDAT_DEFAULT_TIMEOUT	30
+
+static int timeout = WDAT_DEFAULT_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
+		 __MODULE_STRING(WDAT_DEFAULT_TIMEOUT) ")");
+
 static int wdat_wdt_read(struct wdat_wdt *wdat,
 	 const struct wdat_instruction *instr, u32 *value)
 {
@@ -308,6 +315,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	const struct acpi_wdat_entry *entries;
 	const struct acpi_table_wdat *tbl;
+	int default_timeout = timeout;
 	struct wdat_wdt *wdat;
 	struct resource *res;
 	void __iomem **regs;
@@ -438,6 +446,22 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, wdat);
 
+	/*
+	 * Set initial timeout so that userspace has time to configure
+	 * the watchdog properly after it has opened the device. In some
+	 * cases the BIOS default is too short and causes immediate reboot.
+	 */
+	default_timeout = timeout;
+	if (timeout < wdat->wdd.min_hw_heartbeat_ms ||
+	    timeout > wdat->wdd.max_hw_heartbeat_ms)
+		default_timeout = WDAT_DEFAULT_TIMEOUT;
+	else
+		default_timeout = timeout;
+
+	ret = wdat_wdt_set_timeout(&wdat->wdd, timeout);
+	if (ret)
+		return ret;
+
 	watchdog_set_nowayout(&wdat->wdd, nowayout);
 	return devm_watchdog_register_device(dev, &wdat->wdd);
 }
-- 
2.25.0

Re: [PATCH 3/3] ACPI / watchdog: Set default timeout in probe

[Jean Delvare]

On Wed, 12 Feb 2020 14:05:40 +0300, Mika Westerberg wrote:
> If the BIOS default timeout for the watchdog is too small userspace may
> not have enough time to configure new timeout after opening the device
> before the system is already reset. For this reason program default
> timeout of 30 seconds in the driver probe and allow userspace to change
> this from command line or through module parameter (wdat_wdt.timeout).
> 
> Reported-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/watchdog/wdat_wdt.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> index 2132018f031d..7b0257163522 100644
> --- a/drivers/watchdog/wdat_wdt.c
> +++ b/drivers/watchdog/wdat_wdt.c
> @@ -54,6 +54,13 @@ module_param(nowayout, bool, 0);
>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>  		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
> +#define WDAT_DEFAULT_TIMEOUT	30
> +
> +static int timeout = WDAT_DEFAULT_TIMEOUT;
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
> +		 __MODULE_STRING(WDAT_DEFAULT_TIMEOUT) ")");
> +
>  static int wdat_wdt_read(struct wdat_wdt *wdat,
>  	 const struct wdat_instruction *instr, u32 *value)
>  {
> @@ -308,6 +315,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	const struct acpi_wdat_entry *entries;
>  	const struct acpi_table_wdat *tbl;
> +	int default_timeout = timeout;
>  	struct wdat_wdt *wdat;
>  	struct resource *res;
>  	void __iomem **regs;
> @@ -438,6 +446,22 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, wdat);
>  
> +	/*
> +	 * Set initial timeout so that userspace has time to configure
> +	 * the watchdog properly after it has opened the device. In some
> +	 * cases the BIOS default is too short and causes immediate reboot.
> +	 */
> +	default_timeout = timeout;

You have already done that at variable declaration time.

> +	if (timeout < wdat->wdd.min_hw_heartbeat_ms ||
> +	    timeout > wdat->wdd.max_hw_heartbeat_ms)

Comparing seconds to milliseconds is unlikely to give the expected
result.

> +		default_timeout = WDAT_DEFAULT_TIMEOUT;
> +	else
> +		default_timeout = timeout;

You have already done that twice ;-)

> +
> +	ret = wdat_wdt_set_timeout(&wdat->wdd, timeout);

You must pass "default_timeout" here, not "timeout", else the check
before serves no purpose. It might be less confusing to not introduce a
separate variable and just tweak timeout in place?

> +	if (ret)
> +		return ret;
> +
>  	watchdog_set_nowayout(&wdat->wdd, nowayout);
>  	return devm_watchdog_register_device(dev, &wdat->wdd);
>  }


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/3] ACPI / watchdog: Set default timeout in probe

[Mika Westerberg]

On Wed, Feb 12, 2020 at 01:07:01PM +0100, Jean Delvare wrote:
> On Wed, 12 Feb 2020 14:05:40 +0300, Mika Westerberg wrote:
> > If the BIOS default timeout for the watchdog is too small userspace may
> > not have enough time to configure new timeout after opening the device
> > before the system is already reset. For this reason program default
> > timeout of 30 seconds in the driver probe and allow userspace to change
> > this from command line or through module parameter (wdat_wdt.timeout).
> > 
> > Reported-by: Jean Delvare <jdelvare@suse.de>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/watchdog/wdat_wdt.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> > index 2132018f031d..7b0257163522 100644
> > --- a/drivers/watchdog/wdat_wdt.c
> > +++ b/drivers/watchdog/wdat_wdt.c
> > @@ -54,6 +54,13 @@ module_param(nowayout, bool, 0);
> >  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> >  		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> >  
> > +#define WDAT_DEFAULT_TIMEOUT	30
> > +
> > +static int timeout = WDAT_DEFAULT_TIMEOUT;
> > +module_param(timeout, int, 0);
> > +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
> > +		 __MODULE_STRING(WDAT_DEFAULT_TIMEOUT) ")");
> > +
> >  static int wdat_wdt_read(struct wdat_wdt *wdat,
> >  	 const struct wdat_instruction *instr, u32 *value)
> >  {
> > @@ -308,6 +315,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
> >  	struct device *dev = &pdev->dev;
> >  	const struct acpi_wdat_entry *entries;
> >  	const struct acpi_table_wdat *tbl;
> > +	int default_timeout = timeout;
> >  	struct wdat_wdt *wdat;
> >  	struct resource *res;
> >  	void __iomem **regs;
> > @@ -438,6 +446,22 @@ static int wdat_wdt_probe(struct platform_device *pdev)
> >  
> >  	platform_set_drvdata(pdev, wdat);
> >  
> > +	/*
> > +	 * Set initial timeout so that userspace has time to configure
> > +	 * the watchdog properly after it has opened the device. In some
> > +	 * cases the BIOS default is too short and causes immediate reboot.
> > +	 */
> > +	default_timeout = timeout;
> 
> You have already done that at variable declaration time.
> 
> > +	if (timeout < wdat->wdd.min_hw_heartbeat_ms ||
> > +	    timeout > wdat->wdd.max_hw_heartbeat_ms)
> 
> Comparing seconds to milliseconds is unlikely to give the expected
> result.
> 
> > +		default_timeout = WDAT_DEFAULT_TIMEOUT;
> > +	else
> > +		default_timeout = timeout;
> 
> You have already done that twice ;-)
> 
> > +
> > +	ret = wdat_wdt_set_timeout(&wdat->wdd, timeout);
> 
> You must pass "default_timeout" here, not "timeout", else the check
> before serves no purpose. It might be less confusing to not introduce a
> separate variable and just tweak timeout in place?

Heh, looks like this whole patch series is completely broken. Thanks for
taking time to review it and spotting all those errors. I'll fix all
these up in v2.

Sorry about this mess.

Re: [PATCH 1/3] ACPICA: Introduce ACPI_ACCESS_BIT_WIDTH() macro

[Jean Delvare]

On Wed, 12 Feb 2020 14:05:38 +0300, Mika Westerberg wrote:
> Sometimes it is useful to find the access_width field value in bytes and
> not in bits so add a helper that can be used for this purpose.

s/ACPI_ACCESS_BIT_WIDTH/ACPI_ACCESS_BYTE_WIDTH/ in the subject.

> 
> Suggested-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  include/acpi/actypes.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index a2583c2bc054..77d40b02f62a 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -537,6 +537,7 @@ typedef u64 acpi_integer;
>   * struct acpi_resource_generic_register.
>   */
>  #define ACPI_ACCESS_BIT_WIDTH(size)     (1 << ((size) + 2))
> +#define ACPI_ACCESS_BYTE_WIDTH(size)    (ACPI_ACCESS_BIT_WIDTH(size) / 8)

One of the points of having this macro being to avoid needless math,
I'd rather do:

#define ACPI_ACCESS_BYTE_WIDTH(size)    (1 << ((size) - 1))

Some compilers might be able to optimize it, but maybe not all, and I
see little point in giving the compiler more work anyway when it can be
easily avoided.

You may also want to replace "bit" by "bit or byte in the comment right
before the macros.

>  
>  /*******************************************************************************
>   *

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3] ACPICA: Introduce ACPI_ACCESS_BIT_WIDTH() macro

[Mika Westerberg]

On Wed, Feb 12, 2020 at 12:52:44PM +0100, Jean Delvare wrote:
> On Wed, 12 Feb 2020 14:05:38 +0300, Mika Westerberg wrote:
> > Sometimes it is useful to find the access_width field value in bytes and
> > not in bits so add a helper that can be used for this purpose.
> 
> s/ACPI_ACCESS_BIT_WIDTH/ACPI_ACCESS_BYTE_WIDTH/ in the subject.

Indeed.

> > Suggested-by: Jean Delvare <jdelvare@suse.de>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  include/acpi/actypes.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> > index a2583c2bc054..77d40b02f62a 100644
> > --- a/include/acpi/actypes.h
> > +++ b/include/acpi/actypes.h
> > @@ -537,6 +537,7 @@ typedef u64 acpi_integer;
> >   * struct acpi_resource_generic_register.
> >   */
> >  #define ACPI_ACCESS_BIT_WIDTH(size)     (1 << ((size) + 2))
> > +#define ACPI_ACCESS_BYTE_WIDTH(size)    (ACPI_ACCESS_BIT_WIDTH(size) / 8)
> 
> One of the points of having this macro being to avoid needless math,
> I'd rather do:
> 
> #define ACPI_ACCESS_BYTE_WIDTH(size)    (1 << ((size) - 1))
> 
> Some compilers might be able to optimize it, but maybe not all, and I
> see little point in giving the compiler more work anyway when it can be
> easily avoided.
> 
> You may also want to replace "bit" by "bit or byte in the comment right
> before the macros.

OK, I'll do that.

> >  
> >  /*******************************************************************************
> >   *
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>

Thanks!

Re: wdat_wdt: access width inconsistency

[Guenter Roeck]

On Tue, Feb 11, 2020 at 05:25:33PM +0100, Jean Delvare wrote:
> On Tue, 11 Feb 2020 15:59:44 +0200, Mika Westerberg wrote:
> > If the default timeout is short then that might happen but I think WDAT
> > spec had some "reasonable" lower limit.
> 
> Could you please point me to the WDAT specification? Somehow my web
> search failed to spot it.
> 
> > You may set bigger default timeout during the probe by doing something
> > like below. This should give some 30s time before the system is rebooted
> > after the device is opened.
> > 
> > diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> > index b069349b52f5..24351afe2718 100644
> > --- a/drivers/watchdog/wdat_wdt.c
> > +++ b/drivers/watchdog/wdat_wdt.c
> > @@ -439,6 +439,10 @@ static int wdat_wdt_probe(struct platform_device *pdev)
> >  	platform_set_drvdata(pdev, wdat);
> >  
> >  	watchdog_set_nowayout(&wdat->wdd, nowayout);
> > +
> > +	/* Increase default timeout */
> > +	wdat_wdt_set_timeout(&wdat->wdd, 30 * 1000);
> > +
> >  	return devm_watchdog_register_device(dev, &wdat->wdd);
> >  }
> 
> That is a very valid point. We know that the device works fine when
> using the iTCO_wdt driver, and the iTCO_wdt driver *does* set a timeout
> value at probe time, it does not rely on the BIOS having set a sane
> value beforehand. So maybe that's the problem.
> 
> Guenter, what is considered best practice for watchdog drivers in this
> respect? Trust the BIOS or set an arbitrary timeout value?
> 
Drivers should either set a valid timeout or read the timeout from
the chip. That does not mean that they always do that. Either case,
trusting the BIOS/ROMMON is not a good idea, because it may change.
When enabling the watchdog, the driver should make sure that the chip
timeout matches what it believes it should be, and that the watchdog
doesn't trigger immediately but only after that timeout period
expires. Not all drivers do that either.

> Maybe we should read the timeout value before enabling the device, and
> if it is unreasonably low (< 5 seconds), log a warning and reset the
> value to a sane default (30 seconds)?
> 
> Alternatively, or in addition to that, maybe the wdat_wdt driver should
> have a module parameter to set the default timeout value, as the
> iTCO_wdt driver has? Or is this deprecated in favor of the
> WDIOC_SETTIMEOUT ioctl? Problem with the ioctl is that it requires the
> device node to be opened, which starts the count down (I think?)
> 
No, the module parameter hasn't been deprecated.

Guenter

Re: wdat_wdt: access width inconsistency

[Jean Delvare]

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

Re: wdat_wdt: access width inconsistency

[Mika Westerberg]

On Wed, Feb 12, 2020 at 11:30:30AM +0100, Jean Delvare wrote:
> 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?)

I think the ALIGN() was there just because I did not realize that
access_width is 3 and not 4 for 32-bit memory. So it is not needed.

I actually have a patch series that should fix this and the other issue
you found (I found a couple of spare cycles in the morning) so if you
don't mind I'll submit them soon.

Re: wdat_wdt: access width inconsistency

[Jean Delvare]

On Wed, 12 Feb 2020 12:47:47 +0200, Mika Westerberg wrote:
> On Wed, Feb 12, 2020 at 11:30:30AM +0100, Jean Delvare wrote:
> > (The underlying question being: can I get rid of that ALIGN()
> > altogether while fixing the gas->access_width misuse bug?)  
> 
> I think the ALIGN() was there just because I did not realize that
> access_width is 3 and not 4 for 32-bit memory. So it is not needed.
> 
> I actually have a patch series that should fix this and the other issue
> you found (I found a couple of spare cycles in the morning) so if you
> don't mind I'll submit them soon.

Sure please do, I don't care about ownership, I only want these bugs to
be fixed. I got myself side-tracked by another issue at work this
morning.

I'll be happy to review your changes.

-- 
Jean Delvare
SUSE L3 Support