| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <25f3fd337ce5e58915125c4e2eb85ef4d7af3627.camel@posteo.de>
Date: Wed, 15 Oct 2025 15:02:40 +0000
From: Markus Probst <markus.probst@...teo.de>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Alice Ryhl <aliceryhl@...gle.com>, Miguel Ojeda <ojeda@...nel.org>, Alex
Gaynor <alex.gaynor@...il.com>, Lee Jones <lee@...nel.org>, Pavel Machek
<pavel@...nel.org>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
Vlastimil Babka <vbabka@...e.cz>, "Liam R. Howlett"
<Liam.Howlett@...cle.com>, Uladzislau Rezki <urezki@...il.com>, Boqun Feng
<boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
bjorn3_gh@...tonmail.com, Benno Lossin <lossin@...nel.org>, Andreas
Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-leds@...r.kernel.org
Subject: Re: [PATCH v4 2/2] rust: leds: add basic led classdev abstractions
On Wed, 2025-10-15 at 16:52 +0200, Danilo Krummrich wrote:
> On Wed Oct 15, 2025 at 3:44 PM CEST, Markus Probst wrote:
> > On Mon, 2025-10-13 at 18:34 +0000, Alice RyhlV wrote:
> > > On Sun, Oct 12, 2025 at 02:52:39PM +0000, Markus Probst wrote:
> > > > Implement the core abstractions needed for led class devices,
> > > > including:
> > > >
> > > > * `led::LedOps` - the trait for handling leds, including
> > > > `brightness_set`, `brightness_get` and `blink_set`
> > > >
> > > > * `led::InitData` - data set for the led class device
> > > >
> > > > * `led::Device` - a safe wrapper around `led_classdev`
> > > >
> > > > Signed-off-by: Markus Probst <markus.probst@...teo.de>
> > >
> > > > +pub trait LedOps: Send + 'static + Sized {
> > > > + /// If set true, [`LedOps::brightness_set`] and
> > > > [`LedOps::blink_set`] must not sleep
> > > > + /// and perform the operation immediately.
> > > > + const BLOCKING: bool;
> > > > + /// The max brightness level
> > > > + const MAX_BRIGHTNESS: u32;
> > > > +
> > > > + /// Sets the brightness level.
> > > > + ///
> > > > + /// See also [`LedOps::BLOCKING`]
> > > > + fn brightness_set(&self, brightness: u32) -> Result<()>;
> > > > +
> > > > + /// Gets the current brightness level.
> > > > + fn brightness_get(&self) -> u32 {
> > > > + build_error!(VTABLE_DEFAULT_ERROR)
> > > > + }
> > > > +
> > > > + /// Activates hardware accelerated blinking.
> > > > + ///
> > > > + /// delays are in milliseconds. If both are zero, a
> > > > sensible
> > > > default should be chosen.
> > > > + /// The caller should adjust the timings in that case and
> > > > if
> > > > it can't match the values
> > > > + /// specified exactly. Setting the brightness to 0 will
> > > > disable the hardware accelerated
> > > > + /// blinking.
> > > > + ///
> > > > + /// See also [`LedOps::BLOCKING`]
> > > > + fn blink_set(&self, _delay_on: &mut usize, _delay_off:
> > > > &mut
> > > > usize) -> Result<()> {
> > > > + build_error!(VTABLE_DEFAULT_ERROR)
> > > > + }
> > >
> > > These functions should probably take a &Device<Bound> argument so
> > > that
> > > they can use methods that require a bound device (such as IO).
> > How about instead something like
> >
> > mod device {
> >
> > unsafe trait Container<Ctx: DeviceContext>: AsRef<Device<Ctx>> {
> > const Offset: usize;
> >
> > unsafe fn from_device(dev: &Device<Ctx>) -> &Self {
> > <implementation here>
> > }
> > }
> >
> > unsafe impl Device<Ctx> for Container<Ctx> {
> > const Offset: usize = 0;
> > }
> >
> > }
> >
> > And instead of passing &Device<Bound> to the functions, we should
> > add a
> > type parameter to LedOps, e.g.:
> >
> > trait LedOps<T: device::Container<device::Bound>> {
> >
> > ...
> >
> > fn brightness_set(&self, dev: &T, brightness: u32) -> Result<()>;
> >
> > ...
> >
> > }
> >
> > impl<T: LedOps<E>, E: device::Container<device::Bound>> Device<T> {
> >
> > pub fn new<'a>(
> > parent: &'a E,
> > init_data: InitData<'a>,
> > ops: T,
> > ) -> impl PinInit<Devres<Self>, Error> + 'a {
> > ...
> > }
> >
> > ...
> >
> > }
> >
> > In the example of i2c (or any other container for `struct device`),
> > we
> > implement the device::Container trait:
> >
> > mod i2c {
> >
> > unsafe impl device::Container for I2cClient {
> > const Offset: usize = offset_of!(bindings::i2c_client, dev);
> > }
> >
> > }
> > This allows the LedOps function to use any functions from the
> > I2cClient
> > or any other device container which may be used (removing the need
> > to
> > store it inside the LedOps implementations struct). It still allows
> > Device<Bound> to be used, as it also would implement
> > device::Container.
>
> I had a similar idea in the past, but it has some disadvantages:
>
> (1) You have to implement the upcast from a generic device to a bus
> device for
> every bus device.
Not necessarily every, like I said `container::Device` itself also
would implement `device::Container` (still allowing &Device<Bound>).
>
> (2) You have to store a Box<dyn T> in the Rust LED class device;
> the C struct
> device can't carry the fat pointer.
Why carry a fat pointer (assuming you mean pointers to unsized types)?
We already know the address of it with the `struct led_classdev`-
>`parent` field, we just need to substract the offset from `<T as
Container>::Offset`, and we have the address of the device container
(like `struct i2c_client`). No Box needed.
Thanks
- Markus Probst
>
> The alternative would be to provide a safe method for bus devices to
> upgrade to
> a bound device by presenting its corresponding &Device<Bound> base
> device, for
> instance:
>
> impl I2cClient {
> pub fn into_bound<'a>(&'a self, &'a Device<Bound>) ->
> Result<&'a I2cClient<Bound>> {
> // Fails if the presented `&Device<Bound` is not the
> base device of `self`.
> ...
> }
> }
>
> The advantage is that this can easily be implemented with a macro for
> all bus
> devices.
>
> There is a slight downside in ergonomics due to the into_bound() call
> though:
>
> fn brightness_set(&self, parent: &Device<Bound>, brightness:
> u32) -> Result {
> let i2c: &I2cClient<Bound> =
> self.i2c.into_bound(parent)?;
>
> ...
> }
Powered by blists - more mailing lists