Re: [RFC PATCH V3 1/8] rust: Add initial bindings for OPP framework

[Boqun Feng]

Hi Viresh,

On Wed, Jul 03, 2024 at 12:44:26PM +0530, Viresh Kumar wrote:
> This commit adds initial Rust bindings for the Operating performance
> points (OPP) core. This adds bindings for `struct dev_pm_opp` and
> `struct dev_pm_opp_data` to begin with.
> 
> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@xxxxxxxxxx>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
[...]
> +
> +/// Operating performance point (OPP).
> +///
> +/// # Invariants
> +///
> +/// The pointer stored in `Self` is non-null and valid for the lifetime of the ARef instance. In
> +/// particular, the ARef instance owns an increment on underlying object´s reference count.

Since you use `ARef` pattern now, you may want to rewrite this
"invariants".

> +#[repr(transparent)]
> +pub struct OPP(Opaque<bindings::dev_pm_opp>);
> +
> +// SAFETY: `OPP` only holds a pointer to a C OPP, which is safe to be used from any thread.
> +unsafe impl Send for OPP {}
> +
> +// SAFETY: `OPP` only holds a pointer to a C OPP, references to which are safe to be used from any
> +// thread.
> +unsafe impl Sync for OPP {}
> +

Same for the above safety comments, as they are still based on the old
implementation.

> +// SAFETY: The type invariants guarantee that [`OPP`] is always refcounted.
> +unsafe impl AlwaysRefCounted for OPP {
> +    fn inc_ref(&self) {
> +        // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> +        unsafe { bindings::dev_pm_opp_get(self.0.get()) };
> +    }
> +
> +    unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
> +        unsafe { bindings::dev_pm_opp_put(obj.cast().as_ptr()) }
> +    }
> +}
> +
> +impl OPP {
[...]
> +
> +impl Drop for OPP {

I don't think you need the `drop` implementation here, since it should
be already handled by `impl AlwaysRefCounted`, could you try to a doc
test for this? Something like:

	let opp: ARef<OPP> = <from a raw dev_pm_opp ponter whose refcount is 1>
	drop(opp);

IIUC, this will result double-free with the current implementation.

Overall, `OPP` is now representing to the actual device instead of the
pointer to the device, so the `drop` function won't need to handle the
refcounting.

Regards,
Boqun

> +    fn drop(&mut self) {
> +        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
> +        unsafe { bindings::dev_pm_opp_put(self.as_raw()) }
> +    }
> +}
> -- 
> 2.31.1.272.g89b43f80a514
>