rust/kernel/auxiliary.rs | 1 + rust/kernel/block/mq/tag_set.rs | 2 +- rust/kernel/cpufreq.rs | 3 ++- rust/kernel/devres.rs | 1 + rust/kernel/dma.rs | 3 +++ rust/kernel/error.rs | 17 ++++++++++++----- rust/kernel/miscdevice.rs | 2 +- rust/kernel/mm/virt.rs | 1 + rust/kernel/pci.rs | 3 ++- rust/kernel/platform.rs | 2 +- rust/kernel/regulator.rs | 5 +++-- 11 files changed, 28 insertions(+), 12 deletions(-)
Current `to_result` helper takes a `c_int` and returns `Ok(())` on
success and this has some issues like:
- Callers lose the original return value and often have to store
it in a temporary variable before calling `to_result`.
- It only supports `c_int`, which makes callers to unnecessarily
cast when working with other types (e.g. `u16` in phy
abstractions). We even have some places that ignore to use
`to_result` helper because the input doesn't fit in `c_int`
(see [0]).
[0]: https://lore.kernel.org/all/20250822080252.773d6f54@nimda.home/
This patch changes `to_result` to be generic and also return the
original value on success.
So that the code that previously looked like:
let ret = unsafe { bindings::some_ffi_call() };
to_result(ret).map(|()| SomeType::new(ret))
can now be written more directly as:
to_result(unsafe { bindings::some_ffi_call() })
.map(|ret| SomeType::new(ret))
Similarly, code such as:
let res: isize = $some_ffi_call();
if res < 0 {
return Err(Error::from_errno(res as i32));
}
can now be used with `to_result` as:
to_result($some_ffi_call())?;
Existing call sites that only care about success/failure are updated
to append `.map(|_| ())` to preserve their previous semantics. They
can also use the equivalent pattern:
to_result($something)?;
Ok(())
This patch only fixes the callers that broke after the changes on `to_result`.
I haven't included all the improvements made possible by the new design since
that could conflict with other ongoing patches [1]. Once this patch is approved
and applied, I am planning to follow up with creating a "good first issue" on [2]
for those additional changes.
[1]: https://lore.kernel.org/rust-for-linux/?q=to_result
[2]: https://github.com/Rust-for-Linux/linux
Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089/topic/x/near/536374456
Signed-off-by: Onur Özkan <work@onurozkan.dev>
---
rust/kernel/auxiliary.rs | 1 +
rust/kernel/block/mq/tag_set.rs | 2 +-
rust/kernel/cpufreq.rs | 3 ++-
rust/kernel/devres.rs | 1 +
rust/kernel/dma.rs | 3 +++
rust/kernel/error.rs | 17 ++++++++++++-----
rust/kernel/miscdevice.rs | 2 +-
rust/kernel/mm/virt.rs | 1 +
rust/kernel/pci.rs | 3 ++-
rust/kernel/platform.rs | 2 +-
rust/kernel/regulator.rs | 5 +++--
11 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index 4749fb6bffef..479c0ad2a572 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -43,6 +43,7 @@ unsafe fn register(
to_result(unsafe {
bindings::__auxiliary_driver_register(adrv.get(), module.0, name.as_char_ptr())
})
+ .map(|_| ())
}
unsafe fn unregister(adrv: &Opaque<Self::RegType>) {
diff --git a/rust/kernel/block/mq/tag_set.rs b/rust/kernel/block/mq/tag_set.rs
index c3cf56d52bee..0e7883163000 100644
--- a/rust/kernel/block/mq/tag_set.rs
+++ b/rust/kernel/block/mq/tag_set.rs
@@ -65,7 +65,7 @@ pub fn new(
// SAFETY: we do not move out of `tag_set`.
let tag_set: &mut Opaque<_> = unsafe { Pin::get_unchecked_mut(tag_set) };
// SAFETY: `tag_set` is a reference to an initialized `blk_mq_tag_set`.
- error::to_result( unsafe { bindings::blk_mq_alloc_tag_set(tag_set.get())})
+ error::to_result( unsafe { bindings::blk_mq_alloc_tag_set(tag_set.get())}).map(|_| ())
}),
_p: PhantomData,
})
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index be2dffbdb546..c3fa20ce229a 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -157,6 +157,7 @@ pub fn as_raw(&self) -> *mut bindings::cpufreq_policy_data {
pub fn generic_verify(&self) -> Result {
// SAFETY: By the type invariant, the pointer stored in `self` is valid.
to_result(unsafe { bindings::cpufreq_generic_frequency_table_verify(self.as_raw()) })
+ .map(|_| ())
}
}
@@ -519,7 +520,7 @@ pub fn set_suspend_freq(&mut self, freq: Hertz) -> &mut Self {
#[inline]
pub fn generic_suspend(&mut self) -> Result {
// SAFETY: By the type invariant, the pointer stored in `self` is valid.
- to_result(unsafe { bindings::cpufreq_generic_suspend(self.as_mut_ref()) })
+ to_result(unsafe { bindings::cpufreq_generic_suspend(self.as_mut_ref()) }).map(|_| ())
}
/// Provides a wrapper to the generic get routine.
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index d04e3fcebafb..214cd9a0ebe0 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -328,6 +328,7 @@ fn register_foreign<P>(dev: &Device<Bound>, data: P) -> Result
// `ForeignOwnable` is released eventually.
bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::<P>), ptr.cast())
})
+ .map(|_| ())
}
/// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 68fe67624424..f614453ddb7d 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -34,6 +34,7 @@ unsafe fn dma_set_mask(&self, mask: DmaMask) -> Result {
// - The safety requirement of this function guarantees that there are no concurrent calls
// to DMA allocation and mapping primitives using this mask.
to_result(unsafe { bindings::dma_set_mask(self.as_ref().as_raw(), mask.value()) })
+ .map(|_| ())
}
/// Set up the device's DMA coherent addressing capabilities.
@@ -51,6 +52,7 @@ unsafe fn dma_set_coherent_mask(&self, mask: DmaMask) -> Result {
// - The safety requirement of this function guarantees that there are no concurrent calls
// to DMA allocation and mapping primitives using this mask.
to_result(unsafe { bindings::dma_set_coherent_mask(self.as_ref().as_raw(), mask.value()) })
+ .map(|_| ())
}
/// Set up the device's DMA addressing capabilities.
@@ -72,6 +74,7 @@ unsafe fn dma_set_mask_and_coherent(&self, mask: DmaMask) -> Result {
to_result(unsafe {
bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(), mask.value())
})
+ .map(|_| ())
}
}
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index db14da976722..f76afa4b7ec1 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -378,12 +378,19 @@ fn from(e: core::convert::Infallible) -> Error {
pub type Result<T = (), E = Error> = core::result::Result<T, E>;
/// 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: crate::ffi::c_int) -> Result {
- if err < 0 {
- Err(Error::from_errno(err))
+/// returns the original value otherwise.
+pub fn to_result<T>(code: T) -> Result<T>
+where
+ T: Copy + TryInto<i32>,
+{
+ // Try casting into `i32`.
+ let casted: crate::ffi::c_int = code.try_into().unwrap_or(0);
+
+ if casted < 0 {
+ Err(Error::from_errno(casted))
} else {
- Ok(())
+ // Return the original input value.
+ Ok(code)
}
}
diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index 6373fe183b27..22b72ae84c03 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -79,7 +79,7 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
// the destructor of this type deallocates the memory.
// INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
// misc device.
- to_result(unsafe { bindings::misc_register(slot) })
+ to_result(unsafe { bindings::misc_register(slot) }).map(|_| ())
}),
_t: PhantomData,
})
diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs
index a1bfa4e19293..5494f96e91b0 100644
--- a/rust/kernel/mm/virt.rs
+++ b/rust/kernel/mm/virt.rs
@@ -195,6 +195,7 @@ pub fn vm_insert_page(&self, address: usize, page: &Page) -> Result {
// SAFETY: By the type invariant of `Self` caller has read access and has verified that
// `VM_MIXEDMAP` is set. By invariant on `Page` the page has order 0.
to_result(unsafe { bindings::vm_insert_page(self.as_ptr(), address, page.as_ptr()) })
+ .map(|_| ())
}
}
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 887ee611b553..6e917752cb89 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -48,6 +48,7 @@ unsafe fn register(
to_result(unsafe {
bindings::__pci_register_driver(pdrv.get(), module.0, name.as_char_ptr())
})
+ .map(|_| ())
}
unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
@@ -437,7 +438,7 @@ impl Device<device::Core> {
/// Enable memory resources for this device.
pub fn enable_device_mem(&self) -> Result {
// SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
- to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) })
+ to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) }).map(|_| ())
}
/// Enable bus-mastering for this device.
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 8f028c76f9fa..5a5561c7326e 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -54,7 +54,7 @@ unsafe fn register(
}
// SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
- to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) })
+ to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) }).map(|_| ())
}
unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
index 65f3a125348f..e17ae6e9a990 100644
--- a/rust/kernel/regulator.rs
+++ b/rust/kernel/regulator.rs
@@ -261,6 +261,7 @@ pub fn set_voltage(&self, min_voltage: Voltage, max_voltage: Voltage) -> Result
max_voltage.as_microvolts(),
)
})
+ .map(|_| ())
}
/// Gets the current voltage of the regulator.
@@ -291,12 +292,12 @@ fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {
fn enable_internal(&mut self) -> Result {
// SAFETY: Safe as per the type invariants of `Regulator`.
- to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) })
+ to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) }).map(|_| ())
}
fn disable_internal(&mut self) -> Result {
// SAFETY: Safe as per the type invariants of `Regulator`.
- to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) })
+ to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) }).map(|_| ())
}
}
--
2.50.0
On Tue, Sep 9, 2025 at 2:33 PM Onur Özkan <work@onurozkan.dev> wrote:
>
> Current `to_result` helper takes a `c_int` and returns `Ok(())` on
> success and this has some issues like:
>
> - Callers lose the original return value and often have to store
> it in a temporary variable before calling `to_result`.
>
> - It only supports `c_int`, which makes callers to unnecessarily
> cast when working with other types (e.g. `u16` in phy
> abstractions). We even have some places that ignore to use
> `to_result` helper because the input doesn't fit in `c_int`
> (see [0]).
>
> [0]: https://lore.kernel.org/all/20250822080252.773d6f54@nimda.home/
>
> This patch changes `to_result` to be generic and also return the
> original value on success.
>
> So that the code that previously looked like:
>
> let ret = unsafe { bindings::some_ffi_call() };
> to_result(ret).map(|()| SomeType::new(ret))
>
> can now be written more directly as:
>
> to_result(unsafe { bindings::some_ffi_call() })
> .map(|ret| SomeType::new(ret))
>
> Similarly, code such as:
>
> let res: isize = $some_ffi_call();
> if res < 0 {
> return Err(Error::from_errno(res as i32));
> }
>
> can now be used with `to_result` as:
>
> to_result($some_ffi_call())?;
>
> Existing call sites that only care about success/failure are updated
> to append `.map(|_| ())` to preserve their previous semantics. They
> can also use the equivalent pattern:
>
> to_result($something)?;
> Ok(())
>
> This patch only fixes the callers that broke after the changes on `to_result`.
> I haven't included all the improvements made possible by the new design since
> that could conflict with other ongoing patches [1]. Once this patch is approved
> and applied, I am planning to follow up with creating a "good first issue" on [2]
> for those additional changes.
>
> [1]: https://lore.kernel.org/rust-for-linux/?q=to_result
> [2]: https://github.com/Rust-for-Linux/linux
>
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089/topic/x/near/536374456
> Signed-off-by: Onur Özkan <work@onurozkan.dev>
> ---
> rust/kernel/auxiliary.rs | 1 +
> rust/kernel/block/mq/tag_set.rs | 2 +-
> rust/kernel/cpufreq.rs | 3 ++-
> rust/kernel/devres.rs | 1 +
> rust/kernel/dma.rs | 3 +++
> rust/kernel/error.rs | 17 ++++++++++++-----
> rust/kernel/miscdevice.rs | 2 +-
> rust/kernel/mm/virt.rs | 1 +
> rust/kernel/pci.rs | 3 ++-
> rust/kernel/platform.rs | 2 +-
> rust/kernel/regulator.rs | 5 +++--
> 11 files changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
> index 4749fb6bffef..479c0ad2a572 100644
> --- a/rust/kernel/auxiliary.rs
> +++ b/rust/kernel/auxiliary.rs
> @@ -43,6 +43,7 @@ unsafe fn register(
> to_result(unsafe {
> bindings::__auxiliary_driver_register(adrv.get(), module.0, name.as_char_ptr())
> })
> + .map(|_| ())
> }
>
> unsafe fn unregister(adrv: &Opaque<Self::RegType>) {
> diff --git a/rust/kernel/block/mq/tag_set.rs b/rust/kernel/block/mq/tag_set.rs
> index c3cf56d52bee..0e7883163000 100644
> --- a/rust/kernel/block/mq/tag_set.rs
> +++ b/rust/kernel/block/mq/tag_set.rs
> @@ -65,7 +65,7 @@ pub fn new(
> // SAFETY: we do not move out of `tag_set`.
> let tag_set: &mut Opaque<_> = unsafe { Pin::get_unchecked_mut(tag_set) };
> // SAFETY: `tag_set` is a reference to an initialized `blk_mq_tag_set`.
> - error::to_result( unsafe { bindings::blk_mq_alloc_tag_set(tag_set.get())})
> + error::to_result( unsafe { bindings::blk_mq_alloc_tag_set(tag_set.get())}).map(|_| ())
> }),
> _p: PhantomData,
> })
> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> index be2dffbdb546..c3fa20ce229a 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -157,6 +157,7 @@ pub fn as_raw(&self) -> *mut bindings::cpufreq_policy_data {
> pub fn generic_verify(&self) -> Result {
> // SAFETY: By the type invariant, the pointer stored in `self` is valid.
> to_result(unsafe { bindings::cpufreq_generic_frequency_table_verify(self.as_raw()) })
> + .map(|_| ())
> }
> }
>
> @@ -519,7 +520,7 @@ pub fn set_suspend_freq(&mut self, freq: Hertz) -> &mut Self {
> #[inline]
> pub fn generic_suspend(&mut self) -> Result {
> // SAFETY: By the type invariant, the pointer stored in `self` is valid.
> - to_result(unsafe { bindings::cpufreq_generic_suspend(self.as_mut_ref()) })
> + to_result(unsafe { bindings::cpufreq_generic_suspend(self.as_mut_ref()) }).map(|_| ())
> }
>
> /// Provides a wrapper to the generic get routine.
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index d04e3fcebafb..214cd9a0ebe0 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -328,6 +328,7 @@ fn register_foreign<P>(dev: &Device<Bound>, data: P) -> Result
> // `ForeignOwnable` is released eventually.
> bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::<P>), ptr.cast())
> })
> + .map(|_| ())
> }
>
> /// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index 68fe67624424..f614453ddb7d 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -34,6 +34,7 @@ unsafe fn dma_set_mask(&self, mask: DmaMask) -> Result {
> // - The safety requirement of this function guarantees that there are no concurrent calls
> // to DMA allocation and mapping primitives using this mask.
> to_result(unsafe { bindings::dma_set_mask(self.as_ref().as_raw(), mask.value()) })
> + .map(|_| ())
> }
>
> /// Set up the device's DMA coherent addressing capabilities.
> @@ -51,6 +52,7 @@ unsafe fn dma_set_coherent_mask(&self, mask: DmaMask) -> Result {
> // - The safety requirement of this function guarantees that there are no concurrent calls
> // to DMA allocation and mapping primitives using this mask.
> to_result(unsafe { bindings::dma_set_coherent_mask(self.as_ref().as_raw(), mask.value()) })
> + .map(|_| ())
> }
>
> /// Set up the device's DMA addressing capabilities.
> @@ -72,6 +74,7 @@ unsafe fn dma_set_mask_and_coherent(&self, mask: DmaMask) -> Result {
> to_result(unsafe {
> bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(), mask.value())
> })
> + .map(|_| ())
> }
> }
>
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index db14da976722..f76afa4b7ec1 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -378,12 +378,19 @@ fn from(e: core::convert::Infallible) -> Error {
> pub type Result<T = (), E = Error> = core::result::Result<T, E>;
>
> /// 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: crate::ffi::c_int) -> Result {
> - if err < 0 {
> - Err(Error::from_errno(err))
> +/// returns the original value otherwise.
> +pub fn to_result<T>(code: T) -> Result<T>
> +where
> + T: Copy + TryInto<i32>,
> +{
> + // Try casting into `i32`.
> + let casted: crate::ffi::c_int = code.try_into().unwrap_or(0);
> +
> + if casted < 0 {
> + Err(Error::from_errno(casted))
> } else {
> - Ok(())
> + // Return the original input value.
> + Ok(code)
> }
> }
>
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index 6373fe183b27..22b72ae84c03 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -79,7 +79,7 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> // the destructor of this type deallocates the memory.
> // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
> // misc device.
> - to_result(unsafe { bindings::misc_register(slot) })
> + to_result(unsafe { bindings::misc_register(slot) }).map(|_| ())
> }),
> _t: PhantomData,
> })
> diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs
> index a1bfa4e19293..5494f96e91b0 100644
> --- a/rust/kernel/mm/virt.rs
> +++ b/rust/kernel/mm/virt.rs
> @@ -195,6 +195,7 @@ pub fn vm_insert_page(&self, address: usize, page: &Page) -> Result {
> // SAFETY: By the type invariant of `Self` caller has read access and has verified that
> // `VM_MIXEDMAP` is set. By invariant on `Page` the page has order 0.
> to_result(unsafe { bindings::vm_insert_page(self.as_ptr(), address, page.as_ptr()) })
> + .map(|_| ())
> }
> }
>
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 887ee611b553..6e917752cb89 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -48,6 +48,7 @@ unsafe fn register(
> to_result(unsafe {
> bindings::__pci_register_driver(pdrv.get(), module.0, name.as_char_ptr())
> })
> + .map(|_| ())
> }
>
> unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
> @@ -437,7 +438,7 @@ impl Device<device::Core> {
> /// Enable memory resources for this device.
> pub fn enable_device_mem(&self) -> Result {
> // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> - to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) })
> + to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) }).map(|_| ())
> }
>
> /// Enable bus-mastering for this device.
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index 8f028c76f9fa..5a5561c7326e 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -54,7 +54,7 @@ unsafe fn register(
> }
>
> // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> - to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) })
> + to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) }).map(|_| ())
> }
>
> unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
> index 65f3a125348f..e17ae6e9a990 100644
> --- a/rust/kernel/regulator.rs
> +++ b/rust/kernel/regulator.rs
> @@ -261,6 +261,7 @@ pub fn set_voltage(&self, min_voltage: Voltage, max_voltage: Voltage) -> Result
> max_voltage.as_microvolts(),
> )
> })
> + .map(|_| ())
> }
>
> /// Gets the current voltage of the regulator.
> @@ -291,12 +292,12 @@ fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {
>
> fn enable_internal(&mut self) -> Result {
> // SAFETY: Safe as per the type invariants of `Regulator`.
> - to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) })
> + to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) }).map(|_| ())
> }
>
> fn disable_internal(&mut self) -> Result {
> // SAFETY: Safe as per the type invariants of `Regulator`.
> - to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) })
> + to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) }).map(|_| ())
IMO all of the new map calls in this patch should use the
to_result(...)?
Ok(())
syntax.
Alice
On Tue, 9 Sep 2025 14:36:13 +0200
Alice Ryhl <aliceryhl@google.com> wrote:
> On Tue, Sep 9, 2025 at 2:33 PM Onur Özkan <work@onurozkan.dev> wrote:
> >
> > Current `to_result` helper takes a `c_int` and returns `Ok(())` on
> > success and this has some issues like:
> >
> > - Callers lose the original return value and often have to store
> > it in a temporary variable before calling `to_result`.
> >
> > - It only supports `c_int`, which makes callers to unnecessarily
> > cast when working with other types (e.g. `u16` in phy
> > abstractions). We even have some places that ignore to use
> > `to_result` helper because the input doesn't fit in `c_int`
> > (see [0]).
> >
> > [0]: https://lore.kernel.org/all/20250822080252.773d6f54@nimda.home/
> >
> > This patch changes `to_result` to be generic and also return the
> > original value on success.
> >
> > So that the code that previously looked like:
> >
> > let ret = unsafe { bindings::some_ffi_call() };
> > to_result(ret).map(|()| SomeType::new(ret))
> >
> > can now be written more directly as:
> >
> > to_result(unsafe { bindings::some_ffi_call() })
> > .map(|ret| SomeType::new(ret))
> >
> > Similarly, code such as:
> >
> > let res: isize = $some_ffi_call();
> > if res < 0 {
> > return Err(Error::from_errno(res as i32));
> > }
> >
> > can now be used with `to_result` as:
> >
> > to_result($some_ffi_call())?;
> >
> > Existing call sites that only care about success/failure are updated
> > to append `.map(|_| ())` to preserve their previous semantics. They
> > can also use the equivalent pattern:
> >
> > to_result($something)?;
> > Ok(())
> >
> > This patch only fixes the callers that broke after the changes on
> > `to_result`. I haven't included all the improvements made possible
> > by the new design since that could conflict with other ongoing
> > patches [1]. Once this patch is approved and applied, I am planning
> > to follow up with creating a "good first issue" on [2] for those
> > additional changes.
> >
> > [1]: https://lore.kernel.org/rust-for-linux/?q=to_result
> > [2]: https://github.com/Rust-for-Linux/linux
> >
> > Link:
> > https://rust-for-linux.zulipchat.com/#narrow/channel/288089/topic/x/near/536374456
> > Signed-off-by: Onur Özkan <work@onurozkan.dev> ---
> > rust/kernel/auxiliary.rs | 1 +
> > rust/kernel/block/mq/tag_set.rs | 2 +-
> > rust/kernel/cpufreq.rs | 3 ++-
> > rust/kernel/devres.rs | 1 +
> > rust/kernel/dma.rs | 3 +++
> > rust/kernel/error.rs | 17 ++++++++++++-----
> > rust/kernel/miscdevice.rs | 2 +-
> > rust/kernel/mm/virt.rs | 1 +
> > rust/kernel/pci.rs | 3 ++-
> > rust/kernel/platform.rs | 2 +-
> > rust/kernel/regulator.rs | 5 +++--
> > 11 files changed, 28 insertions(+), 12 deletions(-)
> >
> > diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
> > index 4749fb6bffef..479c0ad2a572 100644
> > --- a/rust/kernel/auxiliary.rs
> > +++ b/rust/kernel/auxiliary.rs
> > @@ -43,6 +43,7 @@ unsafe fn register(
> > to_result(unsafe {
> > bindings::__auxiliary_driver_register(adrv.get(),
> > module.0, name.as_char_ptr()) })
> > + .map(|_| ())
> > }
> >
> > unsafe fn unregister(adrv: &Opaque<Self::RegType>) {
> > diff --git a/rust/kernel/block/mq/tag_set.rs
> > b/rust/kernel/block/mq/tag_set.rs index c3cf56d52bee..0e7883163000
> > 100644 --- a/rust/kernel/block/mq/tag_set.rs
> > +++ b/rust/kernel/block/mq/tag_set.rs
> > @@ -65,7 +65,7 @@ pub fn new(
> > // SAFETY: we do not move out of `tag_set`.
> > let tag_set: &mut Opaque<_> = unsafe {
> > Pin::get_unchecked_mut(tag_set) }; // SAFETY: `tag_set` is a
> > reference to an initialized `blk_mq_tag_set`.
> > - error::to_result( unsafe {
> > bindings::blk_mq_alloc_tag_set(tag_set.get())})
> > + error::to_result( unsafe {
> > bindings::blk_mq_alloc_tag_set(tag_set.get())}).map(|_| ()) }),
> > _p: PhantomData,
> > })
> > diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> > index be2dffbdb546..c3fa20ce229a 100644
> > --- a/rust/kernel/cpufreq.rs
> > +++ b/rust/kernel/cpufreq.rs
> > @@ -157,6 +157,7 @@ pub fn as_raw(&self) -> *mut
> > bindings::cpufreq_policy_data { pub fn generic_verify(&self) ->
> > Result { // SAFETY: By the type invariant, the pointer stored in
> > `self` is valid. to_result(unsafe {
> > bindings::cpufreq_generic_frequency_table_verify(self.as_raw()) })
> > + .map(|_| ())
> > }
> > }
> >
> > @@ -519,7 +520,7 @@ pub fn set_suspend_freq(&mut self, freq: Hertz)
> > -> &mut Self { #[inline]
> > pub fn generic_suspend(&mut self) -> Result {
> > // SAFETY: By the type invariant, the pointer stored in
> > `self` is valid.
> > - to_result(unsafe {
> > bindings::cpufreq_generic_suspend(self.as_mut_ref()) })
> > + to_result(unsafe {
> > bindings::cpufreq_generic_suspend(self.as_mut_ref()) }).map(|_| ())
> > }
> >
> > /// Provides a wrapper to the generic get routine.
> > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> > index d04e3fcebafb..214cd9a0ebe0 100644
> > --- a/rust/kernel/devres.rs
> > +++ b/rust/kernel/devres.rs
> > @@ -328,6 +328,7 @@ fn register_foreign<P>(dev: &Device<Bound>,
> > data: P) -> Result // `ForeignOwnable` is released eventually.
> > bindings::devm_add_action_or_reset(dev.as_raw(),
> > Some(callback::<P>), ptr.cast()) })
> > + .map(|_| ())
> > }
> >
> > /// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data`
> > once `dev` is unbound. diff --git a/rust/kernel/dma.rs
> > b/rust/kernel/dma.rs index 68fe67624424..f614453ddb7d 100644
> > --- a/rust/kernel/dma.rs
> > +++ b/rust/kernel/dma.rs
> > @@ -34,6 +34,7 @@ unsafe fn dma_set_mask(&self, mask: DmaMask) ->
> > Result { // - The safety requirement of this function guarantees
> > that there are no concurrent calls // to DMA allocation and
> > mapping primitives using this mask. to_result(unsafe {
> > bindings::dma_set_mask(self.as_ref().as_raw(), mask.value()) })
> > + .map(|_| ())
> > }
> >
> > /// Set up the device's DMA coherent addressing capabilities.
> > @@ -51,6 +52,7 @@ unsafe fn dma_set_coherent_mask(&self, mask:
> > DmaMask) -> Result { // - The safety requirement of this function
> > guarantees that there are no concurrent calls // to DMA
> > allocation and mapping primitives using this mask. to_result(unsafe
> > { bindings::dma_set_coherent_mask(self.as_ref().as_raw(),
> > mask.value()) })
> > + .map(|_| ())
> > }
> >
> > /// Set up the device's DMA addressing capabilities.
> > @@ -72,6 +74,7 @@ unsafe fn dma_set_mask_and_coherent(&self, mask:
> > DmaMask) -> Result { to_result(unsafe {
> > bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(),
> > mask.value()) })
> > + .map(|_| ())
> > }
> > }
> >
> > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > index db14da976722..f76afa4b7ec1 100644
> > --- a/rust/kernel/error.rs
> > +++ b/rust/kernel/error.rs
> > @@ -378,12 +378,19 @@ fn from(e: core::convert::Infallible) ->
> > Error { pub type Result<T = (), E = Error> =
> > core::result::Result<T, E>;
> >
> > /// 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: crate::ffi::c_int) -> Result {
> > - if err < 0 {
> > - Err(Error::from_errno(err))
> > +/// returns the original value otherwise.
> > +pub fn to_result<T>(code: T) -> Result<T>
> > +where
> > + T: Copy + TryInto<i32>,
> > +{
> > + // Try casting into `i32`.
> > + let casted: crate::ffi::c_int = code.try_into().unwrap_or(0);
> > +
> > + if casted < 0 {
> > + Err(Error::from_errno(casted))
> > } else {
> > - Ok(())
> > + // Return the original input value.
> > + Ok(code)
> > }
> > }
> >
> > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > index 6373fe183b27..22b72ae84c03 100644
> > --- a/rust/kernel/miscdevice.rs
> > +++ b/rust/kernel/miscdevice.rs
> > @@ -79,7 +79,7 @@ pub fn register(opts: MiscDeviceOptions) -> impl
> > PinInit<Self, Error> { // the destructor of this type deallocates
> > the memory. // INVARIANT: If this returns `Ok(())`, then the `slot`
> > will contain a registered // misc device.
> > - to_result(unsafe { bindings::misc_register(slot) })
> > + to_result(unsafe { bindings::misc_register(slot)
> > }).map(|_| ()) }),
> > _t: PhantomData,
> > })
> > diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs
> > index a1bfa4e19293..5494f96e91b0 100644
> > --- a/rust/kernel/mm/virt.rs
> > +++ b/rust/kernel/mm/virt.rs
> > @@ -195,6 +195,7 @@ pub fn vm_insert_page(&self, address: usize,
> > page: &Page) -> Result { // SAFETY: By the type invariant of `Self`
> > caller has read access and has verified that // `VM_MIXEDMAP` is
> > set. By invariant on `Page` the page has order 0. to_result(unsafe
> > { bindings::vm_insert_page(self.as_ptr(), address, page.as_ptr()) })
> > + .map(|_| ())
> > }
> > }
> >
> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > index 887ee611b553..6e917752cb89 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -48,6 +48,7 @@ unsafe fn register(
> > to_result(unsafe {
> > bindings::__pci_register_driver(pdrv.get(), module.0,
> > name.as_char_ptr()) })
> > + .map(|_| ())
> > }
> >
> > unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
> > @@ -437,7 +438,7 @@ impl Device<device::Core> {
> > /// Enable memory resources for this device.
> > pub fn enable_device_mem(&self) -> Result {
> > // SAFETY: `self.as_raw` is guaranteed to be a pointer to
> > a valid `struct pci_dev`.
> > - to_result(unsafe {
> > bindings::pci_enable_device_mem(self.as_raw()) })
> > + to_result(unsafe {
> > bindings::pci_enable_device_mem(self.as_raw()) }).map(|_| ()) }
> >
> > /// Enable bus-mastering for this device.
> > diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> > index 8f028c76f9fa..5a5561c7326e 100644
> > --- a/rust/kernel/platform.rs
> > +++ b/rust/kernel/platform.rs
> > @@ -54,7 +54,7 @@ unsafe fn register(
> > }
> >
> > // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> > - to_result(unsafe {
> > bindings::__platform_driver_register(pdrv.get(), module.0) })
> > + to_result(unsafe {
> > bindings::__platform_driver_register(pdrv.get(), module.0)
> > }).map(|_| ()) }
> >
> > unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
> > diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
> > index 65f3a125348f..e17ae6e9a990 100644
> > --- a/rust/kernel/regulator.rs
> > +++ b/rust/kernel/regulator.rs
> > @@ -261,6 +261,7 @@ pub fn set_voltage(&self, min_voltage: Voltage,
> > max_voltage: Voltage) -> Result max_voltage.as_microvolts(),
> > )
> > })
> > + .map(|_| ())
> > }
> >
> > /// Gets the current voltage of the regulator.
> > @@ -291,12 +292,12 @@ fn get_internal(dev: &Device, name: &CStr) ->
> > Result<Regulator<T>> {
> >
> > fn enable_internal(&mut self) -> Result {
> > // SAFETY: Safe as per the type invariants of `Regulator`.
> > - to_result(unsafe {
> > bindings::regulator_enable(self.inner.as_ptr()) })
> > + to_result(unsafe {
> > bindings::regulator_enable(self.inner.as_ptr()) }).map(|_| ()) }
> >
> > fn disable_internal(&mut self) -> Result {
> > // SAFETY: Safe as per the type invariants of `Regulator`.
> > - to_result(unsafe {
> > bindings::regulator_disable(self.inner.as_ptr()) })
> > + to_result(unsafe {
> > bindings::regulator_disable(self.inner.as_ptr()) }).map(|_| ())
>
> IMO all of the new map calls in this patch should use the
>
> to_result(...)?
> Ok(())
>
> syntax.
>
> Alice
Agreed, will do it in v2.
-Onur
> On 9 Sep 2025, at 09:36, Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Tue, Sep 9, 2025 at 2:33 PM Onur Özkan <work@onurozkan.dev> wrote:
>>
>> Current `to_result` helper takes a `c_int` and returns `Ok(())` on
>> success and this has some issues like:
>>
>> - Callers lose the original return value and often have to store
>> it in a temporary variable before calling `to_result`.
>>
>> - It only supports `c_int`, which makes callers to unnecessarily
>> cast when working with other types (e.g. `u16` in phy
>> abstractions). We even have some places that ignore to use
>> `to_result` helper because the input doesn't fit in `c_int`
>> (see [0]).
>>
>> [0]: https://lore.kernel.org/all/20250822080252.773d6f54@nimda.home/
>>
>> This patch changes `to_result` to be generic and also return the
>> original value on success.
>>
>> So that the code that previously looked like:
>>
>> let ret = unsafe { bindings::some_ffi_call() };
>> to_result(ret).map(|()| SomeType::new(ret))
>>
>> can now be written more directly as:
>>
>> to_result(unsafe { bindings::some_ffi_call() })
>> .map(|ret| SomeType::new(ret))
>>
>> Similarly, code such as:
>>
>> let res: isize = $some_ffi_call();
>> if res < 0 {
>> return Err(Error::from_errno(res as i32));
>> }
>>
>> can now be used with `to_result` as:
>>
>> to_result($some_ffi_call())?;
>>
>> Existing call sites that only care about success/failure are updated
>> to append `.map(|_| ())` to preserve their previous semantics. They
>> can also use the equivalent pattern:
>>
>> to_result($something)?;
>> Ok(())
>>
>> This patch only fixes the callers that broke after the changes on `to_result`.
What exactly broke for regulator.rs <http://regulator.rs/>? It works just fine.
>> I haven't included all the improvements made possible by the new design since
>> that could conflict with other ongoing patches [1]. Once this patch is approved
>> and applied, I am planning to follow up with creating a "good first issue" on [2]
>> for those additional changes.
>>
>> [1]: https://lore.kernel.org/rust-for-linux/?q=to_result
>> [2]: https://github.com/Rust-for-Linux/linux
>>
>> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089/topic/x/near/536374456
>> Signed-off-by: Onur Özkan <work@onurozkan.dev>
>> ---
>> rust/kernel/auxiliary.rs | 1 +
>> rust/kernel/block/mq/tag_set.rs | 2 +-
>> rust/kernel/cpufreq.rs | 3 ++-
>> rust/kernel/devres.rs | 1 +
>> rust/kernel/dma.rs | 3 +++
>> rust/kernel/error.rs | 17 ++++++++++++-----
>> rust/kernel/miscdevice.rs | 2 +-
>> rust/kernel/mm/virt.rs | 1 +
>> rust/kernel/pci.rs | 3 ++-
>> rust/kernel/platform.rs | 2 +-
>> rust/kernel/regulator.rs | 5 +++--
>> 11 files changed, 28 insertions(+), 12 deletions(-)
>>
>> diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
>> index 4749fb6bffef..479c0ad2a572 100644
>> --- a/rust/kernel/auxiliary.rs
>> +++ b/rust/kernel/auxiliary.rs
>> @@ -43,6 +43,7 @@ unsafe fn register(
>> to_result(unsafe {
>> bindings::__auxiliary_driver_register(adrv.get(), module.0, name.as_char_ptr())
>> })
>> + .map(|_| ())
>> }
>>
>> unsafe fn unregister(adrv: &Opaque<Self::RegType>) {
>> diff --git a/rust/kernel/block/mq/tag_set.rs b/rust/kernel/block/mq/tag_set.rs
>> index c3cf56d52bee..0e7883163000 100644
>> --- a/rust/kernel/block/mq/tag_set.rs
>> +++ b/rust/kernel/block/mq/tag_set.rs
>> @@ -65,7 +65,7 @@ pub fn new(
>> // SAFETY: we do not move out of `tag_set`.
>> let tag_set: &mut Opaque<_> = unsafe { Pin::get_unchecked_mut(tag_set) };
>> // SAFETY: `tag_set` is a reference to an initialized `blk_mq_tag_set`.
>> - error::to_result( unsafe { bindings::blk_mq_alloc_tag_set(tag_set.get())})
>> + error::to_result( unsafe { bindings::blk_mq_alloc_tag_set(tag_set.get())}).map(|_| ())
>> }),
>> _p: PhantomData,
>> })
>> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
>> index be2dffbdb546..c3fa20ce229a 100644
>> --- a/rust/kernel/cpufreq.rs
>> +++ b/rust/kernel/cpufreq.rs
>> @@ -157,6 +157,7 @@ pub fn as_raw(&self) -> *mut bindings::cpufreq_policy_data {
>> pub fn generic_verify(&self) -> Result {
>> // SAFETY: By the type invariant, the pointer stored in `self` is valid.
>> to_result(unsafe { bindings::cpufreq_generic_frequency_table_verify(self.as_raw()) })
>> + .map(|_| ())
>> }
>> }
>>
>> @@ -519,7 +520,7 @@ pub fn set_suspend_freq(&mut self, freq: Hertz) -> &mut Self {
>> #[inline]
>> pub fn generic_suspend(&mut self) -> Result {
>> // SAFETY: By the type invariant, the pointer stored in `self` is valid.
>> - to_result(unsafe { bindings::cpufreq_generic_suspend(self.as_mut_ref()) })
>> + to_result(unsafe { bindings::cpufreq_generic_suspend(self.as_mut_ref()) }).map(|_| ())
>> }
>>
>> /// Provides a wrapper to the generic get routine.
>> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
>> index d04e3fcebafb..214cd9a0ebe0 100644
>> --- a/rust/kernel/devres.rs
>> +++ b/rust/kernel/devres.rs
>> @@ -328,6 +328,7 @@ fn register_foreign<P>(dev: &Device<Bound>, data: P) -> Result
>> // `ForeignOwnable` is released eventually.
>> bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::<P>), ptr.cast())
>> })
>> + .map(|_| ())
>> }
>>
>> /// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
>> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
>> index 68fe67624424..f614453ddb7d 100644
>> --- a/rust/kernel/dma.rs
>> +++ b/rust/kernel/dma.rs
>> @@ -34,6 +34,7 @@ unsafe fn dma_set_mask(&self, mask: DmaMask) -> Result {
>> // - The safety requirement of this function guarantees that there are no concurrent calls
>> // to DMA allocation and mapping primitives using this mask.
>> to_result(unsafe { bindings::dma_set_mask(self.as_ref().as_raw(), mask.value()) })
>> + .map(|_| ())
>> }
>>
>> /// Set up the device's DMA coherent addressing capabilities.
>> @@ -51,6 +52,7 @@ unsafe fn dma_set_coherent_mask(&self, mask: DmaMask) -> Result {
>> // - The safety requirement of this function guarantees that there are no concurrent calls
>> // to DMA allocation and mapping primitives using this mask.
>> to_result(unsafe { bindings::dma_set_coherent_mask(self.as_ref().as_raw(), mask.value()) })
>> + .map(|_| ())
>> }
>>
>> /// Set up the device's DMA addressing capabilities.
>> @@ -72,6 +74,7 @@ unsafe fn dma_set_mask_and_coherent(&self, mask: DmaMask) -> Result {
>> to_result(unsafe {
>> bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(), mask.value())
>> })
>> + .map(|_| ())
>> }
>> }
>>
>> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
>> index db14da976722..f76afa4b7ec1 100644
>> --- a/rust/kernel/error.rs
>> +++ b/rust/kernel/error.rs
>> @@ -378,12 +378,19 @@ fn from(e: core::convert::Infallible) -> Error {
>> pub type Result<T = (), E = Error> = core::result::Result<T, E>;
>>
>> /// 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: crate::ffi::c_int) -> Result {
>> - if err < 0 {
>> - Err(Error::from_errno(err))
>> +/// returns the original value otherwise.
>> +pub fn to_result<T>(code: T) -> Result<T>
>> +where
>> + T: Copy + TryInto<i32>,
>> +{
>> + // Try casting into `i32`.
>> + let casted: crate::ffi::c_int = code.try_into().unwrap_or(0);
>> +
>> + if casted < 0 {
>> + Err(Error::from_errno(casted))
>> } else {
>> - Ok(())
>> + // Return the original input value.
>> + Ok(code)
>> }
>> }
>>
>> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
>> index 6373fe183b27..22b72ae84c03 100644
>> --- a/rust/kernel/miscdevice.rs
>> +++ b/rust/kernel/miscdevice.rs
>> @@ -79,7 +79,7 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>> // the destructor of this type deallocates the memory.
>> // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>> // misc device.
>> - to_result(unsafe { bindings::misc_register(slot) })
>> + to_result(unsafe { bindings::misc_register(slot) }).map(|_| ())
>> }),
>> _t: PhantomData,
>> })
>> diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs
>> index a1bfa4e19293..5494f96e91b0 100644
>> --- a/rust/kernel/mm/virt.rs
>> +++ b/rust/kernel/mm/virt.rs
>> @@ -195,6 +195,7 @@ pub fn vm_insert_page(&self, address: usize, page: &Page) -> Result {
>> // SAFETY: By the type invariant of `Self` caller has read access and has verified that
>> // `VM_MIXEDMAP` is set. By invariant on `Page` the page has order 0.
>> to_result(unsafe { bindings::vm_insert_page(self.as_ptr(), address, page.as_ptr()) })
>> + .map(|_| ())
>> }
>> }
>>
>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> index 887ee611b553..6e917752cb89 100644
>> --- a/rust/kernel/pci.rs
>> +++ b/rust/kernel/pci.rs
>> @@ -48,6 +48,7 @@ unsafe fn register(
>> to_result(unsafe {
>> bindings::__pci_register_driver(pdrv.get(), module.0, name.as_char_ptr())
>> })
>> + .map(|_| ())
>> }
>>
>> unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
>> @@ -437,7 +438,7 @@ impl Device<device::Core> {
>> /// Enable memory resources for this device.
>> pub fn enable_device_mem(&self) -> Result {
>> // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
>> - to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) })
>> + to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) }).map(|_| ())
>> }
>>
>> /// Enable bus-mastering for this device.
>> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
>> index 8f028c76f9fa..5a5561c7326e 100644
>> --- a/rust/kernel/platform.rs
>> +++ b/rust/kernel/platform.rs
>> @@ -54,7 +54,7 @@ unsafe fn register(
>> }
>>
>> // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
>> - to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) })
>> + to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) }).map(|_| ())
>> }
>>
>> unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
>> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
>> index 65f3a125348f..e17ae6e9a990 100644
>> --- a/rust/kernel/regulator.rs
>> +++ b/rust/kernel/regulator.rs
>> @@ -261,6 +261,7 @@ pub fn set_voltage(&self, min_voltage: Voltage, max_voltage: Voltage) -> Result
>> max_voltage.as_microvolts(),
>> )
>> })
>> + .map(|_| ())
>> }
>>
>> /// Gets the current voltage of the regulator.
>> @@ -291,12 +292,12 @@ fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {
>>
>> fn enable_internal(&mut self) -> Result {
>> // SAFETY: Safe as per the type invariants of `Regulator`.
>> - to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) })
>> + to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) }).map(|_| ())
>> }
>>
>> fn disable_internal(&mut self) -> Result {
>> // SAFETY: Safe as per the type invariants of `Regulator`.
>> - to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) })
>> + to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) }).map(|_| ())
>
> IMO all of the new map calls in this patch should use the
>
> to_result(...)?
> Ok(())
>
> syntax.
>
> Alice
FYI: A similar patch was already applied by Mark to regulator.rs, but now I see
the same code being submitted a) under a different patch, with its own version
(v1) and b) together as a single patch touching a lot of code at once.
I am a bit confused. Can you make sure that all code touching regulator.rs is
based on the regulator tree?
— Daniel
On Tue, 9 Sep 2025 09:53:09 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:
>
>
> > On 9 Sep 2025, at 09:36, Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Tue, Sep 9, 2025 at 2:33 PM Onur Özkan <work@onurozkan.dev>
> > wrote:
> >>
> >> Current `to_result` helper takes a `c_int` and returns `Ok(())` on
> >> success and this has some issues like:
> >>
> >> - Callers lose the original return value and often have to store
> >> it in a temporary variable before calling `to_result`.
> >>
> >> - It only supports `c_int`, which makes callers to unnecessarily
> >> cast when working with other types (e.g. `u16` in phy
> >> abstractions). We even have some places that ignore to use
> >> `to_result` helper because the input doesn't fit in `c_int`
> >> (see [0]).
> >>
> >> [0]:
> >> https://lore.kernel.org/all/20250822080252.773d6f54@nimda.home/
> >>
> >> This patch changes `to_result` to be generic and also return the
> >> original value on success.
> >>
> >> So that the code that previously looked like:
> >>
> >> let ret = unsafe { bindings::some_ffi_call() };
> >> to_result(ret).map(|()| SomeType::new(ret))
> >>
> >> can now be written more directly as:
> >>
> >> to_result(unsafe { bindings::some_ffi_call() })
> >> .map(|ret| SomeType::new(ret))
> >>
> >> Similarly, code such as:
> >>
> >> let res: isize = $some_ffi_call();
> >> if res < 0 {
> >> return Err(Error::from_errno(res as i32));
> >> }
> >>
> >> can now be used with `to_result` as:
> >>
> >> to_result($some_ffi_call())?;
> >>
> >> Existing call sites that only care about success/failure are
> >> updated to append `.map(|_| ())` to preserve their previous
> >> semantics. They can also use the equivalent pattern:
> >>
> >> to_result($something)?;
> >> Ok(())
> >>
> >> This patch only fixes the callers that broke after the changes on
> >> `to_result`.
>
> What exactly broke for regulator.rs <http://regulator.rs/>? It works
> just fine.
>
> >> I haven't included all the improvements made possible by the new
> >> design since that could conflict with other ongoing patches [1].
> >> Once this patch is approved and applied, I am planning to follow
> >> up with creating a "good first issue" on [2] for those additional
> >> changes.
> >>
> >> [1]: https://lore.kernel.org/rust-for-linux/?q=to_result
> >> [2]: https://github.com/Rust-for-Linux/linux
> >>
> >> Link:
> >> https://rust-for-linux.zulipchat.com/#narrow/channel/288089/topic/x/near/536374456
> >> Signed-off-by: Onur Özkan <work@onurozkan.dev> ---
> >> rust/kernel/auxiliary.rs | 1 +
> >> rust/kernel/block/mq/tag_set.rs | 2 +-
> >> rust/kernel/cpufreq.rs | 3 ++-
> >> rust/kernel/devres.rs | 1 +
> >> rust/kernel/dma.rs | 3 +++
> >> rust/kernel/error.rs | 17 ++++++++++++-----
> >> rust/kernel/miscdevice.rs | 2 +-
> >> rust/kernel/mm/virt.rs | 1 +
> >> rust/kernel/pci.rs | 3 ++-
> >> rust/kernel/platform.rs | 2 +-
> >> rust/kernel/regulator.rs | 5 +++--
> >> 11 files changed, 28 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
> >> index 4749fb6bffef..479c0ad2a572 100644
> >> --- a/rust/kernel/auxiliary.rs
> >> +++ b/rust/kernel/auxiliary.rs
> >> @@ -43,6 +43,7 @@ unsafe fn register(
> >> to_result(unsafe {
> >> bindings::__auxiliary_driver_register(adrv.get(),
> >> module.0, name.as_char_ptr()) })
> >> + .map(|_| ())
> >> }
> >>
> >> unsafe fn unregister(adrv: &Opaque<Self::RegType>) {
> >> diff --git a/rust/kernel/block/mq/tag_set.rs
> >> b/rust/kernel/block/mq/tag_set.rs index c3cf56d52bee..0e7883163000
> >> 100644 --- a/rust/kernel/block/mq/tag_set.rs
> >> +++ b/rust/kernel/block/mq/tag_set.rs
> >> @@ -65,7 +65,7 @@ pub fn new(
> >> // SAFETY: we do not move out of `tag_set`.
> >> let tag_set: &mut Opaque<_> = unsafe {
> >> Pin::get_unchecked_mut(tag_set) }; // SAFETY: `tag_set` is a
> >> reference to an initialized `blk_mq_tag_set`.
> >> - error::to_result( unsafe {
> >> bindings::blk_mq_alloc_tag_set(tag_set.get())})
> >> + error::to_result( unsafe {
> >> bindings::blk_mq_alloc_tag_set(tag_set.get())}).map(|_| ()) }),
> >> _p: PhantomData,
> >> })
> >> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> >> index be2dffbdb546..c3fa20ce229a 100644
> >> --- a/rust/kernel/cpufreq.rs
> >> +++ b/rust/kernel/cpufreq.rs
> >> @@ -157,6 +157,7 @@ pub fn as_raw(&self) -> *mut
> >> bindings::cpufreq_policy_data { pub fn generic_verify(&self) ->
> >> Result { // SAFETY: By the type invariant, the pointer stored in
> >> `self` is valid. to_result(unsafe {
> >> bindings::cpufreq_generic_frequency_table_verify(self.as_raw()) })
> >> + .map(|_| ())
> >> }
> >> }
> >>
> >> @@ -519,7 +520,7 @@ pub fn set_suspend_freq(&mut self, freq:
> >> Hertz) -> &mut Self { #[inline]
> >> pub fn generic_suspend(&mut self) -> Result {
> >> // SAFETY: By the type invariant, the pointer stored in
> >> `self` is valid.
> >> - to_result(unsafe {
> >> bindings::cpufreq_generic_suspend(self.as_mut_ref()) })
> >> + to_result(unsafe {
> >> bindings::cpufreq_generic_suspend(self.as_mut_ref()) }).map(|_|
> >> ()) }
> >>
> >> /// Provides a wrapper to the generic get routine.
> >> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> >> index d04e3fcebafb..214cd9a0ebe0 100644
> >> --- a/rust/kernel/devres.rs
> >> +++ b/rust/kernel/devres.rs
> >> @@ -328,6 +328,7 @@ fn register_foreign<P>(dev: &Device<Bound>,
> >> data: P) -> Result // `ForeignOwnable` is released eventually.
> >> bindings::devm_add_action_or_reset(dev.as_raw(),
> >> Some(callback::<P>), ptr.cast()) })
> >> + .map(|_| ())
> >> }
> >>
> >> /// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data`
> >> once `dev` is unbound. diff --git a/rust/kernel/dma.rs
> >> b/rust/kernel/dma.rs index 68fe67624424..f614453ddb7d 100644
> >> --- a/rust/kernel/dma.rs
> >> +++ b/rust/kernel/dma.rs
> >> @@ -34,6 +34,7 @@ unsafe fn dma_set_mask(&self, mask: DmaMask) ->
> >> Result { // - The safety requirement of this function guarantees
> >> that there are no concurrent calls // to DMA allocation and
> >> mapping primitives using this mask. to_result(unsafe {
> >> bindings::dma_set_mask(self.as_ref().as_raw(), mask.value()) })
> >> + .map(|_| ())
> >> }
> >>
> >> /// Set up the device's DMA coherent addressing capabilities.
> >> @@ -51,6 +52,7 @@ unsafe fn dma_set_coherent_mask(&self, mask:
> >> DmaMask) -> Result { // - The safety requirement of this function
> >> guarantees that there are no concurrent calls // to DMA
> >> allocation and mapping primitives using this mask.
> >> to_result(unsafe {
> >> bindings::dma_set_coherent_mask(self.as_ref().as_raw(),
> >> mask.value()) })
> >> + .map(|_| ())
> >> }
> >>
> >> /// Set up the device's DMA addressing capabilities.
> >> @@ -72,6 +74,7 @@ unsafe fn dma_set_mask_and_coherent(&self, mask:
> >> DmaMask) -> Result { to_result(unsafe {
> >> bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(),
> >> mask.value()) })
> >> + .map(|_| ())
> >> }
> >> }
> >>
> >> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> >> index db14da976722..f76afa4b7ec1 100644
> >> --- a/rust/kernel/error.rs
> >> +++ b/rust/kernel/error.rs
> >> @@ -378,12 +378,19 @@ fn from(e: core::convert::Infallible) ->
> >> Error { pub type Result<T = (), E = Error> =
> >> core::result::Result<T, E>;
> >>
> >> /// 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: crate::ffi::c_int) -> Result {
> >> - if err < 0 {
> >> - Err(Error::from_errno(err))
> >> +/// returns the original value otherwise.
> >> +pub fn to_result<T>(code: T) -> Result<T>
> >> +where
> >> + T: Copy + TryInto<i32>,
> >> +{
> >> + // Try casting into `i32`.
> >> + let casted: crate::ffi::c_int = code.try_into().unwrap_or(0);
> >> +
> >> + if casted < 0 {
> >> + Err(Error::from_errno(casted))
> >> } else {
> >> - Ok(())
> >> + // Return the original input value.
> >> + Ok(code)
> >> }
> >> }
> >>
> >> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> >> index 6373fe183b27..22b72ae84c03 100644
> >> --- a/rust/kernel/miscdevice.rs
> >> +++ b/rust/kernel/miscdevice.rs
> >> @@ -79,7 +79,7 @@ pub fn register(opts: MiscDeviceOptions) -> impl
> >> PinInit<Self, Error> { // the destructor of this type deallocates
> >> the memory. // INVARIANT: If this returns `Ok(())`, then the
> >> `slot` will contain a registered // misc device.
> >> - to_result(unsafe { bindings::misc_register(slot)
> >> })
> >> + to_result(unsafe { bindings::misc_register(slot)
> >> }).map(|_| ()) }),
> >> _t: PhantomData,
> >> })
> >> diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs
> >> index a1bfa4e19293..5494f96e91b0 100644
> >> --- a/rust/kernel/mm/virt.rs
> >> +++ b/rust/kernel/mm/virt.rs
> >> @@ -195,6 +195,7 @@ pub fn vm_insert_page(&self, address: usize,
> >> page: &Page) -> Result { // SAFETY: By the type invariant of
> >> `Self` caller has read access and has verified that //
> >> `VM_MIXEDMAP` is set. By invariant on `Page` the page has order 0.
> >> to_result(unsafe { bindings::vm_insert_page(self.as_ptr(),
> >> address, page.as_ptr()) })
> >> + .map(|_| ())
> >> }
> >> }
> >>
> >> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> >> index 887ee611b553..6e917752cb89 100644
> >> --- a/rust/kernel/pci.rs
> >> +++ b/rust/kernel/pci.rs
> >> @@ -48,6 +48,7 @@ unsafe fn register(
> >> to_result(unsafe {
> >> bindings::__pci_register_driver(pdrv.get(), module.0,
> >> name.as_char_ptr()) })
> >> + .map(|_| ())
> >> }
> >>
> >> unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
> >> @@ -437,7 +438,7 @@ impl Device<device::Core> {
> >> /// Enable memory resources for this device.
> >> pub fn enable_device_mem(&self) -> Result {
> >> // SAFETY: `self.as_raw` is guaranteed to be a pointer to
> >> a valid `struct pci_dev`.
> >> - to_result(unsafe {
> >> bindings::pci_enable_device_mem(self.as_raw()) })
> >> + to_result(unsafe {
> >> bindings::pci_enable_device_mem(self.as_raw()) }).map(|_| ()) }
> >>
> >> /// Enable bus-mastering for this device.
> >> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> >> index 8f028c76f9fa..5a5561c7326e 100644
> >> --- a/rust/kernel/platform.rs
> >> +++ b/rust/kernel/platform.rs
> >> @@ -54,7 +54,7 @@ unsafe fn register(
> >> }
> >>
> >> // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
> >> - to_result(unsafe {
> >> bindings::__platform_driver_register(pdrv.get(), module.0) })
> >> + to_result(unsafe {
> >> bindings::__platform_driver_register(pdrv.get(), module.0)
> >> }).map(|_| ()) }
> >>
> >> unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
> >> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
> >> index 65f3a125348f..e17ae6e9a990 100644
> >> --- a/rust/kernel/regulator.rs
> >> +++ b/rust/kernel/regulator.rs
> >> @@ -261,6 +261,7 @@ pub fn set_voltage(&self, min_voltage:
> >> Voltage, max_voltage: Voltage) -> Result
> >> max_voltage.as_microvolts(), )
> >> })
> >> + .map(|_| ())
> >> }
> >>
> >> /// Gets the current voltage of the regulator.
> >> @@ -291,12 +292,12 @@ fn get_internal(dev: &Device, name: &CStr)
> >> -> Result<Regulator<T>> {
> >>
> >> fn enable_internal(&mut self) -> Result {
> >> // SAFETY: Safe as per the type invariants of `Regulator`.
> >> - to_result(unsafe {
> >> bindings::regulator_enable(self.inner.as_ptr()) })
> >> + to_result(unsafe {
> >> bindings::regulator_enable(self.inner.as_ptr()) }).map(|_| ()) }
> >>
> >> fn disable_internal(&mut self) -> Result {
> >> // SAFETY: Safe as per the type invariants of `Regulator`.
> >> - to_result(unsafe {
> >> bindings::regulator_disable(self.inner.as_ptr()) })
> >> + to_result(unsafe {
> >> bindings::regulator_disable(self.inner.as_ptr()) }).map(|_| ())
> >
> > IMO all of the new map calls in this patch should use the
> >
> > to_result(...)?
> > Ok(())
> >
> > syntax.
> >
> > Alice
>
> FYI: A similar patch was already applied by Mark to regulator.rs, but
> now I see the same code being submitted a) under a different patch,
> with its own version (v1) and b) together as a single patch touching
> a lot of code at once.
>
> I am a bit confused. Can you make sure that all code touching
> regulator.rs is based on the regulator tree?
>
> — Daniel
>
>
Sorry for the mess, will resolve it in v2.
-Onur
© 2016 - 2025 Red Hat, Inc.