linux-kernel - Re: [PATCH v2 1/3] hwmon: (lm90) Add power control

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]

Date:	Thu, 8 Aug 2013 12:01:36 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Wei Ni <wni@...dia.com>
Cc:	khali@...ux-fr.org, linux@...ck-us.net, swarren@...dotorg.org,
	linux-kernel@...r.kernel.org, lm-sensors@...sensors.org,
	MLongnecker@...dia.com, linux-tegra@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 1/3] hwmon: (lm90) Add power control

On Thu, Aug 08, 2013 at 02:56:29PM +0800, Wei Ni wrote:

> +	mutex_lock(&data->update_lock);
> +
> +	if (is_enable)
> +		ret = regulator_enable(data->lm90_reg);
> +	else
> +		ret = regulator_disable(data->lm90_reg);
> +
> +	if (ret < 0)
> +		dev_err(&client->dev,
> +			"Error in %s rail vdd, error %d\n",
> +			(is_enable) ? "enabling" : "disabling", ret);
> +	else
> +		dev_info(&client->dev, "success in %s rail vdd\n",
> +			 (is_enable) ? "enabling" : "disabling");
> +
> +	mutex_unlock(&data->update_lock);

Two things here.  One is that it's not clear what this lokc is
protecting since the only thing in the locked region is the regulator
operation and that is thread safe.  The other thing is that I'm not
seeing anthing that ensures that enables and disables are matched -
regulators are reference counted so two enables need two disables.

> +	data->lm90_reg = regulator_get(&client->dev, "vdd");
> +	if (IS_ERR_OR_NULL(data->lm90_reg)) {

NULL is a valid regulator, use IS_ERR().

> +		if (PTR_ERR(data->lm90_reg) == -ENODEV)
> +			dev_info(&client->dev,
> +				 "No regulator found for vdd. Assuming vdd is always powered.");
> +		else
> +			dev_warn(&client->dev,
> +				 "Error [%ld] in getting the regulator handle for vdd.\n",
> +				 PTR_ERR(data->lm90_reg));

You shouldn't just be ignoring errors here, though there are deployment
difficulties with making sure a stub regulator is provided.  These
should be getting easier after the next merge window, the stubs will be
being tweaked slightly to have an "assume it's there" option even when
regulators are used.  Especially in cases with device tree you should be
paying attention to -EPROBE_DEFER, that will accurately reflect if a
regulator is present but not loaded yet.

That said if you *are* going to do this you should request the
regulator using devm_regulator_get_optional(), this is intended to
support things that don't need regulators (though that's not the case
here).

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