[PATCH] mfd: tc3589:Fix tc3589x->i2c->irq not released warning

[Mengxi Xu]

There is a waring:

warn: 'tc3589x->i2c->irq' from request_threaded_irq()
not released on lines: 426.

operation:add free_irq(tc3589x->i2c->irq, tc3589x) on lines 426.

Signed-off-by: Mengxi Xu <u2021...@hust.edu.cn>
---
drivers/clocksource/timer-digicolor.c | 1 -
drivers/mfd/tc3589x.c | 1 +
2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-digicolor.c b/drivers/clocksource/timer-digicolor.c
index 559aa96089c3..fb1113b4c196 100644
--- a/drivers/clocksource/timer-digicolor.c
+++ b/drivers/clocksource/timer-digicolor.c
@@ -195,7 +195,6 @@ static int __init digicolor_timer_init(struct device_node *node)

dc_timer_dev.ce.cpumask = cpu_possible_mask;
dc_timer_dev.ce.irq = irq;
-
clockevents_config_and_register(&dc_timer_dev.ce, rate, 0, 0xffffffff);

return 0;
diff --git a/drivers/mfd/tc3589x.c b/drivers/mfd/tc3589x.c
index db28eb0c8995..ce88f819ae1a 100644
--- a/drivers/mfd/tc3589x.c
+++ b/drivers/mfd/tc3589x.c
@@ -423,6 +423,7 @@ static int tc3589x_probe(struct i2c_client *i2c)
ret = tc3589x_device_init(tc3589x);
if (ret) {
dev_err(tc3589x->dev, "failed to add child devices\n");
+ free_irq(tc3589x->i2c->irq, tc3589x);
return ret;
}

--
2.34.1

[ktest...@126.com]

Hi, Mengxi Xu
This email is automatically replied by KTestRobot(version 1.0). Please do not reply to this email.
If you have any questions or suggestions about KTestRobot, please contact Lishuchang <U2020...@hust.edu.cn>

--- Changed Paths ---
drivers/clocksource/timer-digicolor.c
drivers/mfd/tc3589x.c

--- Log Message ---

There is a waring:

warn: 'tc3589x->i2c->irq' from request_threaded_irq()
not released on lines: 426.

operation:add free_irq(tc3589x->i2c->irq, tc3589x) on lines 426.

--- Test Result ---
*** CheckPatch PASS ***
*** ApplyTolinux-next PASS ***
*** ApplyTomainline PASS ***
*** BuildCheck PASS ***
*** CheckCocci PASS ***
*** CheckCppcheck PASS ***

--
KTestRobot(version 1.0)

[Dan Carpenter]

Space after :

On Thu, Nov 16, 2023 at 05:37:23PM +0800, Mengxi Xu wrote:
> There is a waring:

s/waring/warning/. Also mention Smatch or people will be confused.

>
> warn: 'tc3589x->i2c->irq' from request_threaded_irq()
> not released on lines: 426.
>
> operation:add free_irq(tc3589x->i2c->irq, tc3589x) on lines 426.

No one cares about the line numbers in the commit message. Have someone
help you with the English.

>
> Signed-off-by: Mengxi Xu <u2021...@hust.edu.cn>

Missing Fixes tag.

> ---
> drivers/clocksource/timer-digicolor.c | 1 -
> drivers/mfd/tc3589x.c | 1 +
> 2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/timer-digicolor.c b/drivers/clocksource/timer-digicolor.c
> index 559aa96089c3..fb1113b4c196 100644
> --- a/drivers/clocksource/timer-digicolor.c
> +++ b/drivers/clocksource/timer-digicolor.c
> @@ -195,7 +195,6 @@ static int __init digicolor_timer_init(struct device_node *node)
>
> dc_timer_dev.ce.cpumask = cpu_possible_mask;
> dc_timer_dev.ce.irq = irq;
> -

Don't make unrelated white space changes.

> clockevents_config_and_register(&dc_timer_dev.ce, rate, 0, 0xffffffff);
>
> return 0;
> diff --git a/drivers/mfd/tc3589x.c b/drivers/mfd/tc3589x.c
> index db28eb0c8995..ce88f819ae1a 100644
> --- a/drivers/mfd/tc3589x.c
> +++ b/drivers/mfd/tc3589x.c
> @@ -423,6 +423,7 @@ static int tc3589x_probe(struct i2c_client *i2c)
> ret = tc3589x_device_init(tc3589x);
> if (ret) {
> dev_err(tc3589x->dev, "failed to add child devices\n");
> + free_irq(tc3589x->i2c->irq, tc3589x);
> return ret;

When you are changing the error handling code then just check the whole
function for error handling bugs. It is very simple if the code is
written nicely. I have written a blog explaining it.

https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

So most of these functions are using devm_ for allocating resources.
That's fine. Those will be cleaned up automatically when we return.

The one which doesn't is tc3589x_irq_init(). That calls
irq_domain_add_simple() so presumably we need to release that manually.
Use cscope and look at other callers.

When you have written the error handling, you can cut and paste it to
form the basis of the remove() function. Here the remove() function is
tc3589x_remove(). I guess that mfd_remove_devices() frees the stuff
from tc3589x_device_init().

But notice that nothing in the remove() function calls free_irq() so
that seems like a bug.

So this patch is Okay, but I wouldn't merge it as a maintainer. I would
want a bigger patch that fixes the remove() as well.

Instead of calling free_irq() manually I would change this to use
devm_request_threaded_irq(). That will fix remove(). Then when I sent
the patch I would ask under the --- cut off line.
---
I don't see where we clean up from tc3589x_irq_init(), neither here nor
in tc3589x_remove(). Can someone take a look?

regards,
dan carpenter

[Mengxi Xu]

There is a waring:

warn: 'tc3589x->i2c->irq' from request_threaded_irq()
not released on lines: 426.

operation:add free_irq(tc3589x->i2c->irq, tc3589x) on lines 426.

Signed-off-by: Mengxi Xu <u2021...@hust.edu.cn>
---

drivers/mfd/tc3589x.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/tc3589x.c b/drivers/mfd/tc3589x.c
index db28eb0c8995..ce88f819ae1a 100644
--- a/drivers/mfd/tc3589x.c
+++ b/drivers/mfd/tc3589x.c
@@ -423,6 +423,7 @@ static int tc3589x_probe(struct i2c_client *i2c)
ret = tc3589x_device_init(tc3589x);
if (ret) {
dev_err(tc3589x->dev, "failed to add child devices\n");
+ free_irq(tc3589x->i2c->irq, tc3589x);
return ret;
}

--
2.34.1

[Mengxi Xu]