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.
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.
Best Regards,
Serban Constantinescu
Serban Constantinescu (8):
staging: android: binder: replace explicit size 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 printk() format specifier
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
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 <[email protected]>
---
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
Since the binder driver uses both uint32_t and unsigned int any further
kernel changes will be difficult to read. This patch fixes the inconsistent
types usage.
The patch does not change in any way the functionality of the binder driver.
Signed-off-by: Serban Constantinescu <[email protected]>
---
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
Fix format specifiers warnings introduced by the previous patch which
changes some structures in binder.h.
The patch does not change in any way the functionality of the binder driver.
Signed-off-by: Serban Constantinescu <[email protected]>
---
drivers/staging/android/binder.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index caa6dde..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;
@@ -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))) {
--
1.7.9.5
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
and make use of kernel types as well as user-exportable ones.
The changes do not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu <[email protected]>
---
drivers/staging/android/binder.c | 6 +++---
drivers/staging/android/binder.h | 16 ++++++++--------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 5794cf6..caa6dde 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)
{
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;
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
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 <[email protected]>
---
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
On Thu, Apr 04, 2013 at 01:32:30PM +0100, Serban Constantinescu wrote:
> 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.
>
> 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.
I need some acks from some of the Android developers before I can take
this.
thanks,
greg k-h
On Thu, Apr 4, 2013 at 5:32 AM, Serban Constantinescu
<[email protected]> wrote:
>
> Since the binder driver uses both uint32_t and unsigned int any further
> kernel changes will be difficult to read. This patch fixes the inconsistent
> types usage.
>
Would it make more sense to only change the types that need to be
larger on a 64 bit system?
--
Arve Hj?nnev?g
On Fri, Apr 5, 2013 at 3:00 PM, Greg KH <[email protected]> wrote:
> On Thu, Apr 04, 2013 at 01:32:30PM +0100, Serban Constantinescu wrote:
>> 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.
>>
>> 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.
>
> I need some acks from some of the Android developers before I can take
> this.
>
I still think it is better to change user-space to use 64 bit pointer
types when running on a 64 bit kernel. These changes do not seem to
allow 64 bit user-space processes on a 64 bit kernel.
--
Arve Hj?nnev?g
On Fri, Apr 05, 2013 at 04:17:47PM -0700, Arve Hj?nnev?g wrote:
> On Thu, Apr 4, 2013 at 5:32 AM, Serban Constantinescu
> <[email protected]> wrote:
> >
> > Since the binder driver uses both uint32_t and unsigned int any further
> > kernel changes will be difficult to read. This patch fixes the inconsistent
> > types usage.
> >
>
> Would it make more sense to only change the types that need to be
> larger on a 64 bit system?
I agree. You are also changing the type from explicit to "unexplicit",
the exact opposite from what I would be expecting to see here. Why?
On Thu, Apr 04, 2013 at 01:32:34PM +0100, Serban Constantinescu wrote:
> Fix format specifiers warnings introduced by the previous patch which
> changes some structures in binder.h.
Please merge this with the patch that caused the problems.
On 06/04/13 07:00, Greg KH wrote:
> On Fri, Apr 05, 2013 at 04:17:47PM -0700, Arve Hj?nnev?g wrote:
>> On Thu, Apr 4, 2013 at 5:32 AM, Serban Constantinescu
>> <[email protected]> wrote:
>>>
>>> Since the binder driver uses both uint32_t and unsigned int any further
>>> kernel changes will be difficult to read. This patch fixes the inconsistent
>>> types usage.
>>>
>>
>> Would it make more sense to only change the types that need to be
>> larger on a 64 bit system?
>
> I agree. You are also changing the type from explicit to "unexplicit",
> the exact opposite from what I would be expecting to see here. Why?
I have changed the types used so that they seem consistent throughout
the driver(uint32_t is used in some of the internal structures and some
of the function prototypes). Changing these types to kernel explicit
types would have meant that we would have the same inconsistency for u32
types.
We can live without this patch and I can take a look at modifying the
function prototypes, internal structures and kernel/userspace
interaction to use explicit kernel types it that is preferred.
Thanks for your feedback,
Serban
On 06/04/13 00:38, Arve Hj?nnev?g wrote:
> On Fri, Apr 5, 2013 at 3:00 PM, Greg KH <[email protected]> wrote:
>> On Thu, Apr 04, 2013 at 01:32:30PM +0100, Serban Constantinescu wrote:
>>> 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.
>>>
>>> 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.
>>
>> I need some acks from some of the Android developers before I can take
>> this.
>>
>
> I still think it is better to change user-space to use 64 bit pointer
> types when running on a 64 bit kernel. These changes do not seem to
> allow 64 bit user-space processes on a 64 bit kernel.
This patch set is independent of whether the binder compat layer lives
in the kernel or in the userspace. However we need these changes so that
the same kernel driver supports calls from 32bit userspace and calls
from 64bit userspace(from the binder kernel driver perspective same as
64bit kernel/32bit userspace with the compat layer expanding pointers
and structures to 64bit in the userspace).
We have tested 64/64 functionality using a 64bit Linux filesystem and
64bit binder unit tests. All tests passed for both kernel/filesystem
configurations supported by this patch set - 32/32 and 64/64.
Thanks for your feedback,
Serban
On Mon, Apr 08, 2013 at 01:25:14PM +0100, Serban Constantinescu wrote:
> On 06/04/13 07:00, Greg KH wrote:
> >On Fri, Apr 05, 2013 at 04:17:47PM -0700, Arve Hj?nnev?g wrote:
> >>On Thu, Apr 4, 2013 at 5:32 AM, Serban Constantinescu
> >><[email protected]> wrote:
> >>>
> >>>Since the binder driver uses both uint32_t and unsigned int any further
> >>>kernel changes will be difficult to read. This patch fixes the inconsistent
> >>>types usage.
> >>>
> >>
> >>Would it make more sense to only change the types that need to be
> >>larger on a 64 bit system?
> >
> >I agree. You are also changing the type from explicit to "unexplicit",
> >the exact opposite from what I would be expecting to see here. Why?
>
> I have changed the types used so that they seem consistent
> throughout the driver(uint32_t is used in some of the internal
> structures and some of the function prototypes). Changing these
> types to kernel explicit types would have meant that we would have
> the same inconsistency for u32 types.
What do you mean? You changed uint32_t types to "int", which seems like
a step backwards from being explicit on how things should be.
If this is just a "cleanup" change, then great, but say it is such.
Your commit log message is odd in that you are saying it is making
things easier to read, yet I'm sure confused by it :)
greg k-h