[PATCH 0/3] MediaTek clocks: various fixes

[PATCH 0/3] MediaTek clocks: various fixes

[AngeloGioacchino Del Regno]

This series performs some fixing action for some issues on clk-mtk
and on mt8173 apmixedsys clock drivers, which have not made any
real appearance (no kernel panics, yet) because of corner cases
not being triggered on the current MediaTek clock drivers.

Still, it was totally necessary to fix those, ensuring that the
MediaTek clock helpers and drivers keep being solid.

AngeloGioacchino Del Regno (3):
  clk: mediatek: clk-mtk: Grab iomem pointer for divider clocks
  clk: mediatek: clk-mt8173-apmixedsys: Fix return value for of_iomap()
    error
  clk: mediatek: clk-mt8173-apmixedsys: Fix iomap not released issue

 drivers/clk/mediatek/clk-mt8173-apmixedsys.c | 7 +++++--
 drivers/clk/mediatek/clk-mtk.c               | 6 +++---
 2 files changed, 8 insertions(+), 5 deletions(-)

-- 
2.40.1

[PATCH 1/3] clk: mediatek: clk-mtk: Grab iomem pointer for divider clocks

[AngeloGioacchino Del Regno]

In the rare case in which one of the clock drivers has divider clocks
but not composite clocks, mtk_clk_simple_probe() would not io(re)map,
hence passing a NULL pointer to mtk_clk_register_dividers().

To fix this issue, extend the `if` conditional to also check if any
divider clocks are present. While at it, also make sure the iomem
pointer is NULL if no composite/divider clocks are declared, as we
are checking for that when iounmapping it in the error path.

This hasn't been seen on any MediaTek clock driver as the current ones
always declare composite clocks along with divider clocks, but this is
still an important fix for a future potential KP.

Fixes: 1fe074b1f112 ("clk: mediatek: Add divider clocks to mtk_clk_simple_{probe,remove}()")
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/clk/mediatek/clk-mtk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
index cf3514c8e97e..b00ef4213335 100644
--- a/drivers/clk/mediatek/clk-mtk.c
+++ b/drivers/clk/mediatek/clk-mtk.c
@@ -469,7 +469,7 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
 	const struct platform_device_id *id;
 	const struct mtk_clk_desc *mcd;
 	struct clk_hw_onecell_data *clk_data;
-	void __iomem *base;
+	void __iomem *base = NULL;
 	int num_clks, r;
 
 	mcd = device_get_match_data(&pdev->dev);
@@ -483,8 +483,8 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
 			return -EINVAL;
 	}
 
-	/* Composite clocks needs us to pass iomem pointer */
-	if (mcd->composite_clks) {
+	/* Composite and divider clocks needs us to pass iomem pointer */
+	if (mcd->composite_clks || mcd->divider_clks) {
 		if (!mcd->shared_io)
 			base = devm_platform_ioremap_resource(pdev, 0);
 		else
-- 
2.40.1

[PATCH 2/3] clk: mediatek: clk-mt8173-apmixedsys: Fix return value for of_iomap() error

[AngeloGioacchino Del Regno]

The of_iomap() function returns NULL in case of error so usage of
PTR_ERR() is wrong!
Change that to return -ENOMEM in case of failure.

Fixes: 41138fbf876c ("clk: mediatek: mt8173: Migrate to platform driver and common probe")
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/clk/mediatek/clk-mt8173-apmixedsys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/mediatek/clk-mt8173-apmixedsys.c b/drivers/clk/mediatek/clk-mt8173-apmixedsys.c
index daa2856d1ab7..ba1386e70a24 100644
--- a/drivers/clk/mediatek/clk-mt8173-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt8173-apmixedsys.c
@@ -148,7 +148,7 @@ static int clk_mt8173_apmixed_probe(struct platform_device *pdev)
 
 	base = of_iomap(node, 0);
 	if (!base)
-		return PTR_ERR(base);
+		return -ENOMEM;
 
 	clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
 	if (IS_ERR_OR_NULL(clk_data))
-- 
2.40.1

[PATCH 3/3] clk: mediatek: clk-mt8173-apmixedsys: Fix iomap not released issue

[AngeloGioacchino Del Regno]

In case of error after of_ioremap() the resource must be released:
call iounmap() where appropriate to fix that.

Fixes: 41138fbf876c ("clk: mediatek: mt8173: Migrate to platform driver and common probe")
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/clk/mediatek/clk-mt8173-apmixedsys.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/mediatek/clk-mt8173-apmixedsys.c b/drivers/clk/mediatek/clk-mt8173-apmixedsys.c
index ba1386e70a24..1bbb21ab1786 100644
--- a/drivers/clk/mediatek/clk-mt8173-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt8173-apmixedsys.c
@@ -151,8 +151,10 @@ static int clk_mt8173_apmixed_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
-	if (IS_ERR_OR_NULL(clk_data))
+	if (IS_ERR_OR_NULL(clk_data)) {
+		iounmap(base);
 		return -ENOMEM;
+	}
 
 	fhctl_parse_dt(fhctl_node, pllfhs, ARRAY_SIZE(pllfhs));
 	r = mtk_clk_register_pllfhs(node, plls, ARRAY_SIZE(plls),
@@ -186,6 +188,7 @@ static int clk_mt8173_apmixed_probe(struct platform_device *pdev)
 				  ARRAY_SIZE(pllfhs), clk_data);
 free_clk_data:
 	mtk_free_clk_data(clk_data);
+	iounmap(base);
 	return r;
 }
 
-- 
2.40.1

Re: [PATCH 2/3] clk: mediatek: clk-mt8173-apmixedsys: Fix return value for of_iomap() error

[Chen-Yu Tsai]

On Thu, Jun 15, 2023 at 8:21 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> The of_iomap() function returns NULL in case of error so usage of
> PTR_ERR() is wrong!
> Change that to return -ENOMEM in case of failure.
>
> Fixes: 41138fbf876c ("clk: mediatek: mt8173: Migrate to platform driver and common probe")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

Re: [PATCH 3/3] clk: mediatek: clk-mt8173-apmixedsys: Fix iomap not released issue

[Chen-Yu Tsai]

On Thu, Jun 15, 2023 at 8:21 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> In case of error after of_ioremap() the resource must be released:
> call iounmap() where appropriate to fix that.
>
> Fixes: 41138fbf876c ("clk: mediatek: mt8173: Migrate to platform driver and common probe")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

Re: [PATCH 1/3] clk: mediatek: clk-mtk: Grab iomem pointer for divider clocks

[Chen-Yu Tsai]

On Thu, Jun 15, 2023 at 8:21 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> In the rare case in which one of the clock drivers has divider clocks
> but not composite clocks, mtk_clk_simple_probe() would not io(re)map,
> hence passing a NULL pointer to mtk_clk_register_dividers().
>
> To fix this issue, extend the `if` conditional to also check if any
> divider clocks are present. While at it, also make sure the iomem
> pointer is NULL if no composite/divider clocks are declared, as we
> are checking for that when iounmapping it in the error path.
>
> This hasn't been seen on any MediaTek clock driver as the current ones
> always declare composite clocks along with divider clocks, but this is
> still an important fix for a future potential KP.
>
> Fixes: 1fe074b1f112 ("clk: mediatek: Add divider clocks to mtk_clk_simple_{probe,remove}()")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

Re: [PATCH 1/3] clk: mediatek: clk-mtk: Grab iomem pointer for divider clocks

[Markus Schneider-Pargmann]

On Thu, Jun 15, 2023 at 02:20:49PM +0200, AngeloGioacchino Del Regno wrote:
> In the rare case in which one of the clock drivers has divider clocks
> but not composite clocks, mtk_clk_simple_probe() would not io(re)map,
> hence passing a NULL pointer to mtk_clk_register_dividers().
> 
> To fix this issue, extend the `if` conditional to also check if any
> divider clocks are present. While at it, also make sure the iomem
> pointer is NULL if no composite/divider clocks are declared, as we
> are checking for that when iounmapping it in the error path.
> 
> This hasn't been seen on any MediaTek clock driver as the current ones
> always declare composite clocks along with divider clocks, but this is
> still an important fix for a future potential KP.
> 
> Fixes: 1fe074b1f112 ("clk: mediatek: Add divider clocks to mtk_clk_simple_{probe,remove}()")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Markus Schneider-Pargmann <msp@baylibre.com>

> ---
>  drivers/clk/mediatek/clk-mtk.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> index cf3514c8e97e..b00ef4213335 100644
> --- a/drivers/clk/mediatek/clk-mtk.c
> +++ b/drivers/clk/mediatek/clk-mtk.c
> @@ -469,7 +469,7 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>  	const struct platform_device_id *id;
>  	const struct mtk_clk_desc *mcd;
>  	struct clk_hw_onecell_data *clk_data;
> -	void __iomem *base;
> +	void __iomem *base = NULL;
>  	int num_clks, r;
>  
>  	mcd = device_get_match_data(&pdev->dev);
> @@ -483,8 +483,8 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>  			return -EINVAL;
>  	}
>  
> -	/* Composite clocks needs us to pass iomem pointer */
> -	if (mcd->composite_clks) {
> +	/* Composite and divider clocks needs us to pass iomem pointer */
> +	if (mcd->composite_clks || mcd->divider_clks) {
>  		if (!mcd->shared_io)
>  			base = devm_platform_ioremap_resource(pdev, 0);
>  		else
> -- 
> 2.40.1
> 

Re: [PATCH 2/3] clk: mediatek: clk-mt8173-apmixedsys: Fix return value for of_iomap() error

[Markus Schneider-Pargmann]

On Thu, Jun 15, 2023 at 02:20:50PM +0200, AngeloGioacchino Del Regno wrote:
> The of_iomap() function returns NULL in case of error so usage of
> PTR_ERR() is wrong!
> Change that to return -ENOMEM in case of failure.
> 
> Fixes: 41138fbf876c ("clk: mediatek: mt8173: Migrate to platform driver and common probe")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Markus Schneider-Pargmann <msp@baylibre.com>

> ---
>  drivers/clk/mediatek/clk-mt8173-apmixedsys.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mt8173-apmixedsys.c b/drivers/clk/mediatek/clk-mt8173-apmixedsys.c
> index daa2856d1ab7..ba1386e70a24 100644
> --- a/drivers/clk/mediatek/clk-mt8173-apmixedsys.c
> +++ b/drivers/clk/mediatek/clk-mt8173-apmixedsys.c
> @@ -148,7 +148,7 @@ static int clk_mt8173_apmixed_probe(struct platform_device *pdev)
>  
>  	base = of_iomap(node, 0);
>  	if (!base)
> -		return PTR_ERR(base);
> +		return -ENOMEM;
>  
>  	clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
>  	if (IS_ERR_OR_NULL(clk_data))
> -- 
> 2.40.1
> 

Re: [PATCH 3/3] clk: mediatek: clk-mt8173-apmixedsys: Fix iomap not released issue

[Markus Schneider-Pargmann]

Hi Angelo,

On Thu, Jun 15, 2023 at 02:20:51PM +0200, AngeloGioacchino Del Regno wrote:
> In case of error after of_ioremap() the resource must be released:
> call iounmap() where appropriate to fix that.
> 
> Fixes: 41138fbf876c ("clk: mediatek: mt8173: Migrate to platform driver and common probe")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/clk/mediatek/clk-mt8173-apmixedsys.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mt8173-apmixedsys.c b/drivers/clk/mediatek/clk-mt8173-apmixedsys.c
> index ba1386e70a24..1bbb21ab1786 100644
> --- a/drivers/clk/mediatek/clk-mt8173-apmixedsys.c
> +++ b/drivers/clk/mediatek/clk-mt8173-apmixedsys.c
> @@ -151,8 +151,10 @@ static int clk_mt8173_apmixed_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
> -	if (IS_ERR_OR_NULL(clk_data))
> +	if (IS_ERR_OR_NULL(clk_data)) {
> +		iounmap(base);
>  		return -ENOMEM;

More of a nitpick, but I would prefer if you would use the same error
catching style as the rest of the probe function:

		if (IS_ERR_OR_NULL(clk_data)) {
			r = -ENOMEM;
			goto unmap_io;
		}
	...
	unmap_io:
		iounmap(base)
		return r;


Best,
Markus

Re: [PATCH 1/3] clk: mediatek: clk-mtk: Grab iomem pointer for divider clocks

[Stephen Boyd]

Quoting AngeloGioacchino Del Regno (2023-06-15 05:20:49)
> In the rare case in which one of the clock drivers has divider clocks
> but not composite clocks, mtk_clk_simple_probe() would not io(re)map,
> hence passing a NULL pointer to mtk_clk_register_dividers().
> 
> To fix this issue, extend the `if` conditional to also check if any
> divider clocks are present. While at it, also make sure the iomem
> pointer is NULL if no composite/divider clocks are declared, as we
> are checking for that when iounmapping it in the error path.
> 
> This hasn't been seen on any MediaTek clock driver as the current ones
> always declare composite clocks along with divider clocks, but this is
> still an important fix for a future potential KP.
> 
> Fixes: 1fe074b1f112 ("clk: mediatek: Add divider clocks to mtk_clk_simple_{probe,remove}()")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---

Applied to clk-next

Re: [PATCH 2/3] clk: mediatek: clk-mt8173-apmixedsys: Fix return value for of_iomap() error

[Stephen Boyd]

Quoting AngeloGioacchino Del Regno (2023-06-15 05:20:50)
> The of_iomap() function returns NULL in case of error so usage of
> PTR_ERR() is wrong!
> Change that to return -ENOMEM in case of failure.
> 
> Fixes: 41138fbf876c ("clk: mediatek: mt8173: Migrate to platform driver and common probe")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---

Applied to clk-next

Re: [PATCH 3/3] clk: mediatek: clk-mt8173-apmixedsys: Fix iomap not released issue

[Stephen Boyd]

Quoting AngeloGioacchino Del Regno (2023-06-15 05:20:51)
> In case of error after of_ioremap() the resource must be released:
> call iounmap() where appropriate to fix that.
> 
> Fixes: 41138fbf876c ("clk: mediatek: mt8173: Migrate to platform driver and common probe")
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---

Applied to clk-next