linux-kernel - Re: [PATCH v2 2/5] rust: leds: Add Rust bindings for LED subsystem

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72mSTBC0=kdNxqRvZR+MWwGwO_7yO+nMTvkp4LSNU9HAZA@mail.gmail.com>
Date: Mon, 13 Jan 2025 14:10:48 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Fiona Behrens <me@...enk.dev>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Pavel Machek <pavel@....cz>, 
	Lee Jones <lee@...nel.org>, Jean Delvare <jdelvare@...e.com>, Boqun Feng <boqun.feng@...il.com>, 
	Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Mark Pearson <mpearson-lenovo@...ebb.ca>, Peter Koch <pkoch@...ovo.com>, 
	rust-for-linux@...r.kernel.org, linux-leds@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/5] rust: leds: Add Rust bindings for LED subsystem

Hi Fiona,

Thanks for this! I noticed a procedural thing (the last diff chunk,
please see below), so I took the chance to give a few quick
surface-level comments I noticed while scrolling, mostly on docs :)

On Mon, Jan 13, 2025 at 1:16 PM Fiona Behrens <me@...enk.dev> wrote:
>
> +    /// Get String representation of the Color.

[`String`], [`Color`]

i.e. please format, and use intra-doc links where possible/reasonable.

> +        // SAFETY: pointer from above, valid for static lifetime.

In general, try to explain why, not just what, in the safety comments,
i.e. why it is valid for a static lifetime? e.g. does the C API
guarantee it?

> +    /// Get String representation of the Color as rust type.
> +    #[inline]
> +    pub fn name(&self) -> Option<&'static str> {

"Rust type"

However, I am not sure what it is trying to say. I would have thought
it is a custom newtype of something or perhaps something strange is
going on, but I guess you are referring to `str` vs. `CStr`? In
general, I don't think we need to say "as a Rust type" -- it may
confuse more than help.

> +                // SAFETY: name from C name array which is valid UTF-8.

What guarantees this? i.e. I imagine you looked into all the cases
from the static table, so I would for instance say: "All values in the
C name array are valid UTF-8." or similar (perhaps mention which
array, so that people can e.g. grep).

> +impl core::fmt::Display for Color {
> +    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> +        f.write_str(self.name().unwrap_or("Invalid color"))
> +    }
> +}

Can this happen? If not, should `Color` have an `# Invariant` and
would we need the `Option<...>` for names then?

In any case, here, shouldn't the error be bubbled up?

> +    /// Tris to convert a u32 into a [`Color`].

Typo: Tries
Format: [`u32`]

There are also other typos ("brightnes", "Activae") -- I recommend
`checkpatch.pl` with the `--codespell` flag which may help catching
some of these.

> +    /// Get the LED brightness level.
> +    fn brightness_get(_this: &mut Self::This) -> u8 {
> +        crate::build_error(crate::error::VTABLE_DEFAULT_ERROR)
> +    }

Please use the macro instead (see 15f2f9313a39 ("rust: use the
`build_error!` macro, not the hidden function")). Soon we will have it
in the prelude too (see 4401565fe92b ("rust: add `build_error!` to the
prelude")), by the way.

> +        // SAFETY: By the safety requirement of this function `delay_on` is pointing to a valid `c_ulong`.
> +        let on = Delta::from_millis(unsafe { *delay_on } as i64);

I didn't look into most comments, but e.g. I noticed this one does not
look correct -- the safety requirements for this function do not
mention anything about `delay_on`, no?

> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 38da79925586..3c348ce4a7c2 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -143,4 +143,11 @@ pub fn as_nanos(self) -> i64 {
>      pub fn as_micros_ceil(self) -> i64 {
>          self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
>      }
> +
> +    /// Return the smallest number of milliseconds greater than or equal
> +    /// to the value in the `Delta`.
> +    #[inline]
> +    pub fn as_millis_ceil(self) -> i64 {
> +        self.as_nanos().saturating_add(NSEC_PER_MSEC - 1) / NSEC_PER_MSEC
> +    }

This should probably be its own patch, with Cc to the timekeeping
maintainers etc.

Cheers,
Miguel