On Wed, Feb 06, 2013 at 11:07:07AM +0900, Joonsoo Kim wrote: > Hello, Rob. > > On Tue, Feb 05, 2013 at 01:12:51PM -0600, Rob Herring wrote: > > On 02/05/2013 12:13 PM, Nicolas Pitre wrote: > > > On Tue, 5 Feb 2013, Rob Herring wrote: > > > > > >> On 02/04/2013 10:44 PM, Nicolas Pitre wrote: > > >>> On Tue, 5 Feb 2013, Joonsoo Kim wrote: > > >>> > > >>>> A static mapped area is ARM-specific, so it is better not to use > > >>>> generic vmalloc data structure, that is, vmlist and vmlist_lock > > >>>> for managing static mapped area. And it causes some needless overhead > > >>>> and > > >>>> reducing this overhead is better idea. > > >>>> > > >>>> Now, we have newly introduced static_vm infrastructure. > > >>>> With it, we don't need to iterate all mapped areas. Instead, we just > > >>>> iterate static mapped areas. It helps to reduce an overhead of finding > > >>>> matched area. And architecture dependency on vmalloc layer is removed, > > >>>> so it will help to maintainability for vmalloc layer. > > >>>> > > >>>> Signed-off-by: Joonsoo Kim <iamjoonsoo....@lge.com> > > >> > > >> [snip] > > >> > > >>>> @@ -859,17 +864,12 @@ static void __init pci_reserve_io(void) > > >>>> { > > >>>> struct vm_struct *vm; > > >>>> unsigned long addr; > > >>>> + struct static_vm *svm; > > >>>> > > >>>> - /* we're still single threaded hence no lock needed here */ > > >>>> - for (vm = vmlist; vm; vm = vm->next) { > > >>>> - if (!(vm->flags & VM_ARM_STATIC_MAPPING)) > > >>>> - continue; > > >>>> - addr = (unsigned long)vm->addr; > > >>>> - addr &= ~(SZ_2M - 1); > > >>>> - if (addr == PCI_IO_VIRT_BASE) > > >>>> - return; > > >>>> + svm = find_static_vm_vaddr((void *)PCI_IO_VIRT_BASE); > > >>>> + if (svm) > > >>>> + return; > > >>>> > > >>>> - } > > >>>> > > >>>> vm_reserve_area_early(PCI_IO_VIRT_BASE, SZ_2M, pci_reserve_io); > > >>>> } > > >>> > > >>> The replacement code is not equivalent. I can't recall why the > > >>> original > > >>> is as it is, but it doesn't look right to me. The 2MB round down > > >>> certainly looks suspicious. > > >> > > >> The PCI mapping is at a fixed, aligned 2MB mapping. If we find any > > >> virtual address within that region already mapped, it is an error. > > > Ah, OK. This wasn't clear looking at the code. > > >> We probably should have had a WARN here. > > > > > > Indeed. > > > > > Okay. > I should fix it to find any mapping within PCI reserved region.
Ah... Above comment is my mistake. If there is a region already mapped within PCI reserved region and it is not found by find_static_vm_vaddr(), vm_area_add_early() hit BUG_ON(). So, to leave find_static_vm_vaddr() is safe. > But, I think that it is not an error. > Now, I see your original commit 'c2794437091a4fda72c4a4f3567dd728dcc0c3c9' > and find below message. > > "Platforms which need early i/o mapping (e.g. for vga console) can call > pci_map_io_early in their .map_io function." > > Therfore, for some platform, it is possible that there is a mapping within > PCI reserved range. > > So, I will not add WARN here. > > I will fix and re-send v6 with your ACK. > > Thanks for review. > > > >>> > > >>> The replacement code should be better. However I'd like you to get an > > >>> ACK from Rob Herring as well for this patch. > > >> > > >> It doesn't appear to me the above case is handled. The virt addr is > > >> checked whether it is within an existing mapping, but not whether the > > >> new mapping would overlap an existing mapping. It would be good to check > > >> for this generically rather than specifically for the PCI i/o mapping. > > > > > > Agreed. However that is checked already in vm_area_add_early(). > > > Therefore the overlap test here is redundant. > > > > Ah, right. In that case: > > > > Acked-by: Rob Herring <rob.herr...@calxeda.com> > > > > Rob > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majord...@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/