[PATCH v6 0/1] iio:pressure:ms5611: fix and enhancements

[PATCH v6 0/1] iio:pressure:ms5611: fix and enhancements

[Gregor Boirie]

Changes since v1:
* temperature compensation fix:
    * reword commit message
    * remove useless parenthesis
* power regulator: reword commit message
* add oversampling rate support

Changes since v2:
* remove non printable characters from commit messages

Changes since v3:
* drop first 3 patches since applied onto testing
* fix oops at regulator probing time if regulator support not compiled-in
* complete device-tree support and documentation
* remove continuous sampling support (see Jonathan's "kthread tight loop
  trigger" proposal)
* checkpatch'ed

Changes since v4:
* drop first 2 patches since applied
* amend commit to include "Fixes" tag

Changes since v5:
* reword patch subject line and include "Fixes" tag in commit message

Gregor Boirie (1):
  iio:pressure:ms5611: fix crash when probing regulator

 drivers/iio/pressure/ms5611_core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

-- 
2.1.4

[PATCH v6 1/1] iio:pressure:ms5611: fix crash when probing regulator

[Gregor Boirie]

When not compiled-in, regulator layer will return a NULL pointer when
trying to get a reference to any regulator using devm_regulator_get().  As
IS_ERR() does not consider this an error, the ms5611 probing operation will
try to enable a NULL regulator, which will invariably cause a kernel
crash.
This patch fixes this situation by using devm_regulator_get_optional()
instead of devm_regulator_get().

Fixes: 3145229f9191 ("iio:pressure:ms5611: power regulator support")
Signed-off-by: Gregor Boirie <gregor.boirie-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org>
---
 drivers/iio/pressure/ms5611_core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/pressure/ms5611_core.c b/drivers/iio/pressure/ms5611_core.c
index 37dbc04..a2a871b 100644
--- a/drivers/iio/pressure/ms5611_core.c
+++ b/drivers/iio/pressure/ms5611_core.c
@@ -387,8 +387,8 @@ static const struct iio_info ms5611_info = {
 static int ms5611_init(struct iio_dev *indio_dev)
 {
 	int ret;
-	struct regulator *vdd = devm_regulator_get(indio_dev->dev.parent,
-	                                           "vdd");
+	struct regulator *vdd =
+		devm_regulator_get_optional(indio_dev->dev.parent, "vdd");
 
 	/* Enable attached regulator if any. */
 	if (!IS_ERR(vdd)) {
@@ -398,6 +398,10 @@ static int ms5611_init(struct iio_dev *indio_dev)
 			        "failed to enable Vdd supply: %d\n", ret);
 			return ret;
 		}
+	} else {
+		ret = PTR_ERR(vdd);
+		if (ret != -ENODEV)
+			return ret;
 	}
 
 	ret = ms5611_reset(indio_dev);
-- 
2.1.4

Re: [PATCH v6 0/1] iio:pressure:ms5611: fix and enhancements

[Krzysztof Kozlowski]

On 09.03.2016 20:34, Gregor Boirie wrote:
> Changes since v1:
> * temperature compensation fix:
>     * reword commit message
>     * remove useless parenthesis
> * power regulator: reword commit message
> * add oversampling rate support
> 
> Changes since v2:
> * remove non printable characters from commit messages
> 
> Changes since v3:
> * drop first 3 patches since applied onto testing
> * fix oops at regulator probing time if regulator support not compiled-in
> * complete device-tree support and documentation
> * remove continuous sampling support (see Jonathan's "kthread tight loop
>   trigger" proposal)
> * checkpatch'ed
> 
> Changes since v4:
> * drop first 2 patches since applied
> * amend commit to include "Fixes" tag
> 
> Changes since v5:
> * reword patch subject line and include "Fixes" tag in commit message
> 
> Gregor Boirie (1):
>   iio:pressure:ms5611: fix crash when probing regulator
> 
>  drivers/iio/pressure/ms5611_core.c | 8 ++++++--

Really, how the heck you connected my humble person with this file?
get_maintainer.pl does not print me. Even when run with --git argument.

BR,
Krzysztof

Re: [PATCH v6 1/1] iio:pressure:ms5611: fix crash when probing regulator

[Jonathan Cameron]

On 09/03/16 11:34, Gregor Boirie wrote:
> When not compiled-in, regulator layer will return a NULL pointer when
> trying to get a reference to any regulator using devm_regulator_get().  As
> IS_ERR() does not consider this an error, the ms5611 probing operation will
> try to enable a NULL regulator, which will invariably cause a kernel
> crash.
> This patch fixes this situation by using devm_regulator_get_optional()
> instead of devm_regulator_get().
> 
> Fixes: 3145229f9191 ("iio:pressure:ms5611: power regulator support")
> Signed-off-by: Gregor Boirie <gregor.boirie-ITF29qwbsa/QT0dZR+AlfA@public.gmane.org>

I've been thinking a bit more about these cases and think we are missusing
the optional regulators.  If regulator support is present and a non optional
register is requested then it will feed a dummy regulator back. 
The intent of this is to handle the case where an non optional regulator is
needed, but not specified (typically because it is always on).

So the question is how should we handle the regulators not built in case.
If you do call devm_regulator_get you'll get a null pointer.
Without regulators built in regulator_enable will be fine (returns 0)
as does regulator disable.

So I'm unclear on where the crash is coming from.

Also interesting is this driver enables the regulator but never disables
it (which it should).   I was clearly half asleep when reviewing this.
Sorry!

Jonathan


> ---
>  drivers/iio/pressure/ms5611_core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/pressure/ms5611_core.c b/drivers/iio/pressure/ms5611_core.c
> index 37dbc04..a2a871b 100644
> --- a/drivers/iio/pressure/ms5611_core.c
> +++ b/drivers/iio/pressure/ms5611_core.c
> @@ -387,8 +387,8 @@ static const struct iio_info ms5611_info = {
>  static int ms5611_init(struct iio_dev *indio_dev)
>  {
>  	int ret;
> -	struct regulator *vdd = devm_regulator_get(indio_dev->dev.parent,
> -	                                           "vdd");
> +	struct regulator *vdd =
> +		devm_regulator_get_optional(indio_dev->dev.parent, "vdd");
>  
>  	/* Enable attached regulator if any. */
>  	if (!IS_ERR(vdd)) {
> @@ -398,6 +398,10 @@ static int ms5611_init(struct iio_dev *indio_dev)
>  			        "failed to enable Vdd supply: %d\n", ret);
>  			return ret;
>  		}
> +	} else {
> +		ret = PTR_ERR(vdd);
> +		if (ret != -ENODEV)
> +			return ret;
>  	}
>  
>  	ret = ms5611_reset(indio_dev);
>