[PATCH 0/5] clk: imx: imx8: Fix some error handling paths

[PATCH 0/5] clk: imx: imx8: Fix some error handling paths

[Christophe JAILLET]

This serie fix some error handling paths. It is split in different patches to
ease review because the issues are unrelated and the proposed fixes are maybe
wrong (I don't have the hardware to test anything)

Patch 2 and 3 are more speculative than the 3 oher ones. Review with care.


Finally, I got some problem when generating the serie, and some patches have
been hand-modified afterwards.
They look good to me, but I hope have not screwed up things...

Christophe JAILLET (5):
  clk: imx: imx8: Fix an error handling path in
    clk_imx_acm_attach_pm_domains()
  clk: imx: imx8: Fix an error handling path if
    devm_clk_hw_register_mux_parent_data_table() fails
  clk: imx: imx8: Fix an error handling path in imx8_acm_clk_probe()
  clk: imx: imx8: Add a message in case of
    devm_clk_hw_register_mux_parent_data_table() error
  clk: imx: imx8: Simplify clk_imx_acm_detach_pm_domains()

 drivers/clk/imx/clk-imx8-acm.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH 1/5] clk: imx: imx8: Fix an error handling path in clk_imx_acm_attach_pm_domains()

[Christophe JAILLET]

If a dev_pm_domain_attach_by_id() call fails, previously allocated
resources need to be released.

Fixes: d3a0946d7ac9 ("clk: imx: imx8: add audio clock mux driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/clk/imx/clk-imx8-acm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8-acm.c b/drivers/clk/imx/clk-imx8-acm.c
index 1e82f72b75c6..87025a6772d0 100644
--- a/drivers/clk/imx/clk-imx8-acm.c
+++ b/drivers/clk/imx/clk-imx8-acm.c
@@ -279,8 +279,10 @@ static int clk_imx_acm_attach_pm_domains(struct device *dev,
 
 	for (i = 0; i < dev_pm->num_domains; i++) {
 		dev_pm->pd_dev[i] = dev_pm_domain_attach_by_id(dev, i);
-		if (IS_ERR(dev_pm->pd_dev[i]))
-			return PTR_ERR(dev_pm->pd_dev[i]);
+		if (IS_ERR(dev_pm->pd_dev[i])) {
+			ret = PTR_ERR(dev_pm->pd_dev[i]);
+			goto detach_pm;
+		}
 
 		dev_pm->pd_dev_link[i] = device_link_add(dev,
 							 dev_pm->pd_dev[i],
-- 
2.34.1

[PATCH 2/5] clk: imx: imx8: Fix an error handling path if devm_clk_hw_register_mux_parent_data_table() fails

[Christophe JAILLET]

If a devm_clk_hw_register_mux_parent_data_table() call fails, it is likely
that the probe should fail with an error code.

Set ret before leaving the function.

Fixes: d3a0946d7ac9 ("clk: imx: imx8: add audio clock mux driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is speculative, review with care.
---
 drivers/clk/imx/clk-imx8-acm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/imx/clk-imx8-acm.c b/drivers/clk/imx/clk-imx8-acm.c
index 87025a6772d0..0cd9aa3629f3 100644
--- a/drivers/clk/imx/clk-imx8-acm.c
+++ b/drivers/clk/imx/clk-imx8-acm.c
@@ -373,6 +373,7 @@ static int imx8_acm_clk_probe(struct platform_device *pdev)
 										sels[i].shift, sels[i].width,
 										0, NULL, NULL);
 		if (IS_ERR(hws[sels[i].clkid])) {
+			ret = PTR_ERR(hws[sels[i].clkid]);
 			pm_runtime_disable(&pdev->dev);
 			goto err_clk_register;
 		}
-- 
2.34.1

[PATCH 3/5] clk: imx: imx8: Fix an error handling path in imx8_acm_clk_probe()

[Christophe JAILLET]

If an error occurs after a successful clk_imx_acm_attach_pm_domains() call,
it must be undone.

Add an explicit error handling path, Re-order the code and add the missing
clk_imx_acm_detach_pm_domains() call.

Fixes: d3a0946d7ac9 ("clk: imx: imx8: add audio clock mux driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is speculative, review with care.
---
 drivers/clk/imx/clk-imx8-acm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8-acm.c b/drivers/clk/imx/clk-imx8-acm.c
index 0cd9aa3629f3..19914c1d7c39 100644
--- a/drivers/clk/imx/clk-imx8-acm.c
+++ b/drivers/clk/imx/clk-imx8-acm.c
@@ -374,7 +374,6 @@ static int imx8_acm_clk_probe(struct platform_device *pdev)
 										0, NULL, NULL);
 		if (IS_ERR(hws[sels[i].clkid])) {
 			ret = PTR_ERR(hws[sels[i].clkid]));
-			pm_runtime_disable(&pdev->dev);
 			goto err_clk_register;
 		}
 	}
@@ -384,12 +383,16 @@ static int imx8_acm_clk_probe(struct platform_device *pdev)
 	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_hw_data);
 	if (ret < 0) {
 		dev_err(dev, "failed to register hws for ACM\n");
-		pm_runtime_disable(&pdev->dev);
+		goto err_clk_register;
 	}
 
-err_clk_register:
+	pm_runtime_put_sync(&pdev->dev);
+	return 0;
 
+err_clk_register:
 	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	clk_imx_acm_detach_pm_domains(&pdev->dev, &priv->dev_pm);
 
 	return ret;
 }
-- 
2.34.1

[PATCH 4/5] clk: imx: imx8: Add a message in case of devm_clk_hw_register_mux_parent_data_table() error

[Christophe JAILLET]

If devm_clk_hw_register_mux_parent_data_table() fails, we branch to the
error handling path and imx_check_clk_hws() is never called.

Actually, imx_check_clk_hws() is a no-op because values in hws are either
valid, either NULL.

Move the call to imx_check_clk_hws() in the error handling path, so that
an error is logged.

Fixes: d3a0946d7ac9 ("clk: imx: imx8: add audio clock mux driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/clk/imx/clk-imx8-acm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8-acm.c b/drivers/clk/imx/clk-imx8-acm.c
index 19914c1d7c39..33157bc706ae 100644
--- a/drivers/clk/imx/clk-imx8-acm.c
+++ b/drivers/clk/imx/clk-imx8-acm.c
@@ -374,12 +374,11 @@ static int imx8_acm_clk_probe(struct platform_device *pdev)
 										0, NULL, NULL);
 		if (IS_ERR(hws[sels[i].clkid])) {
 			ret = PTR_ERR(hws[sels[i].clkid]));
+			imx_check_clk_hws(hws, IMX_ADMA_ACM_CLK_END);
 			goto err_clk_register;
 		}
 	}
 
-	imx_check_clk_hws(hws, IMX_ADMA_ACM_CLK_END);
-
 	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_hw_data);
 	if (ret < 0) {
 		dev_err(dev, "failed to register hws for ACM\n");
-- 
2.34.1

[PATCH 5/5] clk: imx: imx8: Simplify clk_imx_acm_detach_pm_domains()

[Christophe JAILLET]

The return value of clk_imx_acm_detach_pm_domains() never used.
Simplify the code and turn it into a void function.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/clk/imx/clk-imx8-acm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8-acm.c b/drivers/clk/imx/clk-imx8-acm.c
index 33157bc706ae..2c920091d678 100644
--- a/drivers/clk/imx/clk-imx8-acm.c
+++ b/drivers/clk/imx/clk-imx8-acm.c
@@ -310,20 +310,18 @@ static int clk_imx_acm_attach_pm_domains(struct device *dev,
  * @dev: deivice pointer
  * @dev_pm: multi power domain for device
  */
-static int clk_imx_acm_detach_pm_domains(struct device *dev,
-					 struct clk_imx_acm_pm_domains *dev_pm)
+static void clk_imx_acm_detach_pm_domains(struct device *dev,
+					  struct clk_imx_acm_pm_domains *dev_pm)
 {
 	int i;
 
 	if (dev_pm->num_domains <= 1)
-		return 0;
+		return;
 
 	for (i = 0; i < dev_pm->num_domains; i++) {
 		device_link_del(dev_pm->pd_dev_link[i]);
 		dev_pm_domain_detach(dev_pm->pd_dev[i], false);
 	}
-
-	return 0;
 }
 
 static int imx8_acm_clk_probe(struct platform_device *pdev)
-- 
2.34.1

Re: [PATCH 0/5] clk: imx: imx8: Fix some error handling paths

[Peng Fan]

On 8/27/2023 5:37 PM, Christophe JAILLET wrote:
> This serie fix some error handling paths. It is split in different patches to
> ease review because the issues are unrelated and the proposed fixes are maybe
> wrong (I don't have the hardware to test anything)
> 
> Patch 2 and 3 are more speculative than the 3 oher ones. Review with care.
> 
> 
> Finally, I got some problem when generating the serie, and some patches have
> been hand-modified afterwards.
> They look good to me, but I hope have not screwed up things...
> 
> Christophe JAILLET (5):
>    clk: imx: imx8: Fix an error handling path in
>      clk_imx_acm_attach_pm_domains()
>    clk: imx: imx8: Fix an error handling path if
>      devm_clk_hw_register_mux_parent_data_table() fails
>    clk: imx: imx8: Fix an error handling path in imx8_acm_clk_probe()
>    clk: imx: imx8: Add a message in case of
>      devm_clk_hw_register_mux_parent_data_table() error
>    clk: imx: imx8: Simplify clk_imx_acm_detach_pm_domains()
> 
>   drivers/clk/imx/clk-imx8-acm.c | 27 +++++++++++++++------------
>   1 file changed, 15 insertions(+), 12 deletions(-)
> 

LGTM, for the patchset

Reviewed-by: Peng Fan <peng.fan@nxp.com>

RE: [PATCH 0/5] clk: imx: imx8: Fix some error handling paths

[S.J. Wang]

> 
> This serie fix some error handling paths. It is split in different patches to ease
> review because the issues are unrelated and the proposed fixes are maybe
> wrong (I don't have the hardware to test anything)
> 
> Patch 2 and 3 are more speculative than the 3 oher ones. Review with care.
> 
> 
> Finally, I got some problem when generating the serie, and some patches
> have been hand-modified afterwards.
> They look good to me, but I hope have not screwed up things...


From the 3rd patch,  it can't be applied, maybe there is generating issue.

Best regards
Wang Shengjiu

> 
> Christophe JAILLET (5):
>   clk: imx: imx8: Fix an error handling path in
>     clk_imx_acm_attach_pm_domains()
>   clk: imx: imx8: Fix an error handling path if
>     devm_clk_hw_register_mux_parent_data_table() fails
>   clk: imx: imx8: Fix an error handling path in imx8_acm_clk_probe()
>   clk: imx: imx8: Add a message in case of
>     devm_clk_hw_register_mux_parent_data_table() error
>   clk: imx: imx8: Simplify clk_imx_acm_detach_pm_domains()
> 
>  drivers/clk/imx/clk-imx8-acm.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> --
> 2.34.1

Re: RE: [PATCH 0/5] clk: imx: imx8: Fix some error handling paths

[Christophe JAILLET]

Le 14/09/2023 à 12:22, S.J. Wang a écrit :
>>
>> This serie fix some error handling paths. It is split in different patches to ease
>> review because the issues are unrelated and the proposed fixes are maybe
>> wrong (I don't have the hardware to test anything)
>>
>> Patch 2 and 3 are more speculative than the 3 oher ones. Review with care.
>>
>>
>> Finally, I got some problem when generating the serie, and some patches
>> have been hand-modified afterwards.
>> They look good to me, but I hope have not screwed up things...
> 
> 
>  From the 3rd patch,  it can't be applied, maybe there is generating issue.

I will resend.

CJ

> 
> Best regards
> Wang Shengjiu
> 
>>
>> Christophe JAILLET (5):
>>    clk: imx: imx8: Fix an error handling path in
>>      clk_imx_acm_attach_pm_domains()
>>    clk: imx: imx8: Fix an error handling path if
>>      devm_clk_hw_register_mux_parent_data_table() fails
>>    clk: imx: imx8: Fix an error handling path in imx8_acm_clk_probe()
>>    clk: imx: imx8: Add a message in case of
>>      devm_clk_hw_register_mux_parent_data_table() error
>>    clk: imx: imx8: Simplify clk_imx_acm_detach_pm_domains()
>>
>>   drivers/clk/imx/clk-imx8-acm.c | 27 +++++++++++++++------------
>>   1 file changed, 15 insertions(+), 12 deletions(-)
>>
>> --
>> 2.34.1
> 
>