Skip to content

Commit

Permalink
ACPI / PNP: Reserve ACPI resources at the fs_initcall_sync stage
Browse files Browse the repository at this point in the history 
This effectively reverts the following three commits:

 7bc1038 ACPI / resources: free memory on error in add_region_before()
 0f1b414 ACPI / PNP: Avoid conflicting resource reservations
 b9a5e5e ACPI / init: Fix the ordering of acpi_reserve_resources()

(commit b9a5e5e introduced regressions some of which, but not
all, were addressed by commit 0f1b414 and commit 7bc1038
was a fixup on top of the latter) and causes ACPI fixed hardware
resources to be reserved at the fs_initcall_sync stage of system
initialization.

The story is as follows.  First, a boot regression was reported due
to an apparent resource reservation ordering change after a commit
that shouldn't lead to such changes.  Investigation led to the
conclusion that the problem happened because acpi_reserve_resources()
was executed at the device_initcall() stage of system initialization
which wasn't strictly ordered with respect to driver initialization
(and with respect to the initialization of the pcieport driver in
particular), so a random change causing the device initcalls to be
run in a different order might break things.

The response to that was to attempt to run acpi_reserve_resources()
as soon as we knew that ACPI would be in use (commit b9a5e5e).
However, that turned out to be too early, because it caused resource
reservations made by the PNP system driver to fail on at least one
system and that failure was addressed by commit 0f1b414.

That fix still turned out to be insufficient, though, because
calling acpi_reserve_resources() before the fs_initcall stage of
system initialization caused a boot regression to happen on the
eCAFE EC-800-H20G/S netbook.  That meant that we only could call
acpi_reserve_resources() at the fs_initcall initialization stage
or later, but then we might just as well call it after the PNP
initalization in which case commit 0f1b414 wouldn't be
necessary any more.

For this reason, the changes made by commit 0f1b414 are reverted
(along with a memory leak fixup on top of that commit), the changes
made by commit b9a5e5e that went too far are reverted too and
acpi_reserve_resources() is changed into fs_initcall_sync, which
will cause it to be executed after the PNP subsystem initialization
(which is an fs_initcall) and before device initcalls (including
the pcieport driver initialization) which should avoid the initial
issue.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=100581
Link: http://marc.info/?t=143092384600002&r=1&w=2
Link: https://bugzilla.kernel.org/show_bug.cgi?id=99831
Link: http://marc.info/?t=143389402600001&r=1&w=2
Fixes: b9a5e5e "ACPI / init: Fix the ordering of acpi_reserve_resources()"
Reported-by: Roland Dreier <roland@purestorage.com>
Cc: All applicable <stable@vger.kernel.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
  • Loading branch information
Rafael J. Wysocki committed Jul 6, 2015
1 parent d770e55 commit 0294112
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 201 deletions.
12 changes: 9 additions & 3 deletions drivers/acpi/osl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,14 @@ static void __init acpi_request_region (struct acpi_generic_address *gas,
if (!addr || !length)
return;

acpi_reserve_region(addr, length, gas->space_id, 0, desc);
/* Resources are never freed */
if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO)
request_region(addr, length, desc);
else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
request_mem_region(addr, length, desc);
}

static void __init acpi_reserve_resources(void)
static int __init acpi_reserve_resources(void)
{
acpi_request_region(&acpi_gbl_FADT.xpm1a_event_block, acpi_gbl_FADT.pm1_event_length,
"ACPI PM1a_EVT_BLK");
Expand Down Expand Up @@ -207,7 +211,10 @@ static void __init acpi_reserve_resources(void)
if (!(acpi_gbl_FADT.gpe1_block_length & 0x1))
acpi_request_region(&acpi_gbl_FADT.xgpe1_block,
acpi_gbl_FADT.gpe1_block_length, "ACPI GPE1_BLK");

return 0;
}
fs_initcall_sync(acpi_reserve_resources);

void acpi_os_printf(const char *fmt, ...)
{
Expand Down Expand Up @@ -1862,7 +1869,6 @@ acpi_status __init acpi_os_initialize(void)

acpi_status __init acpi_os_initialize1(void)
{
acpi_reserve_resources();
kacpid_wq = alloc_workqueue("kacpid", 0, 1);
kacpi_notify_wq = alloc_workqueue("kacpi_notify", 0, 1);
kacpi_hotplug_wq = alloc_ordered_workqueue("kacpi_hotplug", 0);
Expand Down
162 changes: 0 additions & 162 deletions drivers/acpi/resource.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
#include <linux/device.h>
#include <linux/export.h>
#include <linux/ioport.h>
#include <linux/list.h>
#include <linux/slab.h>

#ifdef CONFIG_X86
Expand Down Expand Up @@ -622,164 +621,3 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
return (type & types) ? 0 : 1;
}
EXPORT_SYMBOL_GPL(acpi_dev_filter_resource_type);

struct reserved_region {
struct list_head node;
u64 start;
u64 end;
};

static LIST_HEAD(reserved_io_regions);
static LIST_HEAD(reserved_mem_regions);

static int request_range(u64 start, u64 end, u8 space_id, unsigned long flags,
char *desc)
{
unsigned int length = end - start + 1;
struct resource *res;

res = space_id == ACPI_ADR_SPACE_SYSTEM_IO ?
request_region(start, length, desc) :
request_mem_region(start, length, desc);
if (!res)
return -EIO;

res->flags &= ~flags;
return 0;
}

static int add_region_before(u64 start, u64 end, u8 space_id,
unsigned long flags, char *desc,
struct list_head *head)
{
struct reserved_region *reg;
int error;

reg = kmalloc(sizeof(*reg), GFP_KERNEL);
if (!reg)
return -ENOMEM;

error = request_range(start, end, space_id, flags, desc);
if (error) {
kfree(reg);
return error;
}

reg->start = start;
reg->end = end;
list_add_tail(&reg->node, head);
return 0;
}

/**
* acpi_reserve_region - Reserve an I/O or memory region as a system resource.
* @start: Starting address of the region.
* @length: Length of the region.
* @space_id: Identifier of address space to reserve the region from.
* @flags: Resource flags to clear for the region after requesting it.
* @desc: Region description (for messages).
*
* Reserve an I/O or memory region as a system resource to prevent others from
* using it. If the new region overlaps with one of the regions (in the given
* address space) already reserved by this routine, only the non-overlapping
* parts of it will be reserved.
*
* Returned is either 0 (success) or a negative error code indicating a resource
* reservation problem. It is the code of the first encountered error, but the
* routine doesn't abort until it has attempted to request all of the parts of
* the new region that don't overlap with other regions reserved previously.
*
* The resources requested by this routine are never released.
*/
int acpi_reserve_region(u64 start, unsigned int length, u8 space_id,
unsigned long flags, char *desc)
{
struct list_head *regions;
struct reserved_region *reg;
u64 end = start + length - 1;
int ret = 0, error = 0;

if (space_id == ACPI_ADR_SPACE_SYSTEM_IO)
regions = &reserved_io_regions;
else if (space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
regions = &reserved_mem_regions;
else
return -EINVAL;

if (list_empty(regions))
return add_region_before(start, end, space_id, flags, desc, regions);

list_for_each_entry(reg, regions, node)
if (reg->start == end + 1) {
/* The new region can be prepended to this one. */
ret = request_range(start, end, space_id, flags, desc);
if (!ret)
reg->start = start;

return ret;
} else if (reg->start > end) {
/* No overlap. Add the new region here and get out. */
return add_region_before(start, end, space_id, flags,
desc, &reg->node);
} else if (reg->end == start - 1) {
goto combine;
} else if (reg->end >= start) {
goto overlap;
}

/* The new region goes after the last existing one. */
return add_region_before(start, end, space_id, flags, desc, regions);

overlap:
/*
* The new region overlaps an existing one.
*
* The head part of the new region immediately preceding the existing
* overlapping one can be combined with it right away.
*/
if (reg->start > start) {
error = request_range(start, reg->start - 1, space_id, flags, desc);
if (error)
ret = error;
else
reg->start = start;
}

combine:
/*
* The new region is adjacent to an existing one. If it extends beyond
* that region all the way to the next one, it is possible to combine
* all three of them.
*/
while (reg->end < end) {
struct reserved_region *next = NULL;
u64 a = reg->end + 1, b = end;

if (!list_is_last(&reg->node, regions)) {
next = list_next_entry(reg, node);
if (next->start <= end)
b = next->start - 1;
}
error = request_range(a, b, space_id, flags, desc);
if (!error) {
if (next && next->start == b + 1) {
reg->end = next->end;
list_del(&next->node);
kfree(next);
} else {
reg->end = end;
break;
}
} else if (next) {
if (!ret)
ret = error;

reg = next;
} else {
break;
}
}

return ret ? ret : error;
}
EXPORT_SYMBOL_GPL(acpi_reserve_region);
35 changes: 9 additions & 26 deletions drivers/pnp/system.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
* Bjorn Helgaas <bjorn.helgaas@hp.com>
*/

#include <linux/acpi.h>
#include <linux/pnp.h>
#include <linux/device.h>
#include <linux/init.h>
Expand All @@ -23,41 +22,25 @@ static const struct pnp_device_id pnp_dev_table[] = {
{"", 0}
};

#ifdef CONFIG_ACPI
static bool __reserve_range(u64 start, unsigned int length, bool io, char *desc)
{
u8 space_id = io ? ACPI_ADR_SPACE_SYSTEM_IO : ACPI_ADR_SPACE_SYSTEM_MEMORY;
return !acpi_reserve_region(start, length, space_id, IORESOURCE_BUSY, desc);
}
#else
static bool __reserve_range(u64 start, unsigned int length, bool io, char *desc)
{
struct resource *res;

res = io ? request_region(start, length, desc) :
request_mem_region(start, length, desc);
if (res) {
res->flags &= ~IORESOURCE_BUSY;
return true;
}
return false;
}
#endif

static void reserve_range(struct pnp_dev *dev, struct resource *r, int port)
{
char *regionid;
const char *pnpid = dev_name(&dev->dev);
resource_size_t start = r->start, end = r->end;
bool reserved;
struct resource *res;

regionid = kmalloc(16, GFP_KERNEL);
if (!regionid)
return;

snprintf(regionid, 16, "pnp %s", pnpid);
reserved = __reserve_range(start, end - start + 1, !!port, regionid);
if (!reserved)
if (port)
res = request_region(start, end - start + 1, regionid);
else
res = request_mem_region(start, end - start + 1, regionid);
if (res)
res->flags &= ~IORESOURCE_BUSY;
else
kfree(regionid);

/*
Expand All @@ -66,7 +49,7 @@ static void reserve_range(struct pnp_dev *dev, struct resource *r, int port)
* have double reservations.
*/
dev_info(&dev->dev, "%pR %s reserved\n", r,
reserved ? "has been" : "could not be");
res ? "has been" : "could not be");
}

static void reserve_resources_of_dev(struct pnp_dev *dev)
Expand Down
10 changes: 0 additions & 10 deletions include/linux/acpi.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,9 +309,6 @@ int acpi_check_region(resource_size_t start, resource_size_t n,

int acpi_resources_are_enforced(void);

int acpi_reserve_region(u64 start, unsigned int length, u8 space_id,
unsigned long flags, char *desc);

#ifdef CONFIG_HIBERNATION
void __init acpi_no_s4_hw_signature(void);
#endif
Expand Down Expand Up @@ -507,13 +504,6 @@ static inline int acpi_check_region(resource_size_t start, resource_size_t n,
return 0;
}

static inline int acpi_reserve_region(u64 start, unsigned int length,
u8 space_id, unsigned long flags,
char *desc)
{
return -ENXIO;
}

struct acpi_table_header;
static inline int acpi_table_parse(char *id,
int (*handler)(struct acpi_table_header *))
Expand Down

0 comments on commit 0294112

Please sign in to comment.