public inbox for rust-for-linux@vger.kernel.org
 help / color / mirror / Atom feed
From: Gary Guo <gary@garyguo.net>
To: Alice Ryhl <aliceryhl@google.com>
Cc: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Christian Brauner" <brauner@kernel.org>,
	"Carlos Llamas" <cmllamas@google.com>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Trevor Gross" <tmgross@umich.edu>,
	"Danilo Krummrich" <dakr@kernel.org>,
	"Arnd Bergmann" <arnd@arndb.de>,
	linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] rust: redefine `bindings::compat_ptr_ioctl` in Rust
Date: Tue, 6 Jan 2026 15:33:03 +0000	[thread overview]
Message-ID: <20260106153303.6ff846bf.gary@garyguo.net> (raw)
In-Reply-To: <20260105-redefine-compat_ptr_ioctl-v1-1-25edb3d91acc@google.com>

On Mon, 05 Jan 2026 14:25:03 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> There is currently an inconsistency between C and Rust, which is that
> when Rust requires cfg(CONFIG_COMPAT) on compat_ioctl when using the
> compat_ptr_ioctl symbol because '#define compat_ptr_ioctl NULL' does not
> get translated to anything by bindgen.
> 
> But it's not *just* a matter of translating the '#define' into Rust when
> CONFIG_COMPAT=n. This is because when CONFIG_COMPAT=y, the type of
> compat_ptr_ioctl is a non-nullable function pointer, and to seamlessly
> use it regardless of the config, we need a nullable function pointer.

I had a think about this a while back. I was about to purpose a new
bindgen option to generate function pointers instead of function items,
but then I found that I am not very fond of that idea.

In some sense, the C side does not have a consistent type for
`compat_ptr_ioctl`. If CONFIG_COMPAT=y, it has a function type, otherwise
it has a `void*` type, it just that they happen to coerce/decay to the
same function pointer type.

I think what you did to the bindings crate in this patch is the best way to
address this inconsistency.

> 
> I think it's important to do something about this; I've seen the mistake
> of accidentally forgetting '#[cfg(CONFIG_COMPAT)]' when compat_ptr_ioctl
> is used multiple times now.
> 
> This explicitly declares 'bindings::compat_ptr_ioctl' as an Option that
> is always defined but might be None. This matches C, but isn't ideal:
> it modifies the bindings crate. But I'm not sure if there's a better way
> to do it. If we just redefine in kernel/, then people may still use the
> one in bindings::, since that is where you would normally find it. I am
> open to suggestions.

> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

Best,
Gary

> ---
>  drivers/android/binder/rust_binder_main.rs |  3 +--
>  rust/bindings/lib.rs                       | 13 +++++++++++++
>  rust/kernel/miscdevice.rs                  |  2 +-
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/android/binder/rust_binder_main.rs b/drivers/android/binder/rust_binder_main.rs
> index d84c3c360be0ee79e4dab22d613bc927a70895a7..30e4517f90a3c5c2cf83c91530a6dfcca7316bd6 100644
> --- a/drivers/android/binder/rust_binder_main.rs
> +++ b/drivers/android/binder/rust_binder_main.rs
> @@ -322,8 +322,7 @@ unsafe impl<T> Sync for AssertSync<T> {}
>          owner: THIS_MODULE.as_ptr(),
>          poll: Some(rust_binder_poll),
>          unlocked_ioctl: Some(rust_binder_ioctl),
> -        #[cfg(CONFIG_COMPAT)]
> -        compat_ioctl: Some(bindings::compat_ptr_ioctl),
> +        compat_ioctl: bindings::compat_ptr_ioctl,
>          mmap: Some(rust_binder_mmap),
>          open: Some(rust_binder_open),
>          release: Some(rust_binder_release),
> diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs
> index 0c57cf9b4004f176997c59ecc58a9a9ac76163d9..19f57c5b2fa2a343c4250063e9d5ce1067e6b6ff 100644
> --- a/rust/bindings/lib.rs
> +++ b/rust/bindings/lib.rs
> @@ -67,3 +67,16 @@ mod bindings_helper {
>  }
>  
>  pub use bindings_raw::*;
> +
> +pub const compat_ptr_ioctl: Option<
> +    unsafe extern "C" fn(*mut file, ffi::c_uint, ffi::c_ulong) -> ffi::c_long,
> +> = {  
> +    #[cfg(CONFIG_COMPAT)]
> +    {
> +        Some(bindings_raw::compat_ptr_ioctl)
> +    }
> +    #[cfg(not(CONFIG_COMPAT))]
> +    {
> +        None
> +    }
> +};
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index ba64c8a858f0d74ae54789d1f16a39479fd54b96..c3c2052c92069f6e64b7bb68d6d888c6aca01373 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -410,7 +410,7 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
>          compat_ioctl: if T::HAS_COMPAT_IOCTL {
>              Some(Self::compat_ioctl)
>          } else if T::HAS_IOCTL {
> -            Some(bindings::compat_ptr_ioctl)
> +            bindings::compat_ptr_ioctl
>          } else {
>              None
>          },
> 
> ---
> base-commit: 8314d2c28d3369bc879af8e848f810292b16d0af
> change-id: 20260105-redefine-compat_ptr_ioctl-e64f9462225c
> 
> Best regards,


      reply	other threads:[~2026-01-06 15:33 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-01-05 14:25 [PATCH] rust: redefine `bindings::compat_ptr_ioctl` in Rust Alice Ryhl
2026-01-06 15:33 ` Gary Guo [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260106153303.6ff846bf.gary@garyguo.net \
    --to=gary@garyguo.net \
    --cc=a.hindborg@kernel.org \
    --cc=aliceryhl@google.com \
    --cc=arnd@arndb.de \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=brauner@kernel.org \
    --cc=cmllamas@google.com \
    --cc=dakr@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lossin@kernel.org \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=tmgross@umich.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox