Device::parent() returns a reference to the device' parent device, if
any.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/device.rs | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 21b343a1dc4d..f6bdc2646028 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -65,6 +65,21 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
         self.0.get()
     }
 
+    /// Returns a reference to the parent device, if any.
+    pub fn parent<'a>(&self) -> Option<&'a Self> {
+        // SAFETY:
+        // - By the type invariant `self.as_raw()` is always valid.
+        // - The parent device is only ever set at device creation.
+        let parent = unsafe { (*self.as_raw()).parent };
+
+        if parent.is_null() {
+            None
+        } else {
+            // SAFETY: Since `parent` is not NULL, it must be a valid pointer to a `struct device`.
+            Some(unsafe { Self::as_ref(parent) })
+        }
+    }
+
     /// Convert a raw C `struct device` pointer to a `&'a Device`.
     ///
     /// # Safety
-- 
2.48.1

On Thu, Mar 20, 2025 at 11:27:43PM +0100, Danilo Krummrich wrote:
> Device::parent() returns a reference to the device' parent device, if
> any.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/device.rs | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 21b343a1dc4d..f6bdc2646028 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -65,6 +65,21 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
>          self.0.get()
>      }
>  
> +    /// Returns a reference to the parent device, if any.
> +    pub fn parent<'a>(&self) -> Option<&'a Self> {
> +        // SAFETY:
> +        // - By the type invariant `self.as_raw()` is always valid.
> +        // - The parent device is only ever set at device creation.
> +        let parent = unsafe { (*self.as_raw()).parent };
> +
> +        if parent.is_null() {
> +            None
> +        } else {
> +            // SAFETY: Since `parent` is not NULL, it must be a valid pointer to a `struct device`.
> +            Some(unsafe { Self::as_ref(parent) })

The safety comment also needs to explain why the parent device won't be
gone, I assume a struct device holds a refcount of its parent? Therefore
the borrow checker would ensure the parent exists as long as the Device
is borrowed.

Regards,
Boqun

> +        }
> +    }
> +
>      /// Convert a raw C `struct device` pointer to a `&'a Device`.
>      ///
>      /// # Safety
> -- 
> 2.48.1
>

On Fri, Mar 21, 2025 at 07:40:57AM -0700, Boqun Feng wrote:
> On Thu, Mar 20, 2025 at 11:27:43PM +0100, Danilo Krummrich wrote:
> > Device::parent() returns a reference to the device' parent device, if
> > any.
> > 
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  rust/kernel/device.rs | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 21b343a1dc4d..f6bdc2646028 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -65,6 +65,21 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
> >          self.0.get()
> >      }
> >  
> > +    /// Returns a reference to the parent device, if any.
> > +    pub fn parent<'a>(&self) -> Option<&'a Self> {
> > +        // SAFETY:
> > +        // - By the type invariant `self.as_raw()` is always valid.
> > +        // - The parent device is only ever set at device creation.
> > +        let parent = unsafe { (*self.as_raw()).parent };
> > +
> > +        if parent.is_null() {
> > +            None
> > +        } else {
> > +            // SAFETY: Since `parent` is not NULL, it must be a valid pointer to a `struct device`.
> > +            Some(unsafe { Self::as_ref(parent) })
> 
> The safety comment also needs to explain why the parent device won't be
> gone, I assume a struct device holds a refcount of its parent?

Correct, this is taken generically in device_add().

> Therefore
> the borrow checker would ensure the parent exists as long as the Device
> is borrowed.

Yes.

On Thu, Mar 20, 2025 at 11:27:43PM +0100, Danilo Krummrich wrote:
> Device::parent() returns a reference to the device' parent device, if
> any.

Ok, but why?  You don't use it in this series, or did I miss it?

A driver shouldn't care about the parent of a device, as it shouldn't
really know what it is.  So what is this needed for?

thanks,

greg k-h

On Thu, Mar 20, 2025 at 06:40:56PM -0700, Greg KH wrote:
> On Thu, Mar 20, 2025 at 11:27:43PM +0100, Danilo Krummrich wrote:
> > Device::parent() returns a reference to the device' parent device, if
> > any.
> 
> Ok, but why?  You don't use it in this series, or did I miss it?

Indeed, it should rather be at the auxbus series.

> A driver shouldn't care about the parent of a device, as it shouldn't
> really know what it is.  So what is this needed for?

Generally, that's true. I use in the auxbus example and in nova-drm [1] (which
is connected through the auxbus to nova-core) to gather some information about
the device managed by nova-core.

Later on, since that's surely not enough, we'll have an interface to nova-core
that takes the corresponding auxiliary device. nova-core can then check whether
the given device originates from nova-core, and through the parent find its own
device.

[1] https://gitlab.freedesktop.org/drm/nova/-/blob/staging/nova-drm/drivers/gpu/drm/nova/driver.rs

On Fri, Mar 21, 2025 at 10:04:26AM +0100, Danilo Krummrich wrote:
> On Thu, Mar 20, 2025 at 06:40:56PM -0700, Greg KH wrote:
> > On Thu, Mar 20, 2025 at 11:27:43PM +0100, Danilo Krummrich wrote:
> > > Device::parent() returns a reference to the device' parent device, if
> > > any.
> > 
> > Ok, but why?  You don't use it in this series, or did I miss it?
> 
> Indeed, it should rather be at the auxbus series.
> 
> > A driver shouldn't care about the parent of a device, as it shouldn't
> > really know what it is.  So what is this needed for?
> 
> Generally, that's true. I use in the auxbus example and in nova-drm [1] (which
> is connected through the auxbus to nova-core) to gather some information about
> the device managed by nova-core.
> 
> Later on, since that's surely not enough, we'll have an interface to nova-core
> that takes the corresponding auxiliary device. nova-core can then check whether
> the given device originates from nova-core, and through the parent find its own
> device.
> 
> [1] https://gitlab.freedesktop.org/drm/nova/-/blob/staging/nova-drm/drivers/gpu/drm/nova/driver.rs

Another category of drivers that came to my mind and seems valid for this is
MFD.

Other than that I found a couple of cases where platform drivers interact with
their corresponding parent devices (mostly embedded platforms where the topology
is known), as well as a couple of HID devices that access their parent to issue
USB transactions etc., which all seems more like an abuse due to a lack of
proper APIs, which may or may not exist at the time the corresponding driver was
written.

So, maybe we should make Device::parent() crate private instead, such that it
can't be accessed by drivers, but only the core abstractions and instead only
provide accessors for the parent device for specific bus devices, where this is
reasonable to be used by drivers, e.g. auxiliary.

On Fri, Mar 21, 2025 at 02:03:30PM +0100, Danilo Krummrich wrote:
> So, maybe we should make Device::parent() crate private instead, such that it
> can't be accessed by drivers, but only the core abstractions and instead only
> provide accessors for the parent device for specific bus devices, where this is
> reasonable to be used by drivers, e.g. auxiliary.
> 

That sounds reasonable, thanks!

greg k-h

On Fri, Mar 21, 2025 at 06:09:06AM -0700, Greg KH wrote:
> On Fri, Mar 21, 2025 at 02:03:30PM +0100, Danilo Krummrich wrote:
> > So, maybe we should make Device::parent() crate private instead, such that it
> > can't be accessed by drivers, but only the core abstractions and instead only
> > provide accessors for the parent device for specific bus devices, where this is
> > reasonable to be used by drivers, e.g. auxiliary.
> > 
> 
> That sounds reasonable, thanks!

Cool, I will drop the patch from this series then, since when being crate
private it needs an #[expect(unused)] until actually being used. So, I rather
add it to the auxbus series.

On Thu Mar 20, 2025 at 11:27 PM CET, Danilo Krummrich wrote:
> Device::parent() returns a reference to the device' parent device, if
> any.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/device.rs | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 21b343a1dc4d..f6bdc2646028 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -65,6 +65,21 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
>          self.0.get()
>      }
>  
> +    /// Returns a reference to the parent device, if any.
> +    pub fn parent<'a>(&self) -> Option<&'a Self> {
> +        // SAFETY:
> +        // - By the type invariant `self.as_raw()` is always valid.
> +        // - The parent device is only ever set at device creation.
> +        let parent = unsafe { (*self.as_raw()).parent };
> +
> +        if parent.is_null() {
> +            None
> +        } else {
> +            // SAFETY: Since `parent` is not NULL, it must be a valid pointer to a `struct device`.
> +            Some(unsafe { Self::as_ref(parent) })

Why is this valid for `'static`? Since you declare the lifetime `'a`
independently from the elided one on `&self`, the user can set it to
`'static`.

---
Cheers,
Benno

> +        }
> +    }
> +
>      /// Convert a raw C `struct device` pointer to a `&'a Device`.
>      ///
>      /// # Safety

On Thu, Mar 20, 2025 at 10:44:38PM +0000, Benno Lossin wrote:
> On Thu Mar 20, 2025 at 11:27 PM CET, Danilo Krummrich wrote:
> > Device::parent() returns a reference to the device' parent device, if
> > any.
> >
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  rust/kernel/device.rs | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 21b343a1dc4d..f6bdc2646028 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -65,6 +65,21 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
> >          self.0.get()
> >      }
> >  
> > +    /// Returns a reference to the parent device, if any.
> > +    pub fn parent<'a>(&self) -> Option<&'a Self> {
> > +        // SAFETY:
> > +        // - By the type invariant `self.as_raw()` is always valid.
> > +        // - The parent device is only ever set at device creation.
> > +        let parent = unsafe { (*self.as_raw()).parent };
> > +
> > +        if parent.is_null() {
> > +            None
> > +        } else {
> > +            // SAFETY: Since `parent` is not NULL, it must be a valid pointer to a `struct device`.
> > +            Some(unsafe { Self::as_ref(parent) })
> 
> Why is this valid for `'static`? Since you declare the lifetime `'a`
> independently from the elided one on `&self`, the user can set it to
> `'static`.

Good catch -- this is indeed a problem when the &Device comes from an
ARef<Device>, rather than Device::as_ref(), which is what I had in mind
originally.