Re: [PATCHv2 4/8] devfreq: exynos4: Fix bug of resource leak and code clean on probe()
On 17.03.2014 06:05, Chanwoo Choi wrote:
Hi Tomasz,
On 03/15/2014 02:49 AM, Tomasz Figa wrote:
Hi Chanwoo,
On 13.03.2014 09:17, Chanwoo Choi wrote:
This patch fix bug about resource leak when happening probe fail and code clean
to add debug message.
Signed-off-by: Chanwoo Choi cw00.c...@samsung.com
---
drivers/devfreq/exynos/exynos4_bus.c | 32 ++--
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/exynos/exynos4_bus.c
b/drivers/devfreq/exynos/exynos4_bus.c
index a2a3a47..152a3e9 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -1152,8 +1152,11 @@ static int exynos4_busfreq_probe(struct platform_device
*pdev)
dev_err(dev, Cannot determine the device id %d\n, data-type);
err = -EINVAL;
}
-if (err)
+if (err) {
+dev_err(dev, Cannot initialize busfreq table %d\n,
+ data-type);
return err;
+}
rcu_read_lock();
opp = dev_pm_opp_find_freq_floor(dev,
@@ -1176,7 +1179,7 @@ static int exynos4_busfreq_probe(struct platform_device
*pdev)
if (IS_ERR(data-devfreq)) {
dev_err(dev, Failed to add devfreq device\n);
err = PTR_ERR(data-devfreq);
-goto err_opp;
+goto err_devfreq;
}
/*
@@ -1185,18 +1188,35 @@ static int exynos4_busfreq_probe(struct platform_device
*pdev)
*/
busfreq_mon_reset(data);
-devfreq_register_opp_notifier(dev, data-devfreq);
+/* Register opp_notifier for Exynos4 busfreq */
+err = devfreq_register_opp_notifier(dev, data-devfreq);
+if (err 0) {
+dev_err(dev, Failed to register opp notifier\n);
+goto err_notifier_opp;
+}
+/* Register pm_notifier for Exynos4 busfreq */
err = register_pm_notifier(data-pm_notifier);
if (err) {
dev_err(dev, Failed to setup pm notifier\n);
-devfreq_remove_device(data-devfreq);
-return err;
+goto err_notifier_pm;
}
return 0;
-err_opp:
+err_notifier_pm:
+devfreq_unregister_opp_notifier(dev, data-devfreq);
+err_notifier_opp:
+/*
+ * The devfreq_remove_device() would execute finally devfreq-profile
+ * -exit(). To avoid duplicate resource free operation, return directly
+ * before executing resource free below 'err_devfreq' goto statement.
+ */
I'm not quite sure about this. I believe that in this case
devfreq-profile-exit() would be exynos4_bus_exit() and all it does is
devfreq_unregister_opp_notifier(dev, data-devfreq), so all remaining resources
(regulators, clocks, etc.) would get leaked.
This patch execute following sequence to probe exynos4_busfreq.c:
1. Parse dt node to get resource(regulator/clock/memory address).
2. Enable regulator/clock and map memory.
3. Add devfreq device using devfreq_add_device().
The devfreq_add_device() return devfreq instance(data-devfreq).
4. Register opp_notifier using devfreq instance(data-devfreq) which is created
in sequence #3.
Case 1,
If an error happens in sequence #3 for registering devfreq_add_device(),
this case can't execute devfreq-profile-exit() to free resource
because this case has failed to register devfreq-profile to devfreq_list.
So, must goto 'err_devfreq' statement to free resource(regulator/clock/memory).
Case 2,
If an error happens in sequence #4 for registering opp_notifier,
In contrast
this case can execute devfreq-profile-exit() to free resource.
But, After executed devfreq-profile-exit(),
should not execute 'err_devfreq' statement to free resource.
In case, will operate twice of resource.
If my explanation is wrong, please reply your comment.
OK, it will work indeed, however the code is barely readable with this.
For consistency (and readability), resources acquired in platform
driver's .probe() should be freed in .remove() and error path of
.probe() should not rely on external code to do the clean-up.
So, as I proposed in my previous reply:
I believe the correct thing to do would be to remove the .exit() callback from
exynos4_devfreq_profile struct and handle all the clean-up here in error path.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/8] devfreq: exynos4: Fix bug of resource leak and code clean on probe()
Hi Tomasz,
On 03/18/2014 09:18 PM, Tomasz Figa wrote:
On 17.03.2014 06:05, Chanwoo Choi wrote:
Hi Tomasz,
On 03/15/2014 02:49 AM, Tomasz Figa wrote:
Hi Chanwoo,
On 13.03.2014 09:17, Chanwoo Choi wrote:
This patch fix bug about resource leak when happening probe fail and code
clean
to add debug message.
Signed-off-by: Chanwoo Choi cw00.c...@samsung.com
---
drivers/devfreq/exynos/exynos4_bus.c | 32
++--
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/exynos/exynos4_bus.c
b/drivers/devfreq/exynos/exynos4_bus.c
index a2a3a47..152a3e9 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -1152,8 +1152,11 @@ static int exynos4_busfreq_probe(struct
platform_device *pdev)
dev_err(dev, Cannot determine the device id %d\n, data-type);
err = -EINVAL;
}
-if (err)
+if (err) {
+dev_err(dev, Cannot initialize busfreq table %d\n,
+ data-type);
return err;
+}
rcu_read_lock();
opp = dev_pm_opp_find_freq_floor(dev,
@@ -1176,7 +1179,7 @@ static int exynos4_busfreq_probe(struct
platform_device *pdev)
if (IS_ERR(data-devfreq)) {
dev_err(dev, Failed to add devfreq device\n);
err = PTR_ERR(data-devfreq);
-goto err_opp;
+goto err_devfreq;
}
/*
@@ -1185,18 +1188,35 @@ static int exynos4_busfreq_probe(struct
platform_device *pdev)
*/
busfreq_mon_reset(data);
-devfreq_register_opp_notifier(dev, data-devfreq);
+/* Register opp_notifier for Exynos4 busfreq */
+err = devfreq_register_opp_notifier(dev, data-devfreq);
+if (err 0) {
+dev_err(dev, Failed to register opp notifier\n);
+goto err_notifier_opp;
+}
+/* Register pm_notifier for Exynos4 busfreq */
err = register_pm_notifier(data-pm_notifier);
if (err) {
dev_err(dev, Failed to setup pm notifier\n);
-devfreq_remove_device(data-devfreq);
-return err;
+goto err_notifier_pm;
}
return 0;
-err_opp:
+err_notifier_pm:
+devfreq_unregister_opp_notifier(dev, data-devfreq);
+err_notifier_opp:
+/*
+ * The devfreq_remove_device() would execute finally devfreq-profile
+ * -exit(). To avoid duplicate resource free operation, return
directly
+ * before executing resource free below 'err_devfreq' goto statement.
+ */
I'm not quite sure about this. I believe that in this case
devfreq-profile-exit() would be exynos4_bus_exit() and all it does is
devfreq_unregister_opp_notifier(dev, data-devfreq), so all remaining
resources (regulators, clocks, etc.) would get leaked.
This patch execute following sequence to probe exynos4_busfreq.c:
1. Parse dt node to get resource(regulator/clock/memory address).
2. Enable regulator/clock and map memory.
3. Add devfreq device using devfreq_add_device().
The devfreq_add_device() return devfreq instance(data-devfreq).
4. Register opp_notifier using devfreq instance(data-devfreq) which is
created in sequence #3.
Case 1,
If an error happens in sequence #3 for registering devfreq_add_device(),
this case can't execute devfreq-profile-exit() to free resource
because this case has failed to register devfreq-profile to devfreq_list.
So, must goto 'err_devfreq' statement to free
resource(regulator/clock/memory).
Case 2,
If an error happens in sequence #4 for registering opp_notifier,
In contrast
this case can execute devfreq-profile-exit() to free resource.
But, After executed devfreq-profile-exit(),
should not execute 'err_devfreq' statement to free resource.
In case, will operate twice of resource.
If my explanation is wrong, please reply your comment.
OK, it will work indeed, however the code is barely readable with this.
For consistency (and readability), resources acquired in platform driver's
.probe() should be freed in .remove() and error path of .probe() should not
rely on external code to do the clean-up.
So, as I proposed in my previous reply:
OK, I'll modify it according to your previous comment.
I believe the correct thing to do would be to remove the .exit() callback
from exynos4_devfreq_profile struct and handle all the clean-up here in
error path.
Best Regards,
Chanwoo Choi
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/8] devfreq: exynos4: Fix bug of resource leak and code clean on probe()
Hi Tomasz,
On 03/15/2014 02:49 AM, Tomasz Figa wrote:
Hi Chanwoo,
On 13.03.2014 09:17, Chanwoo Choi wrote:
This patch fix bug about resource leak when happening probe fail and code
clean
to add debug message.
Signed-off-by: Chanwoo Choi cw00.c...@samsung.com
---
drivers/devfreq/exynos/exynos4_bus.c | 32 ++--
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/exynos/exynos4_bus.c
b/drivers/devfreq/exynos/exynos4_bus.c
index a2a3a47..152a3e9 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -1152,8 +1152,11 @@ static int exynos4_busfreq_probe(struct
platform_device *pdev)
dev_err(dev, Cannot determine the device id %d\n, data-type);
err = -EINVAL;
}
-if (err)
+if (err) {
+dev_err(dev, Cannot initialize busfreq table %d\n,
+ data-type);
return err;
+}
rcu_read_lock();
opp = dev_pm_opp_find_freq_floor(dev,
@@ -1176,7 +1179,7 @@ static int exynos4_busfreq_probe(struct
platform_device *pdev)
if (IS_ERR(data-devfreq)) {
dev_err(dev, Failed to add devfreq device\n);
err = PTR_ERR(data-devfreq);
-goto err_opp;
+goto err_devfreq;
}
/*
@@ -1185,18 +1188,35 @@ static int exynos4_busfreq_probe(struct
platform_device *pdev)
*/
busfreq_mon_reset(data);
-devfreq_register_opp_notifier(dev, data-devfreq);
+/* Register opp_notifier for Exynos4 busfreq */
+err = devfreq_register_opp_notifier(dev, data-devfreq);
+if (err 0) {
+dev_err(dev, Failed to register opp notifier\n);
+goto err_notifier_opp;
+}
+/* Register pm_notifier for Exynos4 busfreq */
err = register_pm_notifier(data-pm_notifier);
if (err) {
dev_err(dev, Failed to setup pm notifier\n);
-devfreq_remove_device(data-devfreq);
-return err;
+goto err_notifier_pm;
}
return 0;
-err_opp:
+err_notifier_pm:
+devfreq_unregister_opp_notifier(dev, data-devfreq);
+err_notifier_opp:
+/*
+ * The devfreq_remove_device() would execute finally devfreq-profile
+ * -exit(). To avoid duplicate resource free operation, return directly
+ * before executing resource free below 'err_devfreq' goto statement.
+ */
I'm not quite sure about this. I believe that in this case
devfreq-profile-exit() would be exynos4_bus_exit() and all it does is
devfreq_unregister_opp_notifier(dev, data-devfreq), so all remaining
resources (regulators, clocks, etc.) would get leaked.
This patch execute following sequence to probe exynos4_busfreq.c:
1. Parse dt node to get resource(regulator/clock/memory address).
2. Enable regulator/clock and map memory.
3. Add devfreq device using devfreq_add_device().
The devfreq_add_device() return devfreq instance(data-devfreq).
4. Register opp_notifier using devfreq instance(data-devfreq) which is created
in sequence #3.
Case 1,
If an error happens in sequence #3 for registering devfreq_add_device(),
this case can't execute devfreq-profile-exit() to free resource
because this case has failed to register devfreq-profile to devfreq_list.
So, must goto 'err_devfreq' statement to free resource(regulator/clock/memory).
Case 2,
If an error happens in sequence #4 for registering opp_notifier,
In contrast
this case can execute devfreq-profile-exit() to free resource.
But, After executed devfreq-profile-exit(),
should not execute 'err_devfreq' statement to free resource.
In case, will operate twice of resource.
If my explanation is wrong, please reply your comment.
I believe the correct thing to do would be to remove the .exit() callback
from exynos4_devfreq_profile struct and handle all the clean-up here in error
path.
Best Regards,
Chanwoo Choi
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/8] devfreq: exynos4: Fix bug of resource leak and code clean on probe()
Hi Chanwoo,
On 13.03.2014 09:17, Chanwoo Choi wrote:
This patch fix bug about resource leak when happening probe fail and code clean
to add debug message.
Signed-off-by: Chanwoo Choi cw00.c...@samsung.com
---
drivers/devfreq/exynos/exynos4_bus.c | 32 ++--
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/exynos/exynos4_bus.c
b/drivers/devfreq/exynos/exynos4_bus.c
index a2a3a47..152a3e9 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -1152,8 +1152,11 @@ static int exynos4_busfreq_probe(struct platform_device
*pdev)
dev_err(dev, Cannot determine the device id %d\n, data-type);
err = -EINVAL;
}
- if (err)
+ if (err) {
+ dev_err(dev, Cannot initialize busfreq table %d\n,
+data-type);
return err;
+ }
rcu_read_lock();
opp = dev_pm_opp_find_freq_floor(dev,
@@ -1176,7 +1179,7 @@ static int exynos4_busfreq_probe(struct platform_device
*pdev)
if (IS_ERR(data-devfreq)) {
dev_err(dev, Failed to add devfreq device\n);
err = PTR_ERR(data-devfreq);
- goto err_opp;
+ goto err_devfreq;
}
/*
@@ -1185,18 +1188,35 @@ static int exynos4_busfreq_probe(struct platform_device
*pdev)
*/
busfreq_mon_reset(data);
- devfreq_register_opp_notifier(dev, data-devfreq);
+ /* Register opp_notifier for Exynos4 busfreq */
+ err = devfreq_register_opp_notifier(dev, data-devfreq);
+ if (err 0) {
+ dev_err(dev, Failed to register opp notifier\n);
+ goto err_notifier_opp;
+ }
+ /* Register pm_notifier for Exynos4 busfreq */
err = register_pm_notifier(data-pm_notifier);
if (err) {
dev_err(dev, Failed to setup pm notifier\n);
- devfreq_remove_device(data-devfreq);
- return err;
+ goto err_notifier_pm;
}
return 0;
-err_opp:
+err_notifier_pm:
+ devfreq_unregister_opp_notifier(dev, data-devfreq);
+err_notifier_opp:
+ /*
+* The devfreq_remove_device() would execute finally devfreq-profile
+* -exit(). To avoid duplicate resource free operation, return directly
+* before executing resource free below 'err_devfreq' goto statement.
+*/
I'm not quite sure about this. I believe that in this case
devfreq-profile-exit() would be exynos4_bus_exit() and all it does is
devfreq_unregister_opp_notifier(dev, data-devfreq), so all remaining
resources (regulators, clocks, etc.) would get leaked.
I believe the correct thing to do would be to remove the .exit()
callback from exynos4_devfreq_profile struct and handle all the clean-up
here in error path.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 4/8] devfreq: exynos4: Fix bug of resource leak and code clean on probe()
This patch fix bug about resource leak when happening probe fail and code clean
to add debug message.
Signed-off-by: Chanwoo Choi cw00.c...@samsung.com
---
drivers/devfreq/exynos/exynos4_bus.c | 32 ++--
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/exynos/exynos4_bus.c
b/drivers/devfreq/exynos/exynos4_bus.c
index a2a3a47..152a3e9 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -1152,8 +1152,11 @@ static int exynos4_busfreq_probe(struct platform_device
*pdev)
dev_err(dev, Cannot determine the device id %d\n, data-type);
err = -EINVAL;
}
- if (err)
+ if (err) {
+ dev_err(dev, Cannot initialize busfreq table %d\n,
+data-type);
return err;
+ }
rcu_read_lock();
opp = dev_pm_opp_find_freq_floor(dev,
@@ -1176,7 +1179,7 @@ static int exynos4_busfreq_probe(struct platform_device
*pdev)
if (IS_ERR(data-devfreq)) {
dev_err(dev, Failed to add devfreq device\n);
err = PTR_ERR(data-devfreq);
- goto err_opp;
+ goto err_devfreq;
}
/*
@@ -1185,18 +1188,35 @@ static int exynos4_busfreq_probe(struct platform_device
*pdev)
*/
busfreq_mon_reset(data);
- devfreq_register_opp_notifier(dev, data-devfreq);
+ /* Register opp_notifier for Exynos4 busfreq */
+ err = devfreq_register_opp_notifier(dev, data-devfreq);
+ if (err 0) {
+ dev_err(dev, Failed to register opp notifier\n);
+ goto err_notifier_opp;
+ }
+ /* Register pm_notifier for Exynos4 busfreq */
err = register_pm_notifier(data-pm_notifier);
if (err) {
dev_err(dev, Failed to setup pm notifier\n);
- devfreq_remove_device(data-devfreq);
- return err;
+ goto err_notifier_pm;
}
return 0;
-err_opp:
+err_notifier_pm:
+ devfreq_unregister_opp_notifier(dev, data-devfreq);
+err_notifier_opp:
+ /*
+* The devfreq_remove_device() would execute finally devfreq-profile
+* -exit(). To avoid duplicate resource free operation, return directly
+* before executing resource free below 'err_devfreq' goto statement.
+*/
+ devfreq_remove_device(data-devfreq);
+
+ return err;
+
+err_devfreq:
regulator_disable(data-vdd_int);
if (data-type == TYPE_BUSF_EXYNOS4x12)
regulator_disable(data-vdd_mif);
--
1.8.0
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
