Re: [PATCH v1] rust: refactor `to_result` to return the original value

[Onur]

On Tue, 9 Sep 2025 14:36:13 +0200
Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:

> On Tue, Sep 9, 2025 at 2:33 PM Onur Özkan <work@xxxxxxxxxxxxx> 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@xxxxxxxxxx/
> >
> > 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@xxxxxxxxxxxxx> ---
> >  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