Re: [PATCH v2] rust/kernel: Add faux device bindings

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, Feb 07, 2025 at 05:10:29PM -0500, Lyude Paul wrote:
> On Fri, 2025-02-07 at 13:16 +0100, Greg Kroah-Hartman wrote:
> > On Fri, Feb 07, 2025 at 12:42:41PM +0100, Miguel Ojeda wrote:
> > > On Fri, Feb 7, 2025 at 1:42 AM Lyude Paul <lyude@redhat.com> wrote:
> > > > 
> > > > This introduces a crate for working with faux devices in rust, along with
> > > 
> > > s/crate/module
> > > 
> > > (also in the module description)
> > > 
> > > > +//! C header: [`include/linux/device/faux.h`]
> > > > +use crate::{bindings, device, error::code::*, prelude::*};
> > > 
> > > Newline between.
> > > 
> > > > +        // SAFETY: self.0 is a valid registered faux_device via our type invariants.
> > > 
> > > Markdown.
> > > 
> > > > +// SAFETY: The faux device API is thread-safe
> > > > +unsafe impl Send for Registration {}
> > > > +
> > > > +// SAFETY: The faux device API is thread-safe
> > > > +unsafe impl Sync for Registration {}
> > > 
> > > Perhaps some extra notes here would be useful, e.g. is it documented
> > > to be so? Especially since faux is being added now, it may make sense
> > > to e.g. take the chance to work on mentioning this on the C side.
> > 
> > How can or should I mention this on the C side?
> 
> This is a very good question :), especially because it turns out I actually
> think this function is not thread-safe! Though I don't think that's actually
> much of a problem for Send/Sync here:
> 
> So - my original assumption was that since faux_device_destroy() just wraps
> around device_del() and put_device() we'd get thread safety. put_device() is
> thread-safe, but on closer inspection I don't see that device_del() is. It
> _can_ be called from any thread, but only so long as there is a guarantee it's
> called exactly once. I think that's fine both for C and rust, but it
> definitely warrants a more descriptive SAFETY comment from me.
> 
> So for the C side of things I might actually add documentation to device_del()
> for this that would look something like this:
> 
>    device_del() can be called from any thread, but provides no protection
>    against multiple calls. It is up to the caller to ensure this function may
>    only be called once for a given device.

You are correct in that device_del() has to be called only once, but the
idea of "thread safe" really isn't a thing in the kernel because we have
processes entering/exiting the code from all different places so things
better be "thread safe" almost everywhere, what matters to us as you
know is "context" due to sleep/locking issues.

So how about this wording instead:
	device_del() must be called from process context, not interrupt
	context, and also provides no protection against multiple calls
	for the same device.  It MUST be up to the caller to ensure that
	this function can only be called once for a given device.


> And then I suppose we could refer back to device_del() in faux_device_destroy()'s
> documentation if we want. For the rust side of thing the safety comment could
> just be like this:
> 
>    // SAFETY: Our bindings ensure only one `Registration` can exist at a time,
>    meaning faux_device_destroy() can only be called once for a given
>    registration - fulfilling the driver core's requirements for thread-safety.

Sure, but I think we are going to start getting "thread" confusion over
time here, is this something we want to start thinking about or am I
just running into it for the first time here?  I know it's a userspace
thing but not normally thought of in the kernel subsystems I'm familiar
with.

thanks,

greg k-h