Re: [PATCH v3 0/6] Android Binder IPC Fixes

[Serban Constantinescu <Serban.Constantinescu@arm.com>]

Hi Arve,

On 30/04/13 00:13, Arve Hjønnevåg wrote:
> On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu
> <Serban.Constantinescu@arm.com> wrote:
>> Hi all,
>>
>> Any feedback or comments on this patch set?
>>
>
> You don't seem to have addressed my feedback on the previous patch set.

For v3 I have modified the following according to your review:

> Changes for v3:
> 1:  Dropped the patch that was replacing uint32_t types with unsigned int
> 2:  Dropped the patch fixing the IOCTL types(since it has been added to Greg's
>     staging tree)
> 3:  Split one patch into two: 'modify binder_write_read' and '64bit changes'
> 4:  Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's review
> 5:  Modified the binder command IOCTL declarations according to Arve's review

The following were left out:

> On 11/04/13 22:40, Arve Hjønnevåg wrote:
> OK, relaxing the alignment requirement for *offp to what the hardware
>> requires makes sense. Is there any macros in the kernel to help with
>> this, instead of hard-coding it to 4 bytes?

There is no kernel macro that I know which will help here(one that 
springs to mind is PTR_ALIGN but it aligns to (unsigned long) - we need 
one that aligns to (u32)). Any ideas?

> On 11/04/13 21:38, Arve Hjønnevåg wrote:
> OK, but if you are using this change let a 64 bit user-space know that
>> the driver has been fixed, then this patch needs to go after the
>> patches that change the structures on 64 bit systems.

For 32bit systems nothing has changed so they will continue to work as 
before. For 64bit systems the size of binder_version was signed long 
before the patch and __s32 after the patch is applied. Thus a 64bit 
system using the old interface will fail immediately after opening the 
binder driver, while cheeking the binder version (since the 
BINDER_VERSION ioctl will be different pre/post patch - size of 
binder_version differs).

For 64/32 systems once I will have the userspace wrapper ready I will 
add another ioctl(as discussed) that will check if the driver is 64bit 
ready(among the first things to do on binder_open).

Please let me know if there is anything that skipped my review and you 
would like to integrate in this patch set.

Thanks for your feedback and help,
Serban