Skip to contentSkip to conversation

ROHM IIO fixes

PATCH Superseded Matti Vaittinen 4 participants 14 messages Aug 28 – Aug 30

Subsystems (4)
IIO Kionix/ROHM KX022A accelerometer ROHM BU270xx light sensor DRIVERs ROHM BM1390 pressure sensor driver

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Matti Vaittinen

author
#0 lore raw

v2 cover ROHM IIO fixes

Fix a few issues from IIO drivers for ROHM components.

I hired couple of gnomes to work for me. :) (Ran AI reviews). Unsurprizingly some bugs were spotted. Time to try fix mess (mostly) I have authored.

Bugs were found by AI review but fixes are made by hand. Hence no AI-tooling tags are added. Please, let me know if one is needed.

Note that the patch 1/4 is already applied to Jonathan's testing repo. Including it because I couldn't find it from IIO's public repo to rebase the series on it.

Revision history:
v1 => v2:
- Drop already applied patches (except the 1/4 which is in testing)
- all: Fixes tag before SOB
- patch 2/4: Clarify units for minimum integration time
- patch 3/4: Fix the unwinding of fifo enable
- patch 4/4: Fix the broken commit message

---

NOTE: Only _very_ shallow testing is done. Some of the fixes are not tested in the hardware at all. All reviewing and testing is appreciated as usual!

Matti Vaittinen (4):
iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
iio: light: rohm-bu27034: Fix infinite delay on error
iio: accel: kionix-kx022a: Prevent memory leak and fix state
iio: accel: kionix-kx022a: Fix IPOL macro name

 drivers/iio/accel/kionix-kx022a.c  | 28 +++++++++++++++++++++++-----
 drivers/iio/accel/kionix-kx022a.h  |  2 +-
 drivers/iio/light/rohm-bu27034.c   | 10 ++++++++++
 drivers/iio/pressure/rohm-bm1390.c |  4 +++-
 4 files changed, 37 insertions(+), 7 deletions(-)

base-commit: 6b9f23b5460818aaf199dda90210d5fc08d66c8f

Signature · 1 line
2.55.0
author
#1 lore raw

PATCH v2 1/4 iio: pressure: rohm-bm1390: Fix AVE_NUM initialization

From: Matti Vaittinen <[email protected]>

The BM1390 tries to initialize the AVE_NUM to 110b at the start-up. The field location is not taken into account, and value is written unsifted. This causes the AVE_NUM to be initialized to zero.

Use FIELD_PREP() to shift the intended AVE_NUM value to correct field.

Fixes 81ca5979b6ed ("iio: pressure: Support ROHM BU1390")
Signed-off-by Matti Vaittinen <[email protected]>

---
I believe this was already applied to Jonathan's 'testing' branch. I didn't find that from public IIO repository though, so spinning it here just for the sake of the completeness.

---
 drivers/iio/pressure/rohm-bm1390.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/iio/pressure/rohm-bm1390.c +3 -1
index d00d7ed54cb1..29454570f257 100644
@@ -479,6 +479,7 @@ static const struct iio_info bm1390_info = {
 
 static int bm1390_chip_init(struct bm1390_data *data)
 {
+	u8 regval;
 	int ret;
 
 	ret = regmap_write_bits(data->regmap, BM1390_REG_POWER,

@@ -512,8 +513,9 @@ static int bm1390_chip_init(struct bm1390_data *data)
 	 * Default to use IIR filter in "middle" mode. Also the AVE_NUM must
 	 * be fixed when IIR is in use.
 	 */
+	regval = FIELD_PREP(BM1390_MASK_AVE_NUM, BM1390_IIR_AVE_NUM);
 	ret = regmap_update_bits(data->regmap, BM1390_REG_MODE_CTRL,
-				 BM1390_MASK_AVE_NUM, BM1390_IIR_AVE_NUM);
+				 BM1390_MASK_AVE_NUM, regval);
 	if (ret)
 		return ret;
Signature · 1 line
2.55.0
author
#2 lore raw

PATCH v2 2/4 iio: light: rohm-bu27034: Fix infinite delay on error

In reply to Matti Vaittinen

From: Matti Vaittinen <[email protected]>

When reading an integration-time fails, the code will use error code to compute the sleep time.

Fix this by using the smallest integration time as a default if reading fails.

Fixes e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")
Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => v2:
- Moved Fixes before SOB
- Clarified units for the smallest integration time as suggested by Andy

---
 drivers/iio/light/rohm-bu27034.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/iio/light/rohm-bu27034.c +10 -0
index f9a421618406..cb2afddc75f6 100644
@@ -137,6 +137,7 @@ static const struct iio_gain_sel_pair bu27034_gains[] = {
 #define BU27034_MEAS_MODE_200MS		2
 #define BU27034_MEAS_MODE_400MS		4
 
+#define BU27034_INT_TIME_US_MIN (55 * USEC_PER_MSEC)
 static const struct iio_itime_sel_mul bu27034_itimes[] = {
 	GAIN_SCALE_ITIME_US(400000, BU27034_MEAS_MODE_400MS, 8),
 	GAIN_SCALE_ITIME_US(200000, BU27034_MEAS_MODE_200MS, 4),

@@ -1162,6 +1163,15 @@ static int bu27034_buffer_thread(void *arg)
 	data = iio_priv(idev);
 
 	wait_ms = bu27034_get_int_time(data);
+
+	/*
+	 * If reading the integration time fails, default to the minimum so we
+	 * don't lose samples. This may waste CPU cycles, but as a hardening
+	 * against theoretical, once-in-a-blue-moon error, this should be Ok.
+	 */
+	if (wait_ms < 0)
+		wait_ms = BU27034_INT_TIME_US_MIN;
+
 	wait_ms /= 1000;
 
 	wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;
Signature · 1 line
2.55.0
#3 lore raw

Re: [PATCH v2 2/4] iio: light: rohm-bu27034: Fix infinite delay on error

Quoted 5 lines · Matti Vaittinen

When reading an integration-time fails, the code will use error code to compute the sleep time.

Fix this by using the smallest integration time as a default if reading fails.

...

drivers/iio/light/rohm-bu27034.c static int bu27034_buffer_thread(void *arg) · line 1175

Quoted 11 lines
drivers/iio/light/rohm-bu27034.c +9 -0
 	wait_ms = bu27034_get_int_time(data);
+
+	/*
+	 * If reading the integration time fails, default to the minimum so we
+	 * don't lose samples. This may waste CPU cycles, but as a hardening
+	 * against theoretical, once-in-a-blue-moon error, this should be Ok.
+	 */
+	if (wait_ms < 0)
+		wait_ms = BU27034_INT_TIME_US_MIN;
+
 	wait_ms /= 1000;

With the above being open coded the _ms feels not right. I would expect the TIME_MIN to be in MS from the start (and for the consistency's sake with the below) and having all this to be written like

	ret = bu27034_get_int_time(data);
	if (ret < 0)
		wait_ms = _MS_MIN;
	else
		wait_ms = ret / USEC_PER_MSEC;
Quoted 1 line

wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#4 lore raw

Re: [PATCH v2 2/4] iio: light: rohm-bu27034: Fix infinite delay on error

Quoted 27 lines · Andy Shevchenko
Quoted 5 lines · Matti Vaittinen

When reading an integration-time fails, the code will use error code to compute the sleep time.

Fix this by using the smallest integration time as a default if reading fails.

...

drivers/iio/light/rohm-bu27034.c static int bu27034_buffer_thread(void *arg) · line 1175

Quoted 11 lines
  	wait_ms = bu27034_get_int_time(data);
drivers/iio/light/rohm-bu27034.c +9 -0
+
+	/*
+	 * If reading the integration time fails, default to the minimum so we
+	 * don't lose samples. This may waste CPU cycles, but as a hardening
+	 * against theoretical, once-in-a-blue-moon error, this should be Ok.
+	 */
+	if (wait_ms < 0)
+		wait_ms = BU27034_INT_TIME_US_MIN;
+
  	wait_ms /= 1000;

With the above being open coded the _ms feels not right. I would expect the TIME_MIN to be in MS from the start (and for the consistency's sake with the below) and having all this to be written like

Quoted 7 lines
	ret = bu27034_get_int_time(data);
	if (ret < 0)
		wait_ms = _MS_MIN;
	else
		wait_ms = ret / USEC_PER_MSEC;
Quoted 1 line
  	wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;

I don't like using 'ret' there.

At first glance, the
ret = bu27034_get_int_time(data);

looks like ret is containing just the success status. Furthermore,

Quoted 1 line
		wait_ms = ret / USEC_PER_MSEC;

forces one to go back and see WTF the 'ret' is (even if just couple of lines - but this is not an improvement, using ret is obfuscation).

I could change this to:

wait_ms = bu27034_get_int_time(data) / USEC_PER_MSEC; if (wait_ms < BU27034_INT_TIME_MIN_MS)

                wait_ms = BU27034_INT_TIME_MIN_MS;

(but for me this feels like unnecessary bikeshedding than anything else.)

Well, I'll change this if I respin.

Yours,
-- Matti

Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
#5 lore raw

Re: [PATCH v2 2/4] iio: light: rohm-bu27034: Fix infinite delay on error

Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko

On Fri, Aug 28, 2026 at 10:40:36AM +0300, Matti Vaittinen wrote:

...

Quoted 30 lines
Quoted 17 lines

drivers/iio/light/rohm-bu27034.c static int bu27034_buffer_thread(void *arg) · line 1175

Quoted 11 lines
  	wait_ms = bu27034_get_int_time(data);
drivers/iio/light/rohm-bu27034.c +9 -0
+
+	/*
+	 * If reading the integration time fails, default to the minimum so we
+	 * don't lose samples. This may waste CPU cycles, but as a hardening
+	 * against theoretical, once-in-a-blue-moon error, this should be Ok.
+	 */
+	if (wait_ms < 0)
+		wait_ms = BU27034_INT_TIME_US_MIN;
+
  	wait_ms /= 1000;

With the above being open coded the _ms feels not right. I would expect the TIME_MIN to be in MS from the start (and for the consistency's sake with the below) and having all this to be written like

Quoted 7 lines
	ret = bu27034_get_int_time(data);
	if (ret < 0)
		wait_ms = _MS_MIN;
	else
		wait_ms = ret / USEC_PER_MSEC;
Quoted 1 line
  	wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;

I don't like using 'ret' there.

At first glance, the
ret = bu27034_get_int_time(data);

Quoted 7 lines

looks like ret is containing just the success status. Furthermore,

Quoted 1 line
		wait_ms = ret / USEC_PER_MSEC;

forces one to go back and see WTF the 'ret' is (even if just couple of lines
- but this is not an improvement, using ret is obfuscation).

I could change this to:

I suggested without knowing the possible ranges of the returned value.

Quoted 3 lines

wait_ms = bu27034_get_int_time(data) / USEC_PER_MSEC; if (wait_ms < BU27034_INT_TIME_MIN_MS)

                wait_ms = BU27034_INT_TIME_MIN_MS;

This looks sane to me and removes the confusion I was talking about.

Quoted 3 lines

(but for me this feels like unnecessary bikeshedding than anything else.)

Well, I'll change this if I respin.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#6 lore raw

Re: [PATCH v2 2/4] iio: light: rohm-bu27034: Fix infinite delay on error

Quoted 45 lines · On Fri, 28 Aug 2026 14:01:46 +0300 Andy Shevchenko…
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko

On Fri, Aug 28, 2026 at 10:40:36AM +0300, Matti Vaittinen wrote:

...

Quoted 30 lines
Quoted 17 lines

drivers/iio/light/rohm-bu27034.c static int bu27034_buffer_thread(void *arg) · line 1175

Quoted 11 lines
  	wait_ms = bu27034_get_int_time(data);
drivers/iio/light/rohm-bu27034.c +9 -0
+
+	/*
+	 * If reading the integration time fails, default to the minimum so we
+	 * don't lose samples. This may waste CPU cycles, but as a hardening
+	 * against theoretical, once-in-a-blue-moon error, this should be Ok.
+	 */
+	if (wait_ms < 0)
+		wait_ms = BU27034_INT_TIME_US_MIN;
+
  	wait_ms /= 1000;

With the above being open coded the _ms feels not right. I would expect the TIME_MIN to be in MS from the start (and for the consistency's sake with the below) and having all this to be written like

Quoted 7 lines
	ret = bu27034_get_int_time(data);
	if (ret < 0)
		wait_ms = _MS_MIN;
	else
		wait_ms = ret / USEC_PER_MSEC;
Quoted 1 line
  	wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;

I don't like using 'ret' there.

At first glance, the
ret = bu27034_get_int_time(data);

Quoted 7 lines

looks like ret is containing just the success status. Furthermore,

Quoted 1 line
		wait_ms = ret / USEC_PER_MSEC;

forces one to go back and see WTF the 'ret' is (even if just couple of lines
- but this is not an improvement, using ret is obfuscation).

I could change this to:

Quoted 7 lines

I suggested without knowing the possible ranges of the returned value.

Quoted 3 lines

wait_ms = bu27034_get_int_time(data) / USEC_PER_MSEC; if (wait_ms < BU27034_INT_TIME_MIN_MS)

                wait_ms = BU27034_INT_TIME_MIN_MS;

This looks sane to me and removes the confusion I was talking about.

Except that wait_ms can be negative due to a read error and that is obscure by what now looks like

	wait_ms = max(wait_ms, BU27034_INT_TIME_MIN_MS);

so ensuring we clamp an out of range value.

I'd just go a touch further and split return value and parameter
+ provide a default.

wait_us = BU27034_INT_TIME_US;
ret = bu27034_get_int_time(data, &wait_us);
if (ret)

	dev_warn(dev, "Failed to read int time, muddling on\n");
Quoted 5 lines
Quoted 4 lines

(but for me this feels like unnecessary bikeshedding than anything else.)

Well, I'll change this if I respin.

maintainer
#7 lore raw

Re: [PATCH v2 2/4] iio: light: rohm-bu27034: Fix infinite delay on error

Quoted 54 lines · Jonathan Cameron
Quoted 51 lines · On Fri, 28 Aug 2026 14:01:46 +0300 Andy Shevchenko…
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko

On Fri, Aug 28, 2026 at 10:40:36AM +0300, Matti Vaittinen wrote:

...

Quoted 30 lines
Quoted 17 lines

drivers/iio/light/rohm-bu27034.c static int bu27034_buffer_thread(void *arg) · line 1175

Quoted 11 lines
   	wait_ms = bu27034_get_int_time(data);
drivers/iio/light/rohm-bu27034.c +9 -0
+
+	/*
+	 * If reading the integration time fails, default to the minimum so we
+	 * don't lose samples. This may waste CPU cycles, but as a hardening
+	 * against theoretical, once-in-a-blue-moon error, this should be Ok.
+	 */
+	if (wait_ms < 0)
+		wait_ms = BU27034_INT_TIME_US_MIN;
+
   	wait_ms /= 1000;

With the above being open coded the _ms feels not right. I would expect the TIME_MIN to be in MS from the start (and for the consistency's sake with the below) and having all this to be written like

Quoted 7 lines
	ret = bu27034_get_int_time(data);
	if (ret < 0)
		wait_ms = _MS_MIN;
	else
		wait_ms = ret / USEC_PER_MSEC;
Quoted 1 line
   	wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;

I don't like using 'ret' there.

At first glance, the
ret = bu27034_get_int_time(data);

Quoted 7 lines

looks like ret is containing just the success status. Furthermore,

Quoted 1 line
		wait_ms = ret / USEC_PER_MSEC;

forces one to go back and see WTF the 'ret' is (even if just couple of lines
- but this is not an improvement, using ret is obfuscation).

I could change this to:

I suggested without knowing the possible ranges of the returned value.

Quoted 3 lines

wait_ms = bu27034_get_int_time(data) / USEC_PER_MSEC; if (wait_ms < BU27034_INT_TIME_MIN_MS)

                 wait_ms = BU27034_INT_TIME_MIN_MS;
Quoted 6 lines
Quoted 1 line

This looks sane to me and removes the confusion I was talking about.

Except that wait_ms can be negative due to a read error and that is obscure by what now looks like

	wait_ms = max(wait_ms, BU27034_INT_TIME_MIN_MS);

so ensuring we clamp an out of range value.

Which, in this case, is perfectly fine because it does not matter what the cause of the failure is. There is no need to distinguish between read error and 'returned value is below the minimum time' - in both cases we just use the minimum time.

Quoted 7 lines

I'd just go a touch further and split return value and parameter
+ provide a default.

wait_us = BU27034_INT_TIME_US;
ret = bu27034_get_int_time(data, &wait_us);
if (ret)

	dev_warn(dev, "Failed to read int time, muddling on\n");

I do both agree and disagree. This is not the first time when I am pondering if a "getter API" should only return an error, and fill the desired output in location pointed by a parameter (your suggestion here).

In many cases it is simpler in the calling site to just use the return value, and error handling can still be reliably done when all legitimate values are positive (in this case, also greater than minimum). Using the pointer argument for value retrieval typically brings potential issues with value initialization if call is unsuccessful.

Hence, I am often using this "dual meaning" for return value (negative error code or positive valid value) in cases, where all successful values are guaranteed to be positive.

I would definitely favour your suggested signature if retrieved value could be negative, or was a pointer. I have never liked the IS_ERR() and others - but check:
if "value < allowed minimum" and decision "then, something is wrong - use default" is really quite clear to me.

Yours,
-- Matti

Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
author
#8 lore raw

PATCH v2 3/4 iio: accel: kionix-kx022a: Prevent memory leak and fix state

In reply to Matti Vaittinen

From: Matti Vaittinen <[email protected]>

The driver allocates memory for samples at buffer enable path. If regmap operation fails in the kx022a_fifo_enable() at the buffer enable path, the allocated memory is never freed. Furthermore, the state information and previous hardware configuration(s) aren't undone, potentially leaving WMI interrupts and buffers enabled, or driver state flags wrong.

Free the memory and revert the hardware configuration and state flags on error path.

Fixes e7123a4dfcd7 ("iio: accel: kionix-kx022a: Refactor driver and add chip_info structure")
Signed-off-by Matti Vaittinen <[email protected]>
Reviewed-by Mehdi Djait <[email protected]>

---
Revision history:
v1 => v2:
- Fix unwinding the fifo enabling
- Move Fixes before the SOB.

---
 drivers/iio/accel/kionix-kx022a.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)
drivers/iio/accel/kionix-kx022a.c +23 -5
index 8a13f78aeab0..fa94bc0fd8cf 100644
@@ -980,26 +980,44 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
 	guard(mutex)(&data->mutex);
 	ret = __kx022a_turn_on_off(data, false);
 	if (ret)
-		return ret;
+		goto err_free_out;
 
 	/* Update watermark to HW */
 	ret = kx022a_fifo_set_wmi(data);
 	if (ret)
-		return ret;
+		goto err_wmi_out;
 
 	/* Enable buffer */
 	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
 			      KX022A_MASK_BUF_EN);
 	if (ret)
-		return ret;
+		goto err_wmi_out;
 
 	data->state |= KX022A_STATE_FIFO;
 	ret = regmap_set_bits(data->regmap, data->ien_reg,
 			      KX022A_MASK_WMI);
 	if (ret)
-		return ret;
+		goto err_buf_en_out;
 
-	return __kx022a_turn_on_off(data, true);
+	ret = __kx022a_turn_on_off(data, true);
+	if (ret)
+		goto err_on_out;
+
+	return ret;
+
+err_on_out:
+	regmap_clear_bits(data->regmap, data->ien_reg,
+			  KX022A_MASK_WMI);
+err_buf_en_out:
+	regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
+			  KX022A_MASK_BUF_EN);
+	data->state &= ~KX022A_STATE_FIFO;
+err_wmi_out:
+	__kx022a_turn_on_off(data, true);
+err_free_out:
+	kfree(data->fifo_buffer);
+
+	return ret;
 }
 
 static int kx022a_buffer_postenable(struct iio_dev *idev)
Signature · 1 line
2.55.0
#9 lore raw

Re: [PATCH v2 3/4] iio: accel: kionix-kx022a: Prevent memory leak and fix state

Quoted 8 lines · Matti Vaittinen

The driver allocates memory for samples at buffer enable path. If regmap operation fails in the kx022a_fifo_enable() at the buffer enable path, the allocated memory is never freed. Furthermore, the state information and previous hardware configuration(s) aren't undone, potentially leaving WMI interrupts and buffers enabled, or driver state flags wrong.

Free the memory and revert the hardware configuration and state flags on error path.

...

Quoted 1 line

static int kx022a_fifo_enable(struct kx022a_data *data)

With

	struct regmap *map = data->regmap;
Quoted 26 lines
drivers/iio/accel/kionix-kx022a.c +4 -4
 	guard(mutex)(&data->mutex);
 	ret = __kx022a_turn_on_off(data, false);
 	if (ret)
-		return ret;
+		goto err_free_out;
 
 	/* Update watermark to HW */
 	ret = kx022a_fifo_set_wmi(data);
 	if (ret)
-		return ret;
+		goto err_wmi_out;
 
 	/* Enable buffer */
 	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
 			      KX022A_MASK_BUF_EN);
 	if (ret)
-		return ret;
+		goto err_wmi_out;
 
 	data->state |= KX022A_STATE_FIFO;
 	ret = regmap_set_bits(data->regmap, data->ien_reg,
 			      KX022A_MASK_WMI);
 	if (ret)
-		return ret;
+		goto err_buf_en_out;

drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 1010

Quoted 10 lines
drivers/iio/accel/kionix-kx022a.c +9 -1
-	return __kx022a_turn_on_off(data, true);
+	ret = __kx022a_turn_on_off(data, true);
+	if (ret)
+		goto err_on_out;
+
+	return ret;
+
+err_on_out:
+	regmap_clear_bits(data->regmap, data->ien_reg,
+			  KX022A_MASK_WMI);

in particular this will be

	regmap_clear_bits(map, data->ien_reg, KX022A_MASK_WMI);

drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 1013

Quoted 3 lines
drivers/iio/accel/kionix-kx022a.c +3 -0
+err_buf_en_out:
+	regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
+			  KX022A_MASK_BUF_EN);

	regmap_clear_bits(map, data->chip_info->buf_cntl2, KX022A_MASK_BUF_EN);

Which saves a single line, and the followup may shorten the existing code even more.

Quoted 8 lines
drivers/iio/accel/kionix-kx022a.c +7 -0
+	data->state &= ~KX022A_STATE_FIFO;
+err_wmi_out:
+	__kx022a_turn_on_off(data, true);
+err_free_out:
+	kfree(data->fifo_buffer);
+
+	return ret;
 }
Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#10 lore raw

Re: [PATCH v2 3/4] iio: accel: kionix-kx022a: Prevent memory leak and fix state

Quoted 60 lines · Andy Shevchenko
Quoted 8 lines · Matti Vaittinen

The driver allocates memory for samples at buffer enable path. If regmap operation fails in the kx022a_fifo_enable() at the buffer enable path, the allocated memory is never freed. Furthermore, the state information and previous hardware configuration(s) aren't undone, potentially leaving WMI interrupts and buffers enabled, or driver state flags wrong.

Free the memory and revert the hardware configuration and state flags on error path.

...

Quoted 1 line

static int kx022a_fifo_enable(struct kx022a_data *data)

With

	struct regmap *map = data->regmap;
Quoted 26 lines
  	guard(mutex)(&data->mutex);
  	ret = __kx022a_turn_on_off(data, false);
  	if (ret)
drivers/iio/accel/kionix-kx022a.c +4 -4
-		return ret;
+		goto err_free_out;
 
  	/* Update watermark to HW */
  	ret = kx022a_fifo_set_wmi(data);
  	if (ret)
-		return ret;
+		goto err_wmi_out;
 
  	/* Enable buffer */
  	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
  			      KX022A_MASK_BUF_EN);
  	if (ret)
-		return ret;
+		goto err_wmi_out;
 
  	data->state |= KX022A_STATE_FIFO;
  	ret = regmap_set_bits(data->regmap, data->ien_reg,
  			      KX022A_MASK_WMI);
  	if (ret)
-		return ret;
+		goto err_buf_en_out;

drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 1010

Quoted 10 lines
drivers/iio/accel/kionix-kx022a.c +9 -1
-	return __kx022a_turn_on_off(data, true);
+	ret = __kx022a_turn_on_off(data, true);
+	if (ret)
+		goto err_on_out;
+
+	return ret;
+
+err_on_out:
+	regmap_clear_bits(data->regmap, data->ien_reg,
+			  KX022A_MASK_WMI);

in particular this will be

	regmap_clear_bits(map, data->ien_reg, KX022A_MASK_WMI);
Quoted 8 lines

drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 1013

Quoted 3 lines
drivers/iio/accel/kionix-kx022a.c +3 -0
+err_buf_en_out:
+	regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
+			  KX022A_MASK_BUF_EN);

	regmap_clear_bits(map, data->chip_info->buf_cntl2, KX022A_MASK_BUF_EN);

Which saves a single line, and the followup may shorten the existing code even more.

As far as I can see, the driver is consistently using the 'data->regmap' everywhere. Using 'map' just in one function would raise question if it is different regmap from every other place.

I am 100% Ok with anyone changing the data->regmap to 'map' throughout the whole driver in order to cut the amount of lines - but it is not something to be done in a bugfix commit.

Yours,
-- Matti

Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
maintainer
#11 lore raw

Re: [PATCH v2 3/4] iio: accel: kionix-kx022a: Prevent memory leak and fix state

In reply to Matti Vaittinen
Quoted 6 lines

The driver allocates memory for samples at buffer enable path. If regmap operation fails in the kx022a_fifo_enable() at the buffer enable path, the allocated memory is never freed. Furthermore, the state information and previous hardware configuration(s) aren't undone, potentially leaving WMI interrupts and buffers enabled, or driver state flags wrong.

Quoted 6 lines

Free the memory and revert the hardware configuration and state flags on error path.

Fixes e7123a4dfcd7 ("iio: accel: kionix-kx022a: Refactor driver and add chip_info structure")
Signed-off-by Matti Vaittinen <[email protected]>
Reviewed-by Mehdi Djait <[email protected]>

I'm not happy with the label naming. It had me very confused!

drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 983

Quoted 11 lines
drivers/iio/accel/kionix-kx022a.c +1 -1
index 02dd1db7a646..42bac6d724a0 100644
@@ -980,26 +980,44 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
 	guard(mutex)(&data->mutex);
 	ret = __kx022a_turn_on_off(data, false);
 	if (ret)
-		return ret;
+		goto err_free_out;

Label naming wise - this one is talking about what needs doing (works for me!)

drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 988

Quoted 6 lines
drivers/iio/accel/kionix-kx022a.c +1 -1
 	/* Update watermark to HW */
 	ret = kx022a_fifo_set_wmi(data);
 	if (ret)
-		return ret;
+		goto err_wmi_out;

This one is talkign about why we went there. For consistency would need to be something about turning off not this.

drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 994

Quoted 7 lines
drivers/iio/accel/kionix-kx022a.c +1 -1
 	/* Enable buffer */
 	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
 			      KX022A_MASK_BUF_EN);
 	if (ret)
-		return ret;
+		goto err_wmi_out;

This I'd kind of expect to undo the set wmi but it doesn't...

drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 1000

Quoted 7 lines
drivers/iio/accel/kionix-kx022a.c +1 -1
 	data->state |= KX022A_STATE_FIFO;
 	ret = regmap_set_bits(data->regmap, data->ien_reg,
 			      KX022A_MASK_WMI);
 	if (ret)
-		return ret;
+		goto err_buf_en_out;

This one is back to what we are undoing (good).

drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 1004

Quoted 5 lines
drivers/iio/accel/kionix-kx022a.c +3 -1
-	return __kx022a_turn_on_off(data, true);
+	ret = __kx022a_turn_on_off(data, true);
+	if (ret)
+		goto err_on_out;

This is another where we came form, not where we are going to.

Quoted 9 lines
drivers/iio/accel/kionix-kx022a.c +9 -0
+
+	return ret;
+
+err_on_out:
+	regmap_clear_bits(data->regmap, data->ien_reg,
+			  KX022A_MASK_WMI);
+err_buf_en_out:
+	regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
+			  KX022A_MASK_BUF_EN);

drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 1023

Quoted 10 lines
drivers/iio/accel/kionix-kx022a.c +7 -0
+	data->state &= ~KX022A_STATE_FIFO;
+err_wmi_out:
+	__kx022a_turn_on_off(data, true);
+err_free_out:
+	kfree(data->fifo_buffer);
+
+	return ret;
 }
 
 static int kx022a_buffer_postenable(struct iio_dev *idev)

Thanks,

Signature · 1 line
Jonathan Cameron <[email protected]>
maintainer
#12 lore raw

Re: [PATCH v2 3/4] iio: accel: kionix-kx022a: Prevent memory leak and fix state

Quoted 6 lines · Jonathan Cameron
Quoted 6 lines

The driver allocates memory for samples at buffer enable path. If regmap operation fails in the kx022a_fifo_enable() at the buffer enable path, the allocated memory is never freed. Furthermore, the state information and previous hardware configuration(s) aren't undone, potentially leaving WMI interrupts and buffers enabled, or driver state flags wrong.

Quoted 8 lines
Quoted 6 lines

Free the memory and revert the hardware configuration and state flags on error path.

Fixes e7123a4dfcd7 ("iio: accel: kionix-kx022a: Refactor driver and add chip_info structure")
Signed-off-by Matti Vaittinen <[email protected]>
Reviewed-by Mehdi Djait <[email protected]>

I'm not happy with the label naming. It had me very confused!

Hmm. I suppose it could be more consistent :) Thanks.

Yours,
-- Matti

Signature · 5 lines
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~
author
#13 lore raw

PATCH v2 4/4 iio: accel: kionix-kx022a: Fix IPOL macro name

In reply to Matti Vaittinen

From: Matti Vaittinen <[email protected]>

The "interrupt polarity high"-macro for KX022A variant is defined as:
"#define KX022A_MASK_IPOL KX022A_MASK_IPOL1"

However, the KX022A_MASK_IPOL1 is not defined anywhere, so actually using the KX022A_IPOL_HIGH would produce a compile error.

Fix the define by using correct mask.

Fixes 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")
Signed-off-by Matti Vaittinen <[email protected]>
Reviewed-by Mehdi Djait <[email protected]>

---
Revision history:
v1 => v2:
- Fixed the commit message where line: "#define KX022A_MASK_IPOL KX022A_MASK_IPOL1" was lost, as git treated it as a comment :)
- Moved Fixes before SOB

---
 drivers/iio/accel/kionix-kx022a.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/accel/kionix-kx022a.h +1 -1
index 0ed54f584223..a2d122c1e234 100644
@@ -65,7 +65,7 @@
 #define KX022A_MASK_IEN		BIT(5)
 #define KX022A_MASK_IPOL	BIT(4)
 #define KX022A_IPOL_LOW		0
-#define KX022A_IPOL_HIGH	KX022A_MASK_IPOL1
+#define KX022A_IPOL_HIGH	KX022A_MASK_IPOL
 #define KX022A_MASK_ITYP	BIT(3)
 #define KX022A_ITYP_PULSE	KX022A_MASK_ITYP
 #define KX022A_ITYP_LEVEL	0
Signature · 1 line
2.55.0