linux-kernel - Re: [PATCH 1/1] regulator: Only enable disabled regulators on resume

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: <54FD4E64.2070909@collabora.co.uk>
Date:	Mon, 09 Mar 2015 08:40:20 +0100
From:	Javier Martinez Canillas <javier.martinez@...labora.co.uk>
To:	Mark Brown <broonie@...nel.org>
CC:	Doug Anderson <dianders@...omium.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] regulator: Only enable disabled regulators on resume

On 03/08/2015 08:38 PM, Mark Brown wrote:
> On Wed, Mar 04, 2015 at 02:45:00PM +0100, Javier Martinez Canillas wrote:
> 
>> The thing is that _regulator_is_enabled() used to return -EINVAL if
>> the rdev didn't have an .is_enabled callback but that changed in
>> commit 9a7f6a4c6edc8 ("regulator: Assume regulators are enabled if
>> they don't report anything") and now returns 1 in that case. But
>> _regulator_enable() was not changed and is still checking for -EINVAL
>> which seems to me like a left over after the mentioned commit.
> 
> You mean _do_enable(), not _enable() here.  It's not really a leftover

No, I meant _enable() here. What I said is that _enable() is checking
if -EINVAL was returned by _is_enabled():

static int _regulator_enable(struct regulator_dev *rdev)
{
...
		ret = _regulator_is_enabled(rdev);
		if (ret == -EINVAL || ret == 0) {
			if (!_regulator_can_change_status(rdev))
				return -EPERM;

			ret = _regulator_do_enable(rdev);
			if (ret < 0)
				return ret;

		} else if (ret < 0) {
			rdev_err(rdev, "is_enabled() failed: %d\n", ret);
			return ret;
		}
...
}

and my point was that it is checking because _is_enabled() used to return
-EINVAL if the regulator driver didn't provide a .is_enabled callback:

static int _regulator_is_enabled(struct regulator_dev *rdev)
{
...
        if (!rdev->desc->ops->is_enabled)
               return -EINVAL;

        return rdev->desc->ops->is_enabled(rdev);
...
}

so, if a driver didn't provide a way to query if the regulator was enabled,
it was assumed that it was disabled. But after the mentioned commit, the
assumption was changed and now not having .is_enabled means that it's enabled:

static int _regulator_is_enabled(struct regulator_dev *rdev)
{
...
        if (!rdev->desc->ops->is_enabled)
               return 1;

        return rdev->desc->ops->is_enabled(rdev);
...
}

So my question was if _is_enabled() returning -EINVAL should still mean
that the regulator has to be enabled or the error has to be propagated
since now -EINVAL will be returned by the driver .is_enabled callback.

> as the two operations are doing somewhat different things and the
> changes are a bit separate, _is_enabled() is reporting the current state
> while _do_enable() is making a change so it should fail if it can't do
> that.  
>

Yes, I understand that.

> A better way of writing it in the _do_enable() case is that it possibly
> ought to be checking if the regulator is enabled before it does
> anything, though for uncached regulator operations that then means an
> extra I/O which isn't great.  Given that I think rather than ignoring
> the missing op it should instead fall back to checking _is_enabled() -
> that way if we can read the state but not change it the right thing will
> happen.  I'll do a patch, probably tomorrow.
> 

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/