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

[Danilo Krummrich <dakr@redhat.com>]

On Tue, Mar 26, 2024 at 07:12:29PM +0100, Greg KH wrote:
> On Tue, Mar 26, 2024 at 05:54:19PM +0100, Danilo Krummrich wrote:
> > 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}().
> 
> That is a drm specific way to handle devm_* stuff, right?

Depends, if you mean it's a DRM specific way to ensure devm_* managed resources
can't be accessed anymore after the device is "gone" from the perspective of
the driver, then yes. If you confuse it with drmm_* stuff, which ties resources
to the actual lifetime of a struct drm_device (device is freed), then no.

It's basically the DRM way to implement Revocable<>. drm_dev_unplug() marks a
struct drm_device as unplugged (protected by SRCU). drm_dev_{enter,exit}() are
used as conditional entry / exit points of code sections that access resources
that might have been released by devm_* stuff already, such as IO mappings.

> 
> If that is to be "baked" into different bus types, great, let's make it
> a bus-specific thing and put it in the driver core, but don't make it
> something that is tied to rust devices only as that feels odd, right?

You lost me a bit on this one. Can you please clarify?

> 
> Also, work with the patterns we have, don't create new ones as that just
> causes confusion for all of us working on this part of the kernel.
> 
> > 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.
> 
> Great, this is something that we have been asking for in a generic way
> for all drivers/devices in the kernel (see the many talks over the past
> years at conferences about it.)  Please work with the developers who are
> doing this in the .c side to be sure that things are aligned properly to
> work the same.

Unfortunately, I'm neither aware of those efforts, nor the talks. Mind providing
me some pointers?

> 
> > So, this isn't something that replaces devm_*(), but kinda builds on top of it.
> 
> Perhaps document it that way?

Sure, I can add this example.

> 
> > 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.
> 
> If that's the proper place for that, yes.  It could be the proper place
> is in the close() system call as well, right?  But again, document it to
> explain what this is please, as it was not obvious at all.

Correct, it depends on what you use Revocable<> for, device resources is just
one example where this is useful.

> 
> > > 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?
> 
> It depends on what you mean.  Those are different things, as explained
> in other places.  Where do you want this to happen?  Where should it
> happen?  And why in only those places?

For device resources, device / driver detach should be the correct place. Agree?

> 
> > > > +/// 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<>.
> 
> But those are different things happening, and different things happen to
> the objects when those different things happen, so while a driver might
> seem to see the same thing happen from its point of view, the lifespan
> of the object itself is VERY different here (could be passed back to the
> driver, could be freed, could be sent to a different driver, could just
> be ignored for a long time, etc.)
> 
> So be specific as to what viewpoint you are considering here, a device
> object has a lifespan that greatly exceeds that of just the window where
> a driver happens to see it :)

Fully agree, this is indeed from the viewpoint of the driver. We don't really
care what is happening to the device after detach. We only care about all the
cases above lead to a detach, which is the point where we have to make sure
we're not accessing any device resources anymore.

I will try to make this more clear.

> 
> > > 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.
> 
> Please step back and try to determine what you want here.  How devices
> are created, assigned, reassigned, removed, renamed, destroyed, etc.
> They have a well known "phase of life" and one that different parts of
> the kernel sees in different ways (driver core, busses, classes,
> drivers, userspace, etc.)  Be aware of what you are wanting to do here,
> and who is acting on the structure where and what they are wanting to do
> with it.
> 
> thanks,
> 
> greg k-h
>