[PATCH 0/2] thermal/drivers/mediatek: fix a regression affecting other subsystems

[PATCH 0/2] thermal/drivers/mediatek: fix a regression affecting other subsystems

[Icenowy Zheng]

In the commit I reverted as the first commit of this patchset, the
of_iomap function call, which allows multiple mapping of the same
physical memory space, is replaced to calling devm_of_iomap, which
registers exclusivity, and on my system (mt8173-elm), preventing display
from working.

So I reverted it, and to really solve the problem that the original
commit wants to solve, I read the source of auxadc-thermal and realized
that the address of these two memory blocks are not saved after probe,
and they're only used when initializing the thermal sensors. This leads
to my final fix, which is the second commit here, that adds of_iounmap
just to the probe function.

Icenowy Zheng (2):
  Revert "thermal/drivers/mediatek: Use devm_of_iomap to avoid resource
    leak in mtk_thermal_probe"
  thermal/drivers/mediatek: unmap foreign MMIO after probing

 drivers/thermal/mediatek/auxadc_thermal.c | 46 ++++++++++++-----------
 1 file changed, 24 insertions(+), 22 deletions(-)

-- 
2.39.1

[PATCH 1/2] Revert "thermal/drivers/mediatek: Use devm_of_iomap to avoid resource leak in mtk_thermal_probe"

[Icenowy Zheng]

This reverts commit f05c7b7d9ea9477fcc388476c6f4ade8c66d2d26.

As the documentation of devm_of_iomap says, it's not a drop-in
replacement for of_iomap because it will detect existed mapping and
prevent shared access to the memory region, Because these two calls
are mapping memory specified in other device nodes, they are meant to be
shared, and devm_of_iomap is not suitable.

Reverts the code back to use of_iomap, to fix a regression of display
failure on my mt8173-elm board.

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 drivers/thermal/mediatek/auxadc_thermal.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/thermal/mediatek/auxadc_thermal.c b/drivers/thermal/mediatek/auxadc_thermal.c
index 0b5528804bbd..f59d36de20a0 100644
--- a/drivers/thermal/mediatek/auxadc_thermal.c
+++ b/drivers/thermal/mediatek/auxadc_thermal.c
@@ -1222,12 +1222,7 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	auxadc_base = devm_of_iomap(&pdev->dev, auxadc, 0, NULL);
-	if (IS_ERR(auxadc_base)) {
-		of_node_put(auxadc);
-		return PTR_ERR(auxadc_base);
-	}
-
+	auxadc_base = of_iomap(auxadc, 0);
 	auxadc_phys_base = of_get_phys_base(auxadc);
 
 	of_node_put(auxadc);
@@ -1243,12 +1238,7 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	apmixed_base = devm_of_iomap(&pdev->dev, apmixedsys, 0, NULL);
-	if (IS_ERR(apmixed_base)) {
-		of_node_put(apmixedsys);
-		return PTR_ERR(apmixed_base);
-	}
-
+	apmixed_base = of_iomap(apmixedsys, 0);
 	apmixed_phys_base = of_get_phys_base(apmixedsys);
 
 	of_node_put(apmixedsys);
-- 
2.39.1

[PATCH 2/2] thermal/drivers/mediatek: unmap foreign MMIO after probing

[Icenowy Zheng]

The MMIO space from auxadc and apmixedsys devices are only needed when
initializing the thermal sensors, so these memory regions could be
safely unmapped after the probing process to prevent resource leak.

Unmap them at the end of the probe method, and route the unmap codepath
to early exits of the probe method for preventing leak when error
handling.

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 drivers/thermal/mediatek/auxadc_thermal.c | 32 ++++++++++++++++-------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/mediatek/auxadc_thermal.c b/drivers/thermal/mediatek/auxadc_thermal.c
index f59d36de20a0..c010a96f9aca 100644
--- a/drivers/thermal/mediatek/auxadc_thermal.c
+++ b/drivers/thermal/mediatek/auxadc_thermal.c
@@ -1229,13 +1229,15 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 
 	if (auxadc_phys_base == OF_BAD_ADDR) {
 		dev_err(&pdev->dev, "Can't get auxadc phys address\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_unmap_auxadc;
 	}
 
 	apmixedsys = of_parse_phandle(np, "mediatek,apmixedsys", 0);
 	if (!apmixedsys) {
 		dev_err(&pdev->dev, "missing apmixedsys node\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto out_unmap_auxadc;
 	}
 
 	apmixed_base = of_iomap(apmixedsys, 0);
@@ -1245,25 +1247,26 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 
 	if (apmixed_phys_base == OF_BAD_ADDR) {
 		dev_err(&pdev->dev, "Can't get auxadc phys address\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_unmap_apmixed;
 	}
 
 	ret = device_reset_optional(&pdev->dev);
 	if (ret)
-		return ret;
+		goto out_unmap_apmixed;
 
 	mt->clk_auxadc = devm_clk_get_enabled(&pdev->dev, "auxadc");
 	if (IS_ERR(mt->clk_auxadc)) {
 		ret = PTR_ERR(mt->clk_auxadc);
 		dev_err(&pdev->dev, "Can't enable auxadc clk: %d\n", ret);
-		return ret;
+		goto out_unmap_apmixed;
 	}
 
 	mt->clk_peri_therm = devm_clk_get_enabled(&pdev->dev, "therm");
 	if (IS_ERR(mt->clk_peri_therm)) {
 		ret = PTR_ERR(mt->clk_peri_therm);
 		dev_err(&pdev->dev, "Can't enable peri clk: %d\n", ret);
-		return ret;
+		goto out_unmap_apmixed;
 	}
 
 	mtk_thermal_turn_on_buffer(mt, apmixed_base);
@@ -1287,14 +1290,23 @@ static int mtk_thermal_probe(struct platform_device *pdev)
 
 	tzdev = devm_thermal_of_zone_register(&pdev->dev, 0, mt,
 					      &mtk_thermal_ops);
-	if (IS_ERR(tzdev))
-		return PTR_ERR(tzdev);
+	if (IS_ERR(tzdev)) {
+		ret = PTR_ERR(tzdev);
+		goto out_unmap_apmixed;
+	}
 
 	ret = devm_thermal_add_hwmon_sysfs(&pdev->dev, tzdev);
-	if (ret)
+	if (ret) {
 		dev_warn(&pdev->dev, "error in thermal_add_hwmon_sysfs");
+		ret = 0;
+	}
 
-	return 0;
+out_unmap_apmixed:
+	iounmap(apmixed_base);
+out_unmap_auxadc:
+	iounmap(auxadc_base);
+
+	return ret;
 }
 
 static struct platform_driver mtk_thermal_driver = {
-- 
2.39.1

Re: [PATCH 0/2] thermal/drivers/mediatek: fix a regression affecting other subsystems

[Daniel Lezcano]

On 29/05/2023 18:20, Icenowy Zheng wrote:
> In the commit I reverted as the first commit of this patchset, the
> of_iomap function call, which allows multiple mapping of the same
> physical memory space, is replaced to calling devm_of_iomap, which
> registers exclusivity, and on my system (mt8173-elm), preventing display
> from working.
> 
> So I reverted it, and to really solve the problem that the original
> commit wants to solve, I read the source of auxadc-thermal and realized
> that the address of these two memory blocks are not saved after probe,
> and they're only used when initializing the thermal sensors. This leads
> to my final fix, which is the second commit here, that adds of_iounmap
> just to the probe function.
> 
> Icenowy Zheng (2):
>    Revert "thermal/drivers/mediatek: Use devm_of_iomap to avoid resource
>      leak in mtk_thermal_probe"
>    thermal/drivers/mediatek: unmap foreign MMIO after probing
> 
>   drivers/thermal/mediatek/auxadc_thermal.c | 46 ++++++++++++-----------
>   1 file changed, 24 insertions(+), 22 deletions(-)

I'll apply only the revert and let you revisit the patch 2 which could 
be improved.

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/2] thermal/drivers/mediatek: fix a regression affecting other subsystems

[Daniel Lezcano]

On 29/05/2023 18:20, Icenowy Zheng wrote:
> In the commit I reverted as the first commit of this patchset, the
> of_iomap function call, which allows multiple mapping of the same
> physical memory space, is replaced to calling devm_of_iomap, which
> registers exclusivity, and on my system (mt8173-elm), preventing display
> from working.
> 
> So I reverted it, and to really solve the problem that the original
> commit wants to solve, I read the source of auxadc-thermal and realized
> that the address of these two memory blocks are not saved after probe,
> and they're only used when initializing the thermal sensors. This leads
> to my final fix, which is the second commit here, that adds of_iounmap
> just to the probe function.
> 
> Icenowy Zheng (2):
>    Revert "thermal/drivers/mediatek: Use devm_of_iomap to avoid resource
>      leak in mtk_thermal_probe"
>    thermal/drivers/mediatek: unmap foreign MMIO after probing
> 
>   drivers/thermal/mediatek/auxadc_thermal.c | 46 ++++++++++++-----------
>   1 file changed, 24 insertions(+), 22 deletions(-)

I'll apply only the revert and let you revisit the patch 2 which could 
be improved.

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] thermal/drivers/mediatek: fix a regression affecting other subsystems

[Icenowy Zheng]

在 2023-06-13星期二的 10:44 +0200，Daniel Lezcano写道：
> On 29/05/2023 18:20, Icenowy Zheng wrote:
> > In the commit I reverted as the first commit of this patchset, the
> > of_iomap function call, which allows multiple mapping of the same
> > physical memory space, is replaced to calling devm_of_iomap, which
> > registers exclusivity, and on my system (mt8173-elm), preventing
> > display
> > from working.
> > 
> > So I reverted it, and to really solve the problem that the original
> > commit wants to solve, I read the source of auxadc-thermal and
> > realized
> > that the address of these two memory blocks are not saved after
> > probe,
> > and they're only used when initializing the thermal sensors. This
> > leads
> > to my final fix, which is the second commit here, that adds
> > of_iounmap
> > just to the probe function.
> > 
> > Icenowy Zheng (2):
> >    Revert "thermal/drivers/mediatek: Use devm_of_iomap to avoid
> > resource
> >      leak in mtk_thermal_probe"
> >    thermal/drivers/mediatek: unmap foreign MMIO after probing
> > 
> >   drivers/thermal/mediatek/auxadc_thermal.c | 46 ++++++++++++------
> > -----
> >   1 file changed, 24 insertions(+), 22 deletions(-)
> 
> I'll apply only the revert and let you revisit the patch 2 which
> could 
> be improved.

Sorry, is the first patch applied? I didn't see it in any kernel
trees...

I have no current idea about how to fix in patch 2.

> 

Re: [PATCH 0/2] thermal/drivers/mediatek: fix a regression affecting other subsystems

[Icenowy Zheng]

在 2023-06-13星期二的 10:44 +0200，Daniel Lezcano写道：
> On 29/05/2023 18:20, Icenowy Zheng wrote:
> > In the commit I reverted as the first commit of this patchset, the
> > of_iomap function call, which allows multiple mapping of the same
> > physical memory space, is replaced to calling devm_of_iomap, which
> > registers exclusivity, and on my system (mt8173-elm), preventing
> > display
> > from working.
> > 
> > So I reverted it, and to really solve the problem that the original
> > commit wants to solve, I read the source of auxadc-thermal and
> > realized
> > that the address of these two memory blocks are not saved after
> > probe,
> > and they're only used when initializing the thermal sensors. This
> > leads
> > to my final fix, which is the second commit here, that adds
> > of_iounmap
> > just to the probe function.
> > 
> > Icenowy Zheng (2):
> >    Revert "thermal/drivers/mediatek: Use devm_of_iomap to avoid
> > resource
> >      leak in mtk_thermal_probe"
> >    thermal/drivers/mediatek: unmap foreign MMIO after probing
> > 
> >   drivers/thermal/mediatek/auxadc_thermal.c | 46 ++++++++++++------
> > -----
> >   1 file changed, 24 insertions(+), 22 deletions(-)
> 
> I'll apply only the revert and let you revisit the patch 2 which
> could 
> be improved.

Sorry, is the first patch applied? I didn't see it in any kernel
trees...

I have no current idea about how to fix in patch 2.

> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] thermal/drivers/mediatek: fix a regression affecting other subsystems

[Daniel Lezcano]

On 22/07/2023 14:13, Icenowy Zheng wrote:
> 在 2023-06-13星期二的 10:44 +0200，Daniel Lezcano写道：
>> On 29/05/2023 18:20, Icenowy Zheng wrote:
>>> In the commit I reverted as the first commit of this patchset, the
>>> of_iomap function call, which allows multiple mapping of the same
>>> physical memory space, is replaced to calling devm_of_iomap, which
>>> registers exclusivity, and on my system (mt8173-elm), preventing
>>> display
>>> from working.
>>>
>>> So I reverted it, and to really solve the problem that the original
>>> commit wants to solve, I read the source of auxadc-thermal and
>>> realized
>>> that the address of these two memory blocks are not saved after
>>> probe,
>>> and they're only used when initializing the thermal sensors. This
>>> leads
>>> to my final fix, which is the second commit here, that adds
>>> of_iounmap
>>> just to the probe function.
>>>
>>> Icenowy Zheng (2):
>>>     Revert "thermal/drivers/mediatek: Use devm_of_iomap to avoid
>>> resource
>>>       leak in mtk_thermal_probe"
>>>     thermal/drivers/mediatek: unmap foreign MMIO after probing
>>>
>>>    drivers/thermal/mediatek/auxadc_thermal.c | 46 ++++++++++++------
>>> -----
>>>    1 file changed, 24 insertions(+), 22 deletions(-)
>>
>> I'll apply only the revert and let you revisit the patch 2 which
>> could
>> be improved.
> 
> Sorry, is the first patch applied? I didn't see it in any kernel
> trees...

Yes, sorry, I got another patch meanwhile which provided the same revert 
with more tags

https://lore.kernel.org/r/20230525121811.3360268-1-ricardo.canuelo@collabora.com


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/2] thermal/drivers/mediatek: fix a regression affecting other subsystems

[Daniel Lezcano]

On 22/07/2023 14:13, Icenowy Zheng wrote:
> 在 2023-06-13星期二的 10:44 +0200，Daniel Lezcano写道：
>> On 29/05/2023 18:20, Icenowy Zheng wrote:
>>> In the commit I reverted as the first commit of this patchset, the
>>> of_iomap function call, which allows multiple mapping of the same
>>> physical memory space, is replaced to calling devm_of_iomap, which
>>> registers exclusivity, and on my system (mt8173-elm), preventing
>>> display
>>> from working.
>>>
>>> So I reverted it, and to really solve the problem that the original
>>> commit wants to solve, I read the source of auxadc-thermal and
>>> realized
>>> that the address of these two memory blocks are not saved after
>>> probe,
>>> and they're only used when initializing the thermal sensors. This
>>> leads
>>> to my final fix, which is the second commit here, that adds
>>> of_iounmap
>>> just to the probe function.
>>>
>>> Icenowy Zheng (2):
>>>     Revert "thermal/drivers/mediatek: Use devm_of_iomap to avoid
>>> resource
>>>       leak in mtk_thermal_probe"
>>>     thermal/drivers/mediatek: unmap foreign MMIO after probing
>>>
>>>    drivers/thermal/mediatek/auxadc_thermal.c | 46 ++++++++++++------
>>> -----
>>>    1 file changed, 24 insertions(+), 22 deletions(-)
>>
>> I'll apply only the revert and let you revisit the patch 2 which
>> could
>> be improved.
> 
> Sorry, is the first patch applied? I didn't see it in any kernel
> trees...

Yes, sorry, I got another patch meanwhile which provided the same revert 
with more tags

https://lore.kernel.org/r/20230525121811.3360268-1-ricardo.canuelo@collabora.com


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] thermal/drivers/mediatek: fix a regression affecting other subsystems

[Dan Carpenter]

On Tue, Jun 13, 2023 at 10:44:51AM +0200, Daniel Lezcano wrote:
> On 29/05/2023 18:20, Icenowy Zheng wrote:
> > In the commit I reverted as the first commit of this patchset, the
> > of_iomap function call, which allows multiple mapping of the same
> > physical memory space, is replaced to calling devm_of_iomap, which
> > registers exclusivity, and on my system (mt8173-elm), preventing display
> > from working.
> > 
> > So I reverted it, and to really solve the problem that the original
> > commit wants to solve, I read the source of auxadc-thermal and realized
> > that the address of these two memory blocks are not saved after probe,
> > and they're only used when initializing the thermal sensors. This leads
> > to my final fix, which is the second commit here, that adds of_iounmap
> > just to the probe function.
> > 
> > Icenowy Zheng (2):
> >    Revert "thermal/drivers/mediatek: Use devm_of_iomap to avoid resource
> >      leak in mtk_thermal_probe"
> >    thermal/drivers/mediatek: unmap foreign MMIO after probing
> > 
> >   drivers/thermal/mediatek/auxadc_thermal.c | 46 ++++++++++++-----------
> >   1 file changed, 24 insertions(+), 22 deletions(-)
> 
> I'll apply only the revert and let you revisit the patch 2 which could be
> improved.

What's the issue with patch 2/2?  It looks okay to me.

regards,
dan carpenter

Re: [PATCH 0/2] thermal/drivers/mediatek: fix a regression affecting other subsystems

[Dan Carpenter]

On Tue, Jun 13, 2023 at 10:44:51AM +0200, Daniel Lezcano wrote:
> On 29/05/2023 18:20, Icenowy Zheng wrote:
> > In the commit I reverted as the first commit of this patchset, the
> > of_iomap function call, which allows multiple mapping of the same
> > physical memory space, is replaced to calling devm_of_iomap, which
> > registers exclusivity, and on my system (mt8173-elm), preventing display
> > from working.
> > 
> > So I reverted it, and to really solve the problem that the original
> > commit wants to solve, I read the source of auxadc-thermal and realized
> > that the address of these two memory blocks are not saved after probe,
> > and they're only used when initializing the thermal sensors. This leads
> > to my final fix, which is the second commit here, that adds of_iounmap
> > just to the probe function.
> > 
> > Icenowy Zheng (2):
> >    Revert "thermal/drivers/mediatek: Use devm_of_iomap to avoid resource
> >      leak in mtk_thermal_probe"
> >    thermal/drivers/mediatek: unmap foreign MMIO after probing
> > 
> >   drivers/thermal/mediatek/auxadc_thermal.c | 46 ++++++++++++-----------
> >   1 file changed, 24 insertions(+), 22 deletions(-)
> 
> I'll apply only the revert and let you revisit the patch 2 which could be
> improved.

What's the issue with patch 2/2?  It looks okay to me.

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel