Re: [PATCH 3/8] rust: device: Add a stub abstraction for devices

[Boqun Feng <boqun.feng@gmail.com>]

On Tue, Mar 26, 2024 at 07:03:47PM +0100, Greg KH wrote:
[...]
> > > > +impl Clone for Device {
> > > > +    fn clone(&self) -> Self {
> > > > +        Self::from_dev(self)
> > > 
> > > Does this increment the reference count?
> > 
> > Yes, it does. from_dev() calls new() and new() calls get_device() again.
> 
> Ok, but why would you ever allow a device structure to be cloned?

This function actually doesn't clone the device structure, it rather
just clones the "smart pointer" to the device. But it's confusing since
it should use an extra layer of abstraction, as I mentioned:

	https://lore.kernel.org/rust-for-linux/ZgG7TlybSa00cuoy@boqun-archlinux/

Right now `Device` is mapped to `struct device *`, which mixes the
abstraction of a device struct and a pointer to a device struct. We
should do the abstraciton in a different way:

	// `Device` maps to `struct device`.
	#[repr(transparent)]
	struct Devcie(Opaque<bindings::device>);

and then having a `&Device` means you have a reference to the `Device`,
you can use it for any device related options except increase/descrease
the refcount of a device. For example:

	unsafe impl RawDevice for Device {
	    fn raw_device(&self) -> *mut bindings::device {
	        self.0.get()
	    }
	}

Driver code that only requires the existence of the device should only
use `&Device`.

And for the code that want to manage the lifetime of a device, since
`struct device` has a refcount in it, we can claim that `Device` is a
`AlwaysRefCounted` type:

	impl AlwaysRefCounted for Device {
	    fn inc_ref(&self) {
	        get_device(self.0.get());
	    }

	    unsafe fn dec_ref(obj: NonNull<Self>) {
	        put_device(obj.as_mut_ptr());
	    }
	}

with that, we have a type `ARef<Device>`, which is similar to `&Device`
in that it's a pointer type, but you can clone (inc refcount) and drop
(dec refcount) on it. We can still define a `Device::new` but the return
type is `ARef<Device>`, meaning we hold a reference to the device.

	impl Device {
	    pub unsafe fn new(ptr: NonNull<bindingds::device>) -> ARef<Device> {
	        unsafe { ARef::from_raw(ptr) }.clone()
	    }
	}

In this way, if Rust code is not the owner of the device (probably it's
the common case right now), Rust code can use a reference to make sure
it doesn't get freed by the owner. And of course, if it's a driver
callback where the driver guarantees the liveness of the device, the
Rust code should take a `&Device` and don't worry about the ownership
anymore.

Does this make sense?

Regards,
Boqun

> That's not something that should ever happen, once it is created, it is
> the ONLY instance of that structure around, and it can't be changed, as
> you passed it off to be handled by the driver core, and the driver core
> is the one that handles the lifetime of the object, NOT the bus or the
> driver that happens to bind to it.
> 
> I've been worried about how the interaction between the driver core and
> rust code is going to handle structure lifetime rules.  You need to be
> very careful here with what you do.  Rust code can create these
> structures, but once they are passed to the driver core, you lose the
> ability to control them and have to allow the driver core to manage
> them, as that's its job, not the rust code.
> 
> thanks,
> 
> greg k-h