[PATCH] binder: use enum for binder ioctls

[PATCH] binder: use enum for binder ioctls

[Alice Ryhl]

All of the other constants in this file are defined using enums, so make
the constants more consistent by defining the ioctls in an enum as well.

This is necessary for Rust Binder since the _IO macros are too
complicated for bindgen to see that they expand to integer constants.
Replacing the #defines with an enum forces bindgen to evaluate them
properly, which allows us to access them from Rust.

I originally intended to include this change in the first patch of the
Rust Binder patchset [1], but at plumbers Carlos Llamas told me that
this change has been discussed previously [2] and suggested that I send
it upstream separately.

Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-1-08ba9197f637@google.com/ [1]
Link: https://lore.kernel.org/all/YoIK2l6xbQMPGZHy@kroah.com/ [2]
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 include/uapi/linux/android/binder.h | 30 +++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
index 5f636b5afcd7..d44a8118b2ed 100644
--- a/include/uapi/linux/android/binder.h
+++ b/include/uapi/linux/android/binder.h
@@ -251,20 +251,22 @@ struct binder_extended_error {
 	__s32	param;
 };
 
-#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, __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)
-#define BINDER_VERSION			_IOWR('b', 9, struct binder_version)
-#define BINDER_GET_NODE_DEBUG_INFO	_IOWR('b', 11, struct binder_node_debug_info)
-#define BINDER_GET_NODE_INFO_FOR_REF	_IOWR('b', 12, struct binder_node_info_for_ref)
-#define BINDER_SET_CONTEXT_MGR_EXT	_IOW('b', 13, struct flat_binder_object)
-#define BINDER_FREEZE			_IOW('b', 14, struct binder_freeze_info)
-#define BINDER_GET_FROZEN_INFO		_IOWR('b', 15, struct binder_frozen_status_info)
-#define BINDER_ENABLE_ONEWAY_SPAM_DETECTION	_IOW('b', 16, __u32)
-#define BINDER_GET_EXTENDED_ERROR	_IOWR('b', 17, struct binder_extended_error)
+enum {
+	BINDER_WRITE_READ		= _IOWR('b', 1, struct binder_write_read),
+	BINDER_SET_IDLE_TIMEOUT		= _IOW('b', 3, __s64),
+	BINDER_SET_MAX_THREADS		= _IOW('b', 5, __u32),
+	BINDER_SET_IDLE_PRIORITY	= _IOW('b', 6, __s32),
+	BINDER_SET_CONTEXT_MGR		= _IOW('b', 7, __s32),
+	BINDER_THREAD_EXIT		= _IOW('b', 8, __s32),
+	BINDER_VERSION			= _IOWR('b', 9, struct binder_version),
+	BINDER_GET_NODE_DEBUG_INFO	= _IOWR('b', 11, struct binder_node_debug_info),
+	BINDER_GET_NODE_INFO_FOR_REF	= _IOWR('b', 12, struct binder_node_info_for_ref),
+	BINDER_SET_CONTEXT_MGR_EXT	= _IOW('b', 13, struct flat_binder_object),
+	BINDER_FREEZE			= _IOW('b', 14, struct binder_freeze_info),
+	BINDER_GET_FROZEN_INFO		= _IOWR('b', 15, struct binder_frozen_status_info),
+	BINDER_ENABLE_ONEWAY_SPAM_DETECTION	= _IOW('b', 16, __u32),
+	BINDER_GET_EXTENDED_ERROR	= _IOWR('b', 17, struct binder_extended_error),
+};
 
 /*
  * NOTE: Two special error codes you should check for when calling

base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH] binder: use enum for binder ioctls

[Carlos Llamas]

On Fri, Dec 08, 2023 at 03:28:01PM +0000, Alice Ryhl wrote:
> All of the other constants in this file are defined using enums, so make
> the constants more consistent by defining the ioctls in an enum as well.
> 
> This is necessary for Rust Binder since the _IO macros are too
> complicated for bindgen to see that they expand to integer constants.
> Replacing the #defines with an enum forces bindgen to evaluate them
> properly, which allows us to access them from Rust.
> 
> I originally intended to include this change in the first patch of the
> Rust Binder patchset [1], but at plumbers Carlos Llamas told me that
> this change has been discussed previously [2] and suggested that I send
> it upstream separately.
> 
> Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-1-08ba9197f637@google.com/ [1]
> Link: https://lore.kernel.org/all/YoIK2l6xbQMPGZHy@kroah.com/ [2]
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---

Acked-by: Carlos Llamas <cmllamas@google.com>

Thanks,
--
Carlos Llamas

Re: [PATCH] binder: use enum for binder ioctls

[Greg Kroah-Hartman]

On Fri, Dec 08, 2023 at 03:28:01PM +0000, Alice Ryhl wrote:
> All of the other constants in this file are defined using enums, so make
> the constants more consistent by defining the ioctls in an enum as well.
> 
> This is necessary for Rust Binder since the _IO macros are too
> complicated for bindgen to see that they expand to integer constants.
> Replacing the #defines with an enum forces bindgen to evaluate them
> properly, which allows us to access them from Rust.

Does this mean that we will have to wrap every ioctl definition in an
enum just to get access to it in rust code?

What makes these defines so magical that bindgen can't decode them?  Is
it just the complexity of the C preprocessor logic involved?  Any plans
for bindgen to resolve this?

Note, I'm not objecting to this patch (I'll queue it up next week when I
get the chance), just curious about the rust tooling side here.

thanks,

greg k-h

Re: [PATCH] binder: use enum for binder ioctls

[Miguel Ojeda]

> Does this mean that we will have to wrap every ioctl definition in an
> enum just to get access to it in rust code?

Currently, yes (or one can build them on the Rust side using the `_IO*` const
functions that are in mainline at `rust/kernel/ioctl.rs`).

> What makes these defines so magical that bindgen can't decode them?  Is
> it just the complexity of the C preprocessor logic involved?  Any plans
> for bindgen to resolve this?

Yeah, currently bindgen only resolves "trivial" object-like macros. As soon as
a macro is more complex it does not work, even if it would still resolve into
a constant. The upstream issue for this particular case (a macro that uses
other function-like macros) uses `_IO*`s as the example, in fact, and is at:

    https://github.com/rust-lang/rust-bindgen/issues/753

which we track in our bindgen list at:

    https://github.com/Rust-for-Linux/linux/issues/353

There are several ways forward for bindgen here. Ideally, libclang would give
the resolved value to bindgen. This may require changes in upstream Clang.
I contacted Aaron Ballman (the Clang maintainer, Cc'd) a while ago and
he kindly offered to review the changes if they were eventually needed.

Otherwise (or meanwhile), it is also possible to implement a workaround that
stores the information in a way that can be retrieved by bindgen by using
the macro in some way (e.g. initializing a variable and asking for the value
of the variable). It is ugly, but it should work (at least for many cases --
there may be other compounding issues with e.g. 128-bit integers).

John Baublitz (Cc'd) has spent some time on this and, from what I can tell from
the issue, we may be waiting on an answer from bindgen (Cc'ing Emilio and
Christian, the bindgen maintainers) on whether the workaround is OK for them.
The workaround would be nice to have even if we change upstream Clang, because
it would help in many cases we care about, without having to wait until we get
a new LLVM released and so on.

Hope that helps!

Cheers,
Miguel