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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] The patch adds a new invariant but fails to provide a corresponding `// INVARIANT:` comment in the constructor `Registration::new`.
- [Low] The newly added invariant text incorrectly states that the object is 'bound to a device'.
- [Low] The `SAFETY` comment in `as_ref` fails to address the new `Device<Bound>` guarantee.
--

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

rust: faux: Allow retrieving a bound Device

This commit updates the Registration's AsRef implementation to return a
device::Device<device::Bound> instead of device::Device. This reflects
the guarantee that the Registration object acts as proof that a driver
is bound to the device.

> 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: Low]
Is this description completely accurate? Conceptually, devices are bound
to drivers, not to devices. The commit message states "a driver is bound
to the device," so could this comment be updated to say "bound to a driver"
instead?

[Severity: Low]
Since a new invariant was added here, does Registration::new() also need a
new // INVARIANT: comment explaining why this condition holds, to match
the Rust subsystem coding guidelines?

rust/kernel/faux.rs:Registration::new() {
        ...
        // The above function will return either a valid device, or NULL on failure
        // INVARIANT: The device will remain registered until faux_device_destroy() is called, which
        // happens in our Drop implementation.
        ...
}

[ ... ]

> @@ -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`.

[Severity: Low]
Does this safety comment need to be updated to address the new Bound context?

The device::Device::from_raw() documentation requires the caller to justify
the chosen DeviceContext (Bound), ensuring the device is bound to a driver
for the duration of the reference. While faux_device_create() in C guarantees
driver binding upon success, the safety comment currently doesn't document
why this new requirement is met.

>          unsafe { device::Device::from_raw(addr_of_mut!((*self.as_raw()).dev)) }

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