ARM: omap: edma: add suspend suspend/resume hooks
Subsystems (2)
PATCH v6 1/1 ARM: omap: edma: add suspend suspend/resume hooks
This patch makes the edma driver resume correctly after suspend. Tested on an AM33xx platform with cyclic audio streams and omap_hsmmc.
All information can be reconstructed by already known runtime information.
As we now use some functions that were previously only used from __init context, annotations had to be dropped.
---
v6:
amended version from Nishanth Menon, adding error handling for runtime, and using suspend_late/early_resume.
arch/arm/common/edma.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 91 insertions(+), 3 deletions(-)
arch/arm/common/edma.c +91 -3
@@ -239,6 +239,8 @@ struct edma {/* list of channels with no even trigger; terminated by "-1" */ const s8 *noevent; +struct edma_soc_info *info; +/* The edma_inuse bit for each PaRAM slot is clear unless the * channel is in use ... by ARM or DSP, for QDMA, or whatever. */
@@ -290,13 +292,13 @@ static void map_dmach_queue(unsigned ctlr, unsigned ch_no, ~(0x7 << bit), queue_no << bit); } -static void __init map_queue_tc (unsigned ctlr,int queue_no,int tc_no) +static void map_queue_tc (unsigned ctlr,int queue_no,int tc_no) {int bit = queue_no *4 ;edma_modify (ctlr,EDMA_QUETCMAP , ~(0x7 << bit), ((tc_no &0x7 ) << bit)); } -static void __init assign_priority_to_queue (unsigned ctlr,int queue_no, +static void assign_priority_to_queue (unsigned ctlr,int queue_no,int priority) {int bit = queue_no *4 ;
@@ -315,7 +317,7 @@ static void __init assign_priority_to_queue(unsigned ctlr, int queue_no,* included in that particular EDMA variant (Eg : dm646x) * */ -static void __init map_dmach_param (unsigned ctlr) +static void map_dmach_param (unsigned ctlr) {int i;for (i =0 ; i <EDMA_MAX_DMACH ; i++)
@@ -1785,15 +1787,101 @@ static int edma_probe(struct platform_device *pdev)edma_write_array2 (j,EDMA_DRAE , i,1 ,0x0 );edma_write_array (j,EDMA_QRAE , i,0x0 ); } + edma_cc[j]->info = info[j]; arch_num_cc++; }return 0 ; } +static int edma_pm_suspend (struct device *dev) +{ +int j, r; + + r =pm_runtime_get_sync (dev); +if (IS_ERR_VALUE (r)) { +dev_err (dev,"%s: get_sync returned %d\n" , __func__, r); +return r; + } + +for (j =0 ; j < arch_num_cc; j++) { +struct edma *ecc = edma_cc[j]; + +disable_irq (ecc->irq_res_start); +disable_irq (ecc->irq_res_end); + } + +pm_runtime_put_sync (dev); + +return 0 ; +} + +static int edma_pm_resume (struct device *dev) +{ +int i, j, r; + + r =pm_runtime_get_sync (dev); +if (IS_ERR_VALUE (r)) { +dev_err (dev,"%s: get_sync returned %d\n" , __func__, r); +return r; + } + +for (j =0 ; j < arch_num_cc; j++) { +struct edma *cc = edma_cc[j]; + +s8 (*queue_priority_mapping)[2 ]; +s8 (*queue_tc_mapping)[2 ]; + + queue_tc_mapping = cc->info->queue_tc_mapping; + queue_priority_mapping = cc->info->queue_priority_mapping; + +/* Event queue to TC mapping */ +for (i =0 ; queue_tc_mapping[i][0 ] !=-1 ; i++) +map_queue_tc (j, queue_tc_mapping[i][0 ], + queue_tc_mapping[i][1 ]); + +/* Event queue priority mapping */ +for (i =0 ; queue_priority_mapping[i][0 ] !=-1 ; i++) +assign_priority_to_queue (j, + queue_priority_mapping[i][0 ], + queue_priority_mapping[i][1 ]); + +/* Map the channel to param entry if channel mapping logic +* exist +*/ +if (edma_read (j,EDMA_CCCFG ) &CHMAP_EXIST ) +map_dmach_param (j); + +for (i =0 ; i < cc->num_channels; i++) { +if (test_bit (i, cc->edma_inuse)) { +/* ensure access through shadow region 0 */ +edma_or_array2 (j,EDMA_DRAE ,0 , i >>5 , +BIT (i &0x1f )); + +setup_dma_interrupt (i, + cc->intr_data[i].callback, + cc->intr_data[i].data); + } + } + +enable_irq (cc->irq_res_start); +enable_irq (cc->irq_res_end); + } + +pm_runtime_put_sync (dev); + +return 0 ; +} + +static const struct dev_pm_ops edma_pm_ops = { + .suspend_late = edma_pm_suspend, + .resume_early = edma_pm_resume, +}; +static struct platform_driver edma_driver = { .driver = { .name ="edma" , + .pm = &edma_pm_ops, .of_match_table = edma_of_ids, }, .probe = edma_probe,
Signature · 1 line
1.8.4.2
PATCH v6 1/1 ARM: omap: edma: add suspend suspend/resume hooks
Quoted 6 lines · Daniel Mack
This patch makes the edma driver resume correctly after suspend. Tested on an AM33xx platform with cyclic audio streams and
omap_hsmmc.All information can be reconstructed by already known runtime information.
Quoted 8 lines
As we now use some functions that were previously only used from
__initcontext, annotations had to be dropped.[nm at ti.com: added error handling for runtime + suspend_late/early_resume]Signed-off-by Nishanth Menon <[email protected]>Signed-off-by Daniel Mack <[email protected]>Tested-by Joel Fernandes <[email protected]>Acked-by Joel Fernandes <[email protected]>
Hi Sekhar,
Can you consider pulling this patch? It has been tested and Acked. Thanks.
regards,
-Joel
PATCH v6 1/1 ARM: omap: edma: add suspend suspend/resume hooks
+ Kevin
Quoted 16 lines · Joel Fernandes
Quoted 6 lines · Daniel Mack
This patch makes the edma driver resume correctly after suspend. Tested on an AM33xx platform with cyclic audio streams and
omap_hsmmc.All information can be reconstructed by already known runtime information.
Quoted 8 lines
As we now use some functions that were previously only used from
__initcontext, annotations had to be dropped.[nm at ti.com: added error handling for runtime + suspend_late/early_resume]Signed-off-by Nishanth Menon <[email protected]>Signed-off-by Daniel Mack <[email protected]>Tested-by Joel Fernandes <[email protected]>Acked-by Joel Fernandes <[email protected]>
Quoted 3 lines
Hi Sekhar,
Can you consider pulling this patch? It has been tested and Acked. Thanks.
Kevin had some inputs on previous version of this patch. Were you able to make sure he is okay with this version being merged?
I have some comments for which I will send another e-mail.
Thanks,
Sekhar
PATCH v6 1/1 ARM: omap: edma: add suspend suspend/resume hooks
Quoted 19 lines · Sekhar Nori
+ Kevin
Quoted 16 lines · Joel Fernandes
Quoted 14 lines · Daniel Mack
This patch makes the edma driver resume correctly after suspend. Tested on an AM33xx platform with cyclic audio streams and
omap_hsmmc.All information can be reconstructed by already known runtime information.
As we now use some functions that were previously only used from
__initcontext, annotations had to be dropped.[nm at ti.com: added error handling for runtime + suspend_late/early_resume]Signed-off-by Nishanth Menon <[email protected]>Signed-off-by Daniel Mack <[email protected]>Tested-by Joel Fernandes <[email protected]>Acked-by Joel Fernandes <[email protected]>
Quoted 6 lines
Quoted 3 lines
Hi Sekhar,
Can you consider pulling this patch? It has been tested and Acked. Thanks.
Kevin had some inputs on previous version of this patch. Were you able to make sure he is okay with this version being merged?
I had concerns about the feedback I got, and haven't got answers yet.
In particular, I'm not convinced that using runtime PM to suspend channels would actually save any power during runtime, or have any other benefit. But I might be wrong - maybe someone at TI could comment on that?
Thanks,
Daniel
PATCH v6 1/1 ARM: omap: edma: add suspend suspend/resume hooks
Quoted 24 lines · Daniel Mack
Quoted 23 lines · Sekhar Nori
+ Kevin
Quoted 19 lines · Joel Fernandes
Quoted 6 lines · Daniel Mack
This patch makes the edma driver resume correctly after suspend. Tested on an AM33xx platform with cyclic audio streams and
omap_hsmmc.All information can be reconstructed by already known runtime information.
Quoted 8 lines
As we now use some functions that were previously only used from
__initcontext, annotations had to be dropped.[nm at ti.com: added error handling for runtime + suspend_late/early_resume]Signed-off-by Nishanth Menon <[email protected]>Signed-off-by Daniel Mack <[email protected]>Tested-by Joel Fernandes <[email protected]>Acked-by Joel Fernandes <[email protected]>Hi Sekhar,
Can you consider pulling this patch? It has been tested and Acked. Thanks.
Quoted 8 lines
Quoted 2 lines
Kevin had some inputs on previous version of this patch. Were you able to make sure he is okay with this version being merged?
I had concerns about the feedback I got, and haven't got answers yet.
In particular, I'm not convinced that using runtime PM to suspend channels would actually save any power during runtime, or have any other benefit.
/me returning from a week off
The amount of power to be saved depends on the activity in the system. If DMA is unused, at least the clocks could be gated allowing the possibility of the enclosing power domain to be gated if other devices are also clock gated, etc. etc.
However, my comments were not really about power saving, they were about designing things in a way that are scalable and match the longer term goals of converting drivers to be runtime PM centric. For example, if someone did want to add real runtime PM to this driver later, they would need to rework much of this. So my suggestion was to do runtime PM the right way from the beginning.
That being said, I'm not the maintainer of this driver so don't get to make the final call. I will just say that from what I've seen here, I don't think this is the right approach.
Kevin
PATCH v6 1/1 ARM: omap: edma: add suspend suspend/resume hooks
In reply to Daniel Mack+ Kevin
Quoted 69 lines · Daniel Mack
This patch makes the edma driver resume correctly after suspend. Tested on an AM33xx platform with cyclic audio streams and
omap_hsmmc.All information can be reconstructed by already known runtime information.
As we now use some functions that were previously only used from
__initcontext, annotations had to be dropped.[nm at ti.com: added error handling for runtime + suspend_late/early_resume]Signed-off-by Nishanth Menon <[email protected]>Signed-off-by Daniel Mack <[email protected]>Tested-by Joel Fernandes <[email protected]>Acked-by Joel Fernandes <[email protected]>---
v6:
amended version from Nishanth Menon, adding error handling for runtime, and using suspend_late/early_resume.arch/arm/common/edma.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 91 insertions(+), 3 deletions(-)arch/arm/common/edma.c +6
-3@@ -239,6 +239,8 @@ struct edma {/* list of channels with no even trigger; terminated by "-1" */ const s8 *noevent; +struct edma_soc_info *info; +/* The edma_inuse bit for each PaRAM slot is clear unless the * channel is in use ... by ARM or DSP, for QDMA, or whatever. */ @@ -290,13 +292,13 @@ static void map_dmach_queue(unsigned ctlr, unsigned ch_no, ~(0x7 << bit), queue_no << bit); } -static void __init map_queue_tc (unsigned ctlr,int queue_no,int tc_no) +static void map_queue_tc (unsigned ctlr,int queue_no,int tc_no) {int bit = queue_no *4 ;edma_modify (ctlr,EDMA_QUETCMAP , ~(0x7 << bit), ((tc_no &0x7 ) << bit)); } -static void __init assign_priority_to_queue (unsigned ctlr,int queue_no, +static void assign_priority_to_queue (unsigned ctlr,int queue_no,int priority) {int bit = queue_no *4 ;@@ -315,7 +317,7 @@ static void __init assign_priority_to_queue(unsigned ctlr, int queue_no,* included in that particular EDMA variant (Eg : dm646x) * */ -static void __init map_dmach_param (unsigned ctlr) +static void map_dmach_param (unsigned ctlr) {int i;for (i =0 ; i <EDMA_MAX_DMACH ; i++)@@ -1785,15 +1787,101 @@ static int edma_probe(struct platform_device *pdev)edma_write_array2 (j,EDMA_DRAE , i,1 ,0x0 );edma_write_array (j,EDMA_QRAE , i,0x0 ); } + edma_cc[j]->info = info[j]; arch_num_cc++; }
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1802
Quoted 10 lines
arch/arm/common/edma.c +6
-0return 0 ; } +static int edma_pm_suspend (struct device *dev) +{ +int j, r; + + r =pm_runtime_get_sync (dev); +if (IS_ERR_VALUE (r)) {
So IS_ERR_VALUE() is only for functions which may return a negative number outside of MAX_ERRNO as a success indication. pm_runtime_get_sync() does not appear to be one of them so just use"
if (r < 0) { .. }
Quoted 12 lines
arch/arm/common/edma.c +12
-0+dev_err (dev,"%s: get_sync returned %d\n" , __func__, r); +return r; + } + +for (j =0 ; j < arch_num_cc; j++) { +struct edma *ecc = edma_cc[j]; + +disable_irq (ecc->irq_res_start); +disable_irq (ecc->irq_res_end); + } + +pm_runtime_put_sync (dev);
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1824
Quoted 10 lines
arch/arm/common/edma.c +10
-0+ +return 0 ; +} + +static int edma_pm_resume (struct device *dev) +{ +int i, j, r; + + r =pm_runtime_get_sync (dev); +if (IS_ERR_VALUE (r)) {
Same here as above.
Quoted 17 lines
arch/arm/common/edma.c +17
-0+dev_err (dev,"%s: get_sync returned %d\n" , __func__, r); +return r; + } + +for (j =0 ; j < arch_num_cc; j++) { +struct edma *cc = edma_cc[j]; + +s8 (*queue_priority_mapping)[2 ]; +s8 (*queue_tc_mapping)[2 ]; + + queue_tc_mapping = cc->info->queue_tc_mapping; + queue_priority_mapping = cc->info->queue_priority_mapping; + +/* Event queue to TC mapping */ +for (i =0 ; queue_tc_mapping[i][0 ] !=-1 ; i++) +map_queue_tc (j, queue_tc_mapping[i][0 ], + queue_tc_mapping[i][1 ]);
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1851
Quoted 10 lines
arch/arm/common/edma.c +10
-0+ +/* Event queue priority mapping */ +for (i =0 ; queue_priority_mapping[i][0 ] !=-1 ; i++) +assign_priority_to_queue (j, + queue_priority_mapping[i][0 ], + queue_priority_mapping[i][1 ]); + +/* Map the channel to param entry if channel mapping logic +* exist +*/
Please follow the multi-line commenting style.
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1859
Quoted 8 lines
arch/arm/common/edma.c +8
-0+if (edma_read (j,EDMA_CCCFG ) &CHMAP_EXIST ) +map_dmach_param (j); + +for (i =0 ; i < cc->num_channels; i++) { +if (test_bit (i, cc->edma_inuse)) { +/* ensure access through shadow region 0 */ +edma_or_array2 (j,EDMA_DRAE ,0 , i >>5 , +BIT (i &0x1f ));
There are some checkpatch checks that result from lines like this. Please fix these as well.
CHECK: Alignment should match open parenthesis
#179: FILE: arch/arm/common/edma.c:1841:
+ map_queue_tc(j, queue_tc_mapping[i][0],
+ queue_tc_mapping[i][1]);
CHECK: Alignment should match open parenthesis
#184: FILE: arch/arm/common/edma.c:1846:
+ assign_priority_to_queue(j,
+ queue_priority_mapping[i][0],
CHECK: Alignment should match open parenthesis
#197: FILE: arch/arm/common/edma.c:1859:
+ edma_or_array2(j, EDMA_DRAE, 0, i >> 5,
+ BIT(i & 0x1f));
total: 0 errors, 0 warnings, 3 checks, 132 lines checked
Thanks,
Sekhar
PATCH v6 1/1 ARM: omap: edma: add suspend suspend/resume hooks
Hi Sekhar,
Quoted 13 lines · Sekhar Nori
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1802
Quoted 6 lines · Daniel Mack
arch/arm/common/edma.c +6
-0+static int edma_pm_suspend (struct device *dev) +{ +int j, r; + + r =pm_runtime_get_sync (dev); +if (IS_ERR_VALUE (r)) {So
IS_ERR_VALUE()is only for functions which may return a negative number outside ofMAX_ERRNOas a success indication.pm_runtime_get_sync()does not appear to be one of them so just use"
Quoted 1 line
if (r < 0) { .. }
That's true. Thanks for catching this, I'll fix it. However, grepping through the tree, there are quite a lot places where the same mistake is made.
Quoted 5 lines
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1851
Quoted 3 lines
arch/arm/common/edma.c +3
-0+/* Map the channel to param entry if channel mapping logic +* exist +*/ Please follow the multi-line commenting style.
Can do. However, these lines in fact follow the style that is used throughout the entire file ;)
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1841
Quoted 7 lines
There are some checkpatch checks that result from lines like this. Please fix these as well.
CHECK: Alignment should match open parenthesis
#179: FILE:arch/arm/common/edma.c:1841:arch/arm/common/edma.c +2
-0+map_queue_tc (j, queue_tc_mapping[i][0 ], + queue_tc_mapping[i][1 ]);
If you say so, even though I disagree with checkpatch.pl here. The above is actually more readable, right? :)
Thanks,
Daniel
PATCH v6 1/1 ARM: omap: edma: add suspend suspend/resume hooks
Quoted 16 lines · Daniel Mack
Hi Sekhar,
Quoted 13 lines · Sekhar Nori
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1802
Quoted 6 lines · Daniel Mack
arch/arm/common/edma.c +6
-0+static int edma_pm_suspend (struct device *dev) +{ +int j, r; + + r =pm_runtime_get_sync (dev); +if (IS_ERR_VALUE (r)) {So
IS_ERR_VALUE()is only for functions which may return a negative number outside ofMAX_ERRNOas a success indication.pm_runtime_get_sync()does not appear to be one of them so just use"
Quoted 5 lines
Quoted 1 line
if (r < 0) { .. }
That's true. Thanks for catching this, I'll fix it. However, grepping through the tree, there are quite a lot places where the same mistake is made.
Yes, this is a common fallacy. Russell cleaned up a bunch of these a while back.
Quoted 9 lines
Quoted 6 lines
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1851
Quoted 4 lines
arch/arm/common/edma.c +3
-0+/* Map the channel to param entry if channel mapping logic +* exist +*/ Please follow the multi-line commenting style.
Can do. However, these lines in fact follow the style that is used throughout the entire file ;)
:) I did not compare the rest of the file, but hey the bar keep rising all the time.
Quoted 11 lines
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1841
Quoted 8 lines
There are some checkpatch checks that result from lines like this. Please fix these as well.
CHECK: Alignment should match open parenthesis
#179: FILE:arch/arm/common/edma.c:1841:arch/arm/common/edma.c +2
-0+map_queue_tc (j, queue_tc_mapping[i][0 ], + queue_tc_mapping[i][1 ]);If you say so, even though I disagree with checkpatch.pl here. The above is actually more readable, right? :)
In this particular case, I agree so I am okay if you keep it as is. The rest of the two reports are valid though.
Thanks,
Sekhar
pm_runtime functions and IS_ERR_VALUE (was Re: [PATCH v6] ARM: omap: edma: add suspend suspend/resume hooks)
Change in subject line + wider forum
Quoted 14 lines · Sekhar Nori
Quoted 13 lines · Daniel Mack
Quoted 12 lines · Sekhar Nori
Review
Quoted 6 lines · Daniel Mack
(quoted diff) +6
-0+static int edma_pm_suspend(struct device *dev) +{ + int j, r; + + r = pm_runtime_get_sync(dev); + if (IS_ERR_VALUE(r)) {So
IS_ERR_VALUE()is only for functions which may return a negative number outside ofMAX_ERRNOas a success indication.pm_runtime_get_sync()does not appear to be one of them so just use"
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
if (r < 0) { .. }
That's true. Thanks for catching this, I'll fix it. However, grepping through the tree, there are quite a lot places where the same mistake is made.
Yes, this is a common fallacy. Russell cleaned up a bunch of these a while back.
Thinking a little more on this front, to prevent recurrence and fixing the ones we already have, how about something like the following patch?
For example, on 3.13-rc1, with omap2plus_defconfig, I see the following:
http://pastebin.mozilla.org/3681911
-->8--
Quoted 1 line
From
b7946d214fab72b2e18cd67eec01c377f1cddee8Mon Sep 17 00:00:00 2001
From: Nishanth Menon <[email protected]>
Date: Wed, 27 Nov 2013 17:01:20 -0600
Subject: [RFC PATCH] scripts: Coccinelle script for pm_runtime_* return checks
with IS_ERR_VALUE
As indicated by Sekhar in [1], there seems to be a tendency to use IS_ERR_VALUE to check the error result for pm_runtime_* functions which make no sense considering commit c48cd65 (ARM: OMAP: use consistent error checking) - the error values can either be < 0 for error OR 0, 1 in cases where we have success.
So, setup a coccinelle script to help identify the same.
[1] http://marc.info/?t=138472678100003&r=1&w=2
--- scripts/coccinelle/api/pm_runtime.cocci | 109 +++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 scripts/coccinelle/api/pm_runtime.cocci
scripts/coccinelle/api/pm_runtime.cocci new file +109 -0
@@ -0,0 +1,109 @@ +/// Make sure pm_runtime_* calls does not use unnecessary IS_ERR_VALUE +// +// Keywords: pm_runtime +// Confidence: Medium +// Copyright (C) 2013 Texas Instruments Incorporated - GPLv2. +// URL: http://coccinelle.lip6.fr/ +// Options: --include-headers + +virtual patch +virtual context +virtual org +virtual report + +//---------------------------------------------------------- +// Detection +//---------------------------------------------------------- + + at runtime_bad_err_handle exists@ +expression ret; +@@ +( +ret = \(pm_runtime_idle\| + pm_runtime_suspend\| + pm_runtime_autosuspend\| + pm_runtime_resume\| + pm_request_idle\| + pm_request_resume\| + pm_request_autosuspend\| + pm_runtime_get\| + pm_runtime_get_sync\| + pm_runtime_put\| + pm_runtime_put_autosuspend\| + pm_runtime_put_sync\| + pm_runtime_put_sync_suspend\| + pm_runtime_put_sync_autosuspend\| + pm_runtime_set_active\| + pm_schedule_suspend\| + pm_runtime_barrier\| + pm_generic_runtime_suspend\| + pm_generic_runtime_resume\)(...); +... +IS_ERR_VALUE(ret) +... +) + +//---------------------------------------------------------- +// For context mode +//---------------------------------------------------------- + + at depends on runtime_bad_err_handle && context@ +identifier pm_runtime_api; +expression ret; +@@ +( +ret = pm_runtime_api(...); +... +* IS_ERR_VALUE(ret) +... +) + +//---------------------------------------------------------- +// For patch mode +//---------------------------------------------------------- + + at depends on runtime_bad_err_handle && patch@ +identifier pm_runtime_api; +expression ret; +@@ +( +ret = pm_runtime_api(...); +... +- IS_ERR_VALUE(ret) ++ ret < 0 +... +) + +//---------------------------------------------------------- +// For org and report mode +//---------------------------------------------------------- + + at r depends on runtime_bad_err_handle exists@ +position p1, p2; +identifier pm_runtime_api; +expression ret; +@@ +( +ret = pm_runtime_api at p1(...); +... +IS_ERR_VALUE at p2(ret) +... +) + + at script:python depends on org@ +p1 << r.p1; +p2 << r.p2; +pm_runtime_api << r.pm_runtime_api; +@@ + +cocci.print_main(pm_runtime_api,p1) +cocci.print_secs("IS_ERR_VALUE",p2) + + at script:python depends on report@ +p1 << r.p1; +p2 << r.p2; +pm_runtime_api << r.pm_runtime_api; +@@ + +msg = "%s returns < 0 as error. Unecessary IS_ERR_VALUE@line %s" % (pm_runtime_api, p2[0].line) +coccilib.report.print_report(p1[0],msg)
Signature · 5 lines
1.7.9.5 -- Regards, Nishanth Menon
Re: pm_runtime functions and IS_ERR_VALUE (was Re: [PATCH v6] ARM: omap: edma: add suspend suspend/resume hooks)
Quoted 23 lines · Nishanth Menon
Change in subject line + wider forum
Quoted 20 lines · Sekhar Nori
Quoted 18 lines · Daniel Mack
Quoted 12 lines · Sekhar Nori
Review
Quoted 6 lines · Daniel Mack
(quoted diff) +6
-0+static int edma_pm_suspend(struct device *dev) +{ + int j, r; + + r = pm_runtime_get_sync(dev); + if (IS_ERR_VALUE(r)) {So
IS_ERR_VALUE()is only for functions which may return a negative number outside ofMAX_ERRNOas a success indication.pm_runtime_get_sync()does not appear to be one of them so just use"Quoted 1 line
if (r < 0) { .. }
That's true. Thanks for catching this, I'll fix it. However, grepping through the tree, there are quite a lot places where the same mistake is made.
Quoted 6 lines
Quoted 2 lines
Yes, this is a common fallacy. Russell cleaned up a bunch of these a while back.
Thinking a little more on this front, to prevent recurrence and fixing the ones we already have, how about something like the following patch?
Looks reasonable - if the result survives the auto build testing. :-)
Thanks!
Quoted 144 lines
For example, on 3.13-rc1, with
omap2plus_defconfig, I see the following:
http://pastebin.mozilla.org/3681911-->8--
Fromb7946d214fab72b2e18cd67eec01c377f1cddee8Mon Sep 17 00:00:00 2001
From: Nishanth Menon <[email protected]>
Date: Wed, 27 Nov 2013 17:01:20 -0600
Subject: [RFC PATCH] scripts: Coccinelle script for pm_runtime_* return checks
withIS_ERR_VALUEAs indicated by Sekhar in [1], there seems to be a tendency to use
IS_ERR_VALUEto check the error result for pm_runtime_* functions which make no sense considering commit c48cd65 (ARM: OMAP: use consistent error checking) - the error values can either be < 0 for error OR 0, 1 in cases where we have success.So, setup a coccinelle script to help identify the same.
[1] http://marc.info/?t=138472678100003&r=1&w=2
Cc Russell King <[email protected]>Reported-by Sekhar Nori <[email protected]>Signed-off-by Nishanth Menon <[email protected]>--- scripts/coccinelle/api/pm_runtime.cocci | 109 +++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 scripts/coccinelle/api/pm_runtime.cocciscripts/coccinelle/api/pm_runtime.cocci new file +109
-0@@ -0,0 +1,109 @@ +/// Make sure pm_runtime_* calls does not use unnecessary IS_ERR_VALUE +// +// Keywords: pm_runtime +// Confidence: Medium +// Copyright (C) 2013 Texas Instruments Incorporated - GPLv2. +// URL: http://coccinelle.lip6.fr/ +// Options: --include-headers + +virtual patch +virtual context +virtual org +virtual report + +//---------------------------------------------------------- +// Detection +//---------------------------------------------------------- + +@runtime_bad_err_handle exists@ +expression ret; +@@ +( +ret = \(pm_runtime_idle\| + pm_runtime_suspend\| + pm_runtime_autosuspend\| + pm_runtime_resume\| + pm_request_idle\| + pm_request_resume\| + pm_request_autosuspend\| + pm_runtime_get\| + pm_runtime_get_sync\| + pm_runtime_put\| + pm_runtime_put_autosuspend\| + pm_runtime_put_sync\| + pm_runtime_put_sync_suspend\| + pm_runtime_put_sync_autosuspend\| + pm_runtime_set_active\| + pm_schedule_suspend\| + pm_runtime_barrier\| + pm_generic_runtime_suspend\| + pm_generic_runtime_resume\)(...); +... +IS_ERR_VALUE(ret) +... +) + +//---------------------------------------------------------- +// For context mode +//---------------------------------------------------------- + +@depends on runtime_bad_err_handle && context@ +identifier pm_runtime_api; +expression ret; +@@ +( +ret = pm_runtime_api(...); +... +* IS_ERR_VALUE(ret) +... +) + +//---------------------------------------------------------- +// For patch mode +//---------------------------------------------------------- + +@depends on runtime_bad_err_handle && patch@ +identifier pm_runtime_api; +expression ret; +@@ +( +ret = pm_runtime_api(...); +... +- IS_ERR_VALUE(ret) ++ ret < 0 +... +) + +//---------------------------------------------------------- +// For org and report mode +//---------------------------------------------------------- + +@r depends on runtime_bad_err_handle exists@ +position p1, p2; +identifier pm_runtime_api; +expression ret; +@@ +( +ret = pm_runtime_api@p1(...); +... +IS_ERR_VALUE@p2(ret) +... +) + +@script:python depends on org@ +p1 << r.p1; +p2 << r.p2; +pm_runtime_api << r.pm_runtime_api; +@@ + +cocci.print_main(pm_runtime_api,p1) +cocci.print_secs("IS_ERR_VALUE",p2) + +@script:python depends on report@ +p1 << r.p1; +p2 << r.p2; +pm_runtime_api << r.pm_runtime_api; +@@ + +msg = "%s returns < 0 as error. Unecessary IS_ERR_VALUE at line %s" % (pm_runtime_api, p2[0].line) +coccilib.report.print_report(p1[0],msg)
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.