| Message ID | 20250313021550.133041-3-dakr@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Improve soundness of bus device abstractions | expand |
On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote: > Some bus device functions should only be called from bus callbacks, > such as probe(), remove(), resume(), suspend(), etc. > > To ensure this add device context marker structs, that can be used as > generics for bus device implementations. > > Suggested-by: Benno Lossin <benno.lossin@proton.me> > Signed-off-by: Danilo Krummrich <dakr@kernel.org> I would have folded this into #3, but if you prefer it being split, then it's also fine. > --- > rust/kernel/device.rs | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs > index db2d9658ba47..39793740a95c 100644 > --- a/rust/kernel/device.rs > +++ b/rust/kernel/device.rs > @@ -209,6 +209,24 @@ unsafe impl Send for Device {} > // synchronization in `struct device`. > unsafe impl Sync for Device {} > > +/// Marker trait for the context of a bus specific device. > +/// > +/// Some functions of a bus specific device should only be called from a certain context, i.e. bus > +/// callbacks, such as `probe()`. > +/// > +/// This is the marker trait for structures representing the context of a bus specific device. > +pub trait DeviceContext {} I would make this trait sealed. ie: pub trait DeviceContext: private::Sealed {} mod private { pub trait Sealed {} impl Sealed for super::Core {} impl Sealed for super::Normal {} } Since currently a user can create a custom context (it will be useless, but then I think it still is better to give them a compile error). If you make it sealed, Reviewed-by: Benno Lossin <benno.lossin@proton.me> --- Cheers, Benno > + > +/// The [`Normal`] context is the context of a bus specific device when it is not an argument of > +/// any bus callback. > +pub struct Normal; > +impl DeviceContext for Normal {} > + > +/// The [`Core`] context is the context of a bus specific device when it is supplied as argument of > +/// any of the bus callbacks, such as `probe()`. > +pub struct Core; > +impl DeviceContext for Core {} > + > #[doc(hidden)] > #[macro_export] > macro_rules! dev_printk {
On Thu, Mar 13, 2025 at 10:29:35AM +0000, Benno Lossin wrote: > On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote: > > +/// Marker trait for the context of a bus specific device. > > +/// > > +/// Some functions of a bus specific device should only be called from a certain context, i.e. bus > > +/// callbacks, such as `probe()`. > > +/// > > +/// This is the marker trait for structures representing the context of a bus specific device. > > +pub trait DeviceContext {} > > I would make this trait sealed. ie: > > pub trait DeviceContext: private::Sealed {} > > mod private { > pub trait Sealed {} > > impl Sealed for super::Core {} > impl Sealed for super::Normal {} > } > > Since currently a user can create a custom context (it will be useless, > but then I think it still is better to give them a compile error). That is intentional, some busses have bus specific callbacks, hence we may want a bus specific context at some point. However, we can always change visibility as needed, so I'm fine making it sealed for now. > > If you make it sealed, > > Reviewed-by: Benno Lossin <benno.lossin@proton.me>
On Thu Mar 13, 2025 at 11:29 AM CET, Benno Lossin wrote: > On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote: >> Some bus device functions should only be called from bus callbacks, >> such as probe(), remove(), resume(), suspend(), etc. >> >> To ensure this add device context marker structs, that can be used as >> generics for bus device implementations. >> >> Suggested-by: Benno Lossin <benno.lossin@proton.me> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > I would have folded this into #3, but if you prefer it being split, then > it's also fine. Ah I didn't look at patch #4, then I would also keep this separate. --- Cheers, Benno
On Thu Mar 13, 2025 at 11:41 AM CET, Danilo Krummrich wrote: > On Thu, Mar 13, 2025 at 10:29:35AM +0000, Benno Lossin wrote: >> On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote: >> > +/// Marker trait for the context of a bus specific device. >> > +/// >> > +/// Some functions of a bus specific device should only be called from a certain context, i.e. bus >> > +/// callbacks, such as `probe()`. >> > +/// >> > +/// This is the marker trait for structures representing the context of a bus specific device. >> > +pub trait DeviceContext {} >> >> I would make this trait sealed. ie: >> >> pub trait DeviceContext: private::Sealed {} >> >> mod private { >> pub trait Sealed {} >> >> impl Sealed for super::Core {} >> impl Sealed for super::Normal {} >> } >> >> Since currently a user can create a custom context (it will be useless, >> but then I think it still is better to give them a compile error). > > That is intentional, some busses have bus specific callbacks, hence we may want > a bus specific context at some point. Then it's even better we went with the generics vs using mutable references :) > However, we can always change visibility as needed, so I'm fine making it sealed > for now. Couldn't you just add them here? Then sealing wouldn't be a problem. --- Cheers, Benno
On Thu, Mar 13, 2025 at 10:52:34AM +0000, Benno Lossin wrote: > On Thu Mar 13, 2025 at 11:41 AM CET, Danilo Krummrich wrote: > > > However, we can always change visibility as needed, so I'm fine making it sealed > > for now. > > Couldn't you just add them here? Then sealing wouldn't be a problem. Yes, but I wouldn't want bus specific stuff to reside in device.rs.
On Thu Mar 13, 2025 at 3:20 PM CET, Danilo Krummrich wrote: > On Thu, Mar 13, 2025 at 10:52:34AM +0000, Benno Lossin wrote: >> On Thu Mar 13, 2025 at 11:41 AM CET, Danilo Krummrich wrote: >> >> > However, we can always change visibility as needed, so I'm fine making it sealed >> > for now. >> >> Couldn't you just add them here? Then sealing wouldn't be a problem. > > Yes, but I wouldn't want bus specific stuff to reside in device.rs. Sure, we can always make the sealed trait crate-public, since my main concern is that a driver author tries to add a new context. --- Cheers, Benno
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs index db2d9658ba47..39793740a95c 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -209,6 +209,24 @@ unsafe impl Send for Device {} // synchronization in `struct device`. unsafe impl Sync for Device {} +/// Marker trait for the context of a bus specific device. +/// +/// Some functions of a bus specific device should only be called from a certain context, i.e. bus +/// callbacks, such as `probe()`. +/// +/// This is the marker trait for structures representing the context of a bus specific device. +pub trait DeviceContext {} + +/// The [`Normal`] context is the context of a bus specific device when it is not an argument of +/// any bus callback. +pub struct Normal; +impl DeviceContext for Normal {} + +/// The [`Core`] context is the context of a bus specific device when it is supplied as argument of +/// any of the bus callbacks, such as `probe()`. +pub struct Core; +impl DeviceContext for Core {} + #[doc(hidden)] #[macro_export] macro_rules! dev_printk {
Some bus device functions should only be called from bus callbacks, such as probe(), remove(), resume(), suspend(), etc. To ensure this add device context marker structs, that can be used as generics for bus device implementations. Suggested-by: Benno Lossin <benno.lossin@proton.me> Signed-off-by: Danilo Krummrich <dakr@kernel.org> --- rust/kernel/device.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)