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 set 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 v5:
1 6/6: Moved patch to the end of the series; changed handle to use __u32 type
2 4/6: Removed some of the alignment/buffer changes introduced in previous
versions of the patch.
Changes for v4:
1: 5/6: Fix the offset buffer alignment such that it will work for cases where
buffer start + buffer size are not aligned to (void *)
Changes for v3:
1: Dropped the patch that was replacing uint32_t types with unsigned int
2: Dropped the patch fixing the IOCTL types(since it has been added to Greg's
staging tree)
3: Split one patch into two: 'modify binder_write_read' and '64bit changes'
4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's review
5: Modified the binder command IOCTL declarations according to Arve's review
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 (6):
staging: android: binder: modify struct binder_write_read to use
size_t
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
staging: android: binder: fix binder interface for 64bit compat layer
drivers/staging/android/binder.c | 32 ++++++++++++-------------
drivers/staging/android/binder.h | 48 +++++++++++++++++++-------------------
2 files changed, 40 insertions(+), 40 deletions(-)
--
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 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index ce70909..7450d56 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -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]>
Acked-by: Arve Hjønnevåg <[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 b55bba9..dadfce0 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, __u32),
+ BC_ACQUIRE = _IOW('c', 5, __u32),
+ BC_RELEASE = _IOW('c', 6, __u32),
+ BC_DECREFS = _IOW('c', 7, __u32),
/*
* 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 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 <[email protected]>
---
drivers/staging/android/binder.c | 20 ++++++++++----------
drivers/staging/android/binder.h | 10 +++++-----
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 7450d56..056afe7 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 %zd at %08lx, read %zd 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);
diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index dadfce0..b88b263 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 */
+ __u32 handle; /* remote object */
};
/* extra data associated with local object */
@@ -78,7 +78,7 @@ struct binder_write_read {
/* 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. */
@@ -119,7 +119,7 @@ struct binder_transaction_data {
* identifying the target and contents of the transaction.
*/
union {
- size_t handle; /* target descriptor of command transaction */
+ __u32 handle; /* target descriptor of command transaction */
void *ptr; /* target descriptor of return transaction */
} target;
void *cookie; /* target object cookie */
--
1.7.9.5
This change mirrors the userspace operation where struct binder_write_read
members that specify the buffer size and consumed size are size_t elements.
The patch also fixes the binder_thread_write() and binder_thread_read()
functions prototypes to conform with the definition of binder_write_read.
The changes do not affect existing 32bit ABI.
Signed-off-by: Serban Constantinescu <[email protected]>
Acked-by: Arve Hjønnevåg <[email protected]>
---
drivers/staging/android/binder.c | 10 +++++-----
drivers/staging/android/binder.h | 8 ++++----
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 1567ac2..ce70909 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)
{
uint32_t 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 %08lx, read %zd at %08lx\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..edab249 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -67,11 +67,11 @@ 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;
};
--
1.7.9.5
This change will fix the BINDER_SET_MAX_THREADS ioctl to use __u32
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 and 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]>
Acked-by: Arve Hjønnevåg <[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 edab249..6515dd2 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, __u32)
#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
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]>
Acked-by: Arve Hjønnevåg <[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 6515dd2..b55bba9 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
Hi all,
Any feedback or comments on this patch set?
Thanks,
Serban
On 19/06/13 18:12, 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(kernel or userpsace).
>
> The patch set 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 v5:
> 1 6/6: Moved patch to the end of the series; changed handle to use __u32 type
> 2 4/6: Removed some of the alignment/buffer changes introduced in previous
> versions of the patch.
>
> Changes for v4:
> 1: 5/6: Fix the offset buffer alignment such that it will work for cases where
> buffer start + buffer size are not aligned to (void *)
>
> Changes for v3:
> 1: Dropped the patch that was replacing uint32_t types with unsigned int
> 2: Dropped the patch fixing the IOCTL types(since it has been added to Greg's
> staging tree)
> 3: Split one patch into two: 'modify binder_write_read' and '64bit changes'
> 4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's review
> 5: Modified the binder command IOCTL declarations according to Arve's review
>
> 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 (6):
> staging: android: binder: modify struct binder_write_read to use
> size_t
> 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
> staging: android: binder: fix binder interface for 64bit compat layer
>
> drivers/staging/android/binder.c | 32 ++++++++++++-------------
> drivers/staging/android/binder.h | 48 +++++++++++++++++++-------------------
> 2 files changed, 40 insertions(+), 40 deletions(-)
>
--
Best Regards,
Serban Constantinescu
PDSW Engineer ARM Ltd.
On Wed, Jun 19, 2013 at 10:12 AM, Serban Constantinescu
<[email protected]> 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.
>
> Signed-off-by: Serban Constantinescu <[email protected]>
> ---
> drivers/staging/android/binder.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index ce70909..7450d56 100644
> --- a/drivers/staging/android/binder.c
> +++ b/drivers/staging/android/binder.c
> @@ -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
>
Acked-by: Arve Hj?nnev?g <[email protected]>
--
Arve Hj?nnev?g
On Wed, Jun 19, 2013 at 10:12 AM, Serban Constantinescu
<[email protected]> 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 <[email protected]>
> Acked-by: Arve Hj?nnev?g <[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 b55bba9..dadfce0 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;
desc should be __u32 to be consistent with the other changes you are
making in this and the next patch.
> };
>
> 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, __u32),
> + BC_ACQUIRE = _IOW('c', 5, __u32),
> + BC_RELEASE = _IOW('c', 6, __u32),
> + BC_DECREFS = _IOW('c', 7, __u32),
> /*
> * int: descriptor
> */
> --
> 1.7.9.5
>
--
Arve Hj?nnev?g
On Wed, Jun 19, 2013 at 10:12 AM, Serban Constantinescu
<[email protected]> 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 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 <[email protected]>
> ---
> drivers/staging/android/binder.c | 20 ++++++++++----------
> drivers/staging/android/binder.h | 10 +++++-----
> 2 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
> index 7450d56..056afe7 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 %zd at %08lx, read %zd 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);
>
> diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
> index dadfce0..b88b263 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 */
> + __u32 handle; /* remote object */
> };
>
> /* extra data associated with local object */
> @@ -78,7 +78,7 @@ struct binder_write_read {
> /* 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. */
> @@ -119,7 +119,7 @@ struct binder_transaction_data {
> * identifying the target and contents of the transaction.
> */
> union {
> - size_t handle; /* target descriptor of command transaction */
> + __u32 handle; /* target descriptor of command transaction */
> void *ptr; /* target descriptor of return transaction */
> } target;
> void *cookie; /* target object cookie */
> --
> 1.7.9.5
>
Acked-by: Arve Hj?nnev?g <[email protected]>
--
Arve Hj?nnev?g
On Wed, Jul 3, 2013 at 9:35 AM, Serban Constantinescu
<[email protected]> wrote:
> Hi all,
>
> Any feedback or comments on this patch set?
>
> Thanks,
> Serban
>
The new patches look OK, but I would like to also see the patches that
add support for 32 bit user-space on a 64 bit kernel.
>
> On 19/06/13 18:12, 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(kernel or
>> userpsace).
>>
>> The patch set 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 v5:
>> 1 6/6: Moved patch to the end of the series; changed handle to use __u32
>> type
>> 2 4/6: Removed some of the alignment/buffer changes introduced in previous
>> versions of the patch.
>>
>> Changes for v4:
>> 1: 5/6: Fix the offset buffer alignment such that it will work for cases
>> where
>> buffer start + buffer size are not aligned to (void *)
>>
>> Changes for v3:
>> 1: Dropped the patch that was replacing uint32_t types with unsigned int
>> 2: Dropped the patch fixing the IOCTL types(since it has been added to
>> Greg's
>> staging tree)
>> 3: Split one patch into two: 'modify binder_write_read' and '64bit
>> changes'
>> 4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's
>> review
>> 5: Modified the binder command IOCTL declarations according to Arve's
>> review
>>
>> 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 (6):
>> staging: android: binder: modify struct binder_write_read to use
>> size_t
>> 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
>> staging: android: binder: fix binder interface for 64bit compat layer
>>
>> drivers/staging/android/binder.c | 32 ++++++++++++-------------
>> drivers/staging/android/binder.h | 48
>> +++++++++++++++++++-------------------
>> 2 files changed, 40 insertions(+), 40 deletions(-)
>>
>
>
> --
> Best Regards,
>
> Serban Constantinescu
> PDSW Engineer ARM Ltd.
>
--
Arve Hj?nnev?g
On 03/07/13 23:30, Arve Hj?nnev?g wrote:
> On Wed, Jun 19, 2013 at 10:12 AM, Serban Constantinescu
> <[email protected]> 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 <[email protected]>
>> Acked-by: Arve Hj?nnev?g <[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 b55bba9..dadfce0 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;
>
> desc should be __u32 to be consistent with the other changes you are
> making in this and the next patch.
I have added this change to v6 of this patch set which I will send out
shortly.
Thanks again for your feedback,
Serban
>
>> };
>>
>> 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, __u32),
>> + BC_ACQUIRE = _IOW('c', 5, __u32),
>> + BC_RELEASE = _IOW('c', 6, __u32),
>> + BC_DECREFS = _IOW('c', 7, __u32),
>> /*
>> * int: descriptor
>> */
>> --
>> 1.7.9.5
>>
>
>
>
> --
> Arve Hj?nnev?g
>
On 03/07/13 23:32, Arve Hj?nnev?g wrote:
> On Wed, Jul 3, 2013 at 9:35 AM, Serban Constantinescu
> <[email protected]> wrote:
>> Hi all,
>>
>> Any feedback or comments on this patch set?
>>
>> Thanks,
>> Serban
>>
>
> The new patches look OK, but I would like to also see the patches that
> add support for 32 bit user-space on a 64 bit kernel.
I have added the last change to v6 and kept Arve's ack.
I will rebase my binder kernel compat on this latest version and polish
the patch set such that is ready for upstream. I should have something
ready for your review soon.
Thanks for your help,
Serban Constantinescu
>
>>
>> On 19/06/13 18:12, 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(kernel or
>>> userpsace).
>>>
>>> The patch set 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 v5:
>>> 1 6/6: Moved patch to the end of the series; changed handle to use __u32
>>> type
>>> 2 4/6: Removed some of the alignment/buffer changes introduced in previous
>>> versions of the patch.
>>>
>>> Changes for v4:
>>> 1: 5/6: Fix the offset buffer alignment such that it will work for cases
>>> where
>>> buffer start + buffer size are not aligned to (void *)
>>>
>>> Changes for v3:
>>> 1: Dropped the patch that was replacing uint32_t types with unsigned int
>>> 2: Dropped the patch fixing the IOCTL types(since it has been added to
>>> Greg's
>>> staging tree)
>>> 3: Split one patch into two: 'modify binder_write_read' and '64bit
>>> changes'
>>> 4: Modified BINDER_SET_MAX_THREADS ioctl definition accordint to Arve's
>>> review
>>> 5: Modified the binder command IOCTL declarations according to Arve's
>>> review
>>>
>>> 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 (6):
>>> staging: android: binder: modify struct binder_write_read to use
>>> size_t
>>> 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
>>> staging: android: binder: fix binder interface for 64bit compat layer
>>>
>>> drivers/staging/android/binder.c | 32 ++++++++++++-------------
>>> drivers/staging/android/binder.h | 48
>>> +++++++++++++++++++-------------------
>>> 2 files changed, 40 insertions(+), 40 deletions(-)
>>>
>>
>>
>> --
>> Best Regards,
>>
>> Serban Constantinescu
>> PDSW Engineer ARM Ltd.
>>
>
>
>
> --
> Arve Hj?nnev?g
>