Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934012Ab3D3XwP (ORCPT ); Tue, 30 Apr 2013 19:52:15 -0400 Received: from mail-da0-f41.google.com ([209.85.210.41]:59579 "EHLO mail-da0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933900Ab3D3XwO convert rfc822-to-8bit (ORCPT ); Tue, 30 Apr 2013 19:52:14 -0400 MIME-Version: 1.0 In-Reply-To: <517F829B.3080602@arm.com> References: <1365769555-6052-1-git-send-email-serban.constantinescu@arm.com> <517E9CDB.3020906@arm.com> <517F829B.3080602@arm.com> Date: Tue, 30 Apr 2013 16:52:13 -0700 Message-ID: Subject: Re: [PATCH v3 0/6] Android Binder IPC Fixes From: =?ISO-8859-1?Q?Arve_Hj=F8nnev=E5g?= To: Serban Constantinescu Cc: "linux-kernel@vger.kernel.org" , "gregkh@linuxfoundation.org" , "kernel-team@android.com" , "john.stultz@linaro.org" , Dave Butcher Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3160 Lines: 93 On Tue, Apr 30, 2013 at 1:36 AM, Serban Constantinescu wrote: > Hi Arve, > > > On 30/04/13 00:13, Arve Hj?nnev?g wrote: >> >> On Mon, Apr 29, 2013 at 9:16 AM, Serban Constantinescu >> 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? > 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, > Serban > -- Arve Hj?nnev?g -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/