diff mbox

[-next,v2] pinctrl: samsung: Fix return value check in samsung_pinctrl_get_soc_data()

Message ID 20170205155849.4362-1-weiyj.lk@gmail.com
State Not Applicable
Headers show

Commit Message

Wei Yongjun Feb. 5, 2017, 3:58 p.m. UTC
From: Wei Yongjun <weiyongjun1@huawei.com>

In case of error, the function devm_ioremap() returns NULL pointer not
ERR_PTR(). Fix by using devm_ioremap_resource instead of devm_ioremap.

Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple
IORESOURCE_MEM for one pin-bank")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
v1 -> v2: use devm_ioremap_resource instead of devm_ioremap
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Krzysztof Kozlowski Feb. 7, 2017, 7:56 p.m. UTC | #1
On Sun, Feb 05, 2017 at 03:58:49PM +0000, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
> 
> In case of error, the function devm_ioremap() returns NULL pointer not
> ERR_PTR(). Fix by using devm_ioremap_resource instead of devm_ioremap.
> 
> Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple
> IORESOURCE_MEM for one pin-bank")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
> v1 -> v2: use devm_ioremap_resource instead of devm_ioremap
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Feb. 13, 2017, 2:49 p.m. UTC | #2
On Sun, Feb 5, 2017 at 4:58 PM, Wei Yongjun <weiyj.lk@gmail.com> wrote:

> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> In case of error, the function devm_ioremap() returns NULL pointer not
> ERR_PTR(). Fix by using devm_ioremap_resource instead of devm_ioremap.
>
> Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple
> IORESOURCE_MEM for one pin-bank")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
> v1 -> v2: use devm_ioremap_resource instead of devm_ioremap

Patch applied with Krzysztof's ACK.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski Feb. 21, 2017, 1:49 p.m. UTC | #3
Hi All,

On 2017-02-13 15:49, Linus Walleij wrote:
> On Sun, Feb 5, 2017 at 4:58 PM, Wei Yongjun <weiyj.lk@gmail.com> wrote:
>
>> From: Wei Yongjun <weiyongjun1@huawei.com>
>>
>> In case of error, the function devm_ioremap() returns NULL pointer not
>> ERR_PTR(). Fix by using devm_ioremap_resource instead of devm_ioremap.
>>
>> Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple
>> IORESOURCE_MEM for one pin-bank")
>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>> ---
>> v1 -> v2: use devm_ioremap_resource instead of devm_ioremap
> Patch applied with Krzysztof's ACK.

Sadly this patch breaks support for IMEM pinctrl block on Exynos5433/TM2
and it took us some time to find the source of the problem.

devm_ioremap_resource() is not functionally a full equivalent of
devm_ioremap(). The problem here is that registers for IMEM and ALIVE
pin controllers are shared and both devices have <0x11090000 0x1000>
range in their reg property. devm_ioremap_resource() maps given
resource exclusively for the device, while devm_ioremap() allows
non-exclusive mappings.

This patch has to be reverted asap.

Best regards
Tomasz Figa Feb. 21, 2017, 2:14 p.m. UTC | #4
2017-02-21 22:49 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
> Hi All,
>
> On 2017-02-13 15:49, Linus Walleij wrote:
>>
>> On Sun, Feb 5, 2017 at 4:58 PM, Wei Yongjun <weiyj.lk@gmail.com> wrote:
>>
>>> From: Wei Yongjun <weiyongjun1@huawei.com>
>>>
>>> In case of error, the function devm_ioremap() returns NULL pointer not
>>> ERR_PTR(). Fix by using devm_ioremap_resource instead of devm_ioremap.
>>>
>>> Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple
>>> IORESOURCE_MEM for one pin-bank")
>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>>> ---
>>> v1 -> v2: use devm_ioremap_resource instead of devm_ioremap
>>
>> Patch applied with Krzysztof's ACK.
>
>
> Sadly this patch breaks support for IMEM pinctrl block on Exynos5433/TM2
> and it took us some time to find the source of the problem.
>
> devm_ioremap_resource() is not functionally a full equivalent of
> devm_ioremap(). The problem here is that registers for IMEM and ALIVE
> pin controllers are shared and both devices have <0x11090000 0x1000>
> range in their reg property. devm_ioremap_resource() maps given
> resource exclusively for the device, while devm_ioremap() allows
> non-exclusive mappings.
>
> This patch has to be reverted asap.

Are IMEM and ALIVE pincontrollers really separate then? Typically one
models the hardware as different blocks when the registers are
separate and that's also why devm_ioremap_resource() behaves like it
does. Maybe in this case there should be only one pin controller
defined instead?

In any case, I agree that the patch should be reverted for now.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Feb. 21, 2017, 3:32 p.m. UTC | #5
On Tue, Feb 21, 2017 at 3:49 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hi All,
>
> On 2017-02-13 15:49, Linus Walleij wrote:
>>
>> On Sun, Feb 5, 2017 at 4:58 PM, Wei Yongjun <weiyj.lk@gmail.com> wrote:
>>
>>> From: Wei Yongjun <weiyongjun1@huawei.com>
>>>
>>> In case of error, the function devm_ioremap() returns NULL pointer not
>>> ERR_PTR(). Fix by using devm_ioremap_resource instead of devm_ioremap.
>>>
>>> Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple
>>> IORESOURCE_MEM for one pin-bank")
>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>>> ---
>>> v1 -> v2: use devm_ioremap_resource instead of devm_ioremap
>>
>> Patch applied with Krzysztof's ACK.
>
>
> Sadly this patch breaks support for IMEM pinctrl block on Exynos5433/TM2
> and it took us some time to find the source of the problem.
>
> devm_ioremap_resource() is not functionally a full equivalent of
> devm_ioremap(). The problem here is that registers for IMEM and ALIVE
> pin controllers are shared and both devices have <0x11090000 0x1000>
> range in their reg property. devm_ioremap_resource() maps given
> resource exclusively for the device, while devm_ioremap() allows
> non-exclusive mappings.
>
> This patch has to be reverted asap.

Damn, the additional request_mem_region() raised my concerns but I
didn't compare it with actual DTS layout... Which brings us to two
important lessons:
1. Do not accept untested code.
2. Do not fix two things at the same time.

I vote for the revert as well + original version of patch (these two
could be squashed) + a need of Tested-by. Otherwise an untested fix
might not be a fix at all.

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij March 14, 2017, 10:29 a.m. UTC | #6
On Tue, Feb 21, 2017 at 2:49 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:

> Sadly this patch breaks support for IMEM pinctrl block on Exynos5433/TM2
> and it took us some time to find the source of the problem.
>
> devm_ioremap_resource() is not functionally a full equivalent of
> devm_ioremap(). The problem here is that registers for IMEM and ALIVE
> pin controllers are shared and both devices have <0x11090000 0x1000>
> range in their reg property. devm_ioremap_resource() maps given
> resource exclusively for the device, while devm_ioremap() allows
> non-exclusive mappings.
>
> This patch has to be reverted asap.

Patch reverted for fixes, copying the above text as commit message.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrzej Hajda March 14, 2017, 10:44 a.m. UTC | #7
Hi Linus,

I have posted proper fix some times ago[1], no need to revert.

[1]: https://www.spinics.net/lists/linux-samsung-soc/msg58395.html

Regards
Andrzej


On 14.03.2017 11:29, Linus Walleij wrote:
> On Tue, Feb 21, 2017 at 2:49 PM, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>
>> Sadly this patch breaks support for IMEM pinctrl block on Exynos5433/TM2
>> and it took us some time to find the source of the problem.
>>
>> devm_ioremap_resource() is not functionally a full equivalent of
>> devm_ioremap(). The problem here is that registers for IMEM and ALIVE
>> pin controllers are shared and both devices have <0x11090000 0x1000>
>> range in their reg property. devm_ioremap_resource() maps given
>> resource exclusively for the device, while devm_ioremap() allows
>> non-exclusive mappings.
>>
>> This patch has to be reverted asap.
> Patch reverted for fixes, copying the above text as commit message.
>
> Yours,
> Linus Walleij
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij March 15, 2017, 1:45 p.m. UTC | #8
On Tue, Mar 14, 2017 at 11:44 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:

> I have posted proper fix some times ago[1], no need to revert.
>
> [1]: https://www.spinics.net/lists/linux-samsung-soc/msg58395.html

OK fixed that now.

Unreverted the revert, applied your patch.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index d79eada..6dec061 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -988,10 +988,9 @@  samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
 
 	for (i = 0; i < ctrl->nr_ext_resources + 1; i++) {
 		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
-		virt_base[i] = devm_ioremap(&pdev->dev, res->start,
-						resource_size(res));
+		virt_base[i] = devm_ioremap_resource(&pdev->dev, res);
 		if (IS_ERR(virt_base[i]))
-			return ERR_PTR(-EIO);
+			return ERR_CAST(virt_base[i]);
 	}
 
 	bank = d->pin_banks;