Re: [PATCH v5 3/3] ARM: mm: use static_vm for managing static mappedareas

[Rob Herring]

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.kim@xxxxxxx>

[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. We
probably should have had a WARN here.

> 
> 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.

Rob

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