Re: [PATCH] android: binder: use VM_ALLOC to get vm area.

[Ganesh Mahendran]

Hi, Greg:

2016-09-02 3:02 GMT+08:00 Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
> On Thu, Sep 01, 2016 at 02:41:04PM +0800, Ganesh Mahendran wrote:
>> VM_IOREMAP is used to access hardware through a mechanism called
>> I/O mapped memory. Android binder is a IPC machanism which will
>> not access I/O memory.
>>
>> Also VM_IOREMAP has alignment requiement which may not needed in
>> binder.
>> ----
>> __get_vm_area_node()
>> {
>> ...
>>     if (flags & VM_IOREMAP)
>>         align = 1ul << clamp_t(int, fls_long(size),
>>            PAGE_SHIFT, IOREMAP_MAX_ORDER);
>> ...
>> }
>> ----
>>
>> This patch use VM_ALLOC to get vm area.
>>
>> Signed-off-by: Ganesh Mahendran <opensource.ganesh@xxxxxxxxx>
>> ---
>>  drivers/android/binder.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 16288e7..3511d5c 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2885,7 +2885,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
>>               goto err_already_mapped;
>>       }
>>
>> -     area = get_vm_area(vma->vm_end - vma->vm_start, VM_IOREMAP);
>> +     area = get_vm_area(vma->vm_end - vma->vm_start, VM_ALLOC);
>>       if (area == NULL) {
>>               ret = -ENOMEM;
>>               failure_string = "get_vm_area";
>
> What change have you noticed with this patch?  Have you tested it?
> Found that previously reserved iomemory is now free for binder to use
> where it wasn't?  What kind of change does the system now run as because
> of this?

I did some sanity test in Android system.  I will do more test using:
https://android.googlesource.com/platform/frameworks/native/+/master/libs/binder/tests/

>
> And are you _sure_ the alignment requirement isn't needed for binder?
> Have you verified this with the userspace binder library?

I will do the binder test.

Thanks.

>
> This is messy, tricky, stuff, I'm loath to change it without loads of
> testing having happened...
>
> thanks,
>
> greg k-h