[PATCH v2 0/6] rust: error: Add missing wrappers to convert to/from kernel error codes

[PATCH v2 0/6] rust: error: Add missing wrappers to convert to/from kernel error codes

[Asahi Lina]

Hi everyone!

This series is part of the set of dependencies for the drm/asahi
Apple M1/M2 GPU driver.

It adds a bunch of missing wrappers in kernel::error, which are useful
to convert to/from kernel error codes. Since these will be used by many
abstractions coming up soon, I think it makes sense to merge them as
soon as possible instead of bundling them with the first user. Hence,
they have allow() tags to silence dead code warnings. These can be
removed as soon as the first user is in the kernel crate.

Getting this in first allows the subsequent abstractions to be merged in
any order, so we don't have to worry about piecewise rebasing and fixing
conflicts in the Error wrappers. See [1] for a complete tree with the DRM
abstractions and all other miscellaneous work-in-progress prerequisites
rebased on top of mainline.

Most of these have been extracted from the rust-for-linux/rust branch,
with author attribution to the first/primary author and Co-developed-by:
for everyone else who touched the code.

Attribution changes:
- One of the patches had Miguel's old email in the tags, updated that per
  his request.
- Wedson's email changed from @google.com to @gmail.com (I understand
  this is the current one).

Sven: There is one patch from you in this series, do you want to send it
yourself directly? I understand Wedson and Miguel are okay with me
sending stuff on their behalf.

[1] https://github.com/Rust-for-Linux/linux/pull/969/commits

Signed-off-by: Asahi Lina <lina@asahilina.net>
---
Changes in v2:
- Removed the redundant _kernel from various function names.
- Replaced the from_result!{} macro with a from_result() function taking a closure.
- Link to v1: https://lore.kernel.org/r/20230224-rust-error-v1-0-f8f9a9a87303@asahilina.net

---
Asahi Lina (2):
      rust: error: Rename to_kernel_errno() -> to_errno()
      rust: error: Add Error::to_ptr()

Miguel Ojeda (1):
      rust: error: Add Error::from_errno()

Sven Van Asbroeck (1):
      rust: error: Add a helper to convert a C ERR_PTR to a `Result`

Wedson Almeida Filho (2):
      rust: error: Add to_result() helper
      rust: error: Add from_result() helper
 rust/helpers.c        |  19 ++++++++
 rust/kernel/error.rs  | 126 +++++++++++++++++++++++++++++++++++++++++++++++++-
 rust/macros/module.rs |   2 +-
 3 files changed, 145 insertions(+), 2 deletions(-)
---
base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
change-id: 20230224-rust-error-f2871fbc907f

Thank you,
~~ Lina

[PATCH v2 1/6] rust: error: Rename to_kernel_errno() -> to_errno()

[Asahi Lina]

This is kernel code, so specifying "kernel" is redundant. Let's simplify
things and just call it to_errno().

Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 rust/kernel/error.rs  | 2 +-
 rust/macros/module.rs | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 5b9751d7ff1d..35894fa35efe 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -73,7 +73,7 @@ pub struct Error(core::ffi::c_int);
 
 impl Error {
     /// Returns the kernel error code.
-    pub fn to_kernel_errno(self) -> core::ffi::c_int {
+    pub fn to_errno(self) -> core::ffi::c_int {
         self.0
     }
 }
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index a7e363c2b044..143336543866 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -258,7 +258,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
                         return 0;
                     }}
                     Err(e) => {{
-                        return e.to_kernel_errno();
+                        return e.to_errno();
                     }}
                 }}
             }}

-- 
2.40.0

[PATCH v2 2/6] rust: error: Add Error::to_ptr()

[Asahi Lina]

This is the Rust equivalent to ERR_PTR(), for use in C callbacks.
Marked as #[allow(dead_code)] for now, since it does not have any
consumers yet.

Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 rust/helpers.c       | 7 +++++++
 rust/kernel/error.rs | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/rust/helpers.c b/rust/helpers.c
index 09a4d93f9d62..89f4cd1e0df3 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -20,6 +20,7 @@
 
 #include <linux/bug.h>
 #include <linux/build_bug.h>
+#include <linux/err.h>
 #include <linux/refcount.h>
 
 __noreturn void rust_helper_BUG(void)
@@ -46,6 +47,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
 }
 EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
 
+__force void *rust_helper_ERR_PTR(long err)
+{
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(rust_helper_ERR_PTR);
+
 /*
  * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
  * as the Rust `usize` type, so we can use it in contexts where Rust
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 35894fa35efe..e97e652a1aec 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -76,6 +76,13 @@ impl Error {
     pub fn to_errno(self) -> core::ffi::c_int {
         self.0
     }
+
+    /// Returns the error encoded as a pointer.
+    #[allow(dead_code)]
+    pub(crate) fn to_ptr<T>(self) -> *mut T {
+        // SAFETY: Valid as long as self.0 is a valid error
+        unsafe { bindings::ERR_PTR(self.0.into()) as *mut _ }
+    }
 }
 
 impl From<AllocError> for Error {

-- 
2.40.0

[PATCH v2 3/6] rust: error: Add Error::from_errno()

[Asahi Lina]

From: Miguel Ojeda <ojeda@kernel.org>

Add a function to create `Error` values out of a kernel error return,
which safely upholds the invariant that the error code is well-formed
(negative and greater than -MAX_ERRNO). If a malformed code is passed
in, it will be converted to EINVAL.

Lina: Imported from rust-for-linux/rust as authored by Miguel and Fox
with refactoring from Wedson, renamed from_kernel_errno() to
from_errno().

Co-developed-by: Fox Chen <foxhlchen@gmail.com>
Signed-off-by: Fox Chen <foxhlchen@gmail.com>
Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 rust/kernel/error.rs | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index e97e652a1aec..659468bd1735 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -72,6 +72,25 @@ pub mod code {
 pub struct Error(core::ffi::c_int);
 
 impl Error {
+    /// Creates an [`Error`] from a kernel error code.
+    ///
+    /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
+    /// be returned in such a case.
+    pub(crate) fn from_errno(errno: core::ffi::c_int) -> Error {
+        if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 {
+            // TODO: Make it a `WARN_ONCE` once available.
+            crate::pr_warn!(
+                "attempted to create `Error` with out of range `errno`: {}",
+                errno
+            );
+            return code::EINVAL;
+        }
+
+        // INVARIANT: The check above ensures the type invariant
+        // will hold.
+        Error(errno)
+    }
+
     /// Returns the kernel error code.
     pub fn to_errno(self) -> core::ffi::c_int {
         self.0

-- 
2.40.0

[PATCH v2 4/6] rust: error: Add to_result() helper

[Asahi Lina]

From: Wedson Almeida Filho <wedsonaf@gmail.com>

Add a to_result() helper to convert kernel C return values to a Rust
Result, mapping >=0 values to Ok(()) and negative values to Err(...),
with Error::from_errno() ensuring that the errno is within range.

Lina: Imported from rust-for-linux/rust, originally developed by Wedson
as part of the AMBA device driver support.

Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 rust/kernel/error.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 659468bd1735..4f599c4d1752 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -167,3 +167,13 @@ impl From<core::convert::Infallible> for Error {
 /// it should still be modeled as returning a `Result` rather than
 /// just an [`Error`].
 pub type Result<T = ()> = core::result::Result<T, Error>;
+
+/// Converts an integer as returned by a C kernel function to an error if it's negative, and
+/// `Ok(())` otherwise.
+pub fn to_result(err: core::ffi::c_int) -> Result {
+    if err < 0 {
+        Err(Error::from_errno(err))
+    } else {
+        Ok(())
+    }
+}

-- 
2.40.0

[PATCH v2 5/6] rust: error: Add a helper to convert a C ERR_PTR to a `Result`

[Asahi Lina]

From: Sven Van Asbroeck <thesven73@gmail.com>

Some kernel C API functions return a pointer which embeds an optional
`errno`. Callers are supposed to check the returned pointer with
`IS_ERR()` and if this returns `true`, retrieve the `errno` using
`PTR_ERR()`.

Create a Rust helper function to implement the Rust equivalent:
transform a `*mut T` to `Result<*mut T>`.

Lina: Imported from rust-for-linux/linux, with subsequent refactoring
and contributions squashed in and attributed below. Replaced usage of
from_kernel_errno_unchecked() with an open-coded constructor, since this
is the only user anyway. Renamed the function to from_err_ptr().

Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Co-developed-by: Fox Chen <foxhlchen@gmail.com>
Signed-off-by: Fox Chen <foxhlchen@gmail.com>
Co-developed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 rust/helpers.c       | 12 ++++++++++++
 rust/kernel/error.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/rust/helpers.c b/rust/helpers.c
index 89f4cd1e0df3..04b9be46e887 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -53,6 +53,18 @@ __force void *rust_helper_ERR_PTR(long err)
 }
 EXPORT_SYMBOL_GPL(rust_helper_ERR_PTR);
 
+bool rust_helper_IS_ERR(__force const void *ptr)
+{
+	return IS_ERR(ptr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_IS_ERR);
+
+long rust_helper_PTR_ERR(__force const void *ptr)
+{
+	return PTR_ERR(ptr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
+
 /*
  * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
  * as the Rust `usize` type, so we can use it in contexts where Rust
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 4f599c4d1752..6b10129075a7 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -177,3 +177,52 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
         Ok(())
     }
 }
+
+/// Transform a kernel "error pointer" to a normal pointer.
+///
+/// Some kernel C API functions return an "error pointer" which optionally
+/// embeds an `errno`. Callers are supposed to check the returned pointer
+/// for errors. This function performs the check and converts the "error pointer"
+/// to a normal pointer in an idiomatic fashion.
+///
+/// # Examples
+///
+/// ```ignore
+/// # use kernel::from_err_ptr;
+/// # use kernel::bindings;
+/// fn devm_platform_ioremap_resource(
+///     pdev: &mut PlatformDevice,
+///     index: u32,
+/// ) -> Result<*mut core::ffi::c_void> {
+///     // SAFETY: FFI call.
+///     unsafe {
+///         from_err_ptr(bindings::devm_platform_ioremap_resource(
+///             pdev.to_ptr(),
+///             index,
+///         ))
+///     }
+/// }
+/// ```
+// TODO: Remove `dead_code` marker once an in-kernel client is available.
+#[allow(dead_code)]
+pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
+    // CAST: Casting a pointer to `*const core::ffi::c_void` is always valid.
+    let const_ptr: *const core::ffi::c_void = ptr.cast();
+    // SAFETY: The FFI function does not deref the pointer.
+    if unsafe { bindings::IS_ERR(const_ptr) } {
+        // SAFETY: The FFI function does not deref the pointer.
+        let err = unsafe { bindings::PTR_ERR(const_ptr) };
+        // CAST: If `IS_ERR()` returns `true`,
+        // then `PTR_ERR()` is guaranteed to return a
+        // negative value greater-or-equal to `-bindings::MAX_ERRNO`,
+        // which always fits in an `i16`, as per the invariant above.
+        // And an `i16` always fits in an `i32`. So casting `err` to
+        // an `i32` can never overflow, and is always valid.
+        //
+        // SAFETY: `IS_ERR()` ensures `err` is a
+        // negative value greater-or-equal to `-bindings::MAX_ERRNO`.
+        #[cfg_attr(CONFIG_ARM, allow(clippy::unnecessary_cast))]
+        return Err(Error(err as i32));
+    }
+    Ok(ptr)
+}

-- 
2.40.0

[PATCH v2 6/6] rust: error: Add from_result() helper

[Asahi Lina]

From: Wedson Almeida Filho <wedsonaf@gmail.com>

Add a helper function to easily return C result codes from a Rust function
that calls functions which return a Result<T>.

Lina: Imported from rust-for-linux/rust, originally developed by Wedson
as part of file_operations.rs. Added the allow() flags since there is no
user in the kernel crate yet and fixed a typo in a comment. Replaced the
macro with a function taking a closure, per discussion on the ML.

Co-developed-by: Fox Chen <foxhlchen@gmail.com>
Signed-off-by: Fox Chen <foxhlchen@gmail.com>
Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 rust/kernel/error.rs | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 6b10129075a7..4d8b19d5967d 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -226,3 +226,42 @@ pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
     }
     Ok(ptr)
 }
+
+/// Calls a closure returning a [`crate::error::Result<T>`] and converts the result to
+/// a C integer result.
+///
+/// This is useful when calling Rust functions that return [`crate::error::Result<T>`]
+/// from inside `extern "C"` functions that need to return an integer error result.
+///
+/// `T` should be convertible from an `i16` via `From<i16>`.
+///
+/// # Examples
+///
+/// ```ignore
+/// # use kernel::from_result;
+/// # use kernel::bindings;
+/// unsafe extern "C" fn probe_callback(
+///     pdev: *mut bindings::platform_device,
+/// ) -> core::ffi::c_int {
+///     from_result(|| {
+///         let ptr = devm_alloc(pdev)?;
+///         bindings::platform_set_drvdata(pdev, ptr);
+///         Ok(0)
+///     })
+/// }
+/// ```
+// TODO: Remove `dead_code` marker once an in-kernel client is available.
+#[allow(dead_code)]
+pub(crate) fn from_result<T, F>(f: F) -> T
+where
+    T: From<i16>,
+    F: FnOnce() -> Result<T>,
+{
+    match f() {
+        Ok(v) => v,
+        // NO-OVERFLOW: negative `errno`s are no smaller than `-bindings::MAX_ERRNO`,
+        // `-bindings::MAX_ERRNO` fits in an `i16` as per invariant above,
+        // therefore a negative `errno` always fits in an `i16` and will not overflow.
+        Err(e) => T::from(e.to_errno() as i16),
+    }
+}

-- 
2.40.0

Re: [PATCH v2 1/6] rust: error: Rename to_kernel_errno() -> to_errno()

[Martin Rodriguez Reboredo]

On 3/29/23 09:04, Asahi Lina wrote:
> This is kernel code, so specifying "kernel" is redundant. Let's simplify
> things and just call it to_errno().
> 
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
>  rust/kernel/error.rs  | 2 +-
>  rust/macros/module.rs | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 5b9751d7ff1d..35894fa35efe 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -73,7 +73,7 @@ pub struct Error(core::ffi::c_int);
>  
>  impl Error {
>      /// Returns the kernel error code.
> -    pub fn to_kernel_errno(self) -> core::ffi::c_int {
> +    pub fn to_errno(self) -> core::ffi::c_int {
>          self.0
>      }
>  }
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index a7e363c2b044..143336543866 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -258,7 +258,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>                          return 0;
>                      }}
>                      Err(e) => {{
> -                        return e.to_kernel_errno();
> +                        return e.to_errno();
>                      }}
>                  }}
>              }}
> 

Reviewed-by: Martin Rodriguez Reboredo

Re: [PATCH v2 2/6] rust: error: Add Error::to_ptr()

[Martin Rodriguez Reboredo]

On 3/29/23 09:04, Asahi Lina wrote:
> [...]
> 
> +
> +    /// Returns the error encoded as a pointer.
> +    #[allow(dead_code)]
> +    pub(crate) fn to_ptr<T>(self) -> *mut T {
> +        // SAFETY: Valid as long as self.0 is a valid error
> +        unsafe { bindings::ERR_PTR(self.0.into()) as *mut _ }
> +    }
>  }
>  
>  impl From<AllocError> for Error {
> 

Reviewed-by: Martin Rodriguez Reboredo

Re: [PATCH v2 3/6] rust: error: Add Error::from_errno()

[Martin Rodriguez Reboredo]

On 3/29/23 09:04, Asahi Lina wrote:
> [...]
>  
>  impl Error {
> +    /// Creates an [`Error`] from a kernel error code.
> +    ///
> +    /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
> +    /// be returned in such a case.
> +    pub(crate) fn from_errno(errno: core::ffi::c_int) -> Error {
> +        if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 {
> +            // TODO: Make it a `WARN_ONCE` once available.
> +            crate::pr_warn!(
> +                "attempted to create `Error` with out of range `errno`: {}",
> +                errno
> +            );
> +            return code::EINVAL;
> +        }
> +
> +        // INVARIANT: The check above ensures the type invariant
> +        // will hold.
> +        Error(errno)
> +    }
> +
>      /// Returns the kernel error code.
>      pub fn to_errno(self) -> core::ffi::c_int {
>          self.0
> 

Reviewed-by: Martin Rodriguez Reboredo

Re: [PATCH v2 4/6] rust: error: Add to_result() helper

[Martin Rodriguez Reboredo]

On 3/29/23 09:04, Asahi Lina wrote:
> [...]
> 
> +
> +/// Converts an integer as returned by a C kernel function to an error if it's negative, and
> +/// `Ok(())` otherwise.
> +pub fn to_result(err: core::ffi::c_int) -> Result {
> +    if err < 0 {
> +        Err(Error::from_errno(err))
> +    } else {
> +        Ok(())
> +    }
> +}
> 

Reviewed-by: Martin Rodriguez Reboredo

Re: [PATCH v2 5/6] rust: error: Add a helper to convert a C ERR_PTR to a `Result`

[Martin Rodriguez Reboredo]

On 3/29/23 09:04, Asahi Lina wrote:
> [...]
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 89f4cd1e0df3..04b9be46e887 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -53,6 +53,18 @@ __force void *rust_helper_ERR_PTR(long err)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_ERR_PTR);
>  
> +bool rust_helper_IS_ERR(__force const void *ptr)
> +{
> +	return IS_ERR(ptr);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_IS_ERR);
> +
> +long rust_helper_PTR_ERR(__force const void *ptr)
> +{
> +	return PTR_ERR(ptr);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
> +
>  /*
>   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
>   * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 4f599c4d1752..6b10129075a7 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -177,3 +177,52 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
>          Ok(())
>      }
>  }
> +
> +/// Transform a kernel "error pointer" to a normal pointer.
> +///
> +/// Some kernel C API functions return an "error pointer" which optionally
> +/// embeds an `errno`. Callers are supposed to check the returned pointer
> +/// for errors. This function performs the check and converts the "error pointer"
> +/// to a normal pointer in an idiomatic fashion.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// # use kernel::from_err_ptr;
> +/// # use kernel::bindings;
> +/// fn devm_platform_ioremap_resource(
> +///     pdev: &mut PlatformDevice,
> +///     index: u32,
> +/// ) -> Result<*mut core::ffi::c_void> {
> +///     // SAFETY: FFI call.
> +///     unsafe {
> +///         from_err_ptr(bindings::devm_platform_ioremap_resource(
> +///             pdev.to_ptr(),
> +///             index,
> +///         ))
> +///     }
> +/// }
> +/// ```
> +// TODO: Remove `dead_code` marker once an in-kernel client is available.
> +#[allow(dead_code)]
> +pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
> +    // CAST: Casting a pointer to `*const core::ffi::c_void` is always valid.
> +    let const_ptr: *const core::ffi::c_void = ptr.cast();
> +    // SAFETY: The FFI function does not deref the pointer.
> +    if unsafe { bindings::IS_ERR(const_ptr) } {
> +        // SAFETY: The FFI function does not deref the pointer.
> +        let err = unsafe { bindings::PTR_ERR(const_ptr) };
> +        // CAST: If `IS_ERR()` returns `true`,
> +        // then `PTR_ERR()` is guaranteed to return a
> +        // negative value greater-or-equal to `-bindings::MAX_ERRNO`,
> +        // which always fits in an `i16`, as per the invariant above.
> +        // And an `i16` always fits in an `i32`. So casting `err` to
> +        // an `i32` can never overflow, and is always valid.
> +        //
> +        // SAFETY: `IS_ERR()` ensures `err` is a
> +        // negative value greater-or-equal to `-bindings::MAX_ERRNO`.
> +        #[cfg_attr(CONFIG_ARM, allow(clippy::unnecessary_cast))]
> +        return Err(Error(err as i32));
> +    }
> +    Ok(ptr)
> +}
> 

Reviewed-by: Martin Rodriguez Reboredo

Re: [PATCH v2 6/6] rust: error: Add from_result() helper

[Martin Rodriguez Reboredo]

On 3/29/23 09:04, Asahi Lina wrote:
> [...]
> 
> +
> +/// Calls a closure returning a [`crate::error::Result<T>`] and converts the result to
> +/// a C integer result.
> +///
> +/// This is useful when calling Rust functions that return [`crate::error::Result<T>`]
> +/// from inside `extern "C"` functions that need to return an integer error result.
> +///
> +/// `T` should be convertible from an `i16` via `From<i16>`.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// # use kernel::from_result;
> +/// # use kernel::bindings;
> +/// unsafe extern "C" fn probe_callback(
> +///     pdev: *mut bindings::platform_device,
> +/// ) -> core::ffi::c_int {
> +///     from_result(|| {
> +///         let ptr = devm_alloc(pdev)?;
> +///         bindings::platform_set_drvdata(pdev, ptr);
> +///         Ok(0)
> +///     })
> +/// }
> +/// ```
> +// TODO: Remove `dead_code` marker once an in-kernel client is available.
> +#[allow(dead_code)]
> +pub(crate) fn from_result<T, F>(f: F) -> T
> +where
> +    T: From<i16>,
> +    F: FnOnce() -> Result<T>,
> +{
> +    match f() {
> +        Ok(v) => v,
> +        // NO-OVERFLOW: negative `errno`s are no smaller than `-bindings::MAX_ERRNO`,
> +        // `-bindings::MAX_ERRNO` fits in an `i16` as per invariant above,
> +        // therefore a negative `errno` always fits in an `i16` and will not overflow.
> +        Err(e) => T::from(e.to_errno() as i16),
> +    }
> +}
> 

Reviewed-by: Martin Rodriguez Reboredo

Re: [PATCH v2 1/6] rust: error: Rename to_kernel_errno() -> to_errno()

[Miguel Ojeda]

On Wed, Mar 29, 2023 at 4:47 PM Martin Rodriguez Reboredo
<yakoyoku@gmail.com> wrote:
>
> Reviewed-by: Martin Rodriguez Reboredo

These (in the different patches) are supposed to have
<yakoyoku@gmail.com> email when I take them, right? (no need to resend
them)

Cheers,
Miguel

Re: [PATCH v2 1/6] rust: error: Rename to_kernel_errno() -> to_errno()

[Martin Rodriguez Reboredo]

On 3/29/23 12:04, Miguel Ojeda wrote:
> On Wed, Mar 29, 2023 at 4:47 PM Martin Rodriguez Reboredo
> <yakoyoku@gmail.com> wrote:
>>
>> Reviewed-by: Martin Rodriguez Reboredo
> 
> These (in the different patches) are supposed to have
> <yakoyoku@gmail.com> email when I take them, right? (no need to resend
> them)
> 
> Cheers,
> Miguel

Ah, yes, they were supposed to have my email with it. I wasn't that
livened up when I sent them.

Thanks
-> Martin

Re: [PATCH v2 1/6] rust: error: Rename to_kernel_errno() -> to_errno()

[Gary Guo]

On Wed, 29 Mar 2023 21:04:33 +0900
Asahi Lina <lina@asahilina.net> wrote:

> This is kernel code, so specifying "kernel" is redundant. Let's simplify
> things and just call it to_errno().
> 
> Signed-off-by: Asahi Lina <lina@asahilina.net>

Thanks Lina for implementing my suggestion.

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

> ---
>  rust/kernel/error.rs  | 2 +-
>  rust/macros/module.rs | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 5b9751d7ff1d..35894fa35efe 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -73,7 +73,7 @@ pub struct Error(core::ffi::c_int);
>  
>  impl Error {
>      /// Returns the kernel error code.
> -    pub fn to_kernel_errno(self) -> core::ffi::c_int {
> +    pub fn to_errno(self) -> core::ffi::c_int {
>          self.0
>      }
>  }
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index a7e363c2b044..143336543866 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -258,7 +258,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>                          return 0;
>                      }}
>                      Err(e) => {{
> -                        return e.to_kernel_errno();
> +                        return e.to_errno();
>                      }}
>                  }}
>              }}
> 

Re: [PATCH v2 2/6] rust: error: Add Error::to_ptr()

[Gary Guo]

On Wed, 29 Mar 2023 21:04:34 +0900
Asahi Lina <lina@asahilina.net> wrote:

> This is the Rust equivalent to ERR_PTR(), for use in C callbacks.
> Marked as #[allow(dead_code)] for now, since it does not have any
> consumers yet.
> 
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
>  rust/helpers.c       | 7 +++++++
>  rust/kernel/error.rs | 7 +++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 09a4d93f9d62..89f4cd1e0df3 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -20,6 +20,7 @@
>  
>  #include <linux/bug.h>
>  #include <linux/build_bug.h>
> +#include <linux/err.h>
>  #include <linux/refcount.h>
>  
>  __noreturn void rust_helper_BUG(void)
> @@ -46,6 +47,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
>  
> +__force void *rust_helper_ERR_PTR(long err)
> +{
> +	return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_ERR_PTR);
> +
>  /*
>   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
>   * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 35894fa35efe..e97e652a1aec 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -76,6 +76,13 @@ impl Error {
>      pub fn to_errno(self) -> core::ffi::c_int {
>          self.0
>      }
> +
> +    /// Returns the error encoded as a pointer.
> +    #[allow(dead_code)]
> +    pub(crate) fn to_ptr<T>(self) -> *mut T {
> +        // SAFETY: Valid as long as self.0 is a valid error

This should be something like "SAFETY: self.0 is a valid error due to
its invariant."?

Best,
Gary

> +        unsafe { bindings::ERR_PTR(self.0.into()) as *mut _ }
> +    }
>  }
>  
>  impl From<AllocError> for Error {
> 

Re: [PATCH v2 3/6] rust: error: Add Error::from_errno()

[Gary Guo]

On Wed, 29 Mar 2023 21:04:35 +0900
Asahi Lina <lina@asahilina.net> wrote:

> From: Miguel Ojeda <ojeda@kernel.org>
> 
> Add a function to create `Error` values out of a kernel error return,
> which safely upholds the invariant that the error code is well-formed
> (negative and greater than -MAX_ERRNO). If a malformed code is passed
> in, it will be converted to EINVAL.
> 
> Lina: Imported from rust-for-linux/rust as authored by Miguel and Fox
> with refactoring from Wedson, renamed from_kernel_errno() to
> from_errno().
> 
> Co-developed-by: Fox Chen <foxhlchen@gmail.com>
> Signed-off-by: Fox Chen <foxhlchen@gmail.com>
> Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
>  rust/kernel/error.rs | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index e97e652a1aec..659468bd1735 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -72,6 +72,25 @@ pub mod code {
>  pub struct Error(core::ffi::c_int);
>  
>  impl Error {
> +    /// Creates an [`Error`] from a kernel error code.
> +    ///
> +    /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
> +    /// be returned in such a case.
> +    pub(crate) fn from_errno(errno: core::ffi::c_int) -> Error {
> +        if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 {
> +            // TODO: Make it a `WARN_ONCE` once available.
> +            crate::pr_warn!(
> +                "attempted to create `Error` with out of range `errno`: {}",
> +                errno
> +            );
> +            return code::EINVAL;
> +        }
> +
> +        // INVARIANT: The check above ensures the type invariant
> +        // will hold.
> +        Error(errno)
> +    }
> +
>      /// Returns the kernel error code.
>      pub fn to_errno(self) -> core::ffi::c_int {
>          self.0
> 

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

Re: [PATCH v2 4/6] rust: error: Add to_result() helper

[Gary Guo]

On Wed, 29 Mar 2023 21:04:36 +0900
Asahi Lina <lina@asahilina.net> wrote:

> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> Add a to_result() helper to convert kernel C return values to a Rust
> Result, mapping >=0 values to Ok(()) and negative values to Err(...),
> with Error::from_errno() ensuring that the errno is within range.
> 
> Lina: Imported from rust-for-linux/rust, originally developed by Wedson
> as part of the AMBA device driver support.
> 
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
>  rust/kernel/error.rs | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 659468bd1735..4f599c4d1752 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -167,3 +167,13 @@ impl From<core::convert::Infallible> for Error {
>  /// it should still be modeled as returning a `Result` rather than
>  /// just an [`Error`].
>  pub type Result<T = ()> = core::result::Result<T, Error>;
> +
> +/// Converts an integer as returned by a C kernel function to an error if it's negative, and
> +/// `Ok(())` otherwise.
> +pub fn to_result(err: core::ffi::c_int) -> Result {
> +    if err < 0 {
> +        Err(Error::from_errno(err))
> +    } else {
> +        Ok(())
> +    }
> +}
> 

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

Re: [PATCH v2 5/6] rust: error: Add a helper to convert a C ERR_PTR to a `Result`

[Gary Guo]

On Wed, 29 Mar 2023 21:04:37 +0900
Asahi Lina <lina@asahilina.net> wrote:

> From: Sven Van Asbroeck <thesven73@gmail.com>
> 
> Some kernel C API functions return a pointer which embeds an optional
> `errno`. Callers are supposed to check the returned pointer with
> `IS_ERR()` and if this returns `true`, retrieve the `errno` using
> `PTR_ERR()`.
> 
> Create a Rust helper function to implement the Rust equivalent:
> transform a `*mut T` to `Result<*mut T>`.
> 
> Lina: Imported from rust-for-linux/linux, with subsequent refactoring
> and contributions squashed in and attributed below. Replaced usage of
> from_kernel_errno_unchecked() with an open-coded constructor, since this
> is the only user anyway. Renamed the function to from_err_ptr().
> 
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Co-developed-by: Fox Chen <foxhlchen@gmail.com>
> Signed-off-by: Fox Chen <foxhlchen@gmail.com>
> Co-developed-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
>  rust/helpers.c       | 12 ++++++++++++
>  rust/kernel/error.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 89f4cd1e0df3..04b9be46e887 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -53,6 +53,18 @@ __force void *rust_helper_ERR_PTR(long err)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_ERR_PTR);
>  
> +bool rust_helper_IS_ERR(__force const void *ptr)
> +{
> +	return IS_ERR(ptr);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_IS_ERR);
> +
> +long rust_helper_PTR_ERR(__force const void *ptr)
> +{
> +	return PTR_ERR(ptr);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
> +
>  /*
>   * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
>   * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 4f599c4d1752..6b10129075a7 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -177,3 +177,52 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
>          Ok(())
>      }
>  }
> +
> +/// Transform a kernel "error pointer" to a normal pointer.
> +///
> +/// Some kernel C API functions return an "error pointer" which optionally
> +/// embeds an `errno`. Callers are supposed to check the returned pointer
> +/// for errors. This function performs the check and converts the "error pointer"
> +/// to a normal pointer in an idiomatic fashion.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// # use kernel::from_err_ptr;
> +/// # use kernel::bindings;
> +/// fn devm_platform_ioremap_resource(
> +///     pdev: &mut PlatformDevice,
> +///     index: u32,
> +/// ) -> Result<*mut core::ffi::c_void> {
> +///     // SAFETY: FFI call.
> +///     unsafe {
> +///         from_err_ptr(bindings::devm_platform_ioremap_resource(
> +///             pdev.to_ptr(),
> +///             index,
> +///         ))
> +///     }
> +/// }
> +/// ```
> +// TODO: Remove `dead_code` marker once an in-kernel client is available.
> +#[allow(dead_code)]
> +pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
> +    // CAST: Casting a pointer to `*const core::ffi::c_void` is always valid.
> +    let const_ptr: *const core::ffi::c_void = ptr.cast();
> +    // SAFETY: The FFI function does not deref the pointer.
> +    if unsafe { bindings::IS_ERR(const_ptr) } {
> +        // SAFETY: The FFI function does not deref the pointer.
> +        let err = unsafe { bindings::PTR_ERR(const_ptr) };
> +        // CAST: If `IS_ERR()` returns `true`,
> +        // then `PTR_ERR()` is guaranteed to return a
> +        // negative value greater-or-equal to `-bindings::MAX_ERRNO`,
> +        // which always fits in an `i16`, as per the invariant above.
> +        // And an `i16` always fits in an `i32`. So casting `err` to
> +        // an `i32` can never overflow, and is always valid.
> +        //
> +        // SAFETY: `IS_ERR()` ensures `err` is a
> +        // negative value greater-or-equal to `-bindings::MAX_ERRNO`.

The comment here should read `INVARIANT: ` because now you are
constructing `Error` which have type invariants.

Where is `from_kernel_errno_unchecked`? I believe the `rust` branch
on GitHub have it, as it's generally good to limit places where type
invariants are used directly, so the method is used to convert an
invariant requirement to a safety requirement that is more obvious due
to use of `unsafe`.

> +        #[cfg_attr(CONFIG_ARM, allow(clippy::unnecessary_cast))]

This should either be gated based on pointer size, or should just be a
blanket allow without cfg gating.

> +        return Err(Error(err as i32));
> +    }
> +    Ok(ptr)
> +}
> 

Best,
Gary

Re: [PATCH v2 1/6] rust: error: Rename to_kernel_errno() -> to_errno()

[Miguel Ojeda]

On Wed, Mar 29, 2023 at 8:16 PM Martin Rodriguez Reboredo
<yakoyoku@gmail.com> wrote:
>
> Ah, yes, they were supposed to have my email with it. I wasn't that
> livened up when I sent them.

No problem at all! And thanks for reviewing!

Cheers,
Miguel

Re: [PATCH v2 1/6] rust: error: Rename to_kernel_errno() -> to_errno()

[Miguel Ojeda]

On Wed, Mar 29, 2023 at 10:32 PM Gary Guo <gary@garyguo.net> wrote:
>
> Thanks Lina for implementing my suggestion.

I will add a `Suggested-by: you` too when I apply this, shout if you
don't want it!

Cheers,
Miguel