Re: [PATCH 2/2] rust/faux: Add missing parent argument to Registration::new()

[Danilo Krummrich <dakr@kernel.org>]

On Wed, Feb 26, 2025 at 09:23:39AM +0000, Alice Ryhl wrote:
> On Wed, Feb 26, 2025 at 10:06 AM <kernel@dakr.org> wrote:
> >
> > On 2025-02-26 09:38, Alice Ryhl wrote:
> > > On Tue, Feb 25, 2025 at 10:31 PM Lyude Paul <lyude@redhat.com> wrote:
> > >>
> > >> A little late in the review of the faux device interface, we added the
> > >> ability to specify a parent device when creating new faux devices -
> > >> but
> > >> this never got ported over to the rust bindings. So, let's add the
> > >> missing
> > >> argument now so we don't have to convert other users later down the
> > >> line.
> > >>
> > >> Signed-off-by: Lyude Paul <lyude@redhat.com>
> > >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >> ---
> > >>  rust/kernel/faux.rs              | 10 ++++++++--
> > >>  samples/rust/rust_driver_faux.rs |  2 +-
> > >>  2 files changed, 9 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/rust/kernel/faux.rs b/rust/kernel/faux.rs
> > >> index 41751403cd868..ae99ea3d114ef 100644
> > >> --- a/rust/kernel/faux.rs
> > >> +++ b/rust/kernel/faux.rs
> > >> @@ -23,11 +23,17 @@
> > >>
> > >>  impl Registration {
> > >>      /// Create and register a new faux device with the given name.
> > >> -    pub fn new(name: &CStr) -> Result<Self> {
> > >> +    pub fn new(name: &CStr, parent: Option<&device::Device>) ->
> > >> Result<Self> {
> > >>          // SAFETY:
> > >>          // - `name` is copied by this function into its own storage
> > >>          // - `faux_ops` is safe to leave NULL according to the C API
> > >> -        let dev = unsafe {
> > >> bindings::faux_device_create(name.as_char_ptr(), null_mut(), null())
> > >> };
> > >> +        let dev = unsafe {
> > >> +            bindings::faux_device_create(
> > >> +                name.as_char_ptr(),
> > >> +                parent.map_or(null_mut(), |p| p.as_raw()),
> > >> +                null(),
> > >
> > > This function signature only requires that `parent` is valid for the
> > > duration of this call to `new`, but `faux_device_create` stashes a
> > > pointer without touching the refcount. How do you ensure that the
> > > `parent` pointer does not become dangling?
> >
> > I was wondering the same, but it seems that the subsequent device_add()
> > call takes care of that:
> >
> > https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/base/core.c#L3588
> >
> > device_del() drops the reference.
> >
> > This makes device->parent only valid for the duration between
> > faux_device_create() and faux_device_remove().
> >
> > But this detail shouldn’t be relevant for this API.
> 
> I think this could use a few more comments to explain it. E.g.:
> 
> diff --git a/drivers/base/faux.c b/drivers/base/faux.c
> index 531e9d789ee0..674db8863d96 100644
> --- a/drivers/base/faux.c
> +++ b/drivers/base/faux.c
> @@ -131,6 +131,7 @@ struct faux_device *faux_device_create_with_groups(const char *name,
>  
>         device_initialize(dev);
>         dev->release = faux_device_release;
> +       /* The refcount of dev->parent is incremented in device_add. */

Yeah, this one is a bit odd to rely on a subsequent device_add() call, it
clearly deserves a comment.

>         if (parent)
>                 dev->parent = parent;
>         else
> diff --git a/rust/kernel/faux.rs b/rust/kernel/faux.rs
> index 7673501ebe37..713ee6842e3f 100644
> --- a/rust/kernel/faux.rs
> +++ b/rust/kernel/faux.rs
> @@ -28,6 +28,7 @@ pub fn new(name: &CStr, parent: Option<&device::Device>) -> Result<Self> {
>          // SAFETY:
>          // - `name` is copied by this function into its own storage
>          // - `faux_ops` is safe to leave NULL according to the C API
> +        // - `faux_device_create` ensures that `parent` stays alive until `faux_device_destroy`.

Not sure that's a safety requirement for faux_device_create().

The typical convention is that a caller must hold a reference to the object
behind the pointer when passing it to another function. If the callee decides
to store the pointer elsewhere, it's on the callee to take an additional
reference.

I think if we want to add something to the safety comment, it should be somthing
along the line of "the type of `parent` implies that for the duration of this
call `parent` is a valid device with a non-zero reference count".

>          let dev = unsafe {
>              bindings::faux_device_create(
>                  name.as_char_ptr(),
>