| Message ID | 1590120740-22912-2-git-send-email-yangtiezhu@loongson.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] gpio: bcm-kona: Fix return value of bcm_kona_gpio_probe() | expand |
Tiezhu Yang <yangtiezhu@loongson.cn> writes: > When call function devm_platform_ioremap_resource(), we should use IS_ERR() > to check the return value and return PTR_ERR() if failed. > > Fixes: 542c25b7a209 ("drivers: gpio: pxa: use devm_platform_ioremap_resource()") > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > --- > drivers/gpio/gpio-pxa.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c > index 1361270..0cb6600 100644 > --- a/drivers/gpio/gpio-pxa.c > +++ b/drivers/gpio/gpio-pxa.c > @@ -660,8 +660,8 @@ static int pxa_gpio_probe(struct platform_device *pdev) > pchip->irq1 = irq1; > > gpio_reg_base = devm_platform_ioremap_resource(pdev, 0); > - if (!gpio_reg_base) > - return -EINVAL; > + if (IS_ERR(gpio_reg_base)) > + return PTR_ERR(gpio_reg_base); As far as I know, devm_platform_ioremap_resource() could return NULL which is not handled by this test (unless __devm_ioremap() semantics changed since I had a look). Therefore, this patch is incorrect, or rather incomplete. Cheers.
On 05/23/2020 03:07 AM, Robert Jarzmik wrote: > Tiezhu Yang <yangtiezhu@loongson.cn> writes: > >> When call function devm_platform_ioremap_resource(), we should use IS_ERR() >> to check the return value and return PTR_ERR() if failed. >> >> Fixes: 542c25b7a209 ("drivers: gpio: pxa: use devm_platform_ioremap_resource()") >> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> >> --- >> drivers/gpio/gpio-pxa.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c >> index 1361270..0cb6600 100644 >> --- a/drivers/gpio/gpio-pxa.c >> +++ b/drivers/gpio/gpio-pxa.c >> @@ -660,8 +660,8 @@ static int pxa_gpio_probe(struct platform_device *pdev) >> pchip->irq1 = irq1; >> >> gpio_reg_base = devm_platform_ioremap_resource(pdev, 0); >> - if (!gpio_reg_base) >> - return -EINVAL; >> + if (IS_ERR(gpio_reg_base)) >> + return PTR_ERR(gpio_reg_base); > As far as I know, devm_platform_ioremap_resource() could return NULL which is > not handled by this test (unless __devm_ioremap() semantics changed since I had > a look). Hi Robert, In the function __devm_ioremap_resource(), if __devm_ioremap returns NULL, it will return IOMEM_ERR_PTR(-ENOMEM). devm_platform_ioremap_resource() devm_ioremap_resource() __devm_ioremap_resource() __devm_ioremap() static void __iomem * __devm_ioremap_resource(struct device *dev, const struct resource *res, enum devm_ioremap_type type) { ... dest_ptr = __devm_ioremap(dev, res->start, size, type); if (!dest_ptr) { dev_err(dev, "ioremap failed for resource %pR\n", res); devm_release_mem_region(dev, res->start, size); dest_ptr = IOMEM_ERR_PTR(-ENOMEM); } return dest_ptr; } And also, we can see the comment of devm_ioremap_resource(): Usage example: res = platform_get_resource(pdev, IORESOURCE_MEM, 0); base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(base)) return PTR_ERR(base); > > Therefore, this patch is incorrect, or rather incomplete. So I think this patch is correct, do I miss something? Thanks, Tiezhu Yang > > Cheers. >
sob., 23 maj 2020 o 05:25 Tiezhu Yang <yangtiezhu@loongson.cn> napisał(a): > > On 05/23/2020 03:07 AM, Robert Jarzmik wrote: > > Tiezhu Yang <yangtiezhu@loongson.cn> writes: > > > >> When call function devm_platform_ioremap_resource(), we should use IS_ERR() > >> to check the return value and return PTR_ERR() if failed. > >> > >> Fixes: 542c25b7a209 ("drivers: gpio: pxa: use devm_platform_ioremap_resource()") > >> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> > >> --- > >> drivers/gpio/gpio-pxa.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c > >> index 1361270..0cb6600 100644 > >> --- a/drivers/gpio/gpio-pxa.c > >> +++ b/drivers/gpio/gpio-pxa.c > >> @@ -660,8 +660,8 @@ static int pxa_gpio_probe(struct platform_device *pdev) > >> pchip->irq1 = irq1; > >> > >> gpio_reg_base = devm_platform_ioremap_resource(pdev, 0); > >> - if (!gpio_reg_base) > >> - return -EINVAL; > >> + if (IS_ERR(gpio_reg_base)) > >> + return PTR_ERR(gpio_reg_base); > > As far as I know, devm_platform_ioremap_resource() could return NULL which is > > not handled by this test (unless __devm_ioremap() semantics changed since I had > > a look). > > Hi Robert, > > In the function __devm_ioremap_resource(), if __devm_ioremap returns NULL, > it will return IOMEM_ERR_PTR(-ENOMEM). > > devm_platform_ioremap_resource() > devm_ioremap_resource() > __devm_ioremap_resource() > __devm_ioremap() > > static void __iomem * > __devm_ioremap_resource(struct device *dev, const struct resource *res, > enum devm_ioremap_type type) > { > ... > dest_ptr = __devm_ioremap(dev, res->start, size, type); > if (!dest_ptr) { > dev_err(dev, "ioremap failed for resource %pR\n", res); > devm_release_mem_region(dev, res->start, size); > dest_ptr = IOMEM_ERR_PTR(-ENOMEM); > } > > return dest_ptr; > } > > And also, we can see the comment of devm_ioremap_resource(): > > Usage example: > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > base = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(base)) > return PTR_ERR(base); > > > > > Therefore, this patch is incorrect, or rather incomplete. > > So I think this patch is correct, do I miss something? > > Yes, this patch is correct. Applied for fixes. Bart
Tiezhu Yang <yangtiezhu@loongson.cn> writes: > On 05/23/2020 03:07 AM, Robert Jarzmik wrote: >> Tiezhu Yang <yangtiezhu@loongson.cn> writes: >> >>> When call function devm_platform_ioremap_resource(), we should use IS_ERR() >>> to check the return value and return PTR_ERR() if failed. >>> >>> Fixes: 542c25b7a209 ("drivers: gpio: pxa: use devm_platform_ioremap_resource()") >>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> >>> --- >>> drivers/gpio/gpio-pxa.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c >>> index 1361270..0cb6600 100644 >>> --- a/drivers/gpio/gpio-pxa.c >>> +++ b/drivers/gpio/gpio-pxa.c >>> @@ -660,8 +660,8 @@ static int pxa_gpio_probe(struct platform_device *pdev) >>> pchip->irq1 = irq1; >>> gpio_reg_base = devm_platform_ioremap_resource(pdev, 0); >>> - if (!gpio_reg_base) >>> - return -EINVAL; >>> + if (IS_ERR(gpio_reg_base)) >>> + return PTR_ERR(gpio_reg_base); >> As far as I know, devm_platform_ioremap_resource() could return NULL which is >> not handled by this test (unless __devm_ioremap() semantics changed since I had >> a look). > > Hi Robert, > > In the function __devm_ioremap_resource(), if __devm_ioremap returns NULL, > it will return IOMEM_ERR_PTR(-ENOMEM). > > devm_platform_ioremap_resource() > devm_ioremap_resource() > __devm_ioremap_resource() > __devm_ioremap() > > static void __iomem * > __devm_ioremap_resource(struct device *dev, const struct resource *res, > enum devm_ioremap_type type) > { > ... > dest_ptr = __devm_ioremap(dev, res->start, size, type); > if (!dest_ptr) { > dev_err(dev, "ioremap failed for resource %pR\n", res); > devm_release_mem_region(dev, res->start, size); > dest_ptr = IOMEM_ERR_PTR(-ENOMEM); > } > > return dest_ptr; > } > > And also, we can see the comment of devm_ioremap_resource(): > > Usage example: > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > base = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(base)) > return PTR_ERR(base); > >> >> Therefore, this patch is incorrect, or rather incomplete. > > So I think this patch is correct, do I miss something? You're right, my bad, didn't see the test in __devm_ioremap_resource(). Cheers.
diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c index 1361270..0cb6600 100644 --- a/drivers/gpio/gpio-pxa.c +++ b/drivers/gpio/gpio-pxa.c @@ -660,8 +660,8 @@ static int pxa_gpio_probe(struct platform_device *pdev) pchip->irq1 = irq1; gpio_reg_base = devm_platform_ioremap_resource(pdev, 0); - if (!gpio_reg_base) - return -EINVAL; + if (IS_ERR(gpio_reg_base)) + return PTR_ERR(gpio_reg_base); clk = clk_get(&pdev->dev, NULL); if (IS_ERR(clk)) {
When call function devm_platform_ioremap_resource(), we should use IS_ERR() to check the return value and return PTR_ERR() if failed. Fixes: 542c25b7a209 ("drivers: gpio: pxa: use devm_platform_ioremap_resource()") Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn> --- drivers/gpio/gpio-pxa.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)