[PATCH v2 0/7] Android Binder IPC Fixes

[PATCH v2 0/7] Android Binder IPC Fixes

[Serban Constantinescu]

Hi all,

This set of patches will clean-up and fix some of the issues that arise
with the current binder interface when moving to a 64bit kernel. All these
changes will not affect the existing 32bit Android interface and are meant
to stand as the base for the 64bit binder compat layer(kernel or userpsace).

The patch has been successfully tested with a 64bit Linux userspace and 64bit
binder unit-tests.

This patch set has been successfully tested on 32bit platforms(ARMv7 VExpress)
and 64bit platforms(ARMv8 RTSM) running a 32bit Android userspace and an in
kernel binder compat layer.

Changes for v2:
1: 1/7: Modified the commit message according to Greg's feedback;
2: 3/7: Merged with the patch fixing the printk format specifiers.

Serban Constantinescu (7):
  staging: android: binder: clean-up uint32_t types
  staging: android: binder: replace IOCTL types with user-exportable
    types
  staging: android: binder: fix binder interface for 64bit compat layer
  staging: android: binder: fix BINDER_SET_MAX_THREADS declaration
  staging: android: binder: fix BC_FREE_BUFFER ioctl declaration
  staging: android: binder: fix alignment issues
  staging: android: binder: replace types with portable ones

 drivers/staging/android/binder.c |  114 +++++++++++++++++++-------------------
 drivers/staging/android/binder.h |   54 +++++++++---------
 2 files changed, 84 insertions(+), 84 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/7] staging: android: binder: clean-up uint32_t types

[Serban Constantinescu]

uint32_t types are used inconsistently throughout the driver. This patch
replaces "uint32_t" types with "unsigned int" ones.

The patch does not change in any way the functionality of the binder driver.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
 drivers/staging/android/binder.c |   74 +++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 24456a0..5794cf6 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -103,7 +103,7 @@ enum {
 	BINDER_DEBUG_PRIORITY_CAP           = 1U << 14,
 	BINDER_DEBUG_BUFFER_ALLOC_ASYNC     = 1U << 15,
 };
-static uint32_t binder_debug_mask = BINDER_DEBUG_USER_ERROR |
+static unsigned int binder_debug_mask = BINDER_DEBUG_USER_ERROR |
 	BINDER_DEBUG_FAILED_TRANSACTION | BINDER_DEBUG_DEAD_TRANSACTION;
 module_param_named(debug_mask, binder_debug_mask, uint, S_IWUSR | S_IRUGO);
 
@@ -255,7 +255,7 @@ struct binder_ref {
 	struct hlist_node node_entry;
 	struct binder_proc *proc;
 	struct binder_node *node;
-	uint32_t desc;
+	unsigned int desc;
 	int strong;
 	int weak;
 	struct binder_ref_death *death;
@@ -307,7 +307,7 @@ struct binder_proc {
 
 	struct page **pages;
 	size_t buffer_size;
-	uint32_t buffer_free;
+	unsigned int buffer_free;
 	struct list_head todo;
 	wait_queue_head_t wait;
 	struct binder_stats stats;
@@ -336,8 +336,8 @@ struct binder_thread {
 	int looper;
 	struct binder_transaction *transaction_stack;
 	struct list_head todo;
-	uint32_t return_error; /* Write failed, return error code in read buf */
-	uint32_t return_error2; /* Write failed, return error code in read */
+	unsigned int return_error; /* Write failed, return error code in read buf */
+	unsigned int return_error2; /* Write failed, return error code in read */
 		/* buffer. Used when sending a reply to a dead process that */
 		/* we are also waiting on */
 	wait_queue_head_t wait;
@@ -993,7 +993,7 @@ static int binder_dec_node(struct binder_node *node, int strong, int internal)
 
 
 static struct binder_ref *binder_get_ref(struct binder_proc *proc,
-					 uint32_t desc)
+					 unsigned int desc)
 {
 	struct rb_node *n = proc->refs_by_desc.rb_node;
 	struct binder_ref *ref;
@@ -1173,7 +1173,7 @@ static void binder_pop_transaction(struct binder_thread *target_thread,
 }
 
 static void binder_send_failed_reply(struct binder_transaction *t,
-				     uint32_t error_code)
+				     unsigned int error_code)
 {
 	struct binder_thread *target_thread;
 	BUG_ON(t->flags & TF_ONE_WAY);
@@ -1310,7 +1310,7 @@ static void binder_transaction(struct binder_proc *proc,
 	wait_queue_head_t *target_wait;
 	struct binder_transaction *in_reply_to = NULL;
 	struct binder_transaction_log_entry *e;
-	uint32_t return_error;
+	unsigned int return_error;
 
 	e = binder_transaction_log_add(&binder_transaction_log);
 	e->call_type = reply ? 2 : !!(tr->flags & TF_ONE_WAY);
@@ -1702,14 +1702,14 @@ err_no_context_mgr_node:
 int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
 			void __user *buffer, int size, signed long *consumed)
 {
-	uint32_t cmd;
+	unsigned int cmd;
 	void __user *ptr = buffer + *consumed;
 	void __user *end = buffer + size;
 
 	while (ptr < end && thread->return_error == BR_OK) {
-		if (get_user(cmd, (uint32_t __user *)ptr))
+		if (get_user(cmd, (unsigned int __user *)ptr))
 			return -EFAULT;
-		ptr += sizeof(uint32_t);
+		ptr += sizeof(unsigned int);
 		trace_binder_command(cmd);
 		if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.bc)) {
 			binder_stats.bc[_IOC_NR(cmd)]++;
@@ -1721,13 +1721,13 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
 		case BC_ACQUIRE:
 		case BC_RELEASE:
 		case BC_DECREFS: {
-			uint32_t target;
+			unsigned int target;
 			struct binder_ref *ref;
 			const char *debug_string;
 
-			if (get_user(target, (uint32_t __user *)ptr))
+			if (get_user(target, (unsigned int __user *)ptr))
 				return -EFAULT;
-			ptr += sizeof(uint32_t);
+			ptr += sizeof(unsigned int);
 			if (target == 0 && binder_context_mgr_node &&
 			    (cmd == BC_INCREFS || cmd == BC_ACQUIRE)) {
 				ref = binder_get_ref_for_node(proc,
@@ -1922,14 +1922,14 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
 
 		case BC_REQUEST_DEATH_NOTIFICATION:
 		case BC_CLEAR_DEATH_NOTIFICATION: {
-			uint32_t target;
+			unsigned int target;
 			void __user *cookie;
 			struct binder_ref *ref;
 			struct binder_ref_death *death;
 
-			if (get_user(target, (uint32_t __user *)ptr))
+			if (get_user(target, (unsigned int __user *)ptr))
 				return -EFAULT;
-			ptr += sizeof(uint32_t);
+			ptr += sizeof(unsigned int);
 			if (get_user(cookie, (void __user * __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(void *);
@@ -2055,7 +2055,7 @@ int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
 }
 
 void binder_stat_br(struct binder_proc *proc, struct binder_thread *thread,
-		    uint32_t cmd)
+		    unsigned int cmd)
 {
 	trace_binder_return(cmd);
 	if (_IOC_NR(cmd) < ARRAY_SIZE(binder_stats.br)) {
@@ -2090,9 +2090,9 @@ static int binder_thread_read(struct binder_proc *proc,
 	int wait_for_proc_work;
 
 	if (*consumed == 0) {
-		if (put_user(BR_NOOP, (uint32_t __user *)ptr))
+		if (put_user(BR_NOOP, (unsigned int __user *)ptr))
 			return -EFAULT;
-		ptr += sizeof(uint32_t);
+		ptr += sizeof(unsigned int);
 	}
 
 retry:
@@ -2101,17 +2101,17 @@ retry:
 
 	if (thread->return_error != BR_OK && ptr < end) {
 		if (thread->return_error2 != BR_OK) {
-			if (put_user(thread->return_error2, (uint32_t __user *)ptr))
+			if (put_user(thread->return_error2, (unsigned int __user *)ptr))
 				return -EFAULT;
-			ptr += sizeof(uint32_t);
+			ptr += sizeof(unsigned int);
 			binder_stat_br(proc, thread, thread->return_error2);
 			if (ptr == end)
 				goto done;
 			thread->return_error2 = BR_OK;
 		}
-		if (put_user(thread->return_error, (uint32_t __user *)ptr))
+		if (put_user(thread->return_error, (unsigned int __user *)ptr))
 			return -EFAULT;
-		ptr += sizeof(uint32_t);
+		ptr += sizeof(unsigned int);
 		binder_stat_br(proc, thread, thread->return_error);
 		thread->return_error = BR_OK;
 		goto done;
@@ -2159,7 +2159,7 @@ retry:
 		return ret;
 
 	while (1) {
-		uint32_t cmd;
+		unsigned int cmd;
 		struct binder_transaction_data tr;
 		struct binder_work *w;
 		struct binder_transaction *t = NULL;
@@ -2183,9 +2183,9 @@ retry:
 		} break;
 		case BINDER_WORK_TRANSACTION_COMPLETE: {
 			cmd = BR_TRANSACTION_COMPLETE;
-			if (put_user(cmd, (uint32_t __user *)ptr))
+			if (put_user(cmd, (unsigned int __user *)ptr))
 				return -EFAULT;
-			ptr += sizeof(uint32_t);
+			ptr += sizeof(unsigned int);
 
 			binder_stat_br(proc, thread, cmd);
 			binder_debug(BINDER_DEBUG_TRANSACTION_COMPLETE,
@@ -2198,7 +2198,7 @@ retry:
 		} break;
 		case BINDER_WORK_NODE: {
 			struct binder_node *node = container_of(w, struct binder_node, work);
-			uint32_t cmd = BR_NOOP;
+			unsigned int cmd = BR_NOOP;
 			const char *cmd_name;
 			int strong = node->internal_strong_refs || node->local_strong_refs;
 			int weak = !hlist_empty(&node->refs) || node->local_weak_refs || strong;
@@ -2224,9 +2224,9 @@ retry:
 				node->has_weak_ref = 0;
 			}
 			if (cmd != BR_NOOP) {
-				if (put_user(cmd, (uint32_t __user *)ptr))
+				if (put_user(cmd, (unsigned int __user *)ptr))
 					return -EFAULT;
-				ptr += sizeof(uint32_t);
+				ptr += sizeof(unsigned int);
 				if (put_user(node->ptr, (void * __user *)ptr))
 					return -EFAULT;
 				ptr += sizeof(void *);
@@ -2260,16 +2260,16 @@ retry:
 		case BINDER_WORK_DEAD_BINDER_AND_CLEAR:
 		case BINDER_WORK_CLEAR_DEATH_NOTIFICATION: {
 			struct binder_ref_death *death;
-			uint32_t cmd;
+			unsigned int cmd;
 
 			death = container_of(w, struct binder_ref_death, work);
 			if (w->type == BINDER_WORK_CLEAR_DEATH_NOTIFICATION)
 				cmd = BR_CLEAR_DEATH_NOTIFICATION_DONE;
 			else
 				cmd = BR_DEAD_BINDER;
-			if (put_user(cmd, (uint32_t __user *)ptr))
+			if (put_user(cmd, (unsigned int __user *)ptr))
 				return -EFAULT;
-			ptr += sizeof(uint32_t);
+			ptr += sizeof(unsigned int);
 			if (put_user(death->cookie, (void * __user *)ptr))
 				return -EFAULT;
 			ptr += sizeof(void *);
@@ -2334,9 +2334,9 @@ retry:
 					ALIGN(t->buffer->data_size,
 					    sizeof(void *));
 
-		if (put_user(cmd, (uint32_t __user *)ptr))
+		if (put_user(cmd, (unsigned int __user *)ptr))
 			return -EFAULT;
-		ptr += sizeof(uint32_t);
+		ptr += sizeof(unsigned int);
 		if (copy_to_user(ptr, &tr, sizeof(tr)))
 			return -EFAULT;
 		ptr += sizeof(tr);
@@ -2379,7 +2379,7 @@ done:
 		binder_debug(BINDER_DEBUG_THREADS,
 			     "%d:%d BR_SPAWN_LOOPER\n",
 			     proc->pid, thread->pid);
-		if (put_user(BR_SPAWN_LOOPER, (uint32_t __user *)buffer))
+		if (put_user(BR_SPAWN_LOOPER, (unsigned int __user *)buffer))
 			return -EFAULT;
 		binder_stat_br(proc, thread, BR_SPAWN_LOOPER);
 	}
@@ -2752,7 +2752,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
 
 #ifdef CONFIG_CPU_CACHE_VIPT
 	if (cache_is_vipt_aliasing()) {
-		while (CACHE_COLOUR((vma->vm_start ^ (uint32_t)proc->buffer))) {
+		while (CACHE_COLOUR((vma->vm_start ^ (unsigned int)proc->buffer))) {
 			pr_info("binder_mmap: %d %lx-%lx maps %p bad alignment\n", proc->pid, vma->vm_start, vma->vm_end, proc->buffer);
 			vma->vm_start += PAGE_SIZE;
 		}
-- 
1.7.9.5

Re: [PATCH v2 1/7] staging: android: binder: clean-up uint32_t types

[Arve Hjønnevåg]

On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
<serban.constantinescu@arm.com> wrote:
> uint32_t types are used inconsistently throughout the driver. This patch
> replaces "uint32_t" types with "unsigned int" ones.
>

I don't like this change. You fix the header in a later patch to use
explicit sizes, so it does not make sense to do the opposite here.

Re: [PATCH v2 1/7] staging: android: binder: clean-up uint32_t types

[Serban Constantinescu]

On 10/04/13 01:11, Arve Hjønnevåg wrote:
> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
> <serban.constantinescu@arm.com> wrote:
>> uint32_t types are used inconsistently throughout the driver. This patch
>> replaces "uint32_t" types with "unsigned int" ones.
>>
>
> I don't like this change. You fix the header in a later patch to use
> explicit sizes, so it does not make sense to do the opposite here.

As mentioned before we do not need this change. I will drop it from the 
patch set.

Thanks,
Serban

[PATCH v2 2/7] staging: android: binder: replace IOCTL types with user-exportable types

[Serban Constantinescu]

This patch modifies the IOCTL macros to use user-exportable data types,
as they are the referred kernel types for the user/kernel interface.

The patch does not change in any way the functionality of the binder driver.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
 drivers/staging/android/binder.h |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index f240464..dbe81ce 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -85,11 +85,11 @@ struct binder_version {
 #define BINDER_CURRENT_PROTOCOL_VERSION 7
 
 #define BINDER_WRITE_READ		_IOWR('b', 1, struct binder_write_read)
-#define	BINDER_SET_IDLE_TIMEOUT		_IOW('b', 3, int64_t)
+#define	BINDER_SET_IDLE_TIMEOUT		_IOW('b', 3, __s64)
 #define	BINDER_SET_MAX_THREADS		_IOW('b', 5, size_t)
-#define	BINDER_SET_IDLE_PRIORITY	_IOW('b', 6, int)
-#define	BINDER_SET_CONTEXT_MGR		_IOW('b', 7, int)
-#define	BINDER_THREAD_EXIT		_IOW('b', 8, int)
+#define	BINDER_SET_IDLE_PRIORITY	_IOW('b', 6, __s32)
+#define	BINDER_SET_CONTEXT_MGR		_IOW('b', 7, __s32)
+#define	BINDER_THREAD_EXIT		_IOW('b', 8, __s32)
 #define BINDER_VERSION			_IOWR('b', 9, struct binder_version)
 
 /*
-- 
1.7.9.5

Re: [PATCH v2 2/7] staging: android: binder: replace IOCTL types with user-exportable types

[Arve Hjønnevåg]

On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
<serban.constantinescu@arm.com> wrote:
> This patch modifies the IOCTL macros to use user-exportable data types,
> as they are the referred kernel types for the user/kernel interface.
>
> The patch does not change in any way the functionality of the binder driver.
>
> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>

Acked-by: Arve Hjønnevåg <arve@android.com>

> ---
>  drivers/staging/android/binder.h |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
> index f240464..dbe81ce 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -85,11 +85,11 @@ struct binder_version {
>  #define BINDER_CURRENT_PROTOCOL_VERSION 7
>
>  #define BINDER_WRITE_READ              _IOWR('b', 1, struct binder_write_read)
> -#define        BINDER_SET_IDLE_TIMEOUT         _IOW('b', 3, int64_t)
> +#define        BINDER_SET_IDLE_TIMEOUT         _IOW('b', 3, __s64)
>  #define        BINDER_SET_MAX_THREADS          _IOW('b', 5, size_t)
> -#define        BINDER_SET_IDLE_PRIORITY        _IOW('b', 6, int)
> -#define        BINDER_SET_CONTEXT_MGR          _IOW('b', 7, int)
> -#define        BINDER_THREAD_EXIT              _IOW('b', 8, int)
> +#define        BINDER_SET_IDLE_PRIORITY        _IOW('b', 6, __s32)
> +#define        BINDER_SET_CONTEXT_MGR          _IOW('b', 7, __s32)
> +#define        BINDER_THREAD_EXIT              _IOW('b', 8, __s32)

BINDER_SET_CONTEXT_MGR and BINDER_THREAD_EXIT don't actually take an argument.

>  #define BINDER_VERSION                 _IOWR('b', 9, struct binder_version)
>
>  /*
> --
> 1.7.9.5
>



--
Arve Hjønnevåg

[PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer

[Serban Constantinescu]

The changes in this patch will fix the binder interface for use on 64bit
machines and stand as the base of the 64bit compat support. The changes
apply to the structures that are passed between the kernel and
userspace.

Most of the  changes applied mirror the change to struct binder_version
where there is no need for a 64bit wide protocol_version(on 64bit
machines). The change inlines with the existing 32bit userspace(the
structure has the same size) and simplifies the compat layer such that
the same handler can service the BINDER_VERSION ioctl.

Other changes fix the function prototypes for binder_thread_read/write,
make use of kernel types as well as user-exportable ones and fix
format specifier issues.

The changes do not affect existing 32bit ABI.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
 drivers/staging/android/binder.c |   28 ++++++++++++++--------------
 drivers/staging/android/binder.h |   16 ++++++++--------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 5794cf6..a2cdd9e 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1271,7 +1271,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 		case BINDER_TYPE_WEAK_HANDLE: {
 			struct binder_ref *ref = binder_get_ref(proc, fp->handle);
 			if (ref == NULL) {
-				pr_err("transaction release %d bad handle %ld\n",
+				pr_err("transaction release %d bad handle %d\n",
 				 debug_id, fp->handle);
 				break;
 			}
@@ -1283,13 +1283,13 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 
 		case BINDER_TYPE_FD:
 			binder_debug(BINDER_DEBUG_TRANSACTION,
-				     "        fd %ld\n", fp->handle);
+				     "        fd %d\n", fp->handle);
 			if (failed_at)
 				task_close_fd(proc, fp->handle);
 			break;
 
 		default:
-			pr_err("transaction release %d bad object type %lx\n",
+			pr_err("transaction release %d bad object type %x\n",
 				debug_id, fp->type);
 			break;
 		}
@@ -1547,7 +1547,7 @@ static void binder_transaction(struct binder_proc *proc,
 		case BINDER_TYPE_WEAK_HANDLE: {
 			struct binder_ref *ref = binder_get_ref(proc, fp->handle);
 			if (ref == NULL) {
-				binder_user_error("%d:%d got transaction with invalid handle, %ld\n",
+				binder_user_error("%d:%d got transaction with invalid handle, %d\n",
 						proc->pid,
 						thread->pid, fp->handle);
 				return_error = BR_FAILED_REPLY;
@@ -1590,13 +1590,13 @@ static void binder_transaction(struct binder_proc *proc,
 
 			if (reply) {
 				if (!(in_reply_to->flags & TF_ACCEPT_FDS)) {
-					binder_user_error("%d:%d got reply with fd, %ld, but target does not allow fds\n",
+					binder_user_error("%d:%d got reply with fd, %d, but target does not allow fds\n",
 						proc->pid, thread->pid, fp->handle);
 					return_error = BR_FAILED_REPLY;
 					goto err_fd_not_allowed;
 				}
 			} else if (!target_node->accept_fds) {
-				binder_user_error("%d:%d got transaction with fd, %ld, but target does not allow fds\n",
+				binder_user_error("%d:%d got transaction with fd, %d, but target does not allow fds\n",
 					proc->pid, thread->pid, fp->handle);
 				return_error = BR_FAILED_REPLY;
 				goto err_fd_not_allowed;
@@ -1604,7 +1604,7 @@ static void binder_transaction(struct binder_proc *proc,
 
 			file = fget(fp->handle);
 			if (file == NULL) {
-				binder_user_error("%d:%d got transaction with invalid fd, %ld\n",
+				binder_user_error("%d:%d got transaction with invalid fd, %d\n",
 					proc->pid, thread->pid, fp->handle);
 				return_error = BR_FAILED_REPLY;
 				goto err_fget_failed;
@@ -1618,13 +1618,13 @@ static void binder_transaction(struct binder_proc *proc,
 			task_fd_install(target_proc, target_fd, file);
 			trace_binder_transaction_fd(t, fp->handle, target_fd);
 			binder_debug(BINDER_DEBUG_TRANSACTION,
-				     "        fd %ld -> %d\n", fp->handle, target_fd);
+				     "        fd %d -> %d\n", fp->handle, target_fd);
 			/* TODO: fput? */
 			fp->handle = target_fd;
 		} break;
 
 		default:
-			binder_user_error("%d:%d got transaction with invalid object type, %lx\n",
+			binder_user_error("%d:%d got transaction with invalid object type, %x\n",
 				proc->pid, thread->pid, fp->type);
 			return_error = BR_FAILED_REPLY;
 			goto err_bad_object_type;
@@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
 }
 
 int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
-			void __user *buffer, int size, signed long *consumed)
+			void __user *buffer, size_t size, size_t *consumed)
 {
 	unsigned int cmd;
 	void __user *ptr = buffer + *consumed;
@@ -2080,8 +2080,8 @@ static int binder_has_thread_work(struct binder_thread *thread)
 
 static int binder_thread_read(struct binder_proc *proc,
 			      struct binder_thread *thread,
-			      void  __user *buffer, int size,
-			      signed long *consumed, int non_block)
+			      void  __user *buffer, size_t size,
+			      size_t *consumed, int non_block)
 {
 	void __user *ptr = buffer + *consumed;
 	void __user *end = buffer + size;
@@ -2578,7 +2578,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			goto err;
 		}
 		binder_debug(BINDER_DEBUG_READ_WRITE,
-			     "%d:%d write %ld at %08lx, read %ld at %08lx\n",
+			     "%d:%d write %zd at %016lx, read %zd at %016lx\n",
 			     proc->pid, thread->pid, bwr.write_size,
 			     bwr.write_buffer, bwr.read_size, bwr.read_buffer);
 
@@ -2604,7 +2604,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			}
 		}
 		binder_debug(BINDER_DEBUG_READ_WRITE,
-			     "%d:%d wrote %ld of %ld, read return %ld of %ld\n",
+			     "%d:%d wrote %zd of %zd, read return %zd of %zd\n",
 			     proc->pid, thread->pid, bwr.write_consumed, bwr.write_size,
 			     bwr.read_consumed, bwr.read_size);
 		if (copy_to_user(ubuf, &bwr, sizeof(bwr))) {
diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index dbe81ce..8012921 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -48,13 +48,13 @@ enum {
  */
 struct flat_binder_object {
 	/* 8 bytes for large_flat_header. */
-	unsigned long		type;
-	unsigned long		flags;
+	__u32		type;
+	__u32		flags;
 
 	/* 8 bytes of data. */
 	union {
 		void __user	*binder;	/* local object */
-		signed long	handle;		/* remote object */
+		__s32	    handle;		/* remote object */
 	};
 
 	/* extra data associated with local object */
@@ -67,18 +67,18 @@ struct flat_binder_object {
  */
 
 struct binder_write_read {
-	signed long	write_size;	/* bytes to write */
-	signed long	write_consumed;	/* bytes consumed by driver */
+	size_t  write_size;	/* bytes to write */
+	size_t  write_consumed;	/* bytes consumed by driver */
 	unsigned long	write_buffer;
-	signed long	read_size;	/* bytes to read */
-	signed long	read_consumed;	/* bytes consumed by driver */
+	size_t  read_size;	/* bytes to read */
+	size_t  read_consumed;	/* bytes consumed by driver */
 	unsigned long	read_buffer;
 };
 
 /* Use with BINDER_VERSION, driver fills in fields. */
 struct binder_version {
 	/* driver protocol version -- increment with incompatible change */
-	signed long	protocol_version;
+	__s32       protocol_version;
 };
 
 /* This is the current protocol version. */
-- 
1.7.9.5

Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer

[Arve Hjønnevåg]

On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
<serban.constantinescu@arm.com> wrote:
> The changes in this patch will fix the binder interface for use on 64bit
> machines and stand as the base of the 64bit compat support. The changes
> apply to the structures that are passed between the kernel and
> userspace.
>
> Most of the changes applied mirror the change to struct binder_version
> where there is no need for a 64bit wide protocol_version(on 64bit
> machines). The change inlines with the existing 32bit userspace(the
> structure has the same size) and simplifies the compat layer such that
> the same handler can service the BINDER_VERSION ioctl.
>
> Other changes fix the function prototypes for binder_thread_read/write,
> make use of kernel types as well as user-exportable ones and fix
> format specifier issues.
>
> The changes do not affect existing 32bit ABI.
>
> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
> ---
> drivers/staging/android/binder.c | 28 ++++++++++++++--------------
> drivers/staging/android/binder.h | 16 ++++++++--------
> 2 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index 5794cf6..a2cdd9e 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
...
> @@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
> }
>
> int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
> - void __user *buffer, int size, signed long *consumed)
> + void __user *buffer, size_t size, size_t *consumed)

What is this change for? You changed from a signed type to an unsigned
type which seems unrelated to adding 64 bit support.

...
> diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
> index dbe81ce..8012921 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -48,13 +48,13 @@ enum {
> */
> struct flat_binder_object {
> /* 8 bytes for large_flat_header. */
> - unsigned long type;
> - unsigned long flags;
> + __u32 type;
> + __u32 flags;
>
> /* 8 bytes of data. */
> union {
> void __user *binder; /* local object */
> - signed long handle; /* remote object */
> + __s32 handle; /* remote object */

Why limit the handle to 32 bits when the pointer that it shares
storage with need to be 64 bit on 64 bit systems?

> };
>
> /* extra data associated with local object */
> @@ -67,18 +67,18 @@ struct flat_binder_object {
> */
>
> struct binder_write_read {
> - signed long write_size; /* bytes to write */
> - signed long write_consumed; /* bytes consumed by driver */
> + size_t write_size; /* bytes to write */
> + size_t write_consumed; /* bytes consumed by driver */
> unsigned long write_buffer;
> - signed long read_size; /* bytes to read */
> - signed long read_consumed; /* bytes consumed by driver */
> + size_t read_size; /* bytes to read */
> + size_t read_consumed; /* bytes consumed by driver */

What is this change for? You changed from a signed type to an unsigned
type which seems unrelated to adding 64 bit support.

> unsigned long read_buffer;
> };
>
> /* Use with BINDER_VERSION, driver fills in fields. */
> struct binder_version {
> /* driver protocol version -- increment with incompatible change */
> - signed long protocol_version;
> + __s32 protocol_version;

How does user-space know if it should use 32 bit or 64 bit pointers.
Without this change, the BINDER_VERSION ioctl would only match when
the size of long matches.

> };
>
> /* This is the current protocol version. */
> --
> 1.7.9.5
>


--
Arve Hjønnevåg

Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer

[Serban Constantinescu]

On 10/04/13 00:48, Arve Hjønnevåg wrote:
>> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
>> index 5794cf6..a2cdd9e 100644
>> --- a/drivers/staging/android/binder.c
>> +++ b/drivers/staging/android/binder.c
> ...
>> @@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
>> }
>>
>> int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
>> - void __user *buffer, int size, signed long *consumed)
>> + void __user *buffer, size_t size, size_t *consumed)
>
> What is this change for? You changed from a signed type to an unsigned
> type which seems unrelated to adding 64 bit support.

This change is related to the userspace handling of the struct 
binder_write_read. The userspace writes write_size and read_size as 
size_t values(size_t Parcel::dataSize(), size_t Parcel::dataCapacity()).

On return from a BINDER_WRITE_READ ioctl write_consumed and 
read_consumed are checked against positive values(these values will 
represent the difference between the start of the buffer cursor and the 
current buffer start - positive values since buffer cursor = buffer 
start ++).

However if there is any plan for these values to be handled as signed 
longs at some point we can change the patch such that we modify just the 
function prototype to:

> int binder_thread_write(struct binder_proc *proc, struct binder_thread *thread,
>  -               void __user *buffer, int size, signed long *consumed)
>  +               void __user *buffer, signed long size, signed long *consumed)

I will break this change into its own patch such that it is easier to 
grasp.

>
> ...
>> diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
>> index dbe81ce..8012921 100644
>> --- a/drivers/staging/android/binder.h
>> +++ b/drivers/staging/android/binder.h
>> @@ -48,13 +48,13 @@ enum {
>> */
>> struct flat_binder_object {
>> /* 8 bytes for large_flat_header. */
>> - unsigned long type;
>> - unsigned long flags;
>> + __u32 type;
>> + __u32 flags;
>>
>> /* 8 bytes of data. */
>> union {
>> void __user *binder; /* local object */
>> - signed long handle; /* remote object */
>> + __s32 handle; /* remote object */
>
> Why limit the handle to 32 bits when the pointer that it shares
> storage with need to be 64 bit on 64 bit systems?

Here I have mirrored the type being passed in handle - a file 
descriptor(when type == BINDER_TYPE_FD) or a handle - 32bit(when type == 
BINDER_TYPE_HANDLE). This will avoid some casting when handle is used 
inside the kernel/userspace(as 32bit value on 64bit systems). However 
this change does not limit the extension of the API since we can read 
the value as 64bit - binder(on 64bit systems).

I can remove this change if you consider that is the better solution.

>> };
>>
>> /* extra data associated with local object */
>> @@ -67,18 +67,18 @@ struct flat_binder_object {
>> */
>>
>> struct binder_write_read {
>> - signed long write_size; /* bytes to write */
>> - signed long write_consumed; /* bytes consumed by driver */
>> + size_t write_size; /* bytes to write */
>> + size_t write_consumed; /* bytes consumed by driver */
>> unsigned long write_buffer;
>> - signed long read_size; /* bytes to read */
>> - signed long read_consumed; /* bytes consumed by driver */
>> + size_t read_size; /* bytes to read */
>> + size_t read_consumed; /* bytes consumed by driver */
>
> What is this change for? You changed from a signed type to an unsigned
> type which seems unrelated to adding 64 bit support.

See above explanation for binder_thread_write() change, I will break 
this into its own patch.

>> unsigned long read_buffer;
>> };
>>
>> /* Use with BINDER_VERSION, driver fills in fields. */
>> struct binder_version {
>> /* driver protocol version -- increment with incompatible change */
>> - signed long protocol_version;
>> + __s32 protocol_version;
>
> How does user-space know if it should use 32 bit or 64 bit pointers.
> Without this change, the BINDER_VERSION ioctl would only match when
> the size of long matches.

The userspace can check the values returned by uname(). That will 
determine if the kernel is 32 or 64bit and depending on this select what 
binder structures to use. Next a BINDER_VERSION ioctl will fail on 64bit 
kernels using protocol_version as 64bit signed long(that is old kernel 
versions with no 64bit support).

Leaving this value as signed long would mean that older versions of the 
binder(without 64bit support) will pass the check. Furthermore the 
protocol version will probably never exceed the values that could be 
represented on 32bit. It will also mean that BINDER_VERSION will have a 
different userspace/kernel handler for 64/32 systems.

Let me know what are your thoughts related to these changes,
Thanks for your feedback,
Serban

Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer

[Arve Hjønnevåg]

On Wed, Apr 10, 2013 at 6:01 AM, Serban Constantinescu
<Serban.Constantinescu@arm.com> wrote:
> On 10/04/13 00:48, Arve Hjønnevåg wrote:
>>>
>>> diff --git a/drivers/staging/android/binder.c
>>> b/drivers/staging/android/binder.c
>>> index 5794cf6..a2cdd9e 100644
>>> --- a/drivers/staging/android/binder.c
>>> +++ b/drivers/staging/android/binder.c
>>
>> ...
>>>
>>> @@ -1700,7 +1700,7 @@ err_no_context_mgr_node:
>>> }
>>>
>>> int binder_thread_write(struct binder_proc *proc, struct binder_thread
>>> *thread,
>>> - void __user *buffer, int size, signed long *consumed)
>>> + void __user *buffer, size_t size, size_t *consumed)
>>
>>
>> What is this change for? You changed from a signed type to an unsigned
>> type which seems unrelated to adding 64 bit support.
>
>
> This change is related to the userspace handling of the struct
> binder_write_read. The userspace writes write_size and read_size as size_t
> values(size_t Parcel::dataSize(), size_t Parcel::dataCapacity()).
>
> On return from a BINDER_WRITE_READ ioctl write_consumed and read_consumed
> are checked against positive values(these values will represent the
> difference between the start of the buffer cursor and the current buffer
> start - positive values since buffer cursor = buffer start ++).
>
> However if there is any plan for these values to be handled as signed longs
> at some point we can change the patch such that we modify just the function
> prototype to:
>
>> int binder_thread_write(struct binder_proc *proc, struct binder_thread
>> *thread,
>>  -               void __user *buffer, int size, signed long *consumed)
>>  +               void __user *buffer, signed long size, signed long
>> *consumed)
>
>
> I will break this change into its own patch such that it is easier to grasp.
>

OK.

>
>>
>> ...
>>>
>>> diff --git a/drivers/staging/android/binder.h
>>> b/drivers/staging/android/binder.h
>>> index dbe81ce..8012921 100644
>>> --- a/drivers/staging/android/binder.h
>>> +++ b/drivers/staging/android/binder.h
>>> @@ -48,13 +48,13 @@ enum {
>>> */
>>> struct flat_binder_object {
>>> /* 8 bytes for large_flat_header. */
>>> - unsigned long type;
>>> - unsigned long flags;
>>> + __u32 type;
>>> + __u32 flags;
>>>
>>> /* 8 bytes of data. */
>>> union {
>>> void __user *binder; /* local object */
>>> - signed long handle; /* remote object */
>>> + __s32 handle; /* remote object */
>>
>>
>> Why limit the handle to 32 bits when the pointer that it shares
>> storage with need to be 64 bit on 64 bit systems?
>
>
> Here I have mirrored the type being passed in handle - a file
> descriptor(when type == BINDER_TYPE_FD) or a handle - 32bit(when type ==
> BINDER_TYPE_HANDLE). This will avoid some casting when handle is used inside
> the kernel/userspace(as 32bit value on 64bit systems). However this change
> does not limit the extension of the API since we can read the value as 64bit
> - binder(on 64bit systems).
>
> I can remove this change if you consider that is the better solution.
>

I was asking if we should just use 64 bit handles on 64 bit systems,
not adding casts. It would require another union member for a file
descriptor however.

>
>>> };
>>>
>>> /* extra data associated with local object */
>>> @@ -67,18 +67,18 @@ struct flat_binder_object {
>>> */
>>>
>>> struct binder_write_read {
>>> - signed long write_size; /* bytes to write */
>>> - signed long write_consumed; /* bytes consumed by driver */
>>> + size_t write_size; /* bytes to write */
>>> + size_t write_consumed; /* bytes consumed by driver */
>>> unsigned long write_buffer;
>>> - signed long read_size; /* bytes to read */
>>> - signed long read_consumed; /* bytes consumed by driver */
>>> + size_t read_size; /* bytes to read */
>>> + size_t read_consumed; /* bytes consumed by driver */
>>
>>
>> What is this change for? You changed from a signed type to an unsigned
>> type which seems unrelated to adding 64 bit support.
>
>
> See above explanation for binder_thread_write() change, I will break this
> into its own patch.
>
>
>>> unsigned long read_buffer;
>>> };
>>>
>>> /* Use with BINDER_VERSION, driver fills in fields. */
>>> struct binder_version {
>>> /* driver protocol version -- increment with incompatible change */
>>> - signed long protocol_version;
>>> + __s32 protocol_version;
>>
>>
>> How does user-space know if it should use 32 bit or 64 bit pointers.
>> Without this change, the BINDER_VERSION ioctl would only match when
>> the size of long matches.
>
>
> The userspace can check the values returned by uname(). That will determine
> if the kernel is 32 or 64bit and depending on this select what binder
> structures to use. Next a BINDER_VERSION ioctl will fail on 64bit kernels
> using protocol_version as 64bit signed long(that is old kernel versions with
> no 64bit support).
>
> Leaving this value as signed long would mean that older versions of the
> binder(without 64bit support) will pass the check. Furthermore the protocol
> version will probably never exceed the values that could be represented on
> 32bit. It will also mean that BINDER_VERSION will have a different
> userspace/kernel handler for 64/32 systems.
>
> Let me know what are your thoughts related to these changes,
> Thanks for your feedback,
> Serban
>

I think user-space should get the binder pointer size from the binder
driver, not elsewhere. Since the current driver does not appear to be
functional on a 64 bit system, I think adding an ioctl to get the
size, or encoding it into the binder version (use an unsigned type if
you do this), would be best.

--
Arve Hjønnevåg

Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer

[Serban Constantinescu]

On 10/04/13 23:12, Arve Hjønnevåg wrote:
>>>> struct flat_binder_object {
>>>> /* 8 bytes for large_flat_header. */
>>>> - unsigned long type;
>>>> - unsigned long flags;
>>>> + __u32 type;
>>>> + __u32 flags;
>>>>
>>>> /* 8 bytes of data. */
>>>> union {
>>>> void __user *binder; /* local object */
>>>> - signed long handle; /* remote object */
>>>> + __s32 handle; /* remote object */
>>>
>>>
>>> Why limit the handle to 32 bits when the pointer that it shares
>>> storage with need to be 64 bit on 64 bit systems?
>>
>>
>> Here I have mirrored the type being passed in handle - a file
>> descriptor(when type == BINDER_TYPE_FD) or a handle - 32bit(when type ==
>> BINDER_TYPE_HANDLE). This will avoid some casting when handle is used inside
>> the kernel/userspace(as 32bit value on 64bit systems). However this change
>> does not limit the extension of the API since we can read the value as 64bit
>> - binder(on 64bit systems).
>>
>> I can remove this change if you consider that is the better solution.
>>
>
> I was asking if we should just use 64 bit handles on 64 bit systems,
> not adding casts. It would require another union member for a file
> descriptor however.

I will leave this handle as __s32 for this patch set but I will take a 
look into what and if we need to change this for a 64bit system. From a 
top level perspective(a look at binder_*_ref() functions and the 
userspace equivalent) this should work fine as 32bit.

>>
>>>> };
>>>>
>>>> /* extra data associated with local object */
>>>> @@ -67,18 +67,18 @@ struct flat_binder_object {
>>>> */
>>>>
>>>> struct binder_write_read {
>>>> - signed long write_size; /* bytes to write */
>>>> - signed long write_consumed; /* bytes consumed by driver */
>>>> + size_t write_size; /* bytes to write */
>>>> + size_t write_consumed; /* bytes consumed by driver */
>>>> unsigned long write_buffer;
>>>> - signed long read_size; /* bytes to read */
>>>> - signed long read_consumed; /* bytes consumed by driver */
>>>> + size_t read_size; /* bytes to read */
>>>> + size_t read_consumed; /* bytes consumed by driver */
>>>
>>>
>>> What is this change for? You changed from a signed type to an unsigned
>>> type which seems unrelated to adding 64 bit support.
>>
>>
>> See above explanation for binder_thread_write() change, I will break this
>> into its own patch.
>>
>>
>>>> unsigned long read_buffer;
>>>> };
>>>>
>>>> /* Use with BINDER_VERSION, driver fills in fields. */
>>>> struct binder_version {
>>>> /* driver protocol version -- increment with incompatible change */
>>>> - signed long protocol_version;
>>>> + __s32 protocol_version;
>>>
>>>
>>> How does user-space know if it should use 32 bit or 64 bit pointers.
>>> Without this change, the BINDER_VERSION ioctl would only match when
>>> the size of long matches.
>>
>>
>> The userspace can check the values returned by uname(). That will determine
>> if the kernel is 32 or 64bit and depending on this select what binder
>> structures to use. Next a BINDER_VERSION ioctl will fail on 64bit kernels
>> using protocol_version as 64bit signed long(that is old kernel versions with
>> no 64bit support).
>>
>> Leaving this value as signed long would mean that older versions of the
>> binder(without 64bit support) will pass the check. Furthermore the protocol
>> version will probably never exceed the values that could be represented on
>> 32bit. It will also mean that BINDER_VERSION will have a different
>> userspace/kernel handler for 64/32 systems.
>>
>> Let me know what are your thoughts related to these changes,
>> Thanks for your feedback,
>> Serban
>>
>
> I think user-space should get the binder pointer size from the binder
> driver, not elsewhere. Since the current driver does not appear to be
> functional on a 64 bit system, I think adding an ioctl to get the
> size, or encoding it into the binder version (use an unsigned type if
> you do this), would be best.

I will think about the best way of getting the pointer size and add it 
to the patch set for binder compat. For this patch set I will only 
modify the protocol_version from long to __s32.

Thanks for your feedback,
Serban

Re: [PATCH v2 3/7] staging: android: binder: fix binder interface for 64bit compat layer

[Arve Hjønnevåg]

On Thu, Apr 11, 2013 at 8:13 AM, Serban Constantinescu
<Serban.Constantinescu@arm.com> wrote:
...
>>>
>>>>> unsigned long read_buffer;
>>>>> };
>>>>>
>>>>> /* Use with BINDER_VERSION, driver fills in fields. */
>>>>> struct binder_version {
>>>>> /* driver protocol version -- increment with incompatible change */
>>>>> - signed long protocol_version;
>>>>> + __s32 protocol_version;
>>>>
>>>>
>>>>
>>>> How does user-space know if it should use 32 bit or 64 bit pointers.
>>>> Without this change, the BINDER_VERSION ioctl would only match when
>>>> the size of long matches.
>>>
>>>
>>>
>>> The userspace can check the values returned by uname(). That will
>>> determine
>>> if the kernel is 32 or 64bit and depending on this select what binder
>>> structures to use. Next a BINDER_VERSION ioctl will fail on 64bit kernels
>>> using protocol_version as 64bit signed long(that is old kernel versions
>>> with
>>> no 64bit support).
>>>
>>> Leaving this value as signed long would mean that older versions of the
>>> binder(without 64bit support) will pass the check. Furthermore the
>>> protocol
>>> version will probably never exceed the values that could be represented
>>> on
>>> 32bit. It will also mean that BINDER_VERSION will have a different
>>> userspace/kernel handler for 64/32 systems.
>>>
>>> Let me know what are your thoughts related to these changes,
>>> Thanks for your feedback,
>>> Serban
>>>
>>
>> I think user-space should get the binder pointer size from the binder
>> driver, not elsewhere. Since the current driver does not appear to be
>> functional on a 64 bit system, I think adding an ioctl to get the
>> size, or encoding it into the binder version (use an unsigned type if
>> you do this), would be best.
>
>
> I will think about the best way of getting the pointer size and add it to
> the patch set for binder compat. For this patch set I will only modify the
> protocol_version from long to __s32.
>

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.

--
Arve Hjønnevåg

[PATCH v2 4/7] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration

[Serban Constantinescu]

This change will fix the BINDER_SET_MAX_THREADS ioctl to use __s32
instead of size_t for setting the max threads. Thus using the same
handler for 32 and 64bit kernels.

This value is stored internally in struct binder_proc as an int and
is set to 15 on open_binder() in the libbinder API (thus no need for
a 64bit size_t on 64bit platforms).

The change does not affect existing 32bit ABI.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
 drivers/staging/android/binder.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index 8012921..5b9909f 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -86,7 +86,7 @@ struct binder_version {
 
 #define BINDER_WRITE_READ		_IOWR('b', 1, struct binder_write_read)
 #define	BINDER_SET_IDLE_TIMEOUT		_IOW('b', 3, __s64)
-#define	BINDER_SET_MAX_THREADS		_IOW('b', 5, size_t)
+#define	BINDER_SET_MAX_THREADS		_IOW('b', 5, __s32)
 #define	BINDER_SET_IDLE_PRIORITY	_IOW('b', 6, __s32)
 #define	BINDER_SET_CONTEXT_MGR		_IOW('b', 7, __s32)
 #define	BINDER_THREAD_EXIT		_IOW('b', 8, __s32)
-- 
1.7.9.5

Re: [PATCH v2 4/7] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration

[Arve Hjønnevåg]

On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
<serban.constantinescu@arm.com> wrote:
> This change will fix the BINDER_SET_MAX_THREADS ioctl to use __s32
> instead of size_t for setting the max threads. Thus using the same
> handler for 32 and 64bit kernels.
>
> This value is stored internally in struct binder_proc as an int and
> is set to 15 on open_binder() in the libbinder API (thus no need for
> a 64bit size_t on 64bit platforms).
>

Why switch to a signed type?

Re: [PATCH v2 4/7] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration

[Serban Constantinescu]

On 10/04/13 00:53, Arve Hjønnevåg wrote:
> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
> <serban.constantinescu@arm.com> wrote:
>> This change will fix the BINDER_SET_MAX_THREADS ioctl to use __s32
>> instead of size_t for setting the max threads. Thus using the same
>> handler for 32 and 64bit kernels.
>>
>> This value is stored internally in struct binder_proc as an int and
>> is set to 15 on open_binder() in the libbinder API (thus no need for
>> a 64bit size_t on 64bit platforms).
>>
>
> Why switch to a signed type?

The value passed through BINDER_SET_MAX_THREADS ioctl is stored in a 
binder_proc structure as an int.It also mimics the use of pid_t(typedef 
int __kernel_pid_t).

However using __s32 or __u32 here would have the same effect since the 
ioctl macro will compute both as sizeof(32bit).

Let me know if you would like this changed to __u32.

Thanks,
Serban

Re: [PATCH v2 4/7] staging: android: binder: fix BINDER_SET_MAX_THREADS declaration

[Arve Hjønnevåg]

On Wed, Apr 10, 2013 at 5:37 AM, Serban Constantinescu
<Serban.Constantinescu@arm.com> wrote:
> On 10/04/13 00:53, Arve Hjønnevåg wrote:
>>
>> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
>> <serban.constantinescu@arm.com> wrote:
>>>
>>> This change will fix the BINDER_SET_MAX_THREADS ioctl to use __s32
>>> instead of size_t for setting the max threads. Thus using the same
>>> handler for 32 and 64bit kernels.
>>>
>>> This value is stored internally in struct binder_proc as an int and
>>> is set to 15 on open_binder() in the libbinder API (thus no need for
>>> a 64bit size_t on 64bit platforms).
>>>
>>
>> Why switch to a signed type?
>
>
> The value passed through BINDER_SET_MAX_THREADS ioctl is stored in a
> binder_proc structure as an int.It also mimics the use of pid_t(typedef int
> __kernel_pid_t).
>

This is a thread count not a pid.

> However using __s32 or __u32 here would have the same effect since the ioctl
> macro will compute both as sizeof(32bit).
>
> Let me know if you would like this changed to __u32.
>

The user-space api uses a size_t, so __u32 would be a closer match.
Keeping it a size_t would also work since this value is not shared
between processes.

--
Arve Hjønnevåg

[PATCH v2 5/7] staging: android: binder: fix BC_FREE_BUFFER ioctl declaration

[Serban Constantinescu]

BinderDriverCommands mirror the ioctl usage. Thus the size of the
structure passed through the interface should be used to generate the
ioctl No.

The change reflects the type being passed from the user space-a pointer
to a binder_buffer. This change should not affect the existing 32bit
user space since BC_FREE_BUFFER is computed as:

   #define _IOW(type,nr,size)         \
      ((type) << _IOC_TYPESHIFT) |    \
      ((nr)   << _IOC_NRSHIFT) |      \
      ((size) << _IOC_SIZESHIFT))

and for a 32bit compiler BC_FREE_BUFFER will have the same computed
value. This change will also ease our work in differentiating
BC_FREE_BUFFER from COMPAT_BC_FREE_BUFFER.

The change does not affect existing 32bit ABI.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
 drivers/staging/android/binder.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index 5b9909f..8789baa 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -265,7 +265,7 @@ enum binder_driver_command_protocol {
 	 * Else you have acquired a primary reference on the object.
 	 */
 
-	BC_FREE_BUFFER = _IOW('c', 3, int),
+	BC_FREE_BUFFER = _IOW('c', 3, void *),
 	/*
 	 * void *: ptr to transaction data received on a read
 	 */
-- 
1.7.9.5

Re: [PATCH v2 5/7] staging: android: binder: fix BC_FREE_BUFFER ioctl declaration

[Arve Hjønnevåg]

On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
<serban.constantinescu@arm.com> wrote:
> BinderDriverCommands mirror the ioctl usage. Thus the size of the
> structure passed through the interface should be used to generate the
> ioctl No.
>
> The change reflects the type being passed from the user space-a pointer
> to a binder_buffer. This change should not affect the existing 32bit
> user space since BC_FREE_BUFFER is computed as:
>
>    #define _IOW(type,nr,size)         \
>       ((type) << _IOC_TYPESHIFT) |    \
>       ((nr)   << _IOC_NRSHIFT) |      \
>       ((size) << _IOC_SIZESHIFT))
>
> and for a 32bit compiler BC_FREE_BUFFER will have the same computed
> value. This change will also ease our work in differentiating
> BC_FREE_BUFFER from COMPAT_BC_FREE_BUFFER.
>
> The change does not affect existing 32bit ABI.
>
> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>

Acked-by:  Arve Hjønnevåg <arve@android.com>

--
Arve Hjønnevåg

[PATCH v2 6/7] staging: android: binder: fix alignment issues

[Serban Constantinescu]

The Android userspace aligns the data written to the binder buffers to
4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit
Android userspace we can have a buffer looking like this:

platform    buffer(binder_cmd   pointer)      size
32/32                 32b         32b          8B
64/32                 32b         64b          12B
64/64                 32b         64b          12B

Thus the kernel needs to check that the buffer size is aligned to 4bytes
not to (void *) that will be 8bytes on 64bit machines.

The change does not affect existing 32bit ABI.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
 drivers/staging/android/binder.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index a2cdd9e..b5c2b59 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -658,8 +658,8 @@ static struct binder_buffer *binder_alloc_buf(struct binder_proc *proc,
 		return NULL;
 	}
 
-	size = ALIGN(data_size, sizeof(void *)) +
-		ALIGN(offsets_size, sizeof(void *));
+	size = ALIGN(data_size, sizeof(u32)) +
+		ALIGN(offsets_size, sizeof(u32));
 
 	if (size < data_size || size < offsets_size) {
 		binder_user_error("%d: got transaction with invalid size %zd-%zd\n",
@@ -807,8 +807,8 @@ static void binder_free_buf(struct binder_proc *proc,
 
 	buffer_size = binder_buffer_size(proc, buffer);
 
-	size = ALIGN(buffer->data_size, sizeof(void *)) +
-		ALIGN(buffer->offsets_size, sizeof(void *));
+	size = ALIGN(buffer->data_size, sizeof(u32)) +
+		ALIGN(buffer->offsets_size, sizeof(u32));
 
 	binder_debug(BINDER_DEBUG_BUFFER_ALLOC,
 		     "%d: binder_free_buf %p size %zd buffer_size %zd\n",
@@ -1247,7 +1247,7 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 		struct flat_binder_object *fp;
 		if (*offp > buffer->data_size - sizeof(*fp) ||
 		    buffer->data_size < sizeof(*fp) ||
-		    !IS_ALIGNED(*offp, sizeof(void *))) {
+		    !IS_ALIGNED(*offp, sizeof(u32))) {
 			pr_err("transaction release %d bad offset %zd, size %zd\n",
 			 debug_id, *offp, buffer->data_size);
 			continue;
@@ -1496,7 +1496,7 @@ static void binder_transaction(struct binder_proc *proc,
 		struct flat_binder_object *fp;
 		if (*offp > t->buffer->data_size - sizeof(*fp) ||
 		    t->buffer->data_size < sizeof(*fp) ||
-		    !IS_ALIGNED(*offp, sizeof(void *))) {
+		    !IS_ALIGNED(*offp, sizeof(u32))) {
 			binder_user_error("%d:%d got transaction with invalid offset, %zd\n",
 					proc->pid, thread->pid, *offp);
 			return_error = BR_FAILED_REPLY;
-- 
1.7.9.5

Re: [PATCH v2 6/7] staging: android: binder: fix alignment issues

[Arve Hjønnevåg]

On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
<serban.constantinescu@arm.com> wrote:
> The Android userspace aligns the data written to the binder buffers to
> 4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit
> Android userspace we can have a buffer looking like this:
>
> platform    buffer(binder_cmd   pointer)      size
> 32/32                 32b         32b          8B
> 64/32                 32b         64b          12B
> 64/64                 32b         64b          12B
>
> Thus the kernel needs to check that the buffer size is aligned to 4bytes
> not to (void *) that will be 8bytes on 64bit machines.
>
> The change does not affect existing 32bit ABI.
>

Do we not want the pointers to be 8 byte aligned on 64bit platforms?

--
Arve Hjønnevåg

Re: [PATCH v2 6/7] staging: android: binder: fix alignment issues

[Serban Constantinescu]

On 10/04/13 00:58, Arve Hjønnevåg wrote:
> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
> <serban.constantinescu@arm.com> wrote:
>> The Android userspace aligns the data written to the binder buffers to
>> 4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit
>> Android userspace we can have a buffer looking like this:
>>
>> platform    buffer(binder_cmd   pointer)      size
>> 32/32                 32b         32b          8B
>> 64/32                 32b         64b          12B
>> 64/64                 32b         64b          12B
>>
>> Thus the kernel needs to check that the buffer size is aligned to 4bytes
>> not to (void *) that will be 8bytes on 64bit machines.
>>
>> The change does not affect existing 32bit ABI.
>>
>
> Do we not want the pointers to be 8 byte aligned on 64bit platforms?

No since here we do not align pointers we align binder_buffers and 
offsets in a buffer.

Let's assume that from the userspace we receive a sequence of BC_INCREFS 
and BC_FREE_BUFFER. According to their definitions the buffer would look 
like:

Buffer:
[addr]		[element]
0		BC_INCREFS
4		__u32
8		BC_FREE_BUFFER
12		void *        //(8 bytes for 64bit or 4 bytes for 32bit)

Thus the data_size(sizeof(Buffer)) will be 20 bytes for 64bit 
systems(4bytes aligned). Same explanation for offp where it represents 
the offset form the start of the buffer to a flat_binder_object(for 
example here the offset to void* - 12bytes).


Thanks,
Serban

Re: [PATCH v2 6/7] staging: android: binder: fix alignment issues

[Arve Hjønnevåg]

On Wed, Apr 10, 2013 at 9:39 AM, Serban Constantinescu
<Serban.Constantinescu@arm.com> wrote:
> On 10/04/13 00:58, Arve Hjønnevåg wrote:
>>
>> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
>> <serban.constantinescu@arm.com> wrote:
>>>
>>> The Android userspace aligns the data written to the binder buffers to
>>> 4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit
>>> Android userspace we can have a buffer looking like this:
>>>
>>> platform    buffer(binder_cmd   pointer)      size
>>> 32/32                 32b         32b          8B
>>> 64/32                 32b         64b          12B
>>> 64/64                 32b         64b          12B
>>>
>>> Thus the kernel needs to check that the buffer size is aligned to 4bytes
>>> not to (void *) that will be 8bytes on 64bit machines.
>>>
>>> The change does not affect existing 32bit ABI.
>>>
>>
>> Do we not want the pointers to be 8 byte aligned on 64bit platforms?
>
>
> No since here we do not align pointers we align binder_buffers and offsets
> in a buffer.
>

Do any 64 bit systems align pointers in a struct to 8 bytes? If so, we
should keep the start address of the struct 8 byte aligned as well.

> Let's assume that from the userspace we receive a sequence of BC_INCREFS and
> BC_FREE_BUFFER. According to their definitions the buffer would look like:
>
> Buffer:
> [addr]          [element]
> 0               BC_INCREFS
> 4               __u32
> 8               BC_FREE_BUFFER
> 12              void *        //(8 bytes for 64bit or 4 bytes for 32bit)
>
> Thus the data_size(sizeof(Buffer)) will be 20 bytes for 64bit systems(4bytes
> aligned). Same explanation for offp where it represents the offset form the
> start of the buffer to a flat_binder_object(for example here the offset to
> void* - 12bytes).
>

Does this work on every 64 bit system?

--
Arve Hjønnevåg

Re: [PATCH v2 6/7] staging: android: binder: fix alignment issues

[Serban Constantinescu]

On 10/04/13 23:30, Arve Hjønnevåg wrote:
> On Wed, Apr 10, 2013 at 9:39 AM, Serban Constantinescu
> <Serban.Constantinescu@arm.com> wrote:
>> On 10/04/13 00:58, Arve Hjønnevåg wrote:
>>>
>>> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
>>> <serban.constantinescu@arm.com> wrote:
>>>>
>>>> The Android userspace aligns the data written to the binder buffers to
>>>> 4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit
>>>> Android userspace we can have a buffer looking like this:
>>>>
>>>> platform    buffer(binder_cmd   pointer)      size
>>>> 32/32                 32b         32b          8B
>>>> 64/32                 32b         64b          12B
>>>> 64/64                 32b         64b          12B
>>>>
>>>> Thus the kernel needs to check that the buffer size is aligned to 4bytes
>>>> not to (void *) that will be 8bytes on 64bit machines.
>>>>
>>>> The change does not affect existing 32bit ABI.
>>>>
>>>
>>> Do we not want the pointers to be 8 byte aligned on 64bit platforms?
>>
>>
>> No since here we do not align pointers we align binder_buffers and offsets
>> in a buffer.
>>
>
> Do any 64 bit systems align pointers in a struct to 8 bytes? If so, we
> should keep the start address of the struct 8 byte aligned as well.

Most of 64bit compilers will try to align pointers within a structure to 
natural boundaries. However all 64bit variants of currently supported 
Android architectures can service unaligned accesses(possibly with a 
performance degradation compared to an aligned access).

You can take a look at alignment requirements for AArch64 here 
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055a/IHI0055A_aapcs64.pdf 
chapter 4.

What we are modifying in this patch is the alignment requirements on the 
buffer size(as seen above - arbitrary size 4byte aligned) and the 
alignment check on offp.

Let's take a look at what offp does. The userspace will write object 
references to a buffer using:

>>  820 status_t Parcel::writeObject(const flat_binder_object& val, bool nullMetaData)
>>  ...
>>  826         *reinterpret_cast<flat_binder_object*>(mData+mDataPos) = val;

Buffer
|---------------------------------------|val
|					|
|->mData				|->mDataPos	

where mData is the start of the buffer and mDataPos the current position 
within the buffer(equivalent to offp in the kernel space). Since the 
buffer is guaranteed to be u32 aligned but not u64 aligned the pointer 
to flat_binder_object might live on a unaligned boundary(offp will 
always be aligned to sizeof(u32) - see Parcel::writeAligned()).

However this will happen only on buffers where at the time we write the
next object reference(val) the buffer cursor(mDataPos) happens not to be 
on a multiple of sizeof(void *).

Adding an alignment check in the userspace might be more costly than
servicing the unaligned access(for AArch64 serviced in hardware). Also 
we will save some memory by not adding the padding.

On the other hand if instead of writing a pointer we write a 64bit mutex 
lock to an unaligned address and than try to read it in the kernel side 
things are not guaranteed to be sane. The compiler could make the 
assumption that the lock is natural aligned and use load/store exclusive 
that will fail on an unaligned address. However for this situation we 
can extend the userspace API and add a mutex write primitive like:


> status_t Parcel::writeMutex(mutex lock)
> ...
> *reinterpret_cast<mutex>(ALIGN_CHECK_AND_PAD(mData+mDataPos)) = lock;

I am not aware of any situation where you will have 64bit mutexes passed 
in a binder buffer but you would probably know more about this. Since 
all writes to the buffer are 32bit aligned a 32bit mutex would not 
suffer any alignment issues.

Let me know what are your thoughts about this.

>> Let's assume that from the userspace we receive a sequence of BC_INCREFS and
>> BC_FREE_BUFFER. According to their definitions the buffer would look like:
>>
>> Buffer:
>> [addr]          [element]
>> 0               BC_INCREFS
>> 4               __u32
>> 8               BC_FREE_BUFFER
>> 12              void *        //(8 bytes for 64bit or 4 bytes for 32bit)
>>
>> Thus the data_size(sizeof(Buffer)) will be 20 bytes for 64bit systems(4bytes
>> aligned). Same explanation for offp where it represents the offset form the
>> start of the buffer to a flat_binder_object(for example here the offset to
>> void* - 12bytes).
>>
>
> Does this work on every 64 bit system?

See above.


Thanks for your feedback,
Serban

Re: [PATCH v2 6/7] staging: android: binder: fix alignment issues

[Arve Hjønnevåg]

On Thu, Apr 11, 2013 at 12:02 PM, Serban Constantinescu
<Serban.Constantinescu@arm.com> wrote:
> On 10/04/13 23:30, Arve Hjønnevåg wrote:
>>
>> On Wed, Apr 10, 2013 at 9:39 AM, Serban Constantinescu
>> <Serban.Constantinescu@arm.com> wrote:
>>>
>>> On 10/04/13 00:58, Arve Hjønnevåg wrote:
>>>>
>>>>
>>>> On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
>>>> <serban.constantinescu@arm.com> wrote:
>>>>>
>>>>>
>>>>> The Android userspace aligns the data written to the binder buffers to
>>>>> 4bytes. Thus for 32bit platforms or 64bit platforms running an 32bit
>>>>> Android userspace we can have a buffer looking like this:
>>>>>
>>>>> platform    buffer(binder_cmd   pointer)      size
>>>>> 32/32                 32b         32b          8B
>>>>> 64/32                 32b         64b          12B
>>>>> 64/64                 32b         64b          12B
>>>>>
>>>>> Thus the kernel needs to check that the buffer size is aligned to
>>>>> 4bytes
>>>>> not to (void *) that will be 8bytes on 64bit machines.
>>>>>
>>>>> The change does not affect existing 32bit ABI.
>>>>>
>>>>
>>>> Do we not want the pointers to be 8 byte aligned on 64bit platforms?
>>>
>>>
>>>
>>> No since here we do not align pointers we align binder_buffers and
>>> offsets
>>> in a buffer.
>>>
>>
>> Do any 64 bit systems align pointers in a struct to 8 bytes? If so, we
>> should keep the start address of the struct 8 byte aligned as well.
>
>
> Most of 64bit compilers will try to align pointers within a structure to
> natural boundaries. However all 64bit variants of currently supported
> Android architectures can service unaligned accesses(possibly with a
> performance degradation compared to an aligned access).
>
> You can take a look at alignment requirements for AArch64 here
> http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055a/IHI0055A_aapcs64.pdf
> chapter 4.
>
> What we are modifying in this patch is the alignment requirements on the
> buffer size(as seen above - arbitrary size 4byte aligned) and the alignment
> check on offp.
>

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?

I don't think there is any reason to not keep the binder_buffer and
offsets buffer that the kernel allocates aligned to 8 bytes on a 64
bit system. Also, I don't see any changes to where the offsets buffer
starts in this patch, so if datasize is not 8 bytes aligned you seem
to allocate less memory than you use.

> Let's take a look at what offp does. The userspace will write object
> references to a buffer using:
>
>>>  820 status_t Parcel::writeObject(const flat_binder_object& val, bool
>>> nullMetaData)

>>>  ...
>>>  826         *reinterpret_cast<flat_binder_object*>(mData+mDataPos) =
>>> val;
>
>
> Buffer
> |---------------------------------------|val
> |                                       |
> |->mData                                |->mDataPos
>
> where mData is the start of the buffer and mDataPos the current position
> within the buffer(equivalent to offp in the kernel space). Since the buffer
> is guaranteed to be u32 aligned but not u64 aligned the pointer to
> flat_binder_object might live on a unaligned boundary(offp will always be
> aligned to sizeof(u32) - see Parcel::writeAligned()).
>
> However this will happen only on buffers where at the time we write the
> next object reference(val) the buffer cursor(mDataPos) happens not to be on
> a multiple of sizeof(void *).
>
> Adding an alignment check in the userspace might be more costly than
> servicing the unaligned access(for AArch64 serviced in hardware). Also we
> will save some memory by not adding the padding.
>
> On the other hand if instead of writing a pointer we write a 64bit mutex
> lock to an unaligned address and than try to read it in the kernel side
> things are not guaranteed to be sane. The compiler could make the assumption
> that the lock is natural aligned and use load/store exclusive that will fail
> on an unaligned address. However for this situation we can extend the
> userspace API and add a mutex write primitive like:
>
>
>> status_t Parcel::writeMutex(mutex lock)
>> ...
>> *reinterpret_cast<mutex>(ALIGN_CHECK_AND_PAD(mData+mDataPos)) = lock;
>
>
> I am not aware of any situation where you will have 64bit mutexes passed in
> a binder buffer but you would probably know more about this. Since all
> writes to the buffer are 32bit aligned a 32bit mutex would not suffer any
> alignment issues.
>
> Let me know what are your thoughts about this.
>

Storing a mutex in a parcel does not make sense. The data in the
parcel is a copy of the data passed in, and the parcel seen by the
remote process is a copy of the parcel that sender created.

--
Arve Hjønnevåg

[PATCH v2 7/7] staging: android: binder: replace types with portable ones

[Serban Constantinescu]

Since this driver is meant to be used on different types of processors
and a portable driver should specify the size a variable expects to be
this patch changes the types used throughout the binder interface.

We use "userspace" types since this header will be exported and used by
the Android filesystem.

The patch does not change in any way the functionality of the binder driver.

Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
---
 drivers/staging/android/binder.h |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index 8789baa..f3ffacd 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -123,10 +123,10 @@ struct binder_transaction_data {
 		void	*ptr;	/* target descriptor of return transaction */
 	} target;
 	void		*cookie;	/* target object cookie */
-	unsigned int	code;		/* transaction command */
+	__u32		code;		/* transaction command */
 
 	/* General information about the transaction. */
-	unsigned int	flags;
+	__u32	        flags;
 	pid_t		sender_pid;
 	uid_t		sender_euid;
 	size_t		data_size;	/* number of bytes of data */
@@ -143,7 +143,7 @@ struct binder_transaction_data {
 			/* offsets from buffer to flat_binder_object structs */
 			const void __user	*offsets;
 		} ptr;
-		uint8_t	buf[8];
+		__u8	buf[8];
 	} data;
 };
 
@@ -153,18 +153,18 @@ struct binder_ptr_cookie {
 };
 
 struct binder_pri_desc {
-	int priority;
-	int desc;
+	__s32 priority;
+	__s32 desc;
 };
 
 struct binder_pri_ptr_cookie {
-	int priority;
+	__s32 priority;
 	void *ptr;
 	void *cookie;
 };
 
 enum binder_driver_return_protocol {
-	BR_ERROR = _IOR('r', 0, int),
+	BR_ERROR = _IOR('r', 0, __s32),
 	/*
 	 * int: error code
 	 */
@@ -178,7 +178,7 @@ enum binder_driver_return_protocol {
 	 * binder_transaction_data: the received command.
 	 */
 
-	BR_ACQUIRE_RESULT = _IOR('r', 4, int),
+	BR_ACQUIRE_RESULT = _IOR('r', 4, __s32),
 	/*
 	 * not currently supported
 	 * int: 0 if the last bcATTEMPT_ACQUIRE was not successful.
@@ -258,7 +258,7 @@ enum binder_driver_command_protocol {
 	 * binder_transaction_data: the sent command.
 	 */
 
-	BC_ACQUIRE_RESULT = _IOW('c', 2, int),
+	BC_ACQUIRE_RESULT = _IOW('c', 2, __s32),
 	/*
 	 * not currently supported
 	 * int:  0 if the last BR_ATTEMPT_ACQUIRE was not successful.
@@ -270,10 +270,10 @@ enum binder_driver_command_protocol {
 	 * void *: ptr to transaction data received on a read
 	 */
 
-	BC_INCREFS = _IOW('c', 4, int),
-	BC_ACQUIRE = _IOW('c', 5, int),
-	BC_RELEASE = _IOW('c', 6, int),
-	BC_DECREFS = _IOW('c', 7, int),
+	BC_INCREFS = _IOW('c', 4, __s32),
+	BC_ACQUIRE = _IOW('c', 5, __s32),
+	BC_RELEASE = _IOW('c', 6, __s32),
+	BC_DECREFS = _IOW('c', 7, __s32),
 	/*
 	 * int:	descriptor
 	 */
-- 
1.7.9.5

Re: [PATCH v2 7/7] staging: android: binder: replace types with portable ones

[Arve Hjønnevåg]

On Tue, Apr 9, 2013 at 3:00 AM, Serban Constantinescu
<serban.constantinescu@arm.com> wrote:
> Since this driver is meant to be used on different types of processors
> and a portable driver should specify the size a variable expects to be
> this patch changes the types used throughout the binder interface.
>
> We use "userspace" types since this header will be exported and used by
> the Android filesystem.
>
> The patch does not change in any way the functionality of the binder driver.
>
> Signed-off-by: Serban Constantinescu <serban.constantinescu@arm.com>
> ---
>  drivers/staging/android/binder.h |   26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
> index 8789baa..f3ffacd 100644
> --- a/drivers/staging/android/binder.h
> +++ b/drivers/staging/android/binder.h
> @@ -123,10 +123,10 @@ struct binder_transaction_data {
>                 void    *ptr;   /* target descriptor of return transaction */
>         } target;
>         void            *cookie;        /* target object cookie */
> -       unsigned int    code;           /* transaction command */
> +       __u32           code;           /* transaction command */
>
>         /* General information about the transaction. */
> -       unsigned int    flags;
> +       __u32           flags;
>         pid_t           sender_pid;
>         uid_t           sender_euid;
>         size_t          data_size;      /* number of bytes of data */
> @@ -143,7 +143,7 @@ struct binder_transaction_data {
>                         /* offsets from buffer to flat_binder_object structs */
>                         const void __user       *offsets;
>                 } ptr;
> -               uint8_t buf[8];
> +               __u8    buf[8];
>         } data;
>  };
>
> @@ -153,18 +153,18 @@ struct binder_ptr_cookie {
>  };
>
>  struct binder_pri_desc {
> -       int priority;
> -       int desc;
> +       __s32 priority;
> +       __s32 desc;
>  };
>
>  struct binder_pri_ptr_cookie {
> -       int priority;
> +       __s32 priority;
>         void *ptr;
>         void *cookie;
>  };
>
>  enum binder_driver_return_protocol {
> -       BR_ERROR = _IOR('r', 0, int),
> +       BR_ERROR = _IOR('r', 0, __s32),
>         /*
>          * int: error code
>          */
> @@ -178,7 +178,7 @@ enum binder_driver_return_protocol {
>          * binder_transaction_data: the received command.
>          */
>
> -       BR_ACQUIRE_RESULT = _IOR('r', 4, int),
> +       BR_ACQUIRE_RESULT = _IOR('r', 4, __s32),
>         /*
>          * not currently supported
>          * int: 0 if the last bcATTEMPT_ACQUIRE was not successful.
> @@ -258,7 +258,7 @@ enum binder_driver_command_protocol {
>          * binder_transaction_data: the sent command.
>          */
>
> -       BC_ACQUIRE_RESULT = _IOW('c', 2, int),
> +       BC_ACQUIRE_RESULT = _IOW('c', 2, __s32),
>         /*
>          * not currently supported
>          * int:  0 if the last BR_ATTEMPT_ACQUIRE was not successful.
> @@ -270,10 +270,10 @@ enum binder_driver_command_protocol {
>          * void *: ptr to transaction data received on a read
>          */
>
> -       BC_INCREFS = _IOW('c', 4, int),
> -       BC_ACQUIRE = _IOW('c', 5, int),
> -       BC_RELEASE = _IOW('c', 6, int),
> -       BC_DECREFS = _IOW('c', 7, int),
> +       BC_INCREFS = _IOW('c', 4, __s32),
> +       BC_ACQUIRE = _IOW('c', 5, __s32),
> +       BC_RELEASE = _IOW('c', 6, __s32),
> +       BC_DECREFS = _IOW('c', 7, __s32),

These four are actually read as unsigned values, so it would be better
to use __u32 here.

>         /*
>          * int: descriptor
>          */
> --
> 1.7.9.5
>



--
Arve Hjønnevåg