linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jean Delvare <jdelvare@suse.de>
To: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Len Brown <lenb@kernel.org>,
	linux-acpi@vger.kernel.org,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	Guenter Roeck <linux@roeck-us.net>,
	linux-watchdog@vger.kernel.org, Tom Abraham <tabraham@suse.com>
Subject: Re: wdat_wdt: access width inconsistency
Date: Wed, 12 Feb 2020 11:30:30 +0100	[thread overview]
Message-ID: <20200212113030.1c5c9524@endymion> (raw)
In-Reply-To: <20200210112326.GP2667@lahna.fi.intel.com>

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

  parent reply	other threads:[~2020-02-12 10:30 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-10 10:16 wdat_wdt: access width inconsistency Jean Delvare
2020-02-10 11:23 ` Mika Westerberg
2020-02-11 13:11   ` Jean Delvare
2020-02-11 13:59     ` Mika Westerberg
2020-02-11 16:25       ` Jean Delvare
2020-02-11 16:37         ` Mika Westerberg
2020-02-11 17:03           ` Jean Delvare
2020-02-12 11:05             ` [PATCH 1/3] ACPICA: Introduce ACPI_ACCESS_BIT_WIDTH() macro Mika Westerberg
2020-02-12 11:05               ` [PATCH 2/3] ACPI / watchdog: Fix gas->access_width usage Mika Westerberg
2020-02-12 11:56                 ` Jean Delvare
2020-02-12 12:10                   ` Mika Westerberg
2020-02-12 11:05               ` [PATCH 3/3] ACPI / watchdog: Set default timeout in probe Mika Westerberg
2020-02-12 12:07                 ` Jean Delvare
2020-02-12 12:13                   ` Mika Westerberg
2020-02-12 11:52               ` [PATCH 1/3] ACPICA: Introduce ACPI_ACCESS_BIT_WIDTH() macro Jean Delvare
2020-02-12 12:08                 ` Mika Westerberg
2020-02-11 16:45         ` wdat_wdt: access width inconsistency Guenter Roeck
2020-02-12 10:30   ` Jean Delvare [this message]
2020-02-12 10:47     ` Mika Westerberg
2020-02-12 11:05       ` Jean Delvare

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200212113030.1c5c9524@endymion \
    --to=jdelvare@suse.de \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=mika.westerberg@linux.intel.com \
    --cc=rjw@rjwysocki.net \
    --cc=tabraham@suse.com \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).