| Message ID | 20200212110540.83559-2-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/3] ACPICA: Introduce ACPI_ACCESS_BIT_WIDTH() macro | expand |
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().
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.
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) {
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(-)