iio:pressure:ms5611: fix and enhancements
v6 cover iio:pressure:ms5611: fix and enhancements
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(-)
Signature · 1 line
2.1.4
A message that is not in the archive (sent off-list or not yet received): cover.1457522751.git.gregor.boirie-ITF29qwbsa/[email protected]
PATCH v6 1/1 iio:pressure:ms5611: fix crash when probing regulator
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().
--- drivers/iio/pressure/ms5611_core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/iio/pressure/ms5611_core.c +6 -2
@@ -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);
Signature · 1 line
2.1.4
A message that is not in the archive (sent off-list or not yet received): 7767ba4decd3938eb1b37990f707b016228e0ef5.1457522751.git.gregor.boirie-ITF29qwbsa/[email protected]
Re: [PATCH v6 1/1] iio:pressure:ms5611: fix crash when probing regulator
Quoted 10 lines · 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(). AsIS_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 usingdevm_regulator_get_optional()instead ofdevm_regulator_get().Fixes 3145229f9191 ("iio:pressure:ms5611: power regulator support")
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
Quoted 31 lines
--- drivers/iio/pressure/ms5611_core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)drivers/iio/pressure/ms5611_core.c +6
-2@@ -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);
Re: [PATCH v6 1/1] iio:pressure:ms5611: fix crash when probing regulator
Quoted 9 lines · Jonathan Cameron
Quoted 8 lines · 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(). AsIS_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 usingdevm_regulator_get_optional()instead ofdevm_regulator_get().
Quoted 7 lines
Quoted 2 lines
Fixes 3145229f9191 ("iio:pressure:ms5611: power regulator support")Signed-off-by Gregor Boirie <[email protected]>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).
My intent was:
* if regulator support not built-in, keep going ;
* else :
* if regulator not declared (platform or device tree), keep going ;
* else :
* if regulator not properly declared, error ;
* else enable it, then go one initializing.
So, I suppose dummy regulators should be allowed to support all possible cases (use devm_regulator_get()).
Have I missed something ?
Quoted 6 lines
So the question is how should we handle the regulators not built in case. If you do call
devm_regulator_getyou'll get a null pointer. Without regulators built inregulator_enablewill be fine (returns 0) as does regulator disable.So I'm unclear on where the crash is coming from.
Probably from the fact that I use an up to date IIO subsystem ported onto an old kernel (3.10.x), which rather complicates testing with regard to other subsystem dependencies.
Quoted 2 lines
Also interesting is this driver enables the regulator but never disables it (which it should).
Right. I thought regulator_put() implicitly disabled the regulator, which is wrong...
Quoted 2 lines
I was clearly half asleep when reviewing this.
Sorry!
Well, I wasn't that awake either. Sorry for the noise. Grégor.
Quoted 35 lines
Jonathan
Quoted 31 lines
--- drivers/iio/pressure/ms5611_core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)drivers/iio/pressure/ms5611_core.c +6
-2@@ -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);
Quoted 22 lines · Gregor Boirie
Changes since v1:
* temperature compensation fix:
* reword commit message
* remove useless parenthesis
* power regulator: reword commit message
* add oversampling rate supportChanges since v2:
* remove non printable characters from commit messagesChanges 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'edChanges since v4:
* drop first 2 patches since applied
* amend commit to include "Fixes" tag
Quoted 7 lines
Changes since v5:
* reword patch subject line and include "Fixes" tag in commit messageGregor Boirie (1):
iio:pressure:ms5611: fix crash when probing regulatordrivers/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
You appeared since get_maintainer.pl printed your address for v3 of this patch modifying drivers/iio/pressure/ms5611_i2c.c because of 215597 ("iio: Drop owner assignment from i2c_driver").
I did not take time to update addresses list for later versions. Sorry about that. I took you out of CC list.
Regards
Quoted 37 lines · Krzysztof Kozlowski
Quoted 22 lines · Gregor Boirie
Changes since v1:
* temperature compensation fix:
* reword commit message
* remove useless parenthesis
* power regulator: reword commit message
* add oversampling rate supportChanges since v2:
* remove non printable characters from commit messagesChanges 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'edChanges since v4:
* drop first 2 patches since applied
* amend commit to include "Fixes" tagQuoted 7 lines
Changes since v5:
* reword patch subject line and include "Fixes" tag in commit messageGregor Boirie (1):
iio:pressure:ms5611: fix crash when probing regulatordrivers/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