* [PATCH 3/4] [Patch-next] ACPI, APEI Fix the return value(==NULL) of acpi_pre_map always. @ 2010-08-17 0:56 Jin Dongming 2010-08-17 1:37 ` Huang Ying 0 siblings, 1 reply; 5+ messages in thread From: Jin Dongming @ 2010-08-17 0:56 UTC (permalink / raw) To: Huang Ying Cc: Randy Dunlap, Stephen Rothwell, Andi Kleen, Hidetoshi Seto, ACPI, LKLM 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 <jin.dongming@np.css.fujitsu.com> --- drivers/acpi/atomicio.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c index 8f8bd73..1bc2614 100644 --- a/drivers/acpi/atomicio.c +++ b/drivers/acpi/atomicio.c @@ -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)) { 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; -- 1.7.1.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3/4] [Patch-next] ACPI, APEI Fix the return value(==NULL) of acpi_pre_map always. 2010-08-17 0:56 [PATCH 3/4] [Patch-next] ACPI, APEI Fix the return value(==NULL) of acpi_pre_map always Jin Dongming @ 2010-08-17 1:37 ` Huang Ying 2010-08-17 2:43 ` Jin Dongming 0 siblings, 1 reply; 5+ messages in thread From: Huang Ying @ 2010-08-17 1:37 UTC (permalink / raw) To: Jin Dongming Cc: Randy Dunlap, Stephen Rothwell, Andi Kleen, Hidetoshi Seto, ACPI, LKLM On Tue, 2010-08-17 at 08:56 +0800, Jin Dongming wrote: > 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. > 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 <jin.dongming@np.css.fujitsu.com> > --- > drivers/acpi/atomicio.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c > index 8f8bd73..1bc2614 100644 > --- a/drivers/acpi/atomicio.c > +++ b/drivers/acpi/atomicio.c > @@ -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. > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/4] [Patch-next] ACPI, APEI Fix the return value(==NULL) of acpi_pre_map always. 2010-08-17 1:37 ` Huang Ying @ 2010-08-17 2:43 ` Jin Dongming 2010-08-17 4:15 ` Huang Ying 0 siblings, 1 reply; 5+ messages in thread From: Jin Dongming @ 2010-08-17 2:43 UTC (permalink / raw) To: Huang Ying Cc: Randy Dunlap, Stephen Rothwell, Andi Kleen, Hidetoshi Seto, ACPI, LKLM (2010/08/17 10:37), Huang Ying wrote: > On Tue, 2010-08-17 at 08:56 +0800, Jin Dongming wrote: >> 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); > >> 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 <jin.dongming@np.css.fujitsu.com> >> --- >> drivers/acpi/atomicio.c | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c >> index 8f8bd73..1bc2614 100644 >> --- a/drivers/acpi/atomicio.c >> +++ b/drivers/acpi/atomicio.c >> @@ -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. > >> 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 > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/4] [Patch-next] ACPI, APEI Fix the return value(==NULL) of acpi_pre_map always. 2010-08-17 2:43 ` Jin Dongming @ 2010-08-17 4:15 ` Huang Ying 2010-08-17 4:47 ` Jin Dongming 0 siblings, 1 reply; 5+ messages in thread From: Huang Ying @ 2010-08-17 4:15 UTC (permalink / raw) To: Jin Dongming Cc: Randy Dunlap, Stephen Rothwell, Andi Kleen, Hidetoshi Seto, ACPI, LKLM On Tue, 2010-08-17 at 10:43 +0800, Jin Dongming wrote: > (2010/08/17 10:37), Huang Ying wrote: > > On Tue, 2010-08-17 at 08:56 +0800, Jin Dongming wrote: > >> 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. return vaddr + (paddr - pg_off), is not NULL for common cases. > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/4] [Patch-next] ACPI, APEI Fix the return value(==NULL) of acpi_pre_map always. 2010-08-17 4:15 ` Huang Ying @ 2010-08-17 4:47 ` Jin Dongming 0 siblings, 0 replies; 5+ messages in thread From: Jin Dongming @ 2010-08-17 4:47 UTC (permalink / raw) To: Huang Ying Cc: Randy Dunlap, Stephen Rothwell, Andi Kleen, Hidetoshi Seto, ACPI, LKLM (2010/08/17 13:15), Huang Ying wrote: > On Tue, 2010-08-17 at 10:43 +0800, Jin Dongming wrote: >> (2010/08/17 10:37), Huang Ying wrote: >>> On Tue, 2010-08-17 at 08:56 +0800, Jin Dongming wrote: >>>> 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. > > return vaddr + (paddr - pg_off), is not NULL for common cases. > >> 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 > OK. I will modify the description of this patch and resend it. Best Regards, Jin Dongming > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-08-17 4:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-08-17 0:56 [PATCH 3/4] [Patch-next] ACPI, APEI Fix the return value(==NULL) of acpi_pre_map always Jin Dongming 2010-08-17 1:37 ` Huang Ying 2010-08-17 2:43 ` Jin Dongming 2010-08-17 4:15 ` Huang Ying 2010-08-17 4:47 ` Jin Dongming
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).