Re: [PATCH v2 3/3] rust: serdev: Simplify callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] serdev: compilation error in OpenGuard initialization due to type mismatch
- [Low] serdev: missing #[inline] attribute on OpenGuard::drop
--

commit 9ef182dba6207d4d0437751e74011a0c64fbd6e3
Author: Markus Probst <markus.probst@posteo.de>

rust: serdev: Simplify callbacks

Initialize the driver's private data directly on PrivateData.

Introduce OpenGuard for resource cleanup.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index c16d6593a8d2d..3f9165e167764 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
[ ... ]
> +struct OpenGuard<'bound> {
> +    sdev: &'bound Device<device::Bound>,
> +}
> +
> +impl Drop for OpenGuard<'_> {
> +    fn drop(&mut self) {

[Severity: Low]
Does this function need an #[inline] attribute?

According to the kernel coding guidelines, small functions in abstractions
(like the kernel crate) that forward to a C binding should be annotated
with #[inline].

> +        // SAFETY:
> +        // - `self.sdev.as_raw()` is guaranteed to be a pointer to a valid
> +        //   `struct serdev_device`.
> +        // - The existence of self proves that the device is open.
> +        unsafe { bindings::serdev_device_close(self.sdev.as_raw()) };
> +    }
> +}
[ ... ]
> @@ -134,46 +142,30 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
>  
>          from_result(|| {
>              sdev.as_ref().set_drvdata(try_pin_init!(PrivateData::<T> {
> -                sdev: &**sdev,
> -                driver: MaybeUninit::<T::Data<'_>>::zeroed().into(),
> -                open: false.into(),
> +                open: {
> +                    // SAFETY:
> +                    // - `sdev.as_raw()` is guaranteed to be a valid pointer to
> +                    //   `serdev_device`.
> +                    // - It is safe to call before open.
> +                    unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::OPS) };
> +
> +                    // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to
> +                    // `serdev_device`.
> +                    unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
> +
> +                    // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to
> +                    // `serdev_device`.
> +                    to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?;
> +
> +                    OpenGuard { sdev }

[Severity: Medium]
Will this compile successfully?

It looks like sdev is explicitly typed as &Device<device::CoreInternal<'_>>
in probe_callback, but the newly introduced OpenGuard struct expects
sdev: &'bound Device<device::Bound>.

Since Rust struct field initialization does not perform auto-deref coercions,
OpenGuard { sdev } seems to cause a type mismatch compilation error.

Would it be necessary to explicitly dereference it, like
OpenGuard { sdev: &**sdev }?

> +                },
> +                driver <- T::probe(sdev, info),
>              }))?;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920-rust_serdev_probe_refactor-v2-0-43b855162f5d@posteo.de?part=3