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

Greg KH
2025-02-26T10:30:51Z

On Wed, Feb 26, 2025 at 10:42:54AM +0100, Danilo Krummrich wrote:
> 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.

What do you mean? That's the way the driver model works, a parent
always gets the reference count incremented as it can not go away until
all of the children are gone.

So if you pass a parent pointer into a "create" function in the driver
core, it will be incremented, you don't have to worry about it.

I don't understand the issue with the rust binding here, it's not saving
a pointer to the parent device, as long as it is valid going in, that's
all that matters.

> > 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().

It's by default what the driver model does for you.

> 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.

Exactly, the driver core handles this.

> 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".

I don't understand the need for any safety comment about the parent
here. Again, as long as it is valid when the call is made, that is all
that is needed.

thanks,

greg k-h