Re: [PATCH 08/10] rust: driver: remove open-coded matching logic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Missing `#[inline]` annotation on `id_info`.
- [Low] Leftover FFI wrapper `acpi_of_match_device` and its backing C helpers are unused.
--

commit 8ad64c5f4617fb456cb113763d7136737e7f51e2
Author: Gary Guo <gary@garyguo.net>

rust: driver: remove open-coded matching logic
    
This commit replaces the open-coded ACPI and OF matching logic with a single
call to device_get_match_data(), as device ID info now includes pointers 
instead of indices.

> diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> index 824899d76fed2..a881f5ef99ecf 100644
> --- a/rust/kernel/driver.rs
> +++ b/rust/kernel/driver.rs
[ ... ]
> @@ -325,117 +324,22 @@ pub trait Adapter {
[ ... ]
> -        #[cfg(CONFIG_ACPI)]
> -        {
> -            use core::ptr;
> -            use device::property::FwNode;
> -
> -            let mut raw_id = ptr::null();
> -
> -            let fwnode = dev.fwnode().map_or(ptr::null_mut(), FwNode::as_raw);
> -
> -            // SAFETY: `fwnode` is a pointer to a valid `fwnode_handle`. A null pointer will be
> -            // passed through the function.
> -            let adev = unsafe { bindings::to_acpi_device_node(fwnode) };
> -
> -            // SAFETY:
> -            // - `adev` is a valid pointer to `acpi_device` or is null. It is guaranteed to be
> -            //   valid as long as `dev` is alive.
> -            // - `table` has static lifetime, hence it's valid for read.
> -            if unsafe { acpi_of_match_device(adev, table.as_ptr(), &raw mut raw_id) } {

[Severity: Low]
Since the manual ACPI matching is being removed here, are the FFI wrapper
acpi_of_match_device() and its backing C helpers (such as
rust_helper_acpi_of_match_device() and rust_helper_to_acpi_device_node()
in rust/helpers/acpi.c) now completely unused? Should they be removed
in this patch to avoid leaving dead code behind?

[ ... ]
> -    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;
> -        }
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that the `dev` matched data is of type `Self::IdInfo`.
> +    unsafe fn id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
> +        // SAFETY: `dev` is guaranteed to be valid while it's alive, and so is `dev.as_raw()`.
> +        let data = unsafe { bindings::device_get_match_data(dev.as_raw()) };

[Severity: Low]
This isn't a bug, but according to the Rust subsystem coding guidelines,
functions in abstractions that are small or simply forward to a binding
call should be annotated with #[inline]. Since this function consists of a
single FFI call and a cast, should an #[inline] annotation be added here?

>  
> -        None
> +        // SAFETY: Per safety requirement, `data` is of type `Self::IdInfo`.
> +        unsafe { data.cast::<Self::IdInfo>().as_ref() }
>      }
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-id_info-v1-0-96af1e559ef9@garyguo.net?part=8