[PATCH] Staging: iio: trigger: Use devm functions

[PATCH] Staging: iio: trigger: Use devm functions

[Bhumika Goyal]

Use managed resource functions devm_iio_trigger_alloc and
devm_request_irq instead of iio_trigger_alloc and request_irq.
Remove corresponding calls to free_irq in the probe and remove
functions. Drop the now unnecessary goto labels and replace various
gotos with direct returns.
request_irq replacement done using coccinelle.

@platform@
identifier p, probefn, removefn;
@@
struct platform_driver p = {
  .probe = probefn,
  .remove = removefn,
};

@prb@
identifier platform.probefn, pdev;
expression list E;
expression e;

@@
probefn(struct platform_device *pdev, ...) {
  <+...
- e = request_irq(E);
+ e = devm_request_irq(&pdev->dev,E);
  ...
?-free_irq(...);
  ...+>
}

@rem depends on prb@
identifier platform.removefn;
expression e;
@@
removefn(...) {
  <...
- free_irq(...);
  ...>
}

Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
---
 drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
index 38dca69..ea4845c 100644
--- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
+++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
@@ -200,7 +200,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
 	st->timer_num = ret;
 	st->t = &iio_bfin_timer_code[st->timer_num];
 
-	st->trig = iio_trigger_alloc("bfintmr%d", st->timer_num);
+	st->trig = devm_iio_trigger_alloc(&pdev->dev, "bfintmr%d",
+                                          st->timer_num);
 	if (!st->trig)
 		return -ENOMEM;
 
@@ -211,8 +212,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
 	if (ret)
 		goto out;
 
-	ret = request_irq(st->irq, iio_bfin_tmr_trigger_isr,
-			  0, st->trig->name, st);
+	ret = devm_request_irq(&pdev->dev, st->irq, iio_bfin_tmr_trigger_isr,
+			       0, st->trig->name, st);
 	if (ret) {
 		dev_err(&pdev->dev,
 			"request IRQ-%d failed", st->irq);
@@ -229,7 +230,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
 
 		ret = peripheral_request(st->t->pin, st->trig->name);
 		if (ret)
-			goto out_free_irq;
+			return ret;
 
 		val = (unsigned long long)get_sclk() * pdata->duty_ns;
 		do_div(val, NSEC_PER_SEC);
@@ -255,8 +256,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, st);
 
 	return 0;
-out_free_irq:
-	free_irq(st->irq, st);
+
 out1:
 	iio_trigger_unregister(st->trig);
 out:
@@ -271,7 +271,6 @@ static int iio_bfin_tmr_trigger_remove(struct platform_device *pdev)
 	disable_gptimers(st->t->bit);
 	if (st->output_enable)
 		peripheral_free(st->t->pin);
-	free_irq(st->irq, st);
 	iio_trigger_unregister(st->trig);
 	iio_trigger_put(st->trig);
 
-- 
1.9.1

Re: [Outreachy kernel] [PATCH] Staging: iio: trigger: Use devm functions

[Alison Schofield]

On Mon, Sep 19, 2016 at 02:46:50AM +0530, Bhumika Goyal wrote:
> Use managed resource functions devm_iio_trigger_alloc and
> devm_request_irq instead of iio_trigger_alloc and request_irq.
> Remove corresponding calls to free_irq in the probe and remove
> functions. Drop the now unnecessary goto labels and replace various
> gotos with direct returns.
> request_irq replacement done using coccinelle.

Hi Bhumika, 
Thanks for the patch.  This seems like it's worthy of 2 separate
patches. 

Also, I'm a bit confused with why I don't see an iio_trigger_free
that is going away, when you switch to the devm_iio_trigger_alloc
functions, but I realize there isn't one there to remove. hmm???

alisons



> 
> @platform@
> identifier p, probefn, removefn;
> @@
> struct platform_driver p = {
>   .probe = probefn,
>   .remove = removefn,
> };
> 
> @prb@
> identifier platform.probefn, pdev;
> expression list E;
> expression e;
> 
> @@
> probefn(struct platform_device *pdev, ...) {
>   <+...
> - e = request_irq(E);
> + e = devm_request_irq(&pdev->dev,E);
>   ...
> ?-free_irq(...);
>   ...+>
> }
> 
> @rem depends on prb@
> identifier platform.removefn;
> expression e;
> @@
> removefn(...) {
>   <...
> - free_irq(...);
>   ...>
> }
> 
> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
> ---
>  drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> index 38dca69..ea4845c 100644
> --- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> +++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> @@ -200,7 +200,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
>  	st->timer_num = ret;
>  	st->t = &iio_bfin_timer_code[st->timer_num];
>  
> -	st->trig = iio_trigger_alloc("bfintmr%d", st->timer_num);
> +	st->trig = devm_iio_trigger_alloc(&pdev->dev, "bfintmr%d",
> +                                          st->timer_num);
>  	if (!st->trig)
>  		return -ENOMEM;
>  
> @@ -211,8 +212,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto out;
>  
> -	ret = request_irq(st->irq, iio_bfin_tmr_trigger_isr,
> -			  0, st->trig->name, st);
> +	ret = devm_request_irq(&pdev->dev, st->irq, iio_bfin_tmr_trigger_isr,
> +			       0, st->trig->name, st);
>  	if (ret) {
>  		dev_err(&pdev->dev,
>  			"request IRQ-%d failed", st->irq);
> @@ -229,7 +230,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
>  
>  		ret = peripheral_request(st->t->pin, st->trig->name);
>  		if (ret)
> -			goto out_free_irq;
> +			return ret;
>  
>  		val = (unsigned long long)get_sclk() * pdata->duty_ns;
>  		do_div(val, NSEC_PER_SEC);
> @@ -255,8 +256,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, st);
>  
>  	return 0;
> -out_free_irq:
> -	free_irq(st->irq, st);
> +
>  out1:
>  	iio_trigger_unregister(st->trig);
>  out:
> @@ -271,7 +271,6 @@ static int iio_bfin_tmr_trigger_remove(struct platform_device *pdev)
>  	disable_gptimers(st->t->bit);
>  	if (st->output_enable)
>  		peripheral_free(st->t->pin);
> -	free_irq(st->irq, st);
>  	iio_trigger_unregister(st->trig);
>  	iio_trigger_put(st->trig);
>  
> -- 
> 1.9.1
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1474233410-6932-1-git-send-email-bhumirks%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [Outreachy kernel] [PATCH] Staging: iio: trigger: Use devm functions

[Bhumika Goyal]

On Mon, Sep 19, 2016 at 11:14 AM, Alison Schofield <amsfield22@gmail.com> wrote:
> On Mon, Sep 19, 2016 at 02:46:50AM +0530, Bhumika Goyal wrote:
>> Use managed resource functions devm_iio_trigger_alloc and
>> devm_request_irq instead of iio_trigger_alloc and request_irq.
>> Remove corresponding calls to free_irq in the probe and remove
>> functions. Drop the now unnecessary goto labels and replace various
>> gotos with direct returns.
>> request_irq replacement done using coccinelle.
>
> Hi Bhumika,
> Thanks for the patch.  This seems like it's worthy of 2 separate
> patches.
>
> Also, I'm a bit confused with why I don't see an iio_trigger_free
> that is going away, when you switch to the devm_iio_trigger_alloc
> functions, but I realize there isn't one there to remove. hmm???
>
> alisons
>

Hey Alison,
Ok, I will separate this in two patches. I will do
devm_iio_trigger_alloc in patch 1 and devm_request_irq in patch 2 as
devm_request_irq requires all previous memory allocations to be devms.
Regarding iio_trigger_free I didn't find any in either of the two
functions(probe and remove).
Thanks for the input.

Thanks,
Bhumika
>
>
>>
>> @platform@
>> identifier p, probefn, removefn;
>> @@
>> struct platform_driver p = {
>>   .probe = probefn,
>>   .remove = removefn,
>> };
>>
>> @prb@
>> identifier platform.probefn, pdev;
>> expression list E;
>> expression e;
>>
>> @@
>> probefn(struct platform_device *pdev, ...) {
>>   <+...
>> - e = request_irq(E);
>> + e = devm_request_irq(&pdev->dev,E);
>>   ...
>> ?-free_irq(...);
>>   ...+>
>> }
>>
>> @rem depends on prb@
>> identifier platform.removefn;
>> expression e;
>> @@
>> removefn(...) {
>>   <...
>> - free_irq(...);
>>   ...>
>> }
>>
>> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
>> ---
>>  drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
>> index 38dca69..ea4845c 100644
>> --- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
>> +++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
>> @@ -200,7 +200,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
>>       st->timer_num = ret;
>>       st->t = &iio_bfin_timer_code[st->timer_num];
>>
>> -     st->trig = iio_trigger_alloc("bfintmr%d", st->timer_num);
>> +     st->trig = devm_iio_trigger_alloc(&pdev->dev, "bfintmr%d",
>> +                                          st->timer_num);
>>       if (!st->trig)
>>               return -ENOMEM;
>>
>> @@ -211,8 +212,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
>>       if (ret)
>>               goto out;
>>
>> -     ret = request_irq(st->irq, iio_bfin_tmr_trigger_isr,
>> -                       0, st->trig->name, st);
>> +     ret = devm_request_irq(&pdev->dev, st->irq, iio_bfin_tmr_trigger_isr,
>> +                            0, st->trig->name, st);
>>       if (ret) {
>>               dev_err(&pdev->dev,
>>                       "request IRQ-%d failed", st->irq);
>> @@ -229,7 +230,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
>>
>>               ret = peripheral_request(st->t->pin, st->trig->name);
>>               if (ret)
>> -                     goto out_free_irq;
>> +                     return ret;
>>
>>               val = (unsigned long long)get_sclk() * pdata->duty_ns;
>>               do_div(val, NSEC_PER_SEC);
>> @@ -255,8 +256,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
>>       platform_set_drvdata(pdev, st);
>>
>>       return 0;
>> -out_free_irq:
>> -     free_irq(st->irq, st);
>> +
>>  out1:
>>       iio_trigger_unregister(st->trig);
>>  out:
>> @@ -271,7 +271,6 @@ static int iio_bfin_tmr_trigger_remove(struct platform_device *pdev)
>>       disable_gptimers(st->t->bit);
>>       if (st->output_enable)
>>               peripheral_free(st->t->pin);
>> -     free_irq(st->irq, st);
>>       iio_trigger_unregister(st->trig);
>>       iio_trigger_put(st->trig);
>>
>> --
>> 1.9.1
>>
>> --
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1474233410-6932-1-git-send-email-bhumirks%40gmail.com.
>> For more options, visit https://groups.google.com/d/optout.

Re: [Outreachy kernel] [PATCH] Staging: iio: trigger: Use devm functions

[Julia Lawall]

On Mon, 19 Sep 2016, Bhumika Goyal wrote:

> On Mon, Sep 19, 2016 at 11:14 AM, Alison Schofield <amsfield22@gmail.com> wrote:
> > On Mon, Sep 19, 2016 at 02:46:50AM +0530, Bhumika Goyal wrote:
> >> Use managed resource functions devm_iio_trigger_alloc and
> >> devm_request_irq instead of iio_trigger_alloc and request_irq.
> >> Remove corresponding calls to free_irq in the probe and remove
> >> functions. Drop the now unnecessary goto labels and replace various
> >> gotos with direct returns.
> >> request_irq replacement done using coccinelle.
> >
> > Hi Bhumika,
> > Thanks for the patch.  This seems like it's worthy of 2 separate
> > patches.
> >
> > Also, I'm a bit confused with why I don't see an iio_trigger_free
> > that is going away, when you switch to the devm_iio_trigger_alloc
> > functions, but I realize there isn't one there to remove. hmm???
> >
> > alisons
> >
>
> Hey Alison,
> Ok, I will separate this in two patches. I will do
> devm_iio_trigger_alloc in patch 1 and devm_request_irq in patch 2 as
> devm_request_irq requires all previous memory allocations to be devms.
> Regarding iio_trigger_free I didn't find any in either of the two
> functions(probe and remove).
> Thanks for the input.

I think that normally people devmify everything at once.

I dimly recollect that iio_trigger_unregister does the free.  Check
whether this is the case, and whether it is devm safe.

julia


>
> Thanks,
> Bhumika
> >
> >
> >>
> >> @platform@
> >> identifier p, probefn, removefn;
> >> @@
> >> struct platform_driver p = {
> >>   .probe = probefn,
> >>   .remove = removefn,
> >> };
> >>
> >> @prb@
> >> identifier platform.probefn, pdev;
> >> expression list E;
> >> expression e;
> >>
> >> @@
> >> probefn(struct platform_device *pdev, ...) {
> >>   <+...
> >> - e = request_irq(E);
> >> + e = devm_request_irq(&pdev->dev,E);
> >>   ...
> >> ?-free_irq(...);
> >>   ...+>
> >> }
> >>
> >> @rem depends on prb@
> >> identifier platform.removefn;
> >> expression e;
> >> @@
> >> removefn(...) {
> >>   <...
> >> - free_irq(...);
> >>   ...>
> >> }
> >>
> >> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
> >> ---
> >>  drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 13 ++++++-------
> >>  1 file changed, 6 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> >> index 38dca69..ea4845c 100644
> >> --- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> >> +++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> >> @@ -200,7 +200,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
> >>       st->timer_num = ret;
> >>       st->t = &iio_bfin_timer_code[st->timer_num];
> >>
> >> -     st->trig = iio_trigger_alloc("bfintmr%d", st->timer_num);
> >> +     st->trig = devm_iio_trigger_alloc(&pdev->dev, "bfintmr%d",
> >> +                                          st->timer_num);
> >>       if (!st->trig)
> >>               return -ENOMEM;
> >>
> >> @@ -211,8 +212,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
> >>       if (ret)
> >>               goto out;
> >>
> >> -     ret = request_irq(st->irq, iio_bfin_tmr_trigger_isr,
> >> -                       0, st->trig->name, st);
> >> +     ret = devm_request_irq(&pdev->dev, st->irq, iio_bfin_tmr_trigger_isr,
> >> +                            0, st->trig->name, st);
> >>       if (ret) {
> >>               dev_err(&pdev->dev,
> >>                       "request IRQ-%d failed", st->irq);
> >> @@ -229,7 +230,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
> >>
> >>               ret = peripheral_request(st->t->pin, st->trig->name);
> >>               if (ret)
> >> -                     goto out_free_irq;
> >> +                     return ret;
> >>
> >>               val = (unsigned long long)get_sclk() * pdata->duty_ns;
> >>               do_div(val, NSEC_PER_SEC);
> >> @@ -255,8 +256,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
> >>       platform_set_drvdata(pdev, st);
> >>
> >>       return 0;
> >> -out_free_irq:
> >> -     free_irq(st->irq, st);
> >> +
> >>  out1:
> >>       iio_trigger_unregister(st->trig);
> >>  out:
> >> @@ -271,7 +271,6 @@ static int iio_bfin_tmr_trigger_remove(struct platform_device *pdev)
> >>       disable_gptimers(st->t->bit);
> >>       if (st->output_enable)
> >>               peripheral_free(st->t->pin);
> >> -     free_irq(st->irq, st);
> >>       iio_trigger_unregister(st->trig);
> >>       iio_trigger_put(st->trig);
> >>
> >> --
> >> 1.9.1
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> >> To post to this group, send email to outreachy-kernel@googlegroups.com.
> >> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1474233410-6932-1-git-send-email-bhumirks%40gmail.com.
> >> For more options, visit https://groups.google.com/d/optout.
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAOH%2B1jFsWGDVcifqDU0_9MPw%3DuEXQ%2Bu0E8dg0hkimx%2BeJbWq1Q%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH] Staging: iio: trigger: Use devm functions

[Alison Schofield]

On Mon, Sep 19, 2016 at 10:06:08PM +0200, Lars-Peter Clausen wrote:
> On 09/19/2016 09:29 PM, Jonathan Cameron wrote:
> > On 19/09/16 10:21, Julia Lawall wrote:
> >>
> >>
> >> On Mon, 19 Sep 2016, Bhumika Goyal wrote:
> >>
> >>> On Mon, Sep 19, 2016 at 11:14 AM, Alison Schofield <amsfield22@gmail.com> wrote:
> >>>> On Mon, Sep 19, 2016 at 02:46:50AM +0530, Bhumika Goyal wrote:
> >>>>> Use managed resource functions devm_iio_trigger_alloc and
> >>>>> devm_request_irq instead of iio_trigger_alloc and request_irq.
> >>>>> Remove corresponding calls to free_irq in the probe and remove
> >>>>> functions. Drop the now unnecessary goto labels and replace various
> >>>>> gotos with direct returns.
> >>>>> request_irq replacement done using coccinelle.
> >>>>
> >>>> Hi Bhumika,
> >>>> Thanks for the patch.  This seems like it's worthy of 2 separate
> >>>> patches.
> >>>>
> >>>> Also, I'm a bit confused with why I don't see an iio_trigger_free
> >>>> that is going away, when you switch to the devm_iio_trigger_alloc
> >>>> functions, but I realize there isn't one there to remove. hmm???
> >>>>
> >>>> alisons
> >>>>
> >>>
> >>> Hey Alison,
> >>> Ok, I will separate this in two patches. I will do
> >>> devm_iio_trigger_alloc in patch 1 and devm_request_irq in patch 2 as
> >>> devm_request_irq requires all previous memory allocations to be devms.
> >>> Regarding iio_trigger_free I didn't find any in either of the two
> >>> functions(probe and remove).
> >>> Thanks for the input.
> >>
> >> I think that normally people devmify everything at once.
> > I would as well.  It is 'kind of' one change...
> >>
> >> I dimly recollect that iio_trigger_unregister does the free.  Check
> >> whether this is the case, and whether it is devm safe.
> > Given it's referenced on the next line I certainly hope it doesn't ;)
> > There are quite a few reasons this one is in staging!
> 
> For some reason this driver uses iio_trigger_put() instead of
> iio_trigger_free(). Which is wrong, since iio_trigger_put() does an
> additional module_put() which would release a reference we've never gained.
> So the first fix should be to replace iio_trigger_put() with
> iio_trigger_free() (which is a patch for stable) and then switch to the devm
> version and remove the iio_trigger_free() (which is a patch for next).
> 
> This does not seem to be the only driver that gets this wrong,
> iio-trig-sysfs and iio-trig-interrupt are also affected. These would make a
> good target for further patches.
> 
> Bonus points if you can present a proof-of-concept that exploits this bug to
> run arbitrary code with kernel privileges.
> 
> There seems to be another bug regarding trigger reference counting in
> iio_trigger_write_current(). The reference to the trigger should be acquired
> in iio_trigger_find_by_name() while holding mutex. Otherwise we risk that
> the trigger is already freed before we grab the reference later in
> iio_trigger_write_current(). That's another opportunity for sending a good
> patch and also some opportunity for bonus points.
> 

Hi Bhumika,
I have interest in working this issue.  Let me know if it's ok for
me to run with.  
Thanks!
alisons

Re: [Outreachy kernel] [PATCH] Staging: iio: trigger: Use devm functions

[Bhumika Goyal]

On Sat, Oct 15, 2016 at 12:57 AM, Alison Schofield <amsfield22@gmail.com> wrote:
> On Mon, Sep 19, 2016 at 10:06:08PM +0200, Lars-Peter Clausen wrote:
>> On 09/19/2016 09:29 PM, Jonathan Cameron wrote:
>> > On 19/09/16 10:21, Julia Lawall wrote:
>> >>
>> >>
>> >> On Mon, 19 Sep 2016, Bhumika Goyal wrote:
>> >>
>> >>> On Mon, Sep 19, 2016 at 11:14 AM, Alison Schofield <amsfield22@gmail.com> wrote:
>> >>>> On Mon, Sep 19, 2016 at 02:46:50AM +0530, Bhumika Goyal wrote:
>> >>>>> Use managed resource functions devm_iio_trigger_alloc and
>> >>>>> devm_request_irq instead of iio_trigger_alloc and request_irq.
>> >>>>> Remove corresponding calls to free_irq in the probe and remove
>> >>>>> functions. Drop the now unnecessary goto labels and replace various
>> >>>>> gotos with direct returns.
>> >>>>> request_irq replacement done using coccinelle.
>> >>>>
>> >>>> Hi Bhumika,
>> >>>> Thanks for the patch.  This seems like it's worthy of 2 separate
>> >>>> patches.
>> >>>>
>> >>>> Also, I'm a bit confused with why I don't see an iio_trigger_free
>> >>>> that is going away, when you switch to the devm_iio_trigger_alloc
>> >>>> functions, but I realize there isn't one there to remove. hmm???
>> >>>>
>> >>>> alisons
>> >>>>
>> >>>
>> >>> Hey Alison,
>> >>> Ok, I will separate this in two patches. I will do
>> >>> devm_iio_trigger_alloc in patch 1 and devm_request_irq in patch 2 as
>> >>> devm_request_irq requires all previous memory allocations to be devms.
>> >>> Regarding iio_trigger_free I didn't find any in either of the two
>> >>> functions(probe and remove).
>> >>> Thanks for the input.
>> >>
>> >> I think that normally people devmify everything at once.
>> > I would as well.  It is 'kind of' one change...
>> >>
>> >> I dimly recollect that iio_trigger_unregister does the free.  Check
>> >> whether this is the case, and whether it is devm safe.
>> > Given it's referenced on the next line I certainly hope it doesn't ;)
>> > There are quite a few reasons this one is in staging!
>>
>> For some reason this driver uses iio_trigger_put() instead of
>> iio_trigger_free(). Which is wrong, since iio_trigger_put() does an
>> additional module_put() which would release a reference we've never gained.
>> So the first fix should be to replace iio_trigger_put() with
>> iio_trigger_free() (which is a patch for stable) and then switch to the devm
>> version and remove the iio_trigger_free() (which is a patch for next).
>>
>> This does not seem to be the only driver that gets this wrong,
>> iio-trig-sysfs and iio-trig-interrupt are also affected. These would make a
>> good target for further patches.
>>
>> Bonus points if you can present a proof-of-concept that exploits this bug to
>> run arbitrary code with kernel privileges.
>>
>> There seems to be another bug regarding trigger reference counting in
>> iio_trigger_write_current(). The reference to the trigger should be acquired
>> in iio_trigger_find_by_name() while holding mutex. Otherwise we risk that
>> the trigger is already freed before we grab the reference later in
>> iio_trigger_write_current(). That's another opportunity for sending a good
>> patch and also some opportunity for bonus points.
>>
>
> Hi Bhumika,
> I have interest in working this issue.  Let me know if it's ok for
> me to run with.
> Thanks!

Hey Alison,
You can surely go ahead with this issue :)

Thanks,
Bhumika

> alisons