lkml.org 
[lkml]   [2010]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 3/4] [Patch-next] ACPI, APEI Fix the return value(==NULL) of acpi_pre_map always.
    From
    Date
    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




    \
     
     \ /
      Last update: 2010-08-17 06:19    [W:0.022 / U:3.648 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site