linux-kernel - Re: [PATCH 8/9] regulator: add driver for ncv6336 regulator

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: <32b34f2d-21eb-48e6-9383-f2090ad05900@sirena.org.uk>
Date: Fri, 20 Dec 2024 14:50:29 +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 8/9] regulator: add driver for ncv6336 regulator

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

> +regmap::define_regmap_field_descs!(FIELD_DESCS, {
> +    (pid, 0x3, READ, { value => raw([7:0], ro) }),

Looking at this it appears that the whole thing with only exposing
regmap fields is actually a deliberate attempt to elide whole register
access.  This doesn't seem like a good idea, it's quite common to want
to operate on a register as a whole.  For example there's the very
common case where we're updating multiple fields at once, we don't want
to do a separate read/modify/write cycle for each field.

As I mentioned in review of the regmap patch I'd expect that fields
would be layered on the underlying register access interface, and I'd
expect that we'd always be able to work with the registers.

> +static NCV6336_DESC: Desc = Desc::new::<Ncv6336>(c_str!("ncv6336"), Type::Voltage)
> +    .with_owner(&THIS_MODULE)
> +    .with_of_match(c_str!("buck"))
> +    .with_active_discharge(
> +        pgood::addr(),
> +        pgood::dischg::mask(),
> +        pgood::dischg::mask(),
> +        0,
> +    )

I'm not sure these sequences of unnamed arguments are great for
legibility.

> +#[vtable]
> +impl Driver for Ncv6336 {
> +    type Data = Arc<Mutex<Ncv6336RegulatorData>>;
> +
> +    fn list_voltage(reg: &mut Device<Self::Data>, selector: u32) -> Result<i32> {
> +        reg.list_voltage_linear(selector)
> +    }
> +
> +    fn enable(reg: &mut Device<Self::Data>) -> Result {
> +        reg.enable_regmap()
> +    }
> +
> +    fn disable(reg: &mut Device<Self::Data>) -> Result {
> +        reg.disable_regmap()
> +    }
> +
> +    fn is_enabled(reg: &mut Device<Self::Data>) -> Result<bool> {
> +        reg.is_enabled_regmap()
> +    }

I can't help but feel that this isn't a great way to deal with the ops
table - AIUI we're mapping the C functions to rust, then bouncing those
wrapped functions back to the C API.  All of this with no abstraction of
the whole fact that this is a data driven way of specifying the regmap.
It feels like we're loosing a lot of the point of having these data
driven helpers, for most devices where we're data driven it looks like
it would be actively worse to write in rust.

As with the regmap fields I'd really expect to see this handled in a
much more directly rust native manner for drivers, in this case I'd
expect that to wind up with us reusing the C ops but handled by the
support code rather than directly by each driver.  Drivers doing
complicated things might want to peer in and work with the ops (eg, some
devices need a special write sequence but the read side uses helpers)
but it shouldn't be the default.

> +    fn get_status(reg: &mut Device<Self::Data>) -> Result<Status> {
> +        if !Self::is_enabled(reg)? {
> +            return Ok(Status::Off);
> +        }
> +
> +        Ok(Self::get_mode(reg).into())
> +    }

This is buggy, it's just returning the values configured by the driver
not status from the hardware.  If the hardware doesn't report status
don't implement this operation.

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