[PATCH v2 0/1] rust: refactor to_result to return the original value

[PATCH v2 0/1] rust: refactor to_result to return the original value

[Onur Özkan]

Changelog in v2:
  - Removed `map(|_| ())` calls from v1 and replaced them with
  `to_result(...)?` and `Ok(())` (except miscdevice.rs, as it
  required `Ok::<(), Error>(())` which is less clean).

  - Rebased on latest regulator/for-next and fixed the build error.

Onur Özkan (1):
  rust: refactor to_result to return the original value

 rust/kernel/auxiliary.rs        |  3 ++-
 rust/kernel/block/mq/tag_set.rs |  3 ++-
 rust/kernel/cpufreq.rs          |  5 +++--
 rust/kernel/devres.rs           |  3 ++-
 rust/kernel/dma.rs              | 11 ++++++++---
 rust/kernel/error.rs            | 17 ++++++++++++-----
 rust/kernel/miscdevice.rs       |  2 +-
 rust/kernel/mm/virt.rs          |  3 ++-
 rust/kernel/pci.rs              |  6 ++++--
 rust/kernel/platform.rs         |  3 ++-
 rust/kernel/regulator.rs        | 14 ++++++++------
 11 files changed, 46 insertions(+), 24 deletions(-)

--
2.50.0

[PATCH v2 1/1] rust: refactor to_result to return the original value

[Onur Özkan]

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())?;

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        |  3 ++-
 rust/kernel/block/mq/tag_set.rs |  3 ++-
 rust/kernel/cpufreq.rs          |  6 ++++--
 rust/kernel/devres.rs           |  3 ++-
 rust/kernel/dma.rs              | 11 ++++++++---
 rust/kernel/error.rs            | 17 ++++++++++++-----
 rust/kernel/miscdevice.rs       |  2 +-
 rust/kernel/mm/virt.rs          |  3 ++-
 rust/kernel/pci.rs              |  6 ++++--
 rust/kernel/platform.rs         |  3 ++-
 rust/kernel/regulator.rs        | 14 ++++++++------
 11 files changed, 47 insertions(+), 24 deletions(-)

diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index 4749fb6bffef..74b4da1192e1 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -42,7 +42,8 @@ unsafe fn register(
         // SAFETY: `adrv` is guaranteed to be a valid `RegType`.
         to_result(unsafe {
             bindings::__auxiliary_driver_register(adrv.get(), module.0, name.as_char_ptr())
-        })
+        })?;
+        Ok(())
     }

     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..e8adb9d72475 100644
--- a/rust/kernel/block/mq/tag_set.rs
+++ b/rust/kernel/block/mq/tag_set.rs
@@ -65,7 +65,8 @@ 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())})?;
+                Ok(())
             }),
             _p: PhantomData,
         })
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index afc15e72a7c3..b9db81aea5c6 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -157,7 +157,8 @@ pub fn as_raw(&self) -> *mut bindings::cpufreq_policy_data {
     #[inline]
     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()) })
+        to_result(unsafe { bindings::cpufreq_generic_frequency_table_verify(self.as_raw()) })?;
+        Ok(())
     }
 }

@@ -520,7 +521,8 @@ 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()) })?;
+        Ok(())
     }

     /// Provides a wrapper to the generic get routine.
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index d04e3fcebafb..487e194f7d96 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -327,7 +327,8 @@ fn register_foreign<P>(dev: &Device<Bound>, data: P) -> Result
         // `devm_add_action_or_reset()` also calls `callback` on failure, such that the
         // `ForeignOwnable` is released eventually.
         bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::<P>), ptr.cast())
-    })
+    })?;
+    Ok(())
 }

 /// 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 2bc8ab51ec28..04ca70f6f684 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -33,7 +33,8 @@ unsafe fn dma_set_mask(&self, mask: DmaMask) -> Result {
         // - By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
         // - 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()) })
+        to_result(unsafe { bindings::dma_set_mask(self.as_ref().as_raw(), mask.value()) })?;
+        Ok(())
     }

     /// Set up the device's DMA coherent addressing capabilities.
@@ -50,7 +51,10 @@ unsafe fn dma_set_coherent_mask(&self, mask: DmaMask) -> Result {
         // - By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
         // - 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()) })
+        to_result(unsafe {
+            bindings::dma_set_coherent_mask(self.as_ref().as_raw(), mask.value())
+        })?;
+        Ok(())
     }

     /// Set up the device's DMA addressing capabilities.
@@ -71,7 +75,8 @@ unsafe fn dma_set_mask_and_coherent(&self, mask: DmaMask) -> Result {
         //   to DMA allocation and mapping primitives using this mask.
         to_result(unsafe {
             bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(), mask.value())
-        })
+        })?;
+        Ok(())
     }
 }

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index a41de293dcd1..6563ea71e203 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -376,12 +376,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 6086ca981b06..62c00dcdb5e1 100644
--- a/rust/kernel/mm/virt.rs
+++ b/rust/kernel/mm/virt.rs
@@ -194,7 +194,8 @@ pub unsafe fn from_raw<'a>(vma: *const bindings::vm_area_struct) -> &'a Self {
     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()) })
+        to_result(unsafe { bindings::vm_insert_page(self.as_ptr(), address, page.as_ptr()) })?;
+        Ok(())
     }
 }

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 887ee611b553..b52a7be488d0 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -47,7 +47,8 @@ unsafe fn register(
         // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
         to_result(unsafe {
             bindings::__pci_register_driver(pdrv.get(), module.0, name.as_char_ptr())
-        })
+        })?;
+        Ok(())
     }

     unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
@@ -437,7 +438,8 @@ 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()) })?;
+        Ok(())
     }

     /// Enable bus-mastering for this device.
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 8f028c76f9fa..751b65bfc357 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -54,7 +54,8 @@ 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) })?;
+        Ok(())
     }

     unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
index 34bb24ec8d4d..a5f357bda6e9 100644
--- a/rust/kernel/regulator.rs
+++ b/rust/kernel/regulator.rs
@@ -260,15 +260,15 @@ pub fn set_voltage(&self, min_voltage: Voltage, max_voltage: Voltage) -> Result
                 min_voltage.as_microvolts(),
                 max_voltage.as_microvolts(),
             )
-        })
+        })?;
+        Ok(())
     }

     /// Gets the current voltage of the regulator.
     pub fn get_voltage(&self) -> Result<Voltage> {
         // SAFETY: Safe as per the type invariants of `Regulator`.
-        let voltage = unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) };
-
-        to_result(voltage).map(|()| Voltage::from_microvolts(voltage))
+        to_result(unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) })
+            .map(Voltage::from_microvolts)
     }

     fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {
@@ -288,12 +288,14 @@ fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {

     fn enable_internal(&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()) })?;
+        Ok(())
     }

     fn disable_internal(&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()) })?;
+        Ok(())
     }
 }

--
2.50.0

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Miguel Ojeda]

On Tue, Sep 9, 2025 at 7:01 PM Onur Özkan <work@onurozkan.dev> wrote:
>
> 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

I think Daniel asked in the previous version what you mean by "callers
that broke" here -- it is a bit confusing, since it seems this is a
fix (and thus needs to be prioritized).

Is that the case?

Thanks!

Cheers,
Miguel

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Onur Özkan]

On Tue, 9 Sep 2025 19:17:56 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Tue, Sep 9, 2025 at 7:01 PM Onur Özkan <work@onurozkan.dev> wrote:
> >
> > 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
> 
> I think Daniel asked in the previous version what you mean by "callers
> that broke" here -- it is a bit confusing, since it seems this is a
> fix (and thus needs to be prioritized).
> 
> Is that the case?
> 
> Thanks!
> 
> Cheers,
> Miguel

What I meant is that the change on `to_result` signature introduced a
breaking change so I had to update its callers accordingly.

The fix I mentioned in this version is a different matter.

Before the rebase, the regulator module had a get_voltage function like
this:

	let voltage = unsafe {...};

	if voltage < 0 {
    		Err(...)
	} else {
    		Ok(Voltage::from_microvolts(voltage))
	}

But on the regulator/for-next branch, a patch was applied that changed
it to:

	let voltage = unsafe {...};
	to_result(voltage).map(|()| Voltage::from_microvolts(voltage))

That change was incompatible with v1 (due to the different signature of
to_result), which fails to build with my patch. This version (v2)
fixes the issue introduced in v1.

Sorry for the confusion, I hope it's more clear now.

Thanks,
Onur

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Onur]

On Tue, 9 Sep 2025 20:43:08 +0300
Onur Özkan <work@onurozkan.dev> wrote:

> On Tue, 9 Sep 2025 19:17:56 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> > On Tue, Sep 9, 2025 at 7:01 PM Onur Özkan <work@onurozkan.dev>
> > wrote:
> > >
> > > 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
> > 
> > I think Daniel asked in the previous version what you mean by
> > "callers that broke" here -- it is a bit confusing, since it seems
> > this is a fix (and thus needs to be prioritized).
> > 
> > Is that the case?
> > 
> > Thanks!
> > 
> > Cheers,
> > Miguel
> 
> What I meant is that the change on `to_result` signature introduced a
> breaking change so I had to update its callers accordingly.
> 
> The fix I mentioned in this version is a different matter.
> 
> Before the rebase, the regulator module had a get_voltage function
> like this:
> 
> 	let voltage = unsafe {...};
> 
> 	if voltage < 0 {
>     		Err(...)
> 	} else {
>     		Ok(Voltage::from_microvolts(voltage))
> 	}
> 
> But on the regulator/for-next branch, a patch was applied that changed
> it to:
> 
> 	let voltage = unsafe {...};
> 	to_result(voltage).map(|()| Voltage::from_microvolts(voltage))
> 
> That change was incompatible with v1 (due to the different signature
> of to_result), which fails to build with my patch. This version (v2)
> fixes the issue introduced in v1.
> 
> Sorry for the confusion, I hope it's more clear now.
> 
> Thanks,
> Onur

If I am not mistaken the way I prepared v2 is also not correct. I guess
the fix for regulator/for-next should be a separate patch.

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Daniel Almeida]

Onur,

> On 9 Sep 2025, at 14:43, Onur Özkan <work@onurozkan.dev> wrote:
> 
> On Tue, 9 Sep 2025 19:17:56 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
>> On Tue, Sep 9, 2025 at 7:01 PM Onur Özkan <work@onurozkan.dev> wrote:
>>> 
>>> 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
>> 
>> I think Daniel asked in the previous version what you mean by "callers
>> that broke" here -- it is a bit confusing, since it seems this is a
>> fix (and thus needs to be prioritized).
>> 
>> Is that the case?
>> 
>> Thanks!
>> 
>> Cheers,
>> Miguel
> 
> What I meant is that the change on `to_result` signature introduced a
> breaking change so I had to update its callers accordingly.
> 
> The fix I mentioned in this version is a different matter.
> 
> Before the rebase, the regulator module had a get_voltage function like
> this:
> 
> let voltage = unsafe {...};
> 
> if voltage < 0 {
>     Err(...)
> } else {
>     Ok(Voltage::from_microvolts(voltage))
> }
> 
> But on the regulator/for-next branch, a patch was applied that changed
> it to:
> 
> let voltage = unsafe {...};
> to_result(voltage).map(|()| Voltage::from_microvolts(voltage))
> 
> That change was incompatible with v1 (due to the different signature of
> to_result), which fails to build with my patch. This version (v2)
> fixes the issue introduced in v1.

Fixes what issue? What is the actual problem being addressed here?

It looks like a mere change from

to_result(…) and,
to_result(…).map()

To:

to_result(…)?;
Ok(())

and

-        let voltage = unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) };
-
-        to_result(voltage).map(|()| Voltage::from_microvolts(voltage))
+        to_result(unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) })
+            .map(Voltage::from_microvolts)
    }


> 
> Sorry for the confusion, I hope it's more clear now.
> 
> Thanks,
> Onur
> 

Your last regulator patch was minor, correct, and was picked up (merged). It
cleared up an if/else, so that was an improvement.

I now see yet another change, doing apparently the same thing (correct me if
I’m wrong) in a slightly different way, in a patch that now has your previous regulator
patch as a dependency.

So my question is, why do we need this?

diff --git a/rust/kernel/regulator.rsb/rust/kernel/regulator.rs
index 34bb24ec8d4d..a5f357bda6e9 100644
--- a/rust/kernel/regulator.rs
+++ b/rust/kernel/regulator.rs
@@ -260,15 +260,15 @@ pub fn set_voltage(&self, min_voltage: Voltage, max_voltage: Voltage) -> Result
                min_voltage.as_microvolts(),
                max_voltage.as_microvolts(),
            )
-        })
+        })?;
+        Ok(())
    }

    /// Gets the current voltage of the regulator.
    pub fn get_voltage(&self) -> Result<Voltage> {
        // SAFETY: Safe as per the type invariants of `Regulator`.
-        let voltage = unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) };
-
-        to_result(voltage).map(|()| Voltage::from_microvolts(voltage))
+        to_result(unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) })
+            .map(Voltage::from_microvolts)
    }

    fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {
@@ -288,12 +288,14 @@ fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {

    fn enable_internal(&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()) })?;
+        Ok(())
    }

    fn disable_internal(&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()) })?;
+        Ok(())
    }


— Daniel

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Onur Özkan]

On Tue, 9 Sep 2025 15:25:55 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

> Onur,
> 
> > On 9 Sep 2025, at 14:43, Onur Özkan <work@onurozkan.dev> wrote:
> > 
> > On Tue, 9 Sep 2025 19:17:56 +0200
> > Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> > 
> >> On Tue, Sep 9, 2025 at 7:01 PM Onur Özkan <work@onurozkan.dev>
> >> wrote:
> >>> 
> >>> 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
> >> 
> >> I think Daniel asked in the previous version what you mean by
> >> "callers that broke" here -- it is a bit confusing, since it seems
> >> this is a fix (and thus needs to be prioritized).
> >> 
> >> Is that the case?
> >> 
> >> Thanks!
> >> 
> >> Cheers,
> >> Miguel
> > 
> > What I meant is that the change on `to_result` signature introduced
> > a breaking change so I had to update its callers accordingly.
> > 
> > The fix I mentioned in this version is a different matter.
> > 
> > Before the rebase, the regulator module had a get_voltage function
> > like this:
> > 
> > let voltage = unsafe {...};
> > 
> > if voltage < 0 {
> >     Err(...)
> > } else {
> >     Ok(Voltage::from_microvolts(voltage))
> > }
> > 
> > But on the regulator/for-next branch, a patch was applied that
> > changed it to:
> > 
> > let voltage = unsafe {...};
> > to_result(voltage).map(|()| Voltage::from_microvolts(voltage))
> > 
> > That change was incompatible with v1 (due to the different
> > signature of to_result), which fails to build with my patch. This
> > version (v2) fixes the issue introduced in v1.
> 
> Fixes what issue? What is the actual problem being addressed here?
> 
> It looks like a mere change from
> 
> to_result(…) and,
> to_result(…).map()
> 
> To:
> 
> to_result(…)?;
> Ok(())
> 
> and
> 
> -        let voltage = unsafe {
> bindings::regulator_get_voltage(self.inner.as_ptr()) }; -
> -        to_result(voltage).map(|()|
> Voltage::from_microvolts(voltage))
> +        to_result(unsafe {
> bindings::regulator_get_voltage(self.inner.as_ptr()) })
> +            .map(Voltage::from_microvolts)
>     }
> 
> 
> > 
> > Sorry for the confusion, I hope it's more clear now.
> > 
> > Thanks,
> > Onur
> > 
> 
> Your last regulator patch was minor, correct, and was picked up
> (merged). It cleared up an if/else, so that was an improvement.
> 
> I now see yet another change, doing apparently the same thing
> (correct me if I’m wrong) in a slightly different way, in a patch
> that now has your previous regulator patch as a dependency.
> 
> So my question is, why do we need this?
>


If you look at the changes in rust/kernel/error.rs, you will see that
the `to_result` function has been modified and some of its callers were
updated because the function signature changed.

The regulator patch I sent earlier is unrelated to this one. They are
not doing the same thing. That patch aimed to make use of `to_result`
instead of handling errors manually. This patch on the other hand,
changes how `to_result` itself works and some of its callers need to be
updated accordingly just like in the other modules included here. The
reason for changing the `to_result` function is explained in the commit
description.

-Onur 


> diff --git a/rust/kernel/regulator.rsb/rust/kernel/regulator.rs
> index 34bb24ec8d4d..a5f357bda6e9 100644
> --- a/rust/kernel/regulator.rs
> +++ b/rust/kernel/regulator.rs
> @@ -260,15 +260,15 @@ pub fn set_voltage(&self, min_voltage: Voltage,
> max_voltage: Voltage) -> Result min_voltage.as_microvolts(),
>                 max_voltage.as_microvolts(),
>             )
> -        })
> +        })?;
> +        Ok(())
>     }
> 
>     /// Gets the current voltage of the regulator.
>     pub fn get_voltage(&self) -> Result<Voltage> {
>         // SAFETY: Safe as per the type invariants of `Regulator`.
> -        let voltage = unsafe {
> bindings::regulator_get_voltage(self.inner.as_ptr()) }; -
> -        to_result(voltage).map(|()|
> Voltage::from_microvolts(voltage))
> +        to_result(unsafe {
> bindings::regulator_get_voltage(self.inner.as_ptr()) })
> +            .map(Voltage::from_microvolts)
>     }
> 
>     fn get_internal(dev: &Device, name: &CStr) ->
> Result<Regulator<T>> { @@ -288,12 +288,14 @@ fn get_internal(dev:
> &Device, name: &CStr) -> Result<Regulator<T>> {
> 
>     fn enable_internal(&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()) })?;
> +        Ok(())
>     }
> 
>     fn disable_internal(&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()) })?;
> +        Ok(())
>     }
> 
> 
> — Daniel

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Daniel Almeida]

[…]

> If you look at the changes in rust/kernel/error.rs, you will see that
> the `to_result` function has been modified and some of its callers were
> updated because the function signature changed.
> 
> The regulator patch I sent earlier is unrelated to this one. They are
> not doing the same thing. That patch aimed to make use of `to_result`

This is the piece of information I was missing, thanks.

> instead of handling errors manually. This patch on the other hand,
> changes how `to_result` itself works and some of its callers need to be
> updated accordingly just like in the other modules included here. The
> reason for changing the `to_result` function is explained in the commit
> description.
> 
> -Onur 

— Daniel

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Miguel Ojeda]

On Tue, Sep 9, 2025 at 7:43 PM Onur Özkan <work@onurozkan.dev> wrote:
>
> That change was incompatible with v1 (due to the different signature of
> to_result), which fails to build with my patch. This version (v2)
> fixes the issue introduced in v1.

In that case, please try to avoid mentioning "broken" or "fix" or
similar, i.e. there is nothing broken in the tree itself (the commit
message should mention what is done in the patch). If you want to give
extra clarifications, then you can always add them outside the commit
message, below the `---` line.

In addition, if the changes here are required to be done all at once,
then please do not rebase on top of regulator -- this would need to go
into the global Rust tree. Otherwise, any changes that does not need
to go at the same time should go separately so that it is easier to
land.

Finally, I am not sure I follow the `unwrap_or(0)` here. If one passes
a negative enough `i64`, wouldn't that mean `Ok` starts to be
returned? Currently all negatives that are not codes are supposed to
be bugs.

Either way, I think this should probably go on top of
https://lore.kernel.org/rust-for-linux/20250829192243.678079-3-ojeda@kernel.org/,
since that adds documentation, and thus it would be nice to adjust
that one to whatever the generic one should do now, especially if the
semantics are changing.

Thanks!

Cheers,
Miguel

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Daniel Almeida]

> On 9 Sep 2025, at 17:05, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> On Tue, Sep 9, 2025 at 7:43 PM Onur Özkan <work@onurozkan.dev> wrote:
>> 
>> That change was incompatible with v1 (due to the different signature of
>> to_result), which fails to build with my patch. This version (v2)
>> fixes the issue introduced in v1.
> 
> In that case, please try to avoid mentioning "broken" or "fix" or
> similar, i.e. there is nothing broken in the tree itself (the commit
> message should mention what is done in the patch). If you want to give
> extra clarifications, then you can always add them outside the commit
> message, below the `---` line.
> 
> In addition, if the changes here are required to be done all at once,
> then please do not rebase on top of regulator -- this would need to go

Merely rebasing on top of the rust tree will crate a conflict with his previous regulator patch.

My suggestion is to just wait for rc1.

> into the global Rust tree. Otherwise, any changes that does not need
> to go at the same time should go separately so that it is easier to
> land.
> 
> Finally, I am not sure I follow the `unwrap_or(0)` here. If one passes
> a negative enough `i64`, wouldn't that mean `Ok` starts to be
> returned? Currently all negatives that are not codes are supposed to
> be bugs.
> 
> Either way, I think this should probably go on top of
> https://lore.kernel.org/rust-for-linux/20250829192243.678079-3-ojeda@kernel.org/,
> since that adds documentation, and thus it would be nice to adjust
> that one to whatever the generic one should do now, especially if the
> semantics are changing.
> 
> Thanks!
> 
> Cheers,
> Miguel
> 

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Miguel Ojeda]

On Tue, Sep 9, 2025 at 10:12 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Merely rebasing on top of the rust tree will crate a conflict with his previous regulator patch.
>
> My suggestion is to just wait for rc1.

Yes, but note that conflicts may happen either way, i.e. for treewide
patches like this, one may get other code landing that may also need
changing. If what you want is to try to catch as many as those as
possible, then it may be best checking -next.

In any case, I would like to see this on top of the patch adding
documentation I linked, so that we can clearly discuss the intended
semantics, because this is changing them for bigger types.

Cheers,
Miguel

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Onur Özkan]

On Tue, 9 Sep 2025 22:05:53 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Tue, Sep 9, 2025 at 7:43 PM Onur Özkan <work@onurozkan.dev> wrote:
> >
> > That change was incompatible with v1 (due to the different
> > signature of to_result), which fails to build with my patch. This
> > version (v2) fixes the issue introduced in v1.
> 
> In that case, please try to avoid mentioning "broken" or "fix" or
> similar, i.e. there is nothing broken in the tree itself (the commit
> message should mention what is done in the patch). If you want to give
> extra clarifications, then you can always add them outside the commit
> message, below the `---` line.
> 
> In addition, if the changes here are required to be done all at once,
> then please do not rebase on top of regulator -- this would need to go
> into the global Rust tree. Otherwise, any changes that does not need
> to go at the same time should go separately so that it is easier to
> land.
> 
> Finally, I am not sure I follow the `unwrap_or(0)` here. If one passes
> a negative enough `i64`, wouldn't that mean `Ok` starts to be
> returned? Currently all negatives that are not codes are supposed to
> be bugs.
> 

I think the best approach would be to return `EINVAL` from `to_result`,
what do yo think?

> Either way, I think this should probably go on top of
> https://lore.kernel.org/rust-for-linux/20250829192243.678079-3-ojeda@kernel.org/,
> since that adds documentation, and thus it would be nice to adjust
> that one to whatever the generic one should do now, especially if the
> semantics are changing.
> 
> Thanks!
> 
> Cheers,
> Miguel

I will do that in v3.

Thanks,
Onur

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Alice Ryhl]

On Tue, Sep 09, 2025 at 08:00:13PM +0300, Onur Özkan 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())?;
> 
> 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>

> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index a41de293dcd1..6563ea71e203 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -376,12 +376,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)
>      }
>  }

I don't think this is the best way to declare this function. The
conversions I would want are:

* i32 -> Result<u32>
* isize -> Result<usize>
* i64 -> Result<u64>

Your commit messages mentions i16, but does the error types even fit in
16 bits? Maybe. But they don't fit in i8. That is to say, I think it
should support all the types larger than i32 (the errors fit in those
types too), but for the ones that are smaller, it might not make sense
if the type is too small. That's the reverse of what you have now.

We probably need a new trait. E.g.:

trait ToResult {
    type Unsigned;
    fn to_result(self) -> Result<Self::Unsigned, Error>;
}

impl ToResult for i32 {
    type Unsigned = u32;
    fn to_result(self) -> Result<u32, Error> {
        ...
    }
}

impl ToResult for isize {
    type Unsigned = usize;
    fn to_result(self) -> Result<usize, Error> {
        ...
    }
}

pub fn to_result<T: ToResult>(code: T) -> Result<T::Unsigned> {
    T::to_result(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(|_| ())

This still uses the `map` pattern. Please change it too.

Alice

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Onur Özkan]

On Wed, 10 Sep 2025 06:26:27 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> On Tue, Sep 09, 2025 at 08:00:13PM +0300, Onur Özkan 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())?;
> > 
> > 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>
> 
> > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > index a41de293dcd1..6563ea71e203 100644
> > --- a/rust/kernel/error.rs
> > +++ b/rust/kernel/error.rs
> > @@ -376,12 +376,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)
> >      }
> >  }
> 
> I don't think this is the best way to declare this function. The
> conversions I would want are:
> 
> * i32 -> Result<u32>
> * isize -> Result<usize>
> * i64 -> Result<u64>
> 
> Your commit messages mentions i16, but does the error types even fit
> in 16 bits? Maybe. But they don't fit in i8. That is to say, I think
> it should support all the types larger than i32 (the errors fit in
> those types too), but for the ones that are smaller, it might not
> make sense if the type is too small. That's the reverse of what you
> have now.
> 
> We probably need a new trait. E.g.:
> 
> trait ToResult {
>     type Unsigned;
>     fn to_result(self) -> Result<Self::Unsigned, Error>;
> }
> 
> impl ToResult for i32 {
>     type Unsigned = u32;
>     fn to_result(self) -> Result<u32, Error> {
>         ...
>     }
> }
> 
> impl ToResult for isize {
>     type Unsigned = usize;
>     fn to_result(self) -> Result<usize, Error> {
>         ...
>     }
> }
> 
> pub fn to_result<T: ToResult>(code: T) -> Result<T::Unsigned> {
>     T::to_result(code)
> }
> 

`Error::from_errno` is limited to i32, that's why I followed the
`TryInto<i32>` approach, but I like this design too.

> > 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(|_| ())
> 
> This still uses the `map` pattern. Please change it too.
> 
> Alice


I left that part intentionally and explained the reason in v2 changelog.

Thanks,
Onur

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Alice Ryhl]

On Wed, Sep 10, 2025 at 12:59 PM Onur Özkan <work@onurozkan.dev> wrote:
>
> On Wed, 10 Sep 2025 06:26:27 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Tue, Sep 09, 2025 at 08:00:13PM +0300, Onur Özkan 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())?;
> > >
> > > 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>
> >
> > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > > index a41de293dcd1..6563ea71e203 100644
> > > --- a/rust/kernel/error.rs
> > > +++ b/rust/kernel/error.rs
> > > @@ -376,12 +376,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)
> > >      }
> > >  }
> >
> > I don't think this is the best way to declare this function. The
> > conversions I would want are:
> >
> > * i32 -> Result<u32>
> > * isize -> Result<usize>
> > * i64 -> Result<u64>
> >
> > Your commit messages mentions i16, but does the error types even fit
> > in 16 bits? Maybe. But they don't fit in i8. That is to say, I think
> > it should support all the types larger than i32 (the errors fit in
> > those types too), but for the ones that are smaller, it might not
> > make sense if the type is too small. That's the reverse of what you
> > have now.
> >
> > We probably need a new trait. E.g.:
> >
> > trait ToResult {
> >     type Unsigned;
> >     fn to_result(self) -> Result<Self::Unsigned, Error>;
> > }
> >
> > impl ToResult for i32 {
> >     type Unsigned = u32;
> >     fn to_result(self) -> Result<u32, Error> {
> >         ...
> >     }
> > }
> >
> > impl ToResult for isize {
> >     type Unsigned = usize;
> >     fn to_result(self) -> Result<usize, Error> {
> >         ...
> >     }
> > }
> >
> > pub fn to_result<T: ToResult>(code: T) -> Result<T::Unsigned> {
> >     T::to_result(code)
> > }
> >
>
> `Error::from_errno` is limited to i32, that's why I followed the
> `TryInto<i32>` approach, but I like this design too.

If you pass an i32 that is not a valid errno, then it becomes EINVAL.
In the case of `isize`, I would say that if a negative isize does not
fit in i32, then that should fall into the same scenario.


> > > 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(|_| ())
> >
> > This still uses the `map` pattern. Please change it too.
> >
> > Alice
>
>
> I left that part intentionally and explained the reason in v2 changelog.

I missed that, ok. I still prefer Ok::<...>()

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Onur Özkan]

On Wed, 10 Sep 2025 13:05:42 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Wed, Sep 10, 2025 at 12:59 PM Onur Özkan <work@onurozkan.dev>
> wrote:
> >
> > On Wed, 10 Sep 2025 06:26:27 +0000
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > > On Tue, Sep 09, 2025 at 08:00:13PM +0300, Onur Özkan 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())?;
> > > >
> > > > 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>
> > >
> > > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > > > index a41de293dcd1..6563ea71e203 100644
> > > > --- a/rust/kernel/error.rs
> > > > +++ b/rust/kernel/error.rs
> > > > @@ -376,12 +376,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)
> > > >      }
> > > >  }
> > >
> > > I don't think this is the best way to declare this function. The
> > > conversions I would want are:
> > >
> > > * i32 -> Result<u32>
> > > * isize -> Result<usize>
> > > * i64 -> Result<u64>
> > >
> > > Your commit messages mentions i16, but does the error types even
> > > fit in 16 bits? Maybe. But they don't fit in i8. That is to say,
> > > I think it should support all the types larger than i32 (the
> > > errors fit in those types too), but for the ones that are
> > > smaller, it might not make sense if the type is too small. That's
> > > the reverse of what you have now.
> > >
> > > We probably need a new trait. E.g.:
> > >
> > > trait ToResult {
> > >     type Unsigned;
> > >     fn to_result(self) -> Result<Self::Unsigned, Error>;
> > > }
> > >
> > > impl ToResult for i32 {
> > >     type Unsigned = u32;
> > >     fn to_result(self) -> Result<u32, Error> {
> > >         ...
> > >     }
> > > }
> > >
> > > impl ToResult for isize {
> > >     type Unsigned = usize;
> > >     fn to_result(self) -> Result<usize, Error> {
> > >         ...
> > >     }
> > > }
> > >
> > > pub fn to_result<T: ToResult>(code: T) -> Result<T::Unsigned> {
> > >     T::to_result(code)
> > > }
> > >
> >
> > `Error::from_errno` is limited to i32, that's why I followed the
> > `TryInto<i32>` approach, but I like this design too.
> 
> If you pass an i32 that is not a valid errno, then it becomes EINVAL.
> In the case of `isize`, I would say that if a negative isize does not
> fit in i32, then that should fall into the same scenario.
> 

In that case replacing `unwrap_or(0)` with `map_err(|_| code::EINVAL)`
should do the job?

I also thought of an alternative to the custom-trait–based approach.
What do you think about something like this:

    pub fn to_result<T, R>(code: T) -> Result<R>
    where
        T: Copy + TryInto<i32> + TryInto<R>,
    {
        // Try casting into `i32`.
        let casted: crate::ffi::c_int = code.try_into().map_err(|_|
    code::EINVAL)?;

        if casted < 0 {
            Err(Error::from_errno(casted))
        } else {
            // Return the original input value as `R`.
            code.try_into().map_err(|_| code::EINVAL)
        }
    }


On the caller side, it would look like this:

    let val: u16 = to_result(...)?;

The main difference here is that this version can be used to cast into
multiple different types, not just `i32 -> u32` or `i64 -> u64`.

Regards,
Onur

> 
> > > > 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(|_| ())
> > >
> > > This still uses the `map` pattern. Please change it too.
> > >
> > > Alice
> >
> >
> > I left that part intentionally and explained the reason in v2
> > changelog.
> 
> I missed that, ok. I still prefer Ok::<...>()

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Alice Ryhl]

On Wed, Sep 10, 2025 at 2:47 PM Onur Özkan <work@onurozkan.dev> wrote:
>
> On Wed, 10 Sep 2025 13:05:42 +0200
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Wed, Sep 10, 2025 at 12:59 PM Onur Özkan <work@onurozkan.dev>
> > wrote:
> > >
> > > On Wed, 10 Sep 2025 06:26:27 +0000
> > > Alice Ryhl <aliceryhl@google.com> wrote:
> > >
> > > > On Tue, Sep 09, 2025 at 08:00:13PM +0300, Onur Özkan 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())?;
> > > > >
> > > > > 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>
> > > >
> > > > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > > > > index a41de293dcd1..6563ea71e203 100644
> > > > > --- a/rust/kernel/error.rs
> > > > > +++ b/rust/kernel/error.rs
> > > > > @@ -376,12 +376,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)
> > > > >      }
> > > > >  }
> > > >
> > > > I don't think this is the best way to declare this function. The
> > > > conversions I would want are:
> > > >
> > > > * i32 -> Result<u32>
> > > > * isize -> Result<usize>
> > > > * i64 -> Result<u64>
> > > >
> > > > Your commit messages mentions i16, but does the error types even
> > > > fit in 16 bits? Maybe. But they don't fit in i8. That is to say,
> > > > I think it should support all the types larger than i32 (the
> > > > errors fit in those types too), but for the ones that are
> > > > smaller, it might not make sense if the type is too small. That's
> > > > the reverse of what you have now.
> > > >
> > > > We probably need a new trait. E.g.:
> > > >
> > > > trait ToResult {
> > > >     type Unsigned;
> > > >     fn to_result(self) -> Result<Self::Unsigned, Error>;
> > > > }
> > > >
> > > > impl ToResult for i32 {
> > > >     type Unsigned = u32;
> > > >     fn to_result(self) -> Result<u32, Error> {
> > > >         ...
> > > >     }
> > > > }
> > > >
> > > > impl ToResult for isize {
> > > >     type Unsigned = usize;
> > > >     fn to_result(self) -> Result<usize, Error> {
> > > >         ...
> > > >     }
> > > > }
> > > >
> > > > pub fn to_result<T: ToResult>(code: T) -> Result<T::Unsigned> {
> > > >     T::to_result(code)
> > > > }
> > > >
> > >
> > > `Error::from_errno` is limited to i32, that's why I followed the
> > > `TryInto<i32>` approach, but I like this design too.
> >
> > If you pass an i32 that is not a valid errno, then it becomes EINVAL.
> > In the case of `isize`, I would say that if a negative isize does not
> > fit in i32, then that should fall into the same scenario.
> >
>
> In that case replacing `unwrap_or(0)` with `map_err(|_| code::EINVAL)`
> should do the job?
>
> I also thought of an alternative to the custom-trait–based approach.
> What do you think about something like this:
>
>     pub fn to_result<T, R>(code: T) -> Result<R>
>     where
>         T: Copy + TryInto<i32> + TryInto<R>,
>     {
>         // Try casting into `i32`.
>         let casted: crate::ffi::c_int = code.try_into().map_err(|_|
>     code::EINVAL)?;
>
>         if casted < 0 {
>             Err(Error::from_errno(casted))
>         } else {
>             // Return the original input value as `R`.
>             code.try_into().map_err(|_| code::EINVAL)
>         }
>     }
>
>
> On the caller side, it would look like this:
>
>     let val: u16 = to_result(...)?;
>
> The main difference here is that this version can be used to cast into
> multiple different types, not just `i32 -> u32` or `i64 -> u64`.

I think making the return type a separate generic makes this too
difficult to use. It means that any time you would write this:

to_result(unsafe { ... })?;
Ok(())

now you need to tell the compiler what kind of integer you want to get
from to_result, just so you can immediately ignore the integer.

Alice

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Onur Özkan]

On Wed, 10 Sep 2025 14:50:03 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Wed, Sep 10, 2025 at 2:47 PM Onur Özkan <work@onurozkan.dev> wrote:
> >
> > On Wed, 10 Sep 2025 13:05:42 +0200
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > > On Wed, Sep 10, 2025 at 12:59 PM Onur Özkan <work@onurozkan.dev>
> > > wrote:
> > > >
> > > > On Wed, 10 Sep 2025 06:26:27 +0000
> > > > Alice Ryhl <aliceryhl@google.com> wrote:
> > > >
> > > > > On Tue, Sep 09, 2025 at 08:00:13PM +0300, Onur Özkan 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())?;
> > > > > >
> > > > > > 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>
> > > > >
> > > > > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > > > > > index a41de293dcd1..6563ea71e203 100644
> > > > > > --- a/rust/kernel/error.rs
> > > > > > +++ b/rust/kernel/error.rs
> > > > > > @@ -376,12 +376,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)
> > > > > >      }
> > > > > >  }
> > > > >
> > > > > I don't think this is the best way to declare this function.
> > > > > The conversions I would want are:
> > > > >
> > > > > * i32 -> Result<u32>
> > > > > * isize -> Result<usize>
> > > > > * i64 -> Result<u64>
> > > > >
> > > > > Your commit messages mentions i16, but does the error types
> > > > > even fit in 16 bits? Maybe. But they don't fit in i8. That is
> > > > > to say, I think it should support all the types larger than
> > > > > i32 (the errors fit in those types too), but for the ones
> > > > > that are smaller, it might not make sense if the type is too
> > > > > small. That's the reverse of what you have now.
> > > > >
> > > > > We probably need a new trait. E.g.:
> > > > >
> > > > > trait ToResult {
> > > > >     type Unsigned;
> > > > >     fn to_result(self) -> Result<Self::Unsigned, Error>;
> > > > > }
> > > > >
> > > > > impl ToResult for i32 {
> > > > >     type Unsigned = u32;
> > > > >     fn to_result(self) -> Result<u32, Error> {
> > > > >         ...
> > > > >     }
> > > > > }
> > > > >
> > > > > impl ToResult for isize {
> > > > >     type Unsigned = usize;
> > > > >     fn to_result(self) -> Result<usize, Error> {
> > > > >         ...
> > > > >     }
> > > > > }
> > > > >
> > > > > pub fn to_result<T: ToResult>(code: T) -> Result<T::Unsigned>
> > > > > { T::to_result(code)
> > > > > }
> > > > >
> > > >
> > > > `Error::from_errno` is limited to i32, that's why I followed the
> > > > `TryInto<i32>` approach, but I like this design too.
> > >
> > > If you pass an i32 that is not a valid errno, then it becomes
> > > EINVAL. In the case of `isize`, I would say that if a negative
> > > isize does not fit in i32, then that should fall into the same
> > > scenario.
> > >
> >
> > In that case replacing `unwrap_or(0)` with `map_err(|_|
> > code::EINVAL)` should do the job?
> >
> > I also thought of an alternative to the custom-trait–based approach.
> > What do you think about something like this:
> >
> >     pub fn to_result<T, R>(code: T) -> Result<R>
> >     where
> >         T: Copy + TryInto<i32> + TryInto<R>,
> >     {
> >         // Try casting into `i32`.
> >         let casted: crate::ffi::c_int = code.try_into().map_err(|_|
> >     code::EINVAL)?;
> >
> >         if casted < 0 {
> >             Err(Error::from_errno(casted))
> >         } else {
> >             // Return the original input value as `R`.
> >             code.try_into().map_err(|_| code::EINVAL)
> >         }
> >     }
> >
> >
> > On the caller side, it would look like this:
> >
> >     let val: u16 = to_result(...)?;
> >
> > The main difference here is that this version can be used to cast
> > into multiple different types, not just `i32 -> u32` or `i64 ->
> > u64`.
> 
> I think making the return type a separate generic makes this too
> difficult to use. It means that any time you would write this:
> 
> to_result(unsafe { ... })?;
> Ok(())
> 
> now you need to tell the compiler what kind of integer you want to get
> from to_result, just so you can immediately ignore the integer.
> 
> Alice

Yes, and with the custom trait you need to import it in order to use
the `to_result` helper. I don't have a strong preference either way.
I guess I will wait a couple of days to get more feedback from others
as well. Thank you for your quick feedback and review so far!

-Onur

Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[kernel test robot]

Hi Onur,

kernel test robot noticed the following build errors:

[auto build test ERROR on broonie-regulator/for-next]
[cannot apply to driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus rafael-pm/linux-next rafael-pm/bleeding-edge rust/alloc-next rust/rust-next char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus akpm-mm/mm-everything pci/next pci/for-linus rust/rust-block-next linus/master v6.17-rc5 next-20250912]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Onur-zkan/rust-refactor-to_result-to-return-the-original-value/20250910-010803
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
patch link:    https://lore.kernel.org/r/20250909170013.16025-2-work%40onurozkan.dev
patch subject: [PATCH v2 1/1] rust: refactor to_result to return the original value
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250912/202509121607.yxpZ3HBS-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250912/202509121607.yxpZ3HBS-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509121607.yxpZ3HBS-lkp@intel.com/

All errors (new ones prefixed by >>):

>> error[E0277]: the trait bound `i32: From<()>` is not satisfied
   --> rust/kernel/configfs.rs:181:17
   |
>> 179 |             crate::error::to_result(
   |             ----------------------- required by a bound introduced by this call
   180 |                 // SAFETY: We initialized `this.subsystem` according to C API contract above.
   181 |                 unsafe { bindings::configfs_register_subsystem(this.subsystem.get()) },
   |                 ^^^^^^^^^-----------------------------------------------------------^^
   |                 |        |
   |                 |        this tail expression is of type `i32`
   |                 the trait `From<()>` is not implemented for `i32`
   |
   = help: the following other types implement trait `From<T>`:
   `i32` implements `From<CpuId>`
   `i32` implements `From<bool>`
   `i32` implements `From<i16>`
   `i32` implements `From<i8>`
   `i32` implements `From<u16>`
   `i32` implements `From<u8>`
   = note: required for `()` to implement `Into<i32>`
   = note: required for `i32` to implement `TryFrom<()>`
   = note: required for `()` to implement `TryInto<i32>`
   note: required by a bound in `error::to_result`
   --> rust/kernel/error.rs:382:15
   |
   380 | pub fn to_result<T>(code: T) -> Result<T>
   |        --------- required by a bound in this function
   381 | where
   382 |     T: Copy + TryInto<i32>,
   |               ^^^^^^^^^^^^ required by this bound in `to_result`
--
>> error[E0277]: the trait bound `i32: From<()>` is not satisfied
   --> rust/kernel/net/phy/reg.rs:122:19
   |
   122 |           to_result(unsafe {
   |  _________---------_^
   | |         |
   | |         required by a bound introduced by this call
   123 | |             bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, self.0.into(), val)
   | |             ------------------------------------------------------------------------------------ this tail expression is of type `i32`
   124 | |         })
   | |_________^ the trait `From<()>` is not implemented for `i32`
   |
   = help: the following other types implement trait `From<T>`:
   `i32` implements `From<CpuId>`
   `i32` implements `From<bool>`
   `i32` implements `From<i16>`
   `i32` implements `From<i8>`
   `i32` implements `From<u16>`
   `i32` implements `From<u8>`
   = note: required for `()` to implement `Into<i32>`
   = note: required for `i32` to implement `TryFrom<()>`
   = note: required for `()` to implement `TryInto<i32>`
   note: required by a bound in `error::to_result`
   --> rust/kernel/error.rs:382:15
   |
   380 | pub fn to_result<T>(code: T) -> Result<T>
   |        --------- required by a bound in this function
   381 | where
   382 |     T: Copy + TryInto<i32>,
   |               ^^^^^^^^^^^^ required by this bound in `to_result`
--
>> error[E0277]: the trait bound `i32: From<()>` is not satisfied
   --> rust/kernel/net/phy/reg.rs:211:19
   |
   211 |           to_result(unsafe {
   |  _________---------_^
   | |         |
   | |         required by a bound introduced by this call
   212 | |             bindings::phy_write_mmd(phydev, self.devad.0.into(), self.regnum.into(), val)
   | |             ----------------------------------------------------------------------------- this tail expression is of type `i32`
   213 | |         })
   | |_________^ the trait `From<()>` is not implemented for `i32`
   |
   = help: the following other types implement trait `From<T>`:
   `i32` implements `From<CpuId>`
   `i32` implements `From<bool>`
   `i32` implements `From<i16>`
   `i32` implements `From<i8>`
   `i32` implements `From<u16>`
   `i32` implements `From<u8>`
   = note: required for `()` to implement `Into<i32>`
   = note: required for `i32` to implement `TryFrom<()>`
   = note: required for `()` to implement `TryInto<i32>`
   note: required by a bound in `error::to_result`
   --> rust/kernel/error.rs:382:15
   |
   380 | pub fn to_result<T>(code: T) -> Result<T>
   |        --------- required by a bound in this function
   381 | where
   382 |     T: Copy + TryInto<i32>,
   |               ^^^^^^^^^^^^ required by this bound in `to_result`
--
>> error[E0277]: the trait bound `i32: From<()>` is not satisfied
   --> rust/kernel/net/phy.rs:219:19
   |
   219 |         to_result(unsafe { bindings::genphy_soft_reset(phydev) })
   |         --------- ^^^^^^^^^-----------------------------------^^
   |         |         |        |
   |         |         |        this tail expression is of type `i32`
   |         |         the trait `From<()>` is not implemented for `i32`
   |         required by a bound introduced by this call
   |
   = help: the following other types implement trait `From<T>`:
   `i32` implements `From<CpuId>`
   `i32` implements `From<bool>`
   `i32` implements `From<i16>`
   `i32` implements `From<i8>`
   `i32` implements `From<u16>`
   `i32` implements `From<u8>`
   = note: required for `()` to implement `Into<i32>`
   = note: required for `i32` to implement `TryFrom<()>`
   = note: required for `()` to implement `TryInto<i32>`
   note: required by a bound in `error::to_result`
   --> rust/kernel/error.rs:382:15
   |
   380 | pub fn to_result<T>(code: T) -> Result<T>
   |        --------- required by a bound in this function
   381 | where
   382 |     T: Copy + TryInto<i32>,
   |               ^^^^^^^^^^^^ required by this bound in `to_result`
--
>> error[E0277]: the trait bound `i32: From<()>` is not satisfied
   --> rust/kernel/net/phy.rs:227:19
   |
   227 |         to_result(unsafe { bindings::phy_init_hw(phydev) })
   |         --------- ^^^^^^^^^-----------------------------^^
   |         |         |        |
   |         |         |        this tail expression is of type `i32`
   |         |         the trait `From<()>` is not implemented for `i32`
   |         required by a bound introduced by this call
   |
   = help: the following other types implement trait `From<T>`:
   `i32` implements `From<CpuId>`
   `i32` implements `From<bool>`
   `i32` implements `From<i16>`
   `i32` implements `From<i8>`
   `i32` implements `From<u16>`
   `i32` implements `From<u8>`
   = note: required for `()` to implement `Into<i32>`
   = note: required for `i32` to implement `TryFrom<()>`
   = note: required for `()` to implement `TryInto<i32>`
   note: required by a bound in `error::to_result`
   --> rust/kernel/error.rs:382:15
   |
   380 | pub fn to_result<T>(code: T) -> Result<T>
   |        --------- required by a bound in this function
   381 | where
   382 |     T: Copy + TryInto<i32>,
   |               ^^^^^^^^^^^^ required by this bound in `to_result`
--
>> error[E0277]: the trait bound `i32: From<()>` is not satisfied
   --> rust/kernel/net/phy.rs:235:19
   |
   235 |         to_result(unsafe { bindings::_phy_start_aneg(phydev) })
   |         --------- ^^^^^^^^^---------------------------------^^
   |         |         |        |
   |         |         |        this tail expression is of type `i32`
   |         |         the trait `From<()>` is not implemented for `i32`
   |         required by a bound introduced by this call
   |
   = help: the following other types implement trait `From<T>`:
   `i32` implements `From<CpuId>`
   `i32` implements `From<bool>`
   `i32` implements `From<i16>`
   `i32` implements `From<i8>`
   `i32` implements `From<u16>`
   `i32` implements `From<u8>`
   = note: required for `()` to implement `Into<i32>`
   = note: required for `i32` to implement `TryFrom<()>`
   = note: required for `()` to implement `TryInto<i32>`
   note: required by a bound in `error::to_result`
   --> rust/kernel/error.rs:382:15
   |
   380 | pub fn to_result<T>(code: T) -> Result<T>
   |        --------- required by a bound in this function
   381 | where
   382 |     T: Copy + TryInto<i32>,
   |               ^^^^^^^^^^^^ required by this bound in `to_result`
--
>> error[E0277]: the trait bound `i32: From<()>` is not satisfied
   --> rust/kernel/net/phy.rs:243:19
   |
   243 |         to_result(unsafe { bindings::genphy_resume(phydev) })
   |         --------- ^^^^^^^^^-------------------------------^^
   |         |         |        |
   |         |         |        this tail expression is of type `i32`
   |         |         the trait `From<()>` is not implemented for `i32`
   |         required by a bound introduced by this call
   |
   = help: the following other types implement trait `From<T>`:
   `i32` implements `From<CpuId>`
   `i32` implements `From<bool>`
   `i32` implements `From<i16>`
   `i32` implements `From<i8>`
   `i32` implements `From<u16>`
   `i32` implements `From<u8>`
   = note: required for `()` to implement `Into<i32>`
   = note: required for `i32` to implement `TryFrom<()>`
   = note: required for `()` to implement `TryInto<i32>`
   note: required by a bound in `error::to_result`
   --> rust/kernel/error.rs:382:15
   |
   380 | pub fn to_result<T>(code: T) -> Result<T>
   |        --------- required by a bound in this function
   381 | where
   382 |     T: Copy + TryInto<i32>,
   |               ^^^^^^^^^^^^ required by this bound in `to_result`
--
>> error[E0277]: the trait bound `i32: From<()>` is not satisfied
   --> rust/kernel/net/phy.rs:251:19
   |
   251 |         to_result(unsafe { bindings::genphy_suspend(phydev) })
   |         --------- ^^^^^^^^^--------------------------------^^
   |         |         |        |
   |         |         |        this tail expression is of type `i32`
   |         |         the trait `From<()>` is not implemented for `i32`
   |         required by a bound introduced by this call
   |
   = help: the following other types implement trait `From<T>`:
   `i32` implements `From<CpuId>`
   `i32` implements `From<bool>`
   `i32` implements `From<i16>`
   `i32` implements `From<i8>`
   `i32` implements `From<u16>`
   `i32` implements `From<u8>`
   = note: required for `()` to implement `Into<i32>`
   = note: required for `i32` to implement `TryFrom<()>`
   = note: required for `()` to implement `TryInto<i32>`
   note: required by a bound in `error::to_result`
   --> rust/kernel/error.rs:382:15
   |
   380 | pub fn to_result<T>(code: T) -> Result<T>
   |        --------- required by a bound in this function
   381 | where
   382 |     T: Copy + TryInto<i32>,
   |               ^^^^^^^^^^^^ required by this bound in `to_result`
--
>> error[E0277]: the trait bound `i32: From<()>` is not satisfied
   --> rust/kernel/net/phy.rs:264:19
   |
   264 |         to_result(unsafe { bindings::genphy_update_link(phydev) })
   |         --------- ^^^^^^^^^------------------------------------^^
   |         |         |        |
   |         |         |        this tail expression is of type `i32`
   |         |         the trait `From<()>` is not implemented for `i32`
   |         required by a bound introduced by this call
   |
   = help: the following other types implement trait `From<T>`:
   `i32` implements `From<CpuId>`
   `i32` implements `From<bool>`
   `i32` implements `From<i16>`
   `i32` implements `From<i8>`
   `i32` implements `From<u16>`
   `i32` implements `From<u8>`
   = note: required for `()` to implement `Into<i32>`
   = note: required for `i32` to implement `TryFrom<()>`
   = note: required for `()` to implement `TryInto<i32>`
   note: required by a bound in `error::to_result`
   --> rust/kernel/error.rs:382:15
   |
   380 | pub fn to_result<T>(code: T) -> Result<T>
   |        --------- required by a bound in this function
   381 | where
   382 |     T: Copy + TryInto<i32>,
   |               ^^^^^^^^^^^^ required by this bound in `to_result`
--
>> error[E0277]: the trait bound `i32: From<()>` is not satisfied
   --> rust/kernel/net/phy.rs:272:19
   |
   272 |         to_result(unsafe { bindings::genphy_read_lpa(phydev) })
   |         --------- ^^^^^^^^^---------------------------------^^
   |         |         |        |
   |         |         |        this tail expression is of type `i32`
   |         |         the trait `From<()>` is not implemented for `i32`
   |         required by a bound introduced by this call
   |
   = help: the following other types implement trait `From<T>`:
   `i32` implements `From<CpuId>`
   `i32` implements `From<bool>`
   `i32` implements `From<i16>`
   `i32` implements `From<i8>`
   `i32` implements `From<u16>`
   `i32` implements `From<u8>`
   = note: required for `()` to implement `Into<i32>`
   = note: required for `i32` to implement `TryFrom<()>`
   = note: required for `()` to implement `TryInto<i32>`
   note: required by a bound in `error::to_result`
   --> rust/kernel/error.rs:382:15
   |
   380 | pub fn to_result<T>(code: T) -> Result<T>
   |        --------- required by a bound in this function
   381 | where
   382 |     T: Copy + TryInto<i32>,
   |               ^^^^^^^^^^^^ required by this bound in `to_result`
--
>> error[E0277]: the trait bound `i32: From<()>` is not satisfied
   --> rust/kernel/net/phy.rs:280:19
   |
   280 |         to_result(unsafe { bindings::genphy_read_abilities(phydev) })
   |         --------- ^^^^^^^^^---------------------------------------^^
   |         |         |        |
   |         |         |        this tail expression is of type `i32`
   |         |         the trait `From<()>` is not implemented for `i32`
   |         required by a bound introduced by this call
   |
   = help: the following other types implement trait `From<T>`:
   `i32` implements `From<CpuId>`
   `i32` implements `From<bool>`
   `i32` implements `From<i16>`
   `i32` implements `From<i8>`
   `i32` implements `From<u16>`
   `i32` implements `From<u8>`
   = note: required for `()` to implement `Into<i32>`
   = note: required for `i32` to implement `TryFrom<()>`
   = note: required for `()` to implement `TryInto<i32>`
   note: required by a bound in `error::to_result`
   --> rust/kernel/error.rs:382:15
   |
   380 | pub fn to_result<T>(code: T) -> Result<T>
   |        --------- required by a bound in this function
   381 | where
   382 |     T: Copy + TryInto<i32>,
   |               ^^^^^^^^^^^^ required by this bound in `to_result`
..

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki