[PATCH v2 0/1] rust: simplify `Adapter::id_info`

[PATCH v2 0/1] rust: simplify `Adapter::id_info`

[Onur Özkan]

Changes in v2:
    - Rebased to cover new changes in Adapter::id_info
	  since v1.

Onur Özkan (1):
  rust: simplify `Adapter::id_info`

 rust/kernel/driver.rs | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

-- 
2.51.2

[PATCH v2 1/1] rust: simplify `Adapter::id_info`

[Onur Özkan]

id_info() checks ACPI first and falls back to OF.

This replaces the unnecessarily verbose approach with a
simple or_else() chain and drops temporary variables.

No functional change intended.

Signed-off-by: Onur Özkan <work@onurozkan.dev>
---
 rust/kernel/driver.rs | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
index 649d06468f41..6cef792d54e4 100644
--- a/rust/kernel/driver.rs
+++ b/rust/kernel/driver.rs
@@ -307,16 +307,6 @@ fn of_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
     /// If this returns `None`, it means that there is no match in any of the ID tables directly
     /// associated with a [`device::Device`].
     fn id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
-        let id = Self::acpi_id_info(dev);
-        if id.is_some() {
-            return id;
-        }
-
-        let id = Self::of_id_info(dev);
-        if id.is_some() {
-            return id;
-        }
-
-        None
+        Self::acpi_id_info(dev).or_else(|| Self::of_id_info(dev))
     }
 }
-- 
2.51.2

Re: [PATCH v2 1/1] rust: simplify `Adapter::id_info`

[Greg KH]

On Sat, Jan 17, 2026 at 12:47:10PM +0300, Onur Özkan wrote:
> id_info() checks ACPI first and falls back to OF.
> 
> This replaces the unnecessarily verbose approach with a
> simple or_else() chain and drops temporary variables.
> 
> No functional change intended.
> 
> Signed-off-by: Onur Özkan <work@onurozkan.dev>
> ---
>  rust/kernel/driver.rs | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> index 649d06468f41..6cef792d54e4 100644
> --- a/rust/kernel/driver.rs
> +++ b/rust/kernel/driver.rs
> @@ -307,16 +307,6 @@ fn of_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
>      /// If this returns `None`, it means that there is no match in any of the ID tables directly
>      /// associated with a [`device::Device`].
>      fn id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
> -        let id = Self::acpi_id_info(dev);
> -        if id.is_some() {
> -            return id;
> -        }
> -
> -        let id = Self::of_id_info(dev);
> -        if id.is_some() {
> -            return id;
> -        }
> -
> -        None
> +        Self::acpi_id_info(dev).or_else(|| Self::of_id_info(dev))

Have we already started the game of "rust golf" on the kernel?  The
original code here is much easier to read, and the compiler should
produce the same thing for both, right?

thanks,

greg k-h

Re: [PATCH v2 1/1] rust: simplify `Adapter::id_info`

[Onur Özkan]

On Sat, 17 Jan 2026 11:00:11 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sat, Jan 17, 2026 at 12:47:10PM +0300, Onur Özkan wrote:
> > id_info() checks ACPI first and falls back to OF.
> > 
> > This replaces the unnecessarily verbose approach with a
> > simple or_else() chain and drops temporary variables.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > ---
> >  rust/kernel/driver.rs | 12 +-----------
> >  1 file changed, 1 insertion(+), 11 deletions(-)
> > 
> > diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> > index 649d06468f41..6cef792d54e4 100644
> > --- a/rust/kernel/driver.rs
> > +++ b/rust/kernel/driver.rs
> > @@ -307,16 +307,6 @@ fn of_id_info(dev: &device::Device) ->
> > Option<&'static Self::IdInfo> { /// If this returns `None`, it
> > means that there is no match in any of the ID tables directly ///
> > associated with a [`device::Device`]. fn id_info(dev:
> > &device::Device) -> Option<&'static Self::IdInfo> {
> > -        let id = Self::acpi_id_info(dev);
> > -        if id.is_some() {
> > -            return id;
> > -        }
> > -
> > -        let id = Self::of_id_info(dev);
> > -        if id.is_some() {
> > -            return id;
> > -        }
> > -
> > -        None
> > +        Self::acpi_id_info(dev).or_else(|| Self::of_id_info(dev))
> 
> Have we already started the game of "rust golf" on the kernel?  The
> original code here is much easier to read, and the compiler should
> produce the same thing for both, right?
> 
> thanks,
> 
> greg k-h

Hi Greg,

I don't know what "rust golf" means, the main motivation here was to use
a more idiomatic Rust pattern.

Functions called in Adapter::id_info already return the same Result
type and it's quite annoying(IMO) to do "if x.is_some() { return x }"
in cases like this. Clippy would argue on many cases similar to this.
Readability is subjective, but to me the new code is slightly simpler
to follow (as it does the "try X, then Y" thing in a shorter way).

That said, I don't have strong feelings on the new code. The main
motivation was to drop the "annoying" code with more idiomatic one,
but seems like not everyone agrees with it. Feel free to ignore the
patch if you prefer the original code.

Regards,
Onur Özkan

Re: [PATCH v2 1/1] rust: simplify `Adapter::id_info`

[Greg KH]

On Sat, Jan 17, 2026 at 02:02:40PM +0300, Onur Özkan wrote:
> On Sat, 17 Jan 2026 11:00:11 +0100
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Sat, Jan 17, 2026 at 12:47:10PM +0300, Onur Özkan wrote:
> > > id_info() checks ACPI first and falls back to OF.
> > > 
> > > This replaces the unnecessarily verbose approach with a
> > > simple or_else() chain and drops temporary variables.
> > > 
> > > No functional change intended.
> > > 
> > > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > > ---
> > >  rust/kernel/driver.rs | 12 +-----------
> > >  1 file changed, 1 insertion(+), 11 deletions(-)
> > > 
> > > diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> > > index 649d06468f41..6cef792d54e4 100644
> > > --- a/rust/kernel/driver.rs
> > > +++ b/rust/kernel/driver.rs
> > > @@ -307,16 +307,6 @@ fn of_id_info(dev: &device::Device) ->
> > > Option<&'static Self::IdInfo> { /// If this returns `None`, it
> > > means that there is no match in any of the ID tables directly ///
> > > associated with a [`device::Device`]. fn id_info(dev:
> > > &device::Device) -> Option<&'static Self::IdInfo> {
> > > -        let id = Self::acpi_id_info(dev);
> > > -        if id.is_some() {
> > > -            return id;
> > > -        }
> > > -
> > > -        let id = Self::of_id_info(dev);
> > > -        if id.is_some() {
> > > -            return id;
> > > -        }
> > > -
> > > -        None
> > > +        Self::acpi_id_info(dev).or_else(|| Self::of_id_info(dev))
> > 
> > Have we already started the game of "rust golf" on the kernel?  The
> > original code here is much easier to read, and the compiler should
> > produce the same thing for both, right?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi Greg,
> 
> I don't know what "rust golf" means, the main motivation here was to use
> a more idiomatic Rust pattern.

I was referring to this:
	https://en.wikipedia.org/wiki/Code_golf
sorry for not being explicit :)

> Functions called in Adapter::id_info already return the same Result
> type and it's quite annoying(IMO) to do "if x.is_some() { return x }"
> in cases like this. Clippy would argue on many cases similar to this.
> Readability is subjective, but to me the new code is slightly simpler
> to follow (as it does the "try X, then Y" thing in a shorter way).

Does clippy complain about this one?

> That said, I don't have strong feelings on the new code. The main
> motivation was to drop the "annoying" code with more idiomatic one,
> but seems like not everyone agrees with it. Feel free to ignore the
> patch if you prefer the original code.

I don't have strong feelings either, but the original is "easier" for
those of us used to C code.

thanks,

greg k-h

Re: [PATCH v2 1/1] rust: simplify `Adapter::id_info`

[Danilo Krummrich]

On Sat Jan 17, 2026 at 1:07 PM CET, Greg KH wrote:
> Does clippy complain about this one?

No, it does not.

> I don't have strong feelings either, but the original is "easier" for
> those of us used to C code.

I think it's a matter of preference. Personally, I like those functional
characteristics of Rust and the corresponding possibility of compact expressions
as long as it is not overdone.

This case seems pretty simple though. :)

In comparison, this is code from allocating the level 1 page directory for the
GSP (radix3) firmware, which is probably a bit too much.

	level1 <- {
	    // Allocate the level 1 page table, map the level 2 page table onto it, and map it
	    // into the device address space.
	    VVec::<u8>::with_capacity(
	        level2.iter().count() * size_of::<u64>(),
	        GFP_KERNEL,
	    )
	    .map_err(|_| ENOMEM)
	    .and_then(|level1| map_into_lvl(&level2, level1))
	    .map(|level1| SGTable::new(dev, level1, DataDirection::ToDevice, GFP_KERNEL))?
	},

Re: [PATCH v2 1/1] rust: simplify `Adapter::id_info`

[Gary Guo]

On Sat Jan 17, 2026 at 12:53 PM GMT, Danilo Krummrich wrote:
> On Sat Jan 17, 2026 at 1:07 PM CET, Greg KH wrote:
>> Does clippy complain about this one?
>
> No, it does not.
>
>> I don't have strong feelings either, but the original is "easier" for
>> those of us used to C code.
>
> I think it's a matter of preference. Personally, I like those functional
> characteristics of Rust and the corresponding possibility of compact expressions
> as long as it is not overdone.
>
> This case seems pretty simple though. :)
>
> In comparison, this is code from allocating the level 1 page directory for the
> GSP (radix3) firmware, which is probably a bit too much.
>
> 	level1 <- {
> 	    // Allocate the level 1 page table, map the level 2 page table onto it, and map it
> 	    // into the device address space.
> 	    VVec::<u8>::with_capacity(
> 	        level2.iter().count() * size_of::<u64>(),
> 	        GFP_KERNEL,
> 	    )
> 	    .map_err(|_| ENOMEM)
> 	    .and_then(|level1| map_into_lvl(&level2, level1))
> 	    .map(|level1| SGTable::new(dev, level1, DataDirection::ToDevice, GFP_KERNEL))?

For this specific instance, it looks like it could just be

    let level1 = VVec::<u8>::with_capacity(
        level2.iter().count() * size_of::<u64>(),
        GFP_KERNEL,
    )?;
    let level1 = map_into_lvl(&level2, level1)?;
    SGTable::new(dev, level1, DataDirection::ToDevice, GFP_KENREL)?

which IMO looks clearer.
    
I suspect what people want is Elixir's pipe operator so the above is like this
(non-existent, imaginary syntax):

    VVec::<u8>::with_capacity(
        level2.iter().count() * size_of::<u64>(),
        GFP_KERNEL,
    )?
    |> map_into_lvl(&level2, _)?
    |> SGTable::new(dev, _, DataDirection::ToDevice, GFP_KERNEL)?

But I think keeping things as `Result` (and not using `?`) just to use the
functional combinatiors is probably a bad idea.

Best,
Gary

Re: [PATCH v2 1/1] rust: simplify `Adapter::id_info`

[Miguel Ojeda]

On Tue, Jan 20, 2026 at 4:01 PM Gary Guo <gary@garyguo.net> wrote:
>
> For this specific instance, it looks like it could just be
>
>     let level1 = VVec::<u8>::with_capacity(
>         level2.iter().count() * size_of::<u64>(),
>         GFP_KERNEL,
>     )?;
>     let level1 = map_into_lvl(&level2, level1)?;
>     SGTable::new(dev, level1, DataDirection::ToDevice, GFP_KENREL)?
>
> which IMO looks clearer.
>
> I suspect what people want is Elixir's pipe operator so the above is like this
> (non-existent, imaginary syntax):
>
>     VVec::<u8>::with_capacity(
>         level2.iter().count() * size_of::<u64>(),
>         GFP_KERNEL,
>     )?
>     |> map_into_lvl(&level2, _)?
>     |> SGTable::new(dev, _, DataDirection::ToDevice, GFP_KERNEL)?
>
> But I think keeping things as `Result` (and not using `?`) just to use the
> functional combinatiors is probably a bad idea.

Agreed -- I think when `?` is applicable, it can make things cleaner
to get things "out" of the type (and, after all, we did get the
"symbol soup" complaints about closures in the past... :).

Cheers,
Miguel

Re: [PATCH v2 1/1] rust: simplify `Adapter::id_info`

[Alexandre Courbot]

On Sat Jan 17, 2026 at 9:53 PM JST, Danilo Krummrich wrote:
> On Sat Jan 17, 2026 at 1:07 PM CET, Greg KH wrote:
>> Does clippy complain about this one?
>
> No, it does not.
>
>> I don't have strong feelings either, but the original is "easier" for
>> those of us used to C code.
>
> I think it's a matter of preference. Personally, I like those functional
> characteristics of Rust and the corresponding possibility of compact expressions
> as long as it is not overdone.
>
> This case seems pretty simple though. :)

IIUC for this patch the consensus seems to be in favor of preserving the
original C-like style, but FWIW I found the new style proposed by Onur
easier to read.

I understand that it may not be intuitive unless one has some
familiarity with Rust, but it is a common way to write things in that
language and since it has graduated from its experimental status, it
doesn't seem judicious to me to artificially restrict its features.

Re: [PATCH v2 1/1] rust: simplify `Adapter::id_info`

[Onur Özkan]

On Sat, 17 Jan 2026 13:07:54 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sat, Jan 17, 2026 at 02:02:40PM +0300, Onur Özkan wrote:
> > On Sat, 17 Jan 2026 11:00:11 +0100
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Sat, Jan 17, 2026 at 12:47:10PM +0300, Onur Özkan wrote:
> > > > id_info() checks ACPI first and falls back to OF.
> > > > 
> > > > This replaces the unnecessarily verbose approach with a
> > > > simple or_else() chain and drops temporary variables.
> > > > 
> > > > No functional change intended.
> > > > 
> > > > Signed-off-by: Onur Özkan <work@onurozkan.dev>
> > > > ---
> > > >  rust/kernel/driver.rs | 12 +-----------
> > > >  1 file changed, 1 insertion(+), 11 deletions(-)
> > > > 
> > > > diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> > > > index 649d06468f41..6cef792d54e4 100644
> > > > --- a/rust/kernel/driver.rs
> > > > +++ b/rust/kernel/driver.rs
> > > > @@ -307,16 +307,6 @@ fn of_id_info(dev: &device::Device) ->
> > > > Option<&'static Self::IdInfo> { /// If this returns `None`, it
> > > > means that there is no match in any of the ID tables directly
> > > > /// associated with a [`device::Device`]. fn id_info(dev:
> > > > &device::Device) -> Option<&'static Self::IdInfo> {
> > > > -        let id = Self::acpi_id_info(dev);
> > > > -        if id.is_some() {
> > > > -            return id;
> > > > -        }
> > > > -
> > > > -        let id = Self::of_id_info(dev);
> > > > -        if id.is_some() {
> > > > -            return id;
> > > > -        }
> > > > -
> > > > -        None
> > > > +        Self::acpi_id_info(dev).or_else(||
> > > > Self::of_id_info(dev))
> > > 
> > > Have we already started the game of "rust golf" on the kernel?
> > > The original code here is much easier to read, and the compiler
> > > should produce the same thing for both, right?
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Hi Greg,
> > 
> > I don't know what "rust golf" means, the main motivation here was
> > to use a more idiomatic Rust pattern.
> 
> I was referring to this:
> 	https://en.wikipedia.org/wiki/Code_golf
> sorry for not being explicit :)
> 

I didn't know this term before - good to know :)

> > Functions called in Adapter::id_info already return the same Result
> > type and it's quite annoying(IMO) to do "if x.is_some() { return x
> > }" in cases like this. Clippy would argue on many cases similar to
> > this. Readability is subjective, but to me the new code is slightly
> > simpler to follow (as it does the "try X, then Y" thing in a
> > shorter way).
> 
> Does clippy complain about this one?
> 

No, it doesn't.

> > That said, I don't have strong feelings on the new code. The main
> > motivation was to drop the "annoying" code with more idiomatic one,
> > but seems like not everyone agrees with it. Feel free to ignore the
> > patch if you prefer the original code.
> 
> I don't have strong feelings either, but the original is "easier" for
> those of us used to C code.
> 

Fair point, you are right.

Thanks,
Onur Özkan

> thanks,
> 
> greg k-h

Re: [PATCH v2 1/1] rust: simplify `Adapter::id_info`

[Miguel Ojeda]

On Sat, Jan 17, 2026 at 12:03 PM Onur Özkan <work@onurozkan.dev> wrote:
>
> I don't know what "rust golf" means, the main motivation here was to use
> a more idiomatic Rust pattern.

I have seen some patches recently arguing certain methods being more
idiomatic etc.

I would like to caution that the fact that a method exists doesn't
make it a good idea to automatically apply it in every case, and I
don't think using a maximally functional style is actually idiomatic
in Rust to begin with.

Even if it were actually more idiomatic, the idiomatic argument isn't
very strong anyway, i.e. what matters is making the code look best,
and the change needs to be a fair improvement -- otherwise it may not
be worth it to shuffle the code around etc.

In this particular instance, it is a significant reduction in lines,
indeed, but I don't think the original code is "annoying" at all -- it
was already easy to follow.

In general, I think authors and maintainers should pick whatever style
they think suits best for each situation, since both can be overdone.

Cheers,
Miguel