Re: [PATCH v3 0/6] Android Binder IPC Fixes
From: Arve Hjønnevåg
Date: Tue Apr 30 2013 - 19:52:19 EST
On Tue, Apr 30, 2013 at 1:36 AM, Serban Constantinescu
> 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@xxxxxxx> 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
>> staging tree)
>> 3: Split one patch into two: 'modify binder_write_read' and '64bit
>> 4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's
>> 5: Modified the binder command IOCTL declarations according to Arve's
> 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?
Perhaps using __alignof__(struct flat_binder_object) will work. This
is the least important part of that change however. I saw no response
to my concern that your changes can cause less memory to be allocated
than you write to.
>> 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).
Why fix the BINDER_VERSION ioctl to succeed on a 64 bit system before
the driver is usable on a 64 bit system?
> Please let me know if there is anything that skipped my review and you would
> like to integrate in this patch set.
It may be better to reply to my original emails instead of copying
bits of them here.
> Thanks for your feedback and help,
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/