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

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

[Wei Yongjun]

From: Wei Yongjun <weiyongjun1@huawei.com>

In case of error, the function devm_ioremap() returns NULL pointer
not ERR_PTR(). The IS_ERR() test in the return value check should
be replaced with NULL test.

Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple
IORESOURCE_MEM for one pin-bank")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
 drivers/pinctrl/samsung/pinctrl-samsung.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 3bc925f..9c03eab 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -1009,7 +1009,7 @@ samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
 		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
 		virt_base[i] = devm_ioremap(&pdev->dev, res->start,
 						resource_size(res));
-		if (IS_ERR(virt_base[i]))
+		if (!virt_base[i])
 			return ERR_PTR(-EIO);
 	}

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

[Tomasz Figa]

Hi Wei,

2017-01-25 23:03 GMT+09:00 Wei Yongjun <weiyj.lk@gmail.com>:
> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> In case of error, the function devm_ioremap() returns NULL pointer
> not ERR_PTR(). The IS_ERR() test in the return value check should
> be replaced with NULL test.
>
> Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple
> IORESOURCE_MEM for one pin-bank")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
>  drivers/pinctrl/samsung/pinctrl-samsung.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 3bc925f..9c03eab 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -1009,7 +1009,7 @@ samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
>                 res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>                 virt_base[i] = devm_ioremap(&pdev->dev, res->start,
>                                                 resource_size(res));
> -               if (IS_ERR(virt_base[i]))
> +               if (!virt_base[i])

I'd rather replace devm_ioremap() (and explicit resource struct
dereferencing, without any checks by the way), with
devm_ioremap_resource() (which actually returns ERR_PTR()s, so no need
to change the condition with it).

Best regards,
Tomasz

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

[Wei Yongjun]

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(-)

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;

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

[Krzysztof Kozlowski]

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

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

[Linus Walleij]

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

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

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Tomasz Figa]

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

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

[Krzysztof Kozlowski]

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

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

[Linus Walleij]

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

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

[Andrzej Hajda]

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
>
>
>

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

[Linus Walleij]

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