Re: [PATCH v15 4/6] rust: faux: Allow retrieving a bound Device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added `Registration` invariant lacks an `// INVARIANT:` justification at construction, contains a semantic typo, and the corresponding `// SAFETY:` comment in `as_ref()` was not updated to justify the stricter `Device<Bound>` context.
--

commit 17a5a14d96ef7eb6b2fce7469b897ce1072bac56
Author: Lyude Paul <lyude@redhat.com>

rust: faux: Allow retrieving a bound Device

This commit updates faux::Registration to implement AsRef for the stricter
Device<Bound> context rather than the default Device. It also adds a new
invariant to document that Registration is proof of the bound state.

> diff --git a/rust/kernel/faux.rs b/rust/kernel/faux.rs
> index 43b4974f48cd2..e0856b2964a2c 100644
> --- a/rust/kernel/faux.rs
> +++ b/rust/kernel/faux.rs
> @@ -25,7 +25,8 @@
>  ///
>  /// # Invariants
>  ///
> -/// `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`].
> +/// - `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`].
> +/// - This object is proof that the object described by this `Registration` is bound to a device.

[Severity: Medium]
This isn't a bug, but there appears to be a semantic typo in the invariant 
wording. The documentation states "bound to a device", but the Registration
object represents the device itself. Could this be corrected to "bound to a
driver" to match the commit message?

Also, since a new invariant was added to Registration, does
Registration::new() need an updated // INVARIANT: comment to explain why
this new invariant is satisfied at construction?

According to the Rust subsystem guidelines, when a struct has an Invariants
section, its construction site should document why the invariants hold:

rust/kernel/faux.rs:Registration::new() {
    ...
    // INVARIANT: The device will remain registered until faux_device_destroy()
    // is called, which happens in our Drop implementation.
    Self(ptr)
}

>  ///
>  /// [`struct faux_device`]: srctree/include/linux/device/faux.h
>  pub struct Registration(NonNull<bindings::faux_device>);

[ ... ]

> @@ -59,8 +60,8 @@ fn as_raw(&self) -> *mut bindings::faux_device {
>      }
>  }
>  
> -impl AsRef<device::Device> for Registration {
> -    fn as_ref(&self) -> &device::Device {
> +impl AsRef<device::Device<device::Bound>> for Registration {
> +    fn as_ref(&self) -> &device::Device<device::Bound> {
>          // SAFETY: The underlying `device` in `faux_device` is guaranteed by the C API to be
>          // a valid initialized `device`.
>          unsafe { device::Device::from_raw(addr_of_mut!((*self.as_raw()).dev)) }

[Severity: Medium]
Does the // SAFETY: comment here need to be updated to justify why returning
the stricter Device<Bound> context is valid?

The current comment justifies instantiating a Device from a raw pointer, but
does not explain how the caller is guaranteed that the device is actually in
the Bound state, which the device abstraction delegates to the caller.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529183702.677677-1-lyude@redhat.com?part=4