[PATCH iio 1/3] imu: inv_mpu6050: use devm_request_irq

[PATCH iio 1/3] imu: inv_mpu6050: use devm_request_irq

[varkabhadram]

From: Varka Bhadram <varkab@cdac.in>

This patch use the devres API for requesting an IRQ.

Signed-off-by: Varka Bhadram <varkab@cdac.in>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index 926fcce..97f54fb 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -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);
 }
-- 
1.7.9.5

[PATCH iio 2/3] imu: inv_mpu6050: use devm_iio_trigger_alloc

[varkabhadram]

From: Varka Bhadram <varkab@cdac.in>


Signed-off-by: Varka Bhadram <varkab@cdac.in>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index 97f54fb..ab8c85a 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -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);
 }
-- 
1.7.9.5

[PATCH iio 3/3] imu: inv_mpu6050: cleanup on error check

[varkabhadram]

From: Varka Bhadram <varkab@cdac.in>


Signed-off-by: Varka Bhadram <varkab@cdac.in>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c |   17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
index ab8c85a..844610c 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
@@ -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)
-- 
1.7.9.5

Re: [PATCH iio 1/3] imu: inv_mpu6050: use devm_request_irq

[Lars-Peter Clausen]

On 01/21/2015 06:59 AM, varkabhadram@gmail.com wrote:
[...]
>   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);

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.

Re: [PATCH iio 1/3] imu: inv_mpu6050: use devm_request_irq

[Varka Bhadram]

On Wed, Jan 21, 2015 at 3:49 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 01/21/2015 06:59 AM, varkabhadram@gmail.com wrote:
> [...]
>>
>>   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);
>
>
> 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...?
>
>
>



-- 
Thanks and Regards,
Varka Bhadram.

Re: [PATCH iio 1/3] imu: inv_mpu6050: use devm_request_irq

[Jonathan Cameron]

On 21 January 2015 11:14:11 GMT+00:00, Varka Bhadram <varkabhadram@gmail.com> wrote:
>On Wed, Jan 21, 2015 at 3:49 PM, Lars-Peter Clausen <lars@metafoo.de>
>wrote:
>> On 01/21/2015 06:59 AM, varkabhadram@gmail.com wrote:
>> [...]
>>>
>>>   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);
>>
>>
>> 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...?
>>
>>
>>
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!
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH iio 1/3] imu: inv_mpu6050: use devm_request_irq

[Varka Bhadram]

On Wed, Jan 21, 2015 at 10:55 PM, Jonathan Cameron
<jic23@jic23.retrosnub.co.uk> wrote:
>
>
> On 21 January 2015 11:14:11 GMT+00:00, Varka Bhadram <varkabhadram@gmail.com> wrote:
>>On Wed, Jan 21, 2015 at 3:49 PM, Lars-Peter Clausen <lars@metafoo.de>
>>wrote:
>>> On 01/21/2015 06:59 AM, varkabhadram@gmail.com wrote:
>>> [...]
>>>>
>>>>   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);
>>>
>>>
>>> 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...?
>>>
>>>
>>>
> 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...

-- 
Thanks and Regards,
Varka Bhadram.