[PATCH v2] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc

[PATCH v2] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc

[Rickard Strandqvist]

After discussion with especially Wolfram it was decided to convert the code to use the Managed Device Resource.
I have now tried to create a patch with Devres, hope it is correct.
But a code review is probably more relevant than usual.
I also lack this kind of hardware so preferably should someone do a HW test.

Rickard Strandqvist (1):
  i2c: busses: i2c-pxa.c:  Fix for possible null pointer dereferenc

 drivers/i2c/busses/i2c-pxa.c |   37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

-- 
1.7.10.4

[PATCH v2] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc

[Rickard Strandqvist]

Fix for possible null pointer dereferenc, and there is a risk for memory leak in when something unexpected happens and the function returns.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/i2c/busses/i2c-pxa.c |   37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index be671f7..886877a 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1141,10 +1141,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	struct resource *res = NULL;
 	int ret, irq;
 
-	i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
+	i2c = devm_kzalloc(&dev->dev, sizeof(struct pxa_i2c), GFP_KERNEL);
 	if (!i2c) {
 		ret = -ENOMEM;
-		goto emalloc;
+		goto err_nothing_to_release;
 	}
 
 	/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
@@ -1154,18 +1154,19 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	if (ret > 0)
 		ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
 	if (ret < 0)
-		goto eclk;
+		goto err_nothing_to_release;
 
 	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(dev, 0);
 	if (res == NULL || irq < 0) {
 		ret = -ENODEV;
-		goto eclk;
+		goto err_nothing_to_release;
 	}
 
-	if (!request_mem_region(res->start, resource_size(res), res->name)) {
+	if (!devm_request_mem_region(&dev->dev, res->start,
+				resource_size(res), res->name)) {
 		ret = -ENOMEM;
-		goto eclk;
+		goto emalloc;
 	}
 
 	i2c->adap.owner   = THIS_MODULE;
@@ -1176,16 +1177,16 @@ static int i2c_pxa_probe(struct platform_device *dev)
 
 	strlcpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
 
-	i2c->clk = clk_get(&dev->dev, NULL);
+	i2c->clk = devm_clk_get(&dev->dev, NULL);
 	if (IS_ERR(i2c->clk)) {
 		ret = PTR_ERR(i2c->clk);
-		goto eclk;
+		goto emalloc;
 	}
 
-	i2c->reg_base = ioremap(res->start, resource_size(res));
-	if (!i2c->reg_base) {
+	i2c->reg_base = devm_ioremap(&dev->dev, res->start, resource_size(res));
+	if (IS_ERR(i2c->reg_base)) {
 		ret = -EIO;
-		goto eremap;
+		goto emalloc;
 	}
 
 	i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
@@ -1227,10 +1228,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
 		i2c->adap.algo = &i2c_pxa_pio_algorithm;
 	} else {
 		i2c->adap.algo = &i2c_pxa_algorithm;
-		ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
-				  dev_name(&dev->dev), i2c);
+		ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
+				 IRQF_SHARED, dev_name(&dev->dev), i2c);
 		if (ret)
-			goto ereqirq;
+			goto emalloc;
 	}
 
 	i2c_pxa_reset(i2c);
@@ -1261,15 +1262,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
 eadapt:
 	if (!i2c->use_pio)
 		free_irq(irq, i2c);
-ereqirq:
-	clk_disable_unprepare(i2c->clk);
-	iounmap(i2c->reg_base);
-eremap:
-	clk_put(i2c->clk);
-eclk:
-	kfree(i2c);
 emalloc:
 	release_mem_region(res->start, resource_size(res));
+err_nothing_to_release:
 	return ret;
 }
 
-- 
1.7.10.4

Re: [PATCH v2] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc

[Jingoo Han]

On Monday, June 30, 2014 8:29 AM, Rickard Strandqvist wrote:
> 
> Fix for possible null pointer dereferenc, and there is a risk for memory leak in when something
> unexpected happens and the function returns.

Would you split the patch into two patches?

  [PATCH 1/2] i2c: pxa: Fix for possible null pointer dereference
  [PATCH 2/2] i2c: pxa: Use devm_* functions

> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  drivers/i2c/busses/i2c-pxa.c |   37 ++++++++++++++++---------------------
>  1 file changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index be671f7..886877a 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -1141,10 +1141,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
>  	struct resource *res = NULL;
>  	int ret, irq;
> 
> -	i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
> +	i2c = devm_kzalloc(&dev->dev, sizeof(struct pxa_i2c), GFP_KERNEL);
>  	if (!i2c) {
>  		ret = -ENOMEM;
> -		goto emalloc;
> +		goto err_nothing_to_release;
>  	}
> 
>  	/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
> @@ -1154,18 +1154,19 @@ static int i2c_pxa_probe(struct platform_device *dev)
>  	if (ret > 0)
>  		ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
>  	if (ret < 0)
> -		goto eclk;
> +		goto err_nothing_to_release;
> 
>  	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>  	irq = platform_get_irq(dev, 0);
>  	if (res == NULL || irq < 0) {
>  		ret = -ENODEV;
> -		goto eclk;
> +		goto err_nothing_to_release;
>  	}
> 
> -	if (!request_mem_region(res->start, resource_size(res), res->name)) {
> +	if (!devm_request_mem_region(&dev->dev, res->start,
> +				resource_size(res), res->name)) {
>  		ret = -ENOMEM;
> -		goto eclk;
> +		goto emalloc;
>  	}
> 
>  	i2c->adap.owner   = THIS_MODULE;
> @@ -1176,16 +1177,16 @@ static int i2c_pxa_probe(struct platform_device *dev)
> 
>  	strlcpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
> 
> -	i2c->clk = clk_get(&dev->dev, NULL);
> +	i2c->clk = devm_clk_get(&dev->dev, NULL);
>  	if (IS_ERR(i2c->clk)) {
>  		ret = PTR_ERR(i2c->clk);
> -		goto eclk;
> +		goto emalloc;
>  	}
> 
> -	i2c->reg_base = ioremap(res->start, resource_size(res));
> -	if (!i2c->reg_base) {
> +	i2c->reg_base = devm_ioremap(&dev->dev, res->start, resource_size(res));
> +	if (IS_ERR(i2c->reg_base)) {

Did you check what devm_ioremap() returns?
It returns 'valid addr value' Or NULL as below.
So, IS_ERR() should NOT be used.

./lib/devres.c

void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
                           unsigned long size)
{
        void __iomem **ptr, *addr;

        ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
        if (!ptr)
                return NULL;

        addr = ioremap(offset, size);
        if (addr) {
                *ptr = addr;
                devres_add(dev, ptr);
        } else
                devres_free(ptr);

        return addr;
}
EXPORT_SYMBOL(devm_ioremap);

Best regards,
Jingoo Han

>  		ret = -EIO;
> -		goto eremap;
> +		goto emalloc;
>  	}
> 
>  	i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
> @@ -1227,10 +1228,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
>  		i2c->adap.algo = &i2c_pxa_pio_algorithm;
>  	} else {
>  		i2c->adap.algo = &i2c_pxa_algorithm;
> -		ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
> -				  dev_name(&dev->dev), i2c);
> +		ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
> +				 IRQF_SHARED, dev_name(&dev->dev), i2c);
>  		if (ret)
> -			goto ereqirq;
> +			goto emalloc;
>  	}
> 
>  	i2c_pxa_reset(i2c);
> @@ -1261,15 +1262,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
>  eadapt:
>  	if (!i2c->use_pio)
>  		free_irq(irq, i2c);
> -ereqirq:
> -	clk_disable_unprepare(i2c->clk);
> -	iounmap(i2c->reg_base);
> -eremap:
> -	clk_put(i2c->clk);
> -eclk:
> -	kfree(i2c);
>  emalloc:
>  	release_mem_region(res->start, resource_size(res));
> +err_nothing_to_release:
>  	return ret;
>  }
> 
> --
> 1.7.10.4

Re: [PATCH v2] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc

[Rickard Strandqvist]

2014-06-30 2:05 GMT+02:00 Jingoo Han <jg1.han@samsung.com>:
> On Monday, June 30, 2014 8:29 AM, Rickard Strandqvist wrote:
>>
>> Fix for possible null pointer dereferenc, and there is a risk for memory leak in when something
>> unexpected happens and the function returns.
>
> Would you split the patch into two patches?
>
>   [PATCH 1/2] i2c: pxa: Fix for possible null pointer dereference
>   [PATCH 2/2] i2c: pxa: Use devm_* functions
>
>>
>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
>> ---
>>  drivers/i2c/busses/i2c-pxa.c |   37 ++++++++++++++++---------------------
>>  1 file changed, 16 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
>> index be671f7..886877a 100644
>> --- a/drivers/i2c/busses/i2c-pxa.c
>> +++ b/drivers/i2c/busses/i2c-pxa.c
>> @@ -1141,10 +1141,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
>>       struct resource *res = NULL;
>>       int ret, irq;
>>
>> -     i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
>> +     i2c = devm_kzalloc(&dev->dev, sizeof(struct pxa_i2c), GFP_KERNEL);
>>       if (!i2c) {
>>               ret = -ENOMEM;
>> -             goto emalloc;
>> +             goto err_nothing_to_release;
>>       }
>>
>>       /* Default adapter num to device id; i2c_pxa_probe_dt can override. */
>> @@ -1154,18 +1154,19 @@ static int i2c_pxa_probe(struct platform_device *dev)
>>       if (ret > 0)
>>               ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
>>       if (ret < 0)
>> -             goto eclk;
>> +             goto err_nothing_to_release;
>>
>>       res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>>       irq = platform_get_irq(dev, 0);
>>       if (res == NULL || irq < 0) {
>>               ret = -ENODEV;
>> -             goto eclk;
>> +             goto err_nothing_to_release;
>>       }
>>
>> -     if (!request_mem_region(res->start, resource_size(res), res->name)) {
>> +     if (!devm_request_mem_region(&dev->dev, res->start,
>> +                             resource_size(res), res->name)) {
>>               ret = -ENOMEM;
>> -             goto eclk;
>> +             goto emalloc;
>>       }
>>
>>       i2c->adap.owner   = THIS_MODULE;
>> @@ -1176,16 +1177,16 @@ static int i2c_pxa_probe(struct platform_device *dev)
>>
>>       strlcpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
>>
>> -     i2c->clk = clk_get(&dev->dev, NULL);
>> +     i2c->clk = devm_clk_get(&dev->dev, NULL);
>>       if (IS_ERR(i2c->clk)) {
>>               ret = PTR_ERR(i2c->clk);
>> -             goto eclk;
>> +             goto emalloc;
>>       }
>>
>> -     i2c->reg_base = ioremap(res->start, resource_size(res));
>> -     if (!i2c->reg_base) {
>> +     i2c->reg_base = devm_ioremap(&dev->dev, res->start, resource_size(res));
>> +     if (IS_ERR(i2c->reg_base)) {
>
> Did you check what devm_ioremap() returns?
> It returns 'valid addr value' Or NULL as below.
> So, IS_ERR() should NOT be used.
>
> ./lib/devres.c
>
> void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
>                            unsigned long size)
> {
>         void __iomem **ptr, *addr;
>
>         ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
>         if (!ptr)
>                 return NULL;
>
>         addr = ioremap(offset, size);
>         if (addr) {
>                 *ptr = addr;
>                 devres_add(dev, ptr);
>         } else
>                 devres_free(ptr);
>
>         return addr;
> }
> EXPORT_SYMBOL(devm_ioremap);
>
> Best regards,
> Jingoo Han
>
>>               ret = -EIO;
>> -             goto eremap;
>> +             goto emalloc;
>>       }
>>
>>       i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
>> @@ -1227,10 +1228,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
>>               i2c->adap.algo = &i2c_pxa_pio_algorithm;
>>       } else {
>>               i2c->adap.algo = &i2c_pxa_algorithm;
>> -             ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
>> -                               dev_name(&dev->dev), i2c);
>> +             ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
>> +                              IRQF_SHARED, dev_name(&dev->dev), i2c);
>>               if (ret)
>> -                     goto ereqirq;
>> +                     goto emalloc;
>>       }
>>
>>       i2c_pxa_reset(i2c);
>> @@ -1261,15 +1262,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
>>  eadapt:
>>       if (!i2c->use_pio)
>>               free_irq(irq, i2c);
>> -ereqirq:
>> -     clk_disable_unprepare(i2c->clk);
>> -     iounmap(i2c->reg_base);
>> -eremap:
>> -     clk_put(i2c->clk);
>> -eclk:
>> -     kfree(i2c);
>>  emalloc:
>>       release_mem_region(res->start, resource_size(res));
>> +err_nothing_to_release:
>>       return ret;
>>  }
>>
>> --
>> 1.7.10.4
>


Hi

Excuse my late reply.


1)  A fix of the original error I made in my original patch.
But it was completely different from this solution, you have trouble
seeing it as something reasonable to do.

My original patch:

 eclk:
        kfree(i2c);
 emalloc:
-       release_mem_region(res->start, resource_size(res));
+       if(res)
+               release_mem_region(res->start, resource_size(res));
        return ret;
 }

2) Wolfram Sang helped me with a patch example from commit 9b2b98a3b4de
It hade this code below. I thought that it seem a little strange, but
I trust Wolfram.

-       dev->virtbase = ioremap(adev->res.start, resource_size(&adev->res));
-       if (!dev->virtbase) {
+       dev->virtbase = devm_ioremap(&adev->dev, adev->res.start,
+                               resource_size(&adev->res));
+       if (IS_ERR(dev->virtbase)) {
                ret = -ENOMEM;
-               goto err_no_ioremap;
+               goto err_no_mem;
        }


Kind regards
Rickard Strandqvist

Re: [PATCH v2] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc

[Jingoo Han]

On Wednesday, July 02, 2014 3:26 AM, Rickard Strandqvist wrote:
> 2014-06-30 2:05 GMT+02:00 Jingoo Han <jg1.han@samsung.com>:
> > On Monday, June 30, 2014 8:29 AM, Rickard Strandqvist wrote:
> >>
> >> Fix for possible null pointer dereferenc, and there is a risk for memory leak in when something
> >> unexpected happens and the function returns.
> >
> > Would you split the patch into two patches?
> >
> >   [PATCH 1/2] i2c: pxa: Fix for possible null pointer dereference
> >   [PATCH 2/2] i2c: pxa: Use devm_* functions
> >
> >>
> >> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> >> ---
> >>  drivers/i2c/busses/i2c-pxa.c |   37 ++++++++++++++++---------------------
> >>  1 file changed, 16 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> >> index be671f7..886877a 100644
> >> --- a/drivers/i2c/busses/i2c-pxa.c
> >> +++ b/drivers/i2c/busses/i2c-pxa.c
> >> @@ -1141,10 +1141,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
> >>       struct resource *res = NULL;
> >>       int ret, irq;
> >>
> >> -     i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
> >> +     i2c = devm_kzalloc(&dev->dev, sizeof(struct pxa_i2c), GFP_KERNEL);
> >>       if (!i2c) {
> >>               ret = -ENOMEM;
> >> -             goto emalloc;
> >> +             goto err_nothing_to_release;
> >>       }
> >>
> >>       /* Default adapter num to device id; i2c_pxa_probe_dt can override. */
> >> @@ -1154,18 +1154,19 @@ static int i2c_pxa_probe(struct platform_device *dev)
> >>       if (ret > 0)
> >>               ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
> >>       if (ret < 0)
> >> -             goto eclk;
> >> +             goto err_nothing_to_release;
> >>
> >>       res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> >>       irq = platform_get_irq(dev, 0);
> >>       if (res == NULL || irq < 0) {
> >>               ret = -ENODEV;
> >> -             goto eclk;
> >> +             goto err_nothing_to_release;
> >>       }
> >>
> >> -     if (!request_mem_region(res->start, resource_size(res), res->name)) {
> >> +     if (!devm_request_mem_region(&dev->dev, res->start,
> >> +                             resource_size(res), res->name)) {
> >>               ret = -ENOMEM;
> >> -             goto eclk;
> >> +             goto emalloc;
> >>       }
> >>
> >>       i2c->adap.owner   = THIS_MODULE;
> >> @@ -1176,16 +1177,16 @@ static int i2c_pxa_probe(struct platform_device *dev)
> >>
> >>       strlcpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
> >>
> >> -     i2c->clk = clk_get(&dev->dev, NULL);
> >> +     i2c->clk = devm_clk_get(&dev->dev, NULL);
> >>       if (IS_ERR(i2c->clk)) {
> >>               ret = PTR_ERR(i2c->clk);
> >> -             goto eclk;
> >> +             goto emalloc;
> >>       }
> >>
> >> -     i2c->reg_base = ioremap(res->start, resource_size(res));
> >> -     if (!i2c->reg_base) {
> >> +     i2c->reg_base = devm_ioremap(&dev->dev, res->start, resource_size(res));
> >> +     if (IS_ERR(i2c->reg_base)) {
> >
> > Did you check what devm_ioremap() returns?
> > It returns 'valid addr value' Or NULL as below.
> > So, IS_ERR() should NOT be used.
> >
> > ./lib/devres.c
> >
> > void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> >                            unsigned long size)
> > {
> >         void __iomem **ptr, *addr;
> >
> >         ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
> >         if (!ptr)
> >                 return NULL;
> >
> >         addr = ioremap(offset, size);
> >         if (addr) {
> >                 *ptr = addr;
> >                 devres_add(dev, ptr);
> >         } else
> >                 devres_free(ptr);
> >
> >         return addr;
> > }
> > EXPORT_SYMBOL(devm_ioremap);
> >
> > Best regards,
> > Jingoo Han
> >
> >>               ret = -EIO;
> >> -             goto eremap;
> >> +             goto emalloc;
> >>       }
> >>
> >>       i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
> >> @@ -1227,10 +1228,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
> >>               i2c->adap.algo = &i2c_pxa_pio_algorithm;
> >>       } else {
> >>               i2c->adap.algo = &i2c_pxa_algorithm;
> >> -             ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
> >> -                               dev_name(&dev->dev), i2c);
> >> +             ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
> >> +                              IRQF_SHARED, dev_name(&dev->dev), i2c);
> >>               if (ret)
> >> -                     goto ereqirq;
> >> +                     goto emalloc;
> >>       }
> >>
> >>       i2c_pxa_reset(i2c);
> >> @@ -1261,15 +1262,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
> >>  eadapt:
> >>       if (!i2c->use_pio)
> >>               free_irq(irq, i2c);
> >> -ereqirq:
> >> -     clk_disable_unprepare(i2c->clk);
> >> -     iounmap(i2c->reg_base);
> >> -eremap:
> >> -     clk_put(i2c->clk);
> >> -eclk:
> >> -     kfree(i2c);
> >>  emalloc:
> >>       release_mem_region(res->start, resource_size(res));
> >> +err_nothing_to_release:
> >>       return ret;
> >>  }
> >>
> >> --
> >> 1.7.10.4
> >
> 
> 
> Hi
> 
> Excuse my late reply.
> 
> 
> 1)  A fix of the original error I made in my original patch.
> But it was completely different from this solution, you have trouble
> seeing it as something reasonable to do.
> 
> My original patch:
> 
>  eclk:
>         kfree(i2c);
>  emalloc:
> -       release_mem_region(res->start, resource_size(res));
> +       if(res)
> +               release_mem_region(res->start, resource_size(res));
>         return ret;
>  }

I see what you wanted. It looks good.
But, I still think that the patch can be split into two patches. :-)
Then, it can be more readable.

   [PATCH 1/2] i2c: pxa: Fix for possible null pointer dereference
   [PATCH 2/2] i2c: pxa: Use devm_* functions

> 
> 2) Wolfram Sang helped me with a patch example from commit 9b2b98a3b4de
> It hade this code below. I thought that it seem a little strange, but
> I trust Wolfram.

I also trust Wolfram. He is one of the most important and active
person for Linux kernel. However, the mainline kernel code explicitly
reveals that ioremap() cannot return error values. ioremap() can return
valid value or NULL. So, IS_ERR() should NOT be used.

Wolfram may mean devm_ioremap_resource(), not devm_ioremap().
On the contrary, devm_ioremap_resource() can return error values,
NULL and valid value. So, in the case of devm_ioremap_resource(),
IS_ERR() can be used, in order to check the return values of
devm_ioremap_resource().


> 
> -       dev->virtbase = ioremap(adev->res.start, resource_size(&adev->res));
> -       if (!dev->virtbase) {
> +       dev->virtbase = devm_ioremap(&adev->dev, adev->res.start,
> +                               resource_size(&adev->res));
> +       if (IS_ERR(dev->virtbase)) {

So, please refer to the both cases as below.

1. devm_ioremap_resource(): IS_ERR() can be used.

	dev->virtbase = devm_ioremap_resource(&adev->dev, res));
	if (IS_ERR(dev->virtbase)) {

2. devm_ioremap(): Only NULL checking is required, not IS_ERR().

	dev->virtbase = devm_ioremap(&adev->dev, adev->res.start,
				resource_size(&adev->res));
	if (!dev->virtbase)

Then, if you have time, please refer to the following commit,
which was already merged a few months ago. According to the patch,
"devm_ioremap() returns NULL on error, not an error."; thus,
IS_ERR() should NOT be used.

commit 37e5eb0bae7bb4d98c2153c3c3400b5c00c3cad1
(i2c: nomadik: Don't use IS_ERR for devm_ioremap)

In addition, this patch was already accepted by Wolfram Sang.
Thank you.

Best regards,
Jingoo Han

>                 ret = -ENOMEM;
> -               goto err_no_ioremap;
> +               goto err_no_mem;
>         }
> 
> 
> Kind regards
> Rickard Strandqvist

Re: [PATCH v2] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

Hi Rickard, hi Jingoo,

> I also trust Wolfram. He is one of the most important and active
> person for Linux kernel.

Oh, thanks. I'm flattered :)

> Wolfram may mean devm_ioremap_resource(), not devm_ioremap().

Yes, you are right. Sorry for missing this detail when suggesting an
example to convert to devm_*. devm_ioremap_resource should be favoured
over devm_ioremap.

> 1. devm_ioremap_resource(): IS_ERR() can be used.
> 
> 	dev->virtbase = devm_ioremap_resource(&adev->dev, res));
> 	if (IS_ERR(dev->virtbase)) {

This is correct and preferred.

BTW I don't care much about splitting up the patch as long as the commit
message says that the original bug is a motivation to swicth to devm_*.

Thanks Rickard for picking up the task, and thanks Jingoo for reviewing.

Kind regards,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc

[Rickard Strandqvist]

2014-07-03 10:41 GMT+02:00 Wolfram Sang <wsa@the-dreams.de>:
> Hi Rickard, hi Jingoo,
>
>> I also trust Wolfram. He is one of the most important and active
>> person for Linux kernel.
>
> Oh, thanks. I'm flattered :)
>
>> Wolfram may mean devm_ioremap_resource(), not devm_ioremap().
>
> Yes, you are right. Sorry for missing this detail when suggesting an
> example to convert to devm_*. devm_ioremap_resource should be favoured
> over devm_ioremap.
>
>> 1. devm_ioremap_resource(): IS_ERR() can be used.
>>
>>       dev->virtbase = devm_ioremap_resource(&adev->dev, res));
>>       if (IS_ERR(dev->virtbase)) {
>
> This is correct and preferred.
>
> BTW I don't care much about splitting up the patch as long as the commit
> message says that the original bug is a motivation to swicth to devm_*.
>
> Thanks Rickard for picking up the task, and thanks Jingoo for reviewing.
>
> Kind regards,
>
>    Wolfram

Hi

Then I changed like:

-       i2c->reg_base = devm_ioremap(&dev->dev, res->start, resource_size(res));
+       i2c->reg_base = devm_ioremap_resource(&adev->dev, res));


I like Wolfram see no point in doing this patch in two steps, as none
of patch 1, which would be the solution to the original problem would
be includid in patch 2.

I will send a V3 of this patch in a moment...


Thanks to everyone who helped out :-)


Kind regards
Rickard Strandqvist