ACPI, APEI Fix the return value(==NULL) of acpi_pre_map always.

acpi_pre_map() is used for remapping the physical address of I/O, so it should be return NULL or remapped virtual address. The problem is whether I/O remapping is successful or not, the function returns NULL always.

In acpi_pre_map(), after the physical address is remapped sucessfully, it will check whether the physical address has been added into acpi_iomaps list again. If the physical address has beed added into acpi_iomaps, the virtual address will be saved in vaddr. Otherwise, NULL be saved in vaddr.

So if the physical address has never been remapped, the return value of acpi_pre_map will be NULL always.

This patch fixed it and I confirmed it on x86_64 next-tree.

Signed-off-by Jin Dongming <[email protected]>

---
 drivers/acpi/atomicio.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/acpi/atomicio.c +0 -0

index 8f8bd73..1bc2614 100644

Quoted 4 lines · Jin Dongming

acpi_pre_map() is used for remapping the physical address of I/O, so it should be return NULL or remapped virtual address. The problem is whether I/O remapping is successful or not, the function returns NULL always.

No. NULL is returned for error path only. Please check the code again.

Quoted 22 lines

In acpi_pre_map(), after the physical address is remapped sucessfully, it will check whether the physical address has been added into acpi_iomaps list again. If the physical address has beed added into acpi_iomaps, the virtual address will be saved in vaddr. Otherwise, NULL be saved in vaddr.

So if the physical address has never been remapped, the return value of acpi_pre_map will be NULL always.

This patch fixed it and I confirmed it on x86_64 next-tree.

Signed-off-by Jin Dongming <[email protected]>

---
 drivers/acpi/atomicio.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/acpi/atomicio.c +0 -0

index 8f8bd73..1bc2614 100644

drivers/acpi/atomicio.c static void __iomem *acpi_pre_map(phys_addr_t paddr, · line 136

Quoted 5 lines

drivers/acpi/atomicio.c +1 -1

@@ -133,7 +133,7 @@ static void __iomem *acpi_pre_map(phys_addr_t paddr,
 
 	spin_lock_irqsave(&acpi_iomaps_lock, flags);
 	vaddr = __acpi_try_ioremap(paddr, size);
-	if (vaddr) {
+	if (unlikely(vaddr)) {

pre_map is not performance critical, so "unlikely" here helps little.

drivers/acpi/atomicio.c static void __iomem *acpi_pre_map(phys_addr_t paddr, · line 148

Quoted 12 lines

drivers/acpi/atomicio.c +1 -1

 		spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
 		iounmap(map->vaddr);
 		kfree(map);

@@ -142,7 +142,7 @@ static void __iomem *acpi_pre_map(phys_addr_t paddr,
 	list_add_tail_rcu(&map->list, &acpi_iomaps);
 	spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
 
-	return vaddr + (paddr - pg_off);
+	return map->vaddr + (paddr - pg_off);
 err_unmap:
 	iounmap(vaddr);
 	return NULL;

When will vaddr != map->vaddr?

Best Regards,
Huang Ying

Quoted 7 lines · Huang Ying

Quoted 4 lines · Jin Dongming

acpi_pre_map() is used for remapping the physical address of I/O, so it should be return NULL or remapped virtual address. The problem is whether I/O remapping is successful or not, the function returns NULL always.

No. NULL is returned for error path only. Please check the code again.

There three places used the local variable vaddr in acpi_pre_map() in next-tree.

   1. 115         vaddr = __acpi_try_ioremap(paddr, size);
   2. 122         vaddr = ioremap(pg_off, pg_sz);
   3. 135         vaddr = __acpi_try_ioremap(paddr, size);

Let's think about the following statement.
Assumption: the physical address has never been remapped. Step:
1. vaddr == NULL
Because the physical address is not registered in the acpi_iomaps, it should be returned NULL from __acpi_try_ioremap().

2. vaddr == the virtual address of the physical address. Here if ioremap is successful, the value of vaddr should be the virtual address returned from ioremap().

3. vaddr == NULL <== IMPORTANT
Here it is because the physical address has not been registered in the acpi_iomaps yet, it still return NULL from __acpi_try_ioremap(). So it is why vaddr == NULL, even if the physical address has never been remapped.

Result: vaddr == NULL.

And if the vaddr is not NULL, it could not be added into acpi_iomaps. Codes in acpi_pre_map() is like following.

134         spin_lock_irqsave(&acpi_iomaps_lock, flags);
135         vaddr = __acpi_try_ioremap(paddr, size);           <== the 3rd step
136         if (vaddr) {
137                 spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
138                 iounmap(map->vaddr);
139                 kfree(map);
140                 return vaddr;
141         }
142         list_add_tail_rcu(&map->list, &acpi_iomaps);      <== add into acpi_iomaps.
143         spin_unlock_irqrestore(&acpi_iomaps_lock, flags);

Quoted 25 lines · Jin Dongming

Quoted 7 lines · Huang Ying

Quoted 4 lines · Jin Dongming

acpi_pre_map() is used for remapping the physical address of I/O, so it should be return NULL or remapped virtual address. The problem is whether I/O remapping is successful or not, the function returns NULL always.

No. NULL is returned for error path only. Please check the code again.

There three places used the local variable vaddr in acpi_pre_map() in next-tree.

   1. 115         vaddr = __acpi_try_ioremap(paddr, size);
   2. 122         vaddr = ioremap(pg_off, pg_sz);
   3. 135         vaddr = __acpi_try_ioremap(paddr, size);

Let's think about the following statement.
Assumption: the physical address has never been remapped. Step:
1. vaddr == NULL
Because the physical address is not registered in the acpi_iomaps, it should be returned NULL from __acpi_try_ioremap().

2. vaddr == the virtual address of the physical address. Here if ioremap is successful, the value of vaddr should be the virtual address returned from ioremap().

Quoted 7 lines

3. vaddr == NULL <== IMPORTANT
Here it is because the physical address has not been registered in the acpi_iomaps yet, it still return NULL from __acpi_try_ioremap(). So it is why vaddr == NULL, even if the physical address has never been remapped.

Result: vaddr == NULL.

return vaddr + (paddr - pg_off), is not NULL for common cases.

Quoted 13 lines

And if the vaddr is not NULL, it could not be added into acpi_iomaps. Codes in acpi_pre_map() is like following.

134         spin_lock_irqsave(&acpi_iomaps_lock, flags);
135         vaddr = __acpi_try_ioremap(paddr, size);           <== the 3rd step
136         if (vaddr) {
137                 spin_unlock_irqrestore(&acpi_iomaps_lock, flags);
138                 iounmap(map->vaddr);
139                 kfree(map);
140                 return vaddr;
141         }
142         list_add_tail_rcu(&map->list, &acpi_iomaps);      <== add into acpi_iomaps.
143         spin_unlock_irqrestore(&acpi_iomaps_lock, flags);

Oops, this is a bug, and your patch really fix it. But I think you should change the patch description. NULL is not returned in almost all cases. Because (paddr - pg_off) is not zero in common cases.

Best Regards,
Huang Ying

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/