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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Thu, Jun 15, 2023 at 8:21 PM AngeloGioacchino Del Regno
<[email protected]> 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 <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
On Thu, Jun 15, 2023 at 8:21 PM AngeloGioacchino Del Regno
<[email protected]> 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 <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
On Thu, Jun 15, 2023 at 8:21 PM AngeloGioacchino Del Regno
<[email protected]> 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 <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
Reviewed-by: Markus Schneider-Pargmann <[email protected]>
> ---
> 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
>
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 <[email protected]>
Reviewed-by: Markus Schneider-Pargmann <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
Applied to clk-next
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 <[email protected]>
> ---
Applied to clk-next
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 <[email protected]>
> ---
Applied to clk-next