| Message ID | 20250320222823.16509-2-dakr@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Implement TryFrom<&Device> for bus specific devices | expand |
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.
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: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.
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