Re: [PATCH 0/11] ACPI: Fixes and cleanups related to iomaps management

Rafael J. Wysocki
2011-01-21T21:07:05Z

On Friday, January 21, 2011, Jeff Chua wrote:
> 2011/1/21 Rafael J. Wysocki <rjw@sisk.pl>:
> > Thanks, but unfortunately this wasn't conclusive. Please apply the patch below
> > instead of the previous one (on top of [1/11] - [11/11]) and collect dmesg
> > output containing a (failing) suspend attempt.
> >
> > Please attach full dmesg, so that I can see the size and address of your
> > system's NVS region.
>
> Attached.

So, below is a replacement for [11/11]. Please test it on top of
[1/11] - [10/11] (the current Linus' tree already contains [1/11] and [2/11])
and let me know if it works for you (in either case, please attach dmesg
output containing a suspend attempt).

If it works, I'll remove the diagnostic messages and submit along with the
rest of the patchset.

Thanks,
Rafael

---
Subject: ACPI / PM: Make NVS save/restore code use slightly less memory (test)

Remove the unnecessary field phys_start from struct nvs_page and
modify the code in nvs.c to use an array of struct nvs_page objects
instead of a list which makes it a bit more straightforward (in
addition to saving a little memory).

Not-signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
drivers/acpi/nvs.c | 125 +++++++++++++++++++++++++++++++++--------------------
1 file changed, 79 insertions(+), 46 deletions(-)

Index: linux-2.6/drivers/acpi/nvs.c
===================================================================
--- linux-2.6.orig/drivers/acpi/nvs.c
+++ linux-2.6/drivers/acpi/nvs.c
@@ -22,15 +22,15 @@
*/

struct nvs_page {
- unsigned long phys_start;
unsigned int size;
- void *kaddr;
+ void __iomem *kaddr;
void *data;
bool unmap;
- struct list_head node;
};

-static LIST_HEAD(nvs_list);
+static struct nvs_page *nvs;
+static unsigned long nvs_start;
+static unsigned int nvs_span;

/**
* suspend_nvs_register - register platform NVS memory region to save
@@ -43,31 +43,40 @@ static LIST_HEAD(nvs_list);
*/
int suspend_nvs_register(unsigned long start, unsigned long size)
{
- struct nvs_page *entry, *next;
+ struct nvs_page *p, *p_end;
+ unsigned int head, tail;

- while (size > 0) {
- unsigned int nr_bytes;
+ pr_info("PM: Registering ACPI NVS region at %lx (%ld bytes)\n",
+ start, size);

- entry = kzalloc(sizeof(struct nvs_page), GFP_KERNEL);
- if (!entry)
- goto Error;
-
- list_add_tail(&entry->node, &nvs_list);
- entry->phys_start = start;
- nr_bytes = PAGE_SIZE - (start & ~PAGE_MASK);
- entry->size = (size < nr_bytes) ? size : nr_bytes;
-
- start += entry->size;
- size -= entry->size;
+ nvs_start = start;
+ head = PAGE_SIZE - (start & ~PAGE_MASK);
+ if (head) {
+ nvs_span = 1;
+ size -= size > head ? head : size;
+ } else {
+ nvs_span = 0;
+ head = PAGE_SIZE;
}
- return 0;
+ nvs_span += size >> PAGE_SHIFT;
+ tail = size & ~PAGE_MASK;
+ if (tail)
+ nvs_span++;
+ else
+ tail = PAGE_SIZE;
+
+ nvs = kzalloc(sizeof(*nvs) * nvs_span, GFP_KERNEL);
+ if (!nvs)
+ return -ENOMEM;
+
+ p = nvs;
+ p->size = head;
+ for (p++, p_end = nvs + nvs_span - 1; p < p_end; p++)
+ p->size = PAGE_SIZE;
+ if (p_end > nvs)
+ p_end->size = tail;

- Error:
- list_for_each_entry_safe(entry, next, &nvs_list, node) {
- list_del(&entry->node);
- kfree(entry);
- }
- return -ENOMEM;
+ return 0;
}

/**
@@ -75,17 +84,23 @@ int suspend_nvs_register(unsigned long s
*/
void suspend_nvs_free(void)
{
- struct nvs_page *entry;
+ struct nvs_page *entry, *end;

- list_for_each_entry(entry, &nvs_list, node)
+ for (entry = nvs, end = nvs + nvs_span; entry < end; entry++)
if (entry->data) {
free_page((unsigned long)entry->data);
entry->data = NULL;
if (entry->kaddr) {
if (entry->unmap) {
+ pr_info("%s: Unmapping %p\n", __func__,
+ entry->kaddr);
+
iounmap(entry->kaddr);
entry->unmap = false;
} else {
+ pr_info("%s: Releasing %p\n", __func__,
+ entry->kaddr);
+
acpi_os_unmap_memory(entry->kaddr,
entry->size);
}
@@ -99,9 +114,9 @@ void suspend_nvs_free(void)
*/
int suspend_nvs_alloc(void)
{
- struct nvs_page *entry;
+ struct nvs_page *entry, *end;

- list_for_each_entry(entry, &nvs_list, node) {
+ for (entry = nvs, end = nvs + nvs_span; entry < end; entry++) {
entry->data = (void *)__get_free_page(GFP_KERNEL);
if (!entry->data) {
suspend_nvs_free();
@@ -116,26 +131,44 @@ int suspend_nvs_alloc(void)
*/
int suspend_nvs_save(void)
{
- struct nvs_page *entry;
+ struct nvs_page *entry, *end;
+ unsigned long phys = nvs_start;

printk(KERN_INFO "PM: Saving platform NVS memory\n");

- list_for_each_entry(entry, &nvs_list, node)
- if (entry->data) {
- unsigned long phys = entry->phys_start;
- unsigned int size = entry->size;
+ for (entry = nvs, end = nvs + nvs_span; entry < end; entry++) {
+ unsigned int size = entry->size;
+ void __iomem *kaddr;

- entry->kaddr = acpi_os_get_iomem(phys, size);
- if (!entry->kaddr) {
- entry->kaddr = acpi_os_ioremap(phys, size);
- entry->unmap = true;
- }
- if (!entry->kaddr) {
- suspend_nvs_free();
- return -ENOMEM;
- }
- memcpy(entry->data, entry->kaddr, entry->size);
+ if (!entry->data) {
+ suspend_nvs_free();
+ return -ENOMEM;
+ }
+
+ pr_info("%s: Trying to map %lx, %d\n", __func__, phys, size);
+
+ kaddr = acpi_os_get_iomem(phys, size);
+ if (!kaddr) {
+ kaddr = acpi_os_ioremap(phys, size);
+ entry->unmap = !!kaddr;
+ } else {
+ pr_info("%s: Got address %p\n", __func__, kaddr);
}
+ entry->kaddr = kaddr;
+
+ if (!entry->kaddr) {
+ pr_info("%s: Mapping failed\n", __func__);
+
+ suspend_nvs_free();
+ return -EIO;
+ } else {
+ pr_info("%s: Using address %p\n", __func__, entry->kaddr);
+ }
+
+ memcpy(entry->data, entry->kaddr, size);
+
+ phys += size;
+ }

return 0;
}
@@ -148,11 +181,11 @@ int suspend_nvs_save(void)
*/
void suspend_nvs_restore(void)
{
- struct nvs_page *entry;
+ struct nvs_page *entry, *end;

printk(KERN_INFO "PM: Restoring platform NVS memory\n");

- list_for_each_entry(entry, &nvs_list, node)
+ for (entry = nvs, end = nvs + nvs_span; entry < end; entry++)
if (entry->data)
memcpy(entry->kaddr, entry->data, entry->size);
}