| Message ID | 20170205155849.4362-1-weiyj.lk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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 > > >
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
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;