linux-kernel - Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write protected regulators

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: <ZvJ0ICVqXrlYaVpd@finisterre.sirena.org.uk>
Date: Tue, 24 Sep 2024 10:11:12 +0200
From: Mark Brown <broonie@...nel.org>
To: Jerome Brunet <jbrunet@...libre.com>
Cc: Guenter Roeck <linux@...ck-us.net>, Liam Girdwood <lgirdwood@...il.com>,
	Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>,
	linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH 4/5] hwmon: (pmbus/core) improve handling of write
 protected regulators

On Mon, Sep 23, 2024 at 06:53:07PM +0200, Jerome Brunet wrote:
> On Mon 23 Sep 2024 at 14:21, Mark Brown <broonie@...nel.org> wrote:

> > So, I know we talked about this a bit on IRC but I didn't register the
> > specific use case.  Now I see that it's coming down to the fact that the
> > chip is simply write protected I'm wondering if it might not be simpler
> > to handle this at the ops level rather than the constraints level.  When
> > registering the driver could check for write protection and then instead
> > of registering the normal operations register an alternative set with
> > the relevant write operations removed.  That would have the same effect
> > that you're going for AFAICT.  Sorry for not thinking of it earlier.

> Actually I thaught about it, that was my first idea.

> There is tiny difference between the 2 approach:
> * A regulator that does not provide enable()/disable() is considered
> always-on, so it is not really checked if it is enabled or not
> * A regulator that does provide enable()/disable() but disallow status
> change will be checked with is_enabled()

> In the second case, we will pick up on regulator that is disabled and we
> can't enable. In the first case, I don't think we do. I don't know if it
> is a bug of not but that makes the 2nd case more correct for what we do
> with pmbus regs I think

I think for that we should just always use the is_enabled() operation if
it's available.  This is simply an oversight caused by not imagining a
case where a regulator could have an enable control which is locked out
like this, the normal case is that either you can control enable or
the regulator is always on.

> The other thing, although is more a pmbus implemantion consideration,
> is that the type regulator is opaque in
> pmbus_regulator_register. Different types could be registered so
> manipulating the ops is tricky. That's where a callback is helpful 

> If building the ops dynamically is the preferred way, I'll find a way to
> make it happen. I'll need to way to identify which one need it, loose
> the 'const' qualifier in a lot of place, etc ... that will be a bit
> hackish I'm afraid.

With only a limited set of options it might be better to just have a set
of static structs and pick one to register (some other drivers do this
based on hardware options), but the number of combinations with this is
getting a bit big for that.

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