linux-kernel - Re: [PATCH 2/9] rust: add abstraction for regmap

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 for Android: free password hash cracker in your pocket

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

Message-ID: <8b85fbfa-ffb5-48d0-ba7b-0e26001ab3ae@sirena.org.uk>
Date: Fri, 20 Dec 2024 13:16:21 +0000
From: Mark Brown <broonie@...nel.org>
To: Fabien Parent <parent.f@...il.com>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.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>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Wolfram Sang <wsa+renesas@...g-engineering.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konradybcio@...nel.org>, devicetree@...r.kernel.org,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-i2c@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	vinod.koul@...aro.org, Fabien Parent <fabien.parent@...aro.org>
Subject: Re: [PATCH 2/9] rust: add abstraction for regmap

On Wed, Dec 18, 2024 at 03:36:32PM -0800, Fabien Parent wrote:

> The abstraction is bringing only a small subset of all the features
> provided by regmap by only supporting the most vital field from
> `struct regmap_config`.

I'm not so sure about that...

> +int rust_helper_regmap_field_write(struct regmap_field *field, unsigned int val)
> +{
> +	return regmap_field_write(field, val);
> +}

...this is wrapping the field API.  That's not very widely used at all,
and all the usages are in leaf drivers rather than frameworks.  Given
this and that the code is basically simple syntactic sugar rather than
any substantial logic I would suggest not wrapping this for Rust but
instead writing native Rust abstractions that do the same thing.  That
seems likely to be less work and give something that is more pleasant to
use in the Rust environment.

Indeed, it looks like the actual core regmap APIs aren't wrapped at all,
only the field ones?

> +//! ```ignore
> +//! regmap::define_regmap_field_descs!(FIELD_DESCS, {
> +//!     (pid, 0x3, READ, { value => raw([7:0], ro) }),
> +//!     (limconf, 0x16, RW, {
> +//!         rearm     => bit(0, rw),
> +//!         rststatus => bit(1, rw),
> +//!         tpwth     => enum([5:4], rw, {
> +//!             Temp83C  = 0x0,
> +//!             Temp94C  = 0x1,
> +//!             Temp105C  = 0x2,
> +//!             Temp116C  = 0x3,
> +//!         }),
> +//!     })
> +//! });

You might want to include some explanation as to what this does because
it's quite unclear.

> +//! fn probe(client: &mut i2c::Client) -> Result {
> +//!     let config = regmap::Config::<AccessOps>::new(8, 8)
> +//!         .with_max_register(0x16)
> +//!         .with_cache_type(regmap::CacheType::RbTree);

New code should use maple tree unless it's got a good reason.

> +    /// Use RbTree caching
> +    RbTree = bindings::regcache_type_REGCACHE_RBTREE,
> +    /// Use Flat caching
> +    Flat = bindings::regcache_type_REGCACHE_FLAT,
> +    /// Use Maple caching
> +    Maple = bindings::regcache_type_REGCACHE_MAPLE,

Maple Tree.

> +impl Regmap {
> +    #[cfg(CONFIG_REGMAP_I2C = "y")]

Built in only?

> +/// `raw` should be used when bits cannot be represented by any other field types. It provides
> +/// raw access to the register bits.
> +///
> +/// # Syntax
> +///
> +/// `raw(bits_range, access)`

Given how many register fields are just a number I'm not sure calling
them "raw" fields really conveys the right message, it sounds like this
is bypassing things that should be there rather than a perfectly normal
thing to do.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)