Re: [PATCH 8/8] rust: add device::Data

[Danilo Krummrich <dakr@redhat.com>]

On Mon, Mar 25, 2024 at 07:21:04PM +0100, Greg KH wrote:
> On Mon, Mar 25, 2024 at 06:49:12PM +0100, Danilo Krummrich wrote:
> > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > 
> > This allows access to registrations and io resources to be automatically
> > revoked when devices are removed, even if the ref count to their state
> > is non-zero.
> 
> So it's re-implementing the devm_*() calls?  Why?

It's not. Taking IO resources as an example it's more a generalization of what
e.g. DRM solves with drm_dev_unplug() and drm_dev_{enter,exit}().

While devm_*() ensures to, for instance, iounmap() a mapping when a device is
detached, the Revocable<> resource ensures that a driver can't access the
pointer pointing to the just unmapped memory anymore.

So, this isn't something that replaces devm_*(), but kinda builds on top of it.
For instance, we could call Revocable::revoke() from a devm_*() callback.
However, and that' what this patch currently does for simplicity, we could also
just call it from the corresponding driver's remove() callback.

> 
> And this will trigger only on remove, but which remove?  The bus remove?
> Or the unbinding of the driver to the device (two totally different
> things, be specific and very careful here.)

The above comment says "device remove", maybe say device / driver detach
instead?

> 
> If this is implementing the devm_() calls, why not call them the same
> thing?
> 
> 
> > 
> > Co-developed-by: Andreas Hindborg <a.hindborg@samsung.com>
> > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > ---
> >  rust/kernel/device.rs | 120 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 119 insertions(+), 1 deletion(-)
> > 
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 7309a236f512..707c699c3090 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -4,7 +4,22 @@
> >  //!
> >  //! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
> >  
> > -use crate::{bindings, str::CStr};
> > +use macros::pin_data;
> > +
> > +use crate::{
> > +    bindings,
> > +    error::Result,
> > +    init::InPlaceInit,
> > +    init::PinInit,
> > +    pin_init,
> > +    revocable::{Revocable, RevocableGuard},
> > +    str::CStr,
> > +    sync::{LockClassKey, RevocableMutex, UniqueArc},
> > +};
> > +use core::{
> > +    ops::{Deref, DerefMut},
> > +    pin::Pin,
> > +};
> >  
> >  /// A raw device.
> >  ///
> > @@ -95,3 +110,106 @@ fn clone(&self) -> Self {
> >          Self::from_dev(self)
> >      }
> >  }
> > +
> > +/// Device data.
> > +///
> > +/// When a device is removed (for whatever reason, for example, because the device was unplugged or
> > +/// because the user decided to unbind the driver), the driver is given a chance to clean its state
> > +/// up, and all io resources should ideally not be used anymore.
> 
> Wait, unplugging a device and unbinding a device from a driver are two
> totally different things, do NOT get them mixed up or assume that they
> are the same thing at all please.  They have different lifetime rules
> and different patterns of what happens.

I think the comment does not claim that device unplug and device / driver
unbind are the same thing. I rather think in this context the expection is that,
ultimately, both result into the fact that the corresponding device resources
are not available anymore and hence shouldn't be used anymore. Where the latter
is enforced by using Revocable<>.

> 
> So this needs to be taken out and rewritten from the beginning please.
> If the comments describe something that is incorrect, I can't trust that
> the code is correct...

Not sure the comment is actually claiming something incorrect. If, with the
above explanation, you still think so, please let me know how to phrase it
correctly, such that I can improve this patch accordingly.

> 
> thanks,
> 
> greg k-h
>