linux-kernel - Re: [PATCH v3 5/5] hwmon: Add support for Amphenol ChipCap 2

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: <04475f91-bdce-4677-894c-74c2bb8233d9@sirena.org.uk>
Date:   Thu, 7 Dec 2023 20:05:21 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Javier Carrasco <javier.carrasco.cruz@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Jonathan Corbet <corbet@....net>,
        Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 5/5] hwmon: Add support for Amphenol ChipCap 2

On Thu, Dec 07, 2023 at 08:44:55PM +0100, Javier Carrasco wrote:

> +       if (regulator_is_enabled(data->regulator)) {
> +               ret = regulator_disable(data->regulator);
> +               if (ret < 0)
> +                       return ret;
> +
> +               msleep(CC2_POWER_CYCLE_MS); /* ensure a clean power cycle */
> +       }
> +
> +       ret = regulator_enable(data->regulator);
> +       if (ret < 0)
> +               return ret;

This is very buggy.  A consumer should only disable a regulator if it
itself enabled that regulator (or it *requires* an exclusive regulator
which isn't a good fit here), and there's no guarantee that disabling a
regulator will actually result in a power off.  Either the board might
not physically or through constraints permit the state to change or
another user may have enabled the regulator.  The driver needs to keep
track of if it enabled the regulator and only disable it as many times
as it enabled it.

For this usage with trying to bounce the power of the regulator you can
keep track of the actual power state of the supply by listening to
notifications, and should possibly just keep the regulator disabled when
it's not actively in use (if no alarm is active or measurement in
progress?).

> +	data->regulator = devm_regulator_get_optional(dev, "vdd");

Does the device *really* work without power?  The datasheet appears to
suggest that the device has a non-optional supply Vdd

   https://f.hubspotusercontent40.net/hubfs/9035299/Documents/AAS-916-127J-Telaire-ChipCap2-022118-web.pdf

(there's also a Vcore pin but that appears to be for connecting a
decoupling capacitor rather than a supply).

In general _optional() should only be used for supplies which may be
physically absent.

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