Skip to contentSkip to conversation

imu: inv_mpu6050: use devm_request_irq

PATCH Superseded [email protected] 3 participants 7 messages Jan 21, 2015 – Jan 22, 2015

Subsystems (2)
IIO Invensense mpu-6050 IMU driver

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/3 imu: inv_mpu6050: use devm_request_irq

From: Varka Bhadram <[email protected]>

This patch use the devres API for requesting an IRQ.

Signed-off-by Varka Bhadram <[email protected]>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c +6 -8
index 926fcce..97f54fb 100644
@@ -123,10 +123,11 @@ int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
 		ret = -ENOMEM;
 		goto error_ret;
 	}
-	ret = request_irq(st->client->irq, &iio_trigger_generic_data_rdy_poll,
-				IRQF_TRIGGER_RISING,
-				"inv_mpu",
-				st->trig);
+	ret = devm_request_irq(&indio_dev->dev, st->client->irq,
+			       &iio_trigger_generic_data_rdy_poll,
+			       IRQF_TRIGGER_RISING,
+			       "inv_mpu",
+			       st->trig);
 	if (ret)
 		goto error_free_trig;
 	st->trig->dev.parent = &st->client->dev;

@@ -134,13 +135,11 @@ int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
 	iio_trigger_set_drvdata(st->trig, indio_dev);
 	ret = iio_trigger_register(st->trig);
 	if (ret)
-		goto error_free_irq;
+		goto error_free_trig;
 	indio_dev->trig = iio_trigger_get(st->trig);
 
 	return 0;
 
-error_free_irq:
-	free_irq(st->client->irq, st->trig);
 error_free_trig:
 	iio_trigger_free(st->trig);
 error_ret:

@@ -150,6 +149,5 @@ error_ret:
 void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st)
 {
 	iio_trigger_unregister(st->trig);
-	free_irq(st->client->irq, st->trig);
 	iio_trigger_free(st->trig);
 }
Signature · 1 line
1.7.9.5
author
#1 lore raw

PATCH 2/3 imu: inv_mpu6050: use devm_iio_trigger_alloc

From: Varka Bhadram <[email protected]>

Signed-off-by Varka Bhadram <[email protected]>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c +6 -8
index 97f54fb..ab8c85a 100644
@@ -116,9 +116,10 @@ int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
 	int ret;
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 
-	st->trig = iio_trigger_alloc("%s-dev%d",
-					indio_dev->name,
-					indio_dev->id);
+	st->trig = devm_iio_trigger_alloc(&indio_dev->dev,
+					  "%s-dev%d",
+					  indio_dev->name,
+					  indio_dev->id);
 	if (st->trig == NULL) {
 		ret = -ENOMEM;
 		goto error_ret;

@@ -129,19 +130,17 @@ int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
 			       "inv_mpu",
 			       st->trig);
 	if (ret)
-		goto error_free_trig;
+		goto error_ret;
 	st->trig->dev.parent = &st->client->dev;
 	st->trig->ops = &inv_mpu_trigger_ops;
 	iio_trigger_set_drvdata(st->trig, indio_dev);
 	ret = iio_trigger_register(st->trig);
 	if (ret)
-		goto error_free_trig;
+		goto error_ret;
 	indio_dev->trig = iio_trigger_get(st->trig);
 
 	return 0;
 
-error_free_trig:
-	iio_trigger_free(st->trig);
 error_ret:
 	return ret;
 }

@@ -149,5 +148,4 @@ error_ret:
 void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st)
 {
 	iio_trigger_unregister(st->trig);
-	iio_trigger_free(st->trig);
 }
Signature · 1 line
1.7.9.5
author
#2 lore raw

PATCH 3/3 imu: inv_mpu6050: cleanup on error check

In reply to [email protected]

From: Varka Bhadram <[email protected]>

Signed-off-by Varka Bhadram <[email protected]>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |   17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)
drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c +8 -9
index ab8c85a..844610c 100644
@@ -120,29 +120,28 @@ int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev)
 					  "%s-dev%d",
 					  indio_dev->name,
 					  indio_dev->id);
-	if (st->trig == NULL) {
-		ret = -ENOMEM;
-		goto error_ret;
-	}
+	if (!st->trig)
+		return -ENOMEM;
+
 	ret = devm_request_irq(&indio_dev->dev, st->client->irq,
 			       &iio_trigger_generic_data_rdy_poll,
 			       IRQF_TRIGGER_RISING,
 			       "inv_mpu",
 			       st->trig);
 	if (ret)
-		goto error_ret;
+		return ret;
+
 	st->trig->dev.parent = &st->client->dev;
 	st->trig->ops = &inv_mpu_trigger_ops;
 	iio_trigger_set_drvdata(st->trig, indio_dev);
+
 	ret = iio_trigger_register(st->trig);
 	if (ret)
-		goto error_ret;
+		return ret;
+
 	indio_dev->trig = iio_trigger_get(st->trig);
 
 	return 0;
-
-error_ret:
-	return ret;
 }
 
 void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st)
Signature · 1 line
1.7.9.5
#3 lore raw
In reply to [email protected]

On 01/21/2015 06:59 AM, [email protected] wrote:
[...]

Review

Quoted 5 lines
  void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st)
  {
  	iio_trigger_unregister(st->trig);
(quoted diff) +0 -1
-	free_irq(st->client->irq, st->trig);
  	iio_trigger_free(st->trig);

You are changing the relative order between free_irq() and iio_trigger_free() here and by doing so introduce a use-after-free race condition. The IRQ handler uses the trigger, so the IRQ has to be released before the trigger is freed.

This can be easily fixed though by changing the order of patch 1 and patch 2 in this series.

author
#4 lore raw
Quoted 10 lines · Lars-Peter Clausen

On 01/21/2015 06:59 AM, [email protected] wrote:
[...]

Quoted 6 lines

void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st)

  {
        iio_trigger_unregister(st->trig);
-       free_irq(st->client->irq, st->trig);
        iio_trigger_free(st->trig);
Quoted 7 lines

You are changing the relative order between free_irq() and iio_trigger_free() here and by doing so introduce a use-after-free race condition. The IRQ handler uses the trigger, so the IRQ has to be released before the trigger is freed.

This can be easily fixed though by changing the order of patch 1 and patch 2 in this series.

It does not make any difference if we take this patch series...?

Quoted 3 lines
Signature · 2 lines
Thanks and Regards,
Varka Bhadram.
#5 lore raw
Quoted 16 lines · Varka Bhadram
Quoted 10 lines · Lars-Peter Clausen

On 01/21/2015 06:59 AM, [email protected] wrote:
[...]

Quoted 6 lines

void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st)

  {
        iio_trigger_unregister(st->trig);
-       free_irq(st->client->irq, st->trig);
        iio_trigger_free(st->trig);
Quoted 2 lines

You are changing the relative order between free_irq() and iio_trigger_free() here and by doing so introduce a use-after-free

race

Quoted 1 line

condition. The IRQ handler uses the trigger, so the IRQ has to be

Quoted 11 lines

released

Quoted 3 lines

before the trigger is freed.

This can be easily fixed though by changing the order of patch 1 and

patch 2

Quoted 1 line

in this series.

It does not make any difference if we take this patch series...?

Quoted 3 lines

Bad practice to introduce a bug even if for only one patch... It made Lars review two changes together when they were separable .

I'd prefer them reordered but will probably cope if not!

Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
author
#6 lore raw
Quoted 26 lines · Jonathan Cameron
Quoted 23 lines · Varka Bhadram
Quoted 10 lines · Lars-Peter Clausen

On 01/21/2015 06:59 AM, [email protected] wrote:
[...]

Quoted 6 lines

void inv_mpu6050_remove_trigger(struct inv_mpu6050_state *st)

  {
        iio_trigger_unregister(st->trig);
-       free_irq(st->client->irq, st->trig);
        iio_trigger_free(st->trig);
Quoted 2 lines

You are changing the relative order between free_irq() and iio_trigger_free() here and by doing so introduce a use-after-free

race

Quoted 1 line

condition. The IRQ handler uses the trigger, so the IRQ has to be

released

Quoted 3 lines

before the trigger is freed.

This can be easily fixed though by changing the order of patch 1 and

patch 2

Quoted 1 line

in this series.

Quoted 8 lines
Quoted 4 lines

It does not make any difference if we take this patch series...?

Quoted 3 lines

Bad practice to introduce a bug even if for only one patch... It made Lars review two changes together when they were separable .

I'd prefer them reordered but will probably cope if not!

I will resend by reorder the patches...

Signature · 2 lines
Thanks and Regards,
Varka Bhadram.