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 <[email protected]>
---
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/[email protected]
---
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
This is kernel code, so specifying "kernel" is redundant. Let's simplify
things and just call it to_errno().
Signed-off-by: Asahi Lina <[email protected]>
---
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
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 <[email protected]>
---
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
From: Miguel Ojeda <[email protected]>
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 <[email protected]>
Signed-off-by: Fox Chen <[email protected]>
Co-developed-by: Wedson Almeida Filho <[email protected]>
Signed-off-by: Wedson Almeida Filho <[email protected]>
Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Andreas Hindborg <[email protected]>
Signed-off-by: Asahi Lina <[email protected]>
---
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
From: Wedson Almeida Filho <[email protected]>
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 <[email protected]>
Reviewed-by: Andreas Hindborg <[email protected]>
Signed-off-by: Asahi Lina <[email protected]>
---
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
From: Sven Van Asbroeck <[email protected]>
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 <[email protected]>
Signed-off-by: Boqun Feng <[email protected]>
Co-developed-by: Miguel Ojeda <[email protected]>
Signed-off-by: Miguel Ojeda <[email protected]>
Co-developed-by: Fox Chen <[email protected]>
Signed-off-by: Fox Chen <[email protected]>
Co-developed-by: Gary Guo <[email protected]>
Signed-off-by: Gary Guo <[email protected]>
Signed-off-by: Sven Van Asbroeck <[email protected]>
Signed-off-by: Asahi Lina <[email protected]>
---
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
From: Wedson Almeida Filho <[email protected]>
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 <[email protected]>
Signed-off-by: Fox Chen <[email protected]>
Co-developed-by: Miguel Ojeda <[email protected]>
Signed-off-by: Miguel Ojeda <[email protected]>
Signed-off-by: Wedson Almeida Filho <[email protected]>
Signed-off-by: Asahi Lina <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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
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
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
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
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
On Wed, Mar 29, 2023 at 4:47 PM Martin Rodriguez Reboredo
<[email protected]> wrote:
>
> Reviewed-by: Martin Rodriguez Reboredo
These (in the different patches) are supposed to have
<[email protected]> email when I take them, right? (no need to resend
them)
Cheers,
Miguel
On 3/29/23 12:04, Miguel Ojeda wrote:
> On Wed, Mar 29, 2023 at 4:47 PM Martin Rodriguez Reboredo
> <[email protected]> wrote:
>>
>> Reviewed-by: Martin Rodriguez Reboredo
>
> These (in the different patches) are supposed to have
> <[email protected]> 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
On Wed, 29 Mar 2023 21:04:36 +0900
Asahi Lina <[email protected]> wrote:
> From: Wedson Almeida Filho <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Andreas Hindborg <[email protected]>
> Signed-off-by: Asahi Lina <[email protected]>
> ---
> 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 <[email protected]>
On Wed, 29 Mar 2023 21:04:33 +0900
Asahi Lina <[email protected]> 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 <[email protected]>
Thanks Lina for implementing my suggestion.
Reviewed-by: Gary Guo <[email protected]>
> ---
> 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();
> }}
> }}
> }}
>
On Wed, 29 Mar 2023 21:04:34 +0900
Asahi Lina <[email protected]> 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 <[email protected]>
> ---
> 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 {
>
On Wed, 29 Mar 2023 21:04:35 +0900
Asahi Lina <[email protected]> wrote:
> From: Miguel Ojeda <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Fox Chen <[email protected]>
> Co-developed-by: Wedson Almeida Filho <[email protected]>
> Signed-off-by: Wedson Almeida Filho <[email protected]>
> Signed-off-by: Miguel Ojeda <[email protected]>
> Reviewed-by: Andreas Hindborg <[email protected]>
> Signed-off-by: Asahi Lina <[email protected]>
> ---
> 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 <[email protected]>
On Wed, 29 Mar 2023 21:04:37 +0900
Asahi Lina <[email protected]> wrote:
> From: Sven Van Asbroeck <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Boqun Feng <[email protected]>
> Co-developed-by: Miguel Ojeda <[email protected]>
> Signed-off-by: Miguel Ojeda <[email protected]>
> Co-developed-by: Fox Chen <[email protected]>
> Signed-off-by: Fox Chen <[email protected]>
> Co-developed-by: Gary Guo <[email protected]>
> Signed-off-by: Gary Guo <[email protected]>
> Signed-off-by: Sven Van Asbroeck <[email protected]>
> Signed-off-by: Asahi Lina <[email protected]>
> ---
> 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
On Wed, Mar 29, 2023 at 8:16 PM Martin Rodriguez Reboredo
<[email protected]> 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
On Wed, Mar 29, 2023 at 10:32 PM Gary Guo <[email protected]> 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