[PATCH 0/6] I2C cleanup

[PATCH 0/6] I2C cleanup

[Shubhrajyoti D]

Prevent possible NULL pointer access. 

Only the omap patch is tested.
I would be grateful if anone having the platform
helps me test it.

Shubhrajyoti D (6):
  i2c: omap: Prevent NULL pointer dereference in remove
  i2c-xiic:  Fix a possible NULL pointer access
  i2c: designware: Fix a possible NULL pointer access
  i2c: at91: Fix a possible NULL pointer access
  i2c-puv3: Fix a possible NULL pointer access
  i2c: davinci: Fix a possible NULL pointer access

 drivers/i2c/busses/i2c-at91.c               |    2 +-
 drivers/i2c/busses/i2c-davinci.c            |    2 +-
 drivers/i2c/busses/i2c-designware-platdrv.c |    2 +-
 drivers/i2c/busses/i2c-omap.c               |    4 ++--
 drivers/i2c/busses/i2c-puv3.c               |    2 +-
 drivers/i2c/busses/i2c-xiic.c               |    3 +--
 6 files changed, 7 insertions(+), 8 deletions(-)

-- 
1.7.5.4

[PATCH 1/6] i2c: omap: Prevent NULL pointer dereference in remove

[Shubhrajyoti D]

Prevent the NULL pointer access by moving the platform_set_drvdata function
after the access of the pdev.

[  654.961761] Unable to handle kernel NULL pointer dereference at virtual address 00000070
[  654.970611] pgd = df254000
[  654.973480] [00000070] *pgd=9f1da831, *pte=00000000, *ppte=00000000
[  654.980163] Internal error: Oops: 17 [#1] SMP ARM
[  654.985076] Modules linked in:
[  654.988281] CPU: 1    Not tainted  (3.6.0-rc1-00031-ge547de1-dirty #339)
[  654.995330] PC is at omap_i2c_runtime_resume+0x8/0x148
[  655.000732] LR is at omap_i2c_runtime_resume+0x8/0x148

Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c8e3886..0c593d4 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1217,8 +1217,6 @@ static int __devexit omap_i2c_remove(struct platform_device *pdev)
 	struct omap_i2c_dev	*dev = platform_get_drvdata(pdev);
 	int ret;
 
-	platform_set_drvdata(pdev, NULL);
-
 	i2c_del_adapter(&dev->adapter);
 	ret = pm_runtime_get_sync(&pdev->dev);
 	if (IS_ERR_VALUE(ret))
@@ -1227,6 +1225,8 @@ static int __devexit omap_i2c_remove(struct platform_device *pdev)
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	platform_set_drvdata(pdev, NULL);
+
 	return 0;
 }
 
-- 
1.7.5.4

Re: [PATCH 1/6] i2c: omap: Prevent NULL pointer dereference in remove

[Shubhrajyoti Datta]

On Thu, Aug 9, 2012 at 7:07 PM, Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org> wrote:
> Prevent the NULL pointer access by moving the platform_set_drvdata function
> after the access of the pdev.
>
> [  654.961761] Unable to handle kernel NULL pointer dereference at virtual address 00000070
> [  654.970611] pgd = df254000
> [  654.973480] [00000070] *pgd=9f1da831, *pte=00000000, *ppte=00000000
> [  654.980163] Internal error: Oops: 17 [#1] SMP ARM
> [  654.985076] Modules linked in:
> [  654.988281] CPU: 1    Not tainted  (3.6.0-rc1-00031-ge547de1-dirty #339)
> [  654.995330] PC is at omap_i2c_runtime_resume+0x8/0x148
> [  655.000732] LR is at omap_i2c_runtime_resume+0x8/0x148
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
> ---
applies on top of Felipe's cleanup series.

Re: [PATCH 1/6] i2c: omap: Prevent NULL pointer dereference in remove

[Wolfram Sang]

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

On Thu, Aug 09, 2012 at 07:07:42PM +0530, Shubhrajyoti D wrote:
> Prevent the NULL pointer access by moving the platform_set_drvdata function
> after the access of the pdev.
> 
> [  654.961761] Unable to handle kernel NULL pointer dereference at virtual address 00000070
> [  654.970611] pgd = df254000
> [  654.973480] [00000070] *pgd=9f1da831, *pte=00000000, *ppte=00000000
> [  654.980163] Internal error: Oops: 17 [#1] SMP ARM
> [  654.985076] Modules linked in:
> [  654.988281] CPU: 1    Not tainted  (3.6.0-rc1-00031-ge547de1-dirty #339)
> [  654.995330] PC is at omap_i2c_runtime_resume+0x8/0x148
> [  655.000732] LR is at omap_i2c_runtime_resume+0x8/0x148
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-omap.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index c8e3886..0c593d4 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1217,8 +1217,6 @@ static int __devexit omap_i2c_remove(struct platform_device *pdev)
>  	struct omap_i2c_dev	*dev = platform_get_drvdata(pdev);
>  	int ret;
>  
> -	platform_set_drvdata(pdev, NULL);
> -
>  	i2c_del_adapter(&dev->adapter);
>  	ret = pm_runtime_get_sync(&pdev->dev);
>  	if (IS_ERR_VALUE(ret))
> @@ -1227,6 +1225,8 @@ static int __devexit omap_i2c_remove(struct platform_device *pdev)
>  	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>  	pm_runtime_put(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	platform_set_drvdata(pdev, NULL);
> +
>  	return 0;
>  }

I think this patch is correct, because drvdata is used in the PM code of
the driver and thus cleared too early.

As such, this is a bugfix and should be not based on the big cleanup
since it should go into this rc series.

I will pick it as soon as the comments on the other patches are
answered.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH 1/6] i2c: omap: Prevent NULL pointer dereference in remove

[Shubhrajyoti Datta]

On Sat, Aug 18, 2012 at 3:39 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Thu, Aug 09, 2012 at 07:07:42PM +0530, Shubhrajyoti D wrote:
>> Prevent the NULL pointer access by moving the platform_set_drvdata function
>> after the access of the pdev.
>>
>> [  654.961761] Unable to handle kernel NULL pointer dereference at virtual address 00000070
>> [  654.970611] pgd = df254000
>> [  654.973480] [00000070] *pgd=9f1da831, *pte=00000000, *ppte=00000000
>> [  654.980163] Internal error: Oops: 17 [#1] SMP ARM
>> [  654.985076] Modules linked in:
>> [  654.988281] CPU: 1    Not tainted  (3.6.0-rc1-00031-ge547de1-dirty #339)
>> [  654.995330] PC is at omap_i2c_runtime_resume+0x8/0x148
>> [  655.000732] LR is at omap_i2c_runtime_resume+0x8/0x148
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
>> ---
>>  drivers/i2c/busses/i2c-omap.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index c8e3886..0c593d4 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -1217,8 +1217,6 @@ static int __devexit omap_i2c_remove(struct platform_device *pdev)
>>       struct omap_i2c_dev     *dev = platform_get_drvdata(pdev);
>>       int ret;
>>
>> -     platform_set_drvdata(pdev, NULL);
>> -
>>       i2c_del_adapter(&dev->adapter);
>>       ret = pm_runtime_get_sync(&pdev->dev);
>>       if (IS_ERR_VALUE(ret))
>> @@ -1227,6 +1225,8 @@ static int __devexit omap_i2c_remove(struct platform_device *pdev)
>>       omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>>       pm_runtime_put(&pdev->dev);
>>       pm_runtime_disable(&pdev->dev);
>> +     platform_set_drvdata(pdev, NULL);
>> +
>>       return 0;
>>  }
>
> I think this patch is correct, because drvdata is used in the PM code of
> the driver and thus cleared too early.
>
> As such, this is a bugfix and should be not based on the big cleanup
> since it should go into this rc series.
>
> I will pick it as soon as the comments on the other patches are
> answered.
BTW it was pointed out that the The
platform_device object will be destroyed.

Do you agree with

http://www.spinics.net/lists/linux-omap/msg75553.html

>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/6] i2c: omap: Prevent NULL pointer dereference in remove

[Wolfram Sang]

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


> BTW it was pointed out that the The
> platform_device object will be destroyed.
> 
> Do you agree with
> 
> http://www.spinics.net/lists/linux-omap/msg75553.html

Yes. I was actually wondering if that would be possible but wanted to
check that somewhen later. Nice that it has already been done :)

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[PATCH 2/6] i2c-xiic: Fix a possible NULL pointer access

[Shubhrajyoti D]

platform_get_resource uses pdev so move the function
platform_set_drvdata(pdev, NULL) after the get_resource.

Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
untested

 drivers/i2c/busses/i2c-xiic.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 641d0e5..a28479a 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -784,8 +784,6 @@ static int __devexit xiic_i2c_remove(struct platform_device* pdev)
 
 	xiic_deinit(i2c);
 
-	platform_set_drvdata(pdev, NULL);
-
 	free_irq(platform_get_irq(pdev, 0), i2c);
 
 	iounmap(i2c->base);
@@ -795,6 +793,7 @@ static int __devexit xiic_i2c_remove(struct platform_device* pdev)
 		release_mem_region(res->start, resource_size(res));
 
 	kfree(i2c);
+	platform_set_drvdata(pdev, NULL);
 
 	return 0;
 }
-- 
1.7.5.4

Re: [PATCH 2/6] i2c-xiic: Fix a possible NULL pointer access

[Wolfram Sang]

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

On Thu, Aug 09, 2012 at 07:07:43PM +0530, Shubhrajyoti D wrote:
> platform_get_resource uses pdev so move the function
> platform_set_drvdata(pdev, NULL) after the get_resource.

This descriptions sounds as you'd assume pdev is set to NULL here?
Careful, platform_set_drvdata() does not clear pdev, but driver_data of
pdev->dev!

So, I guess the rest of this series is bogus?

It would be worth checking if other drivers have the same pattern as the
omap driver where the fix was valid for other reasons. Are you
interested in doing that (perfectly fine to say no).

Regards,

   Wolfram

> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
> ---
> untested
> 
>  drivers/i2c/busses/i2c-xiic.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index 641d0e5..a28479a 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -784,8 +784,6 @@ static int __devexit xiic_i2c_remove(struct platform_device* pdev)
>  
>  	xiic_deinit(i2c);
>  
> -	platform_set_drvdata(pdev, NULL);
> -
>  	free_irq(platform_get_irq(pdev, 0), i2c);
>  
>  	iounmap(i2c->base);
> @@ -795,6 +793,7 @@ static int __devexit xiic_i2c_remove(struct platform_device* pdev)
>  		release_mem_region(res->start, resource_size(res));
>  
>  	kfree(i2c);
> +	platform_set_drvdata(pdev, NULL);
>  
>  	return 0;
>  }
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH 2/6] i2c-xiic: Fix a possible NULL pointer access

[Shubhrajyoti Datta]

On Sat, Aug 18, 2012 at 3:47 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Thu, Aug 09, 2012 at 07:07:43PM +0530, Shubhrajyoti D wrote:
>> platform_get_resource uses pdev so move the function
>> platform_set_drvdata(pdev, NULL) after the get_resource.
>
Yes agree.

> This descriptions sounds as you'd assume pdev is set to NULL here?
> Careful, platform_set_drvdata() does not clear pdev, but driver_data of
> pdev->dev!
>
> So, I guess the rest of this series is bogus?
>
> It would be worth checking if other drivers have the same pattern as the
> omap driver where the fix was valid for other reasons. Are you
> interested in doing that (perfectly fine to say no).

Yes I can take that , lest someone with the platform/ test set-up can
take that up.
>
> Regards,
>
>    Wolfram
>

Re: [PATCH 2/6] i2c-xiic: Fix a possible NULL pointer access

[Shubhrajyoti Datta]

On Sat, Aug 18, 2012 at 6:52 PM, Shubhrajyoti Datta
<omaplinuxkernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sat, Aug 18, 2012 at 3:47 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>> On Thu, Aug 09, 2012 at 07:07:43PM +0530, Shubhrajyoti D wrote:
>>> platform_get_resource uses pdev so move the function
>>> platform_set_drvdata(pdev, NULL) after the get_resource.
>>
> Yes agree.
>
>> This descriptions sounds as you'd assume pdev is set to NULL here?
>> Careful, platform_set_drvdata() does not clear pdev, but driver_data of
>> pdev->dev!
>>
>> So, I guess the rest of this series is bogus?
>>
>> It would be worth checking if other drivers have the same pattern as the
>> omap driver where the fix was valid for other reasons. Are you
>> interested in doing that (perfectly fine to say no).
>
> Yes I can take that , lest someone with the platform/ test set-up can
> take that up.

By the way davinci and designware seem to have the issue
however having a closer look even the probe err handling looks wrong.

>>
>> Regards,
>>
>>    Wolfram
>>

[PATCH 3/6] i2c: designware: Fix a possible NULL pointer access

[Shubhrajyoti D]

platform_get_resource uses pdev so move the function
platform_set_drvdata(pdev, NULL) after the get_resource.

Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
untested

 drivers/i2c/busses/i2c-designware-platdrv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0506fef..4f03eae 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -174,7 +174,6 @@ static int __devexit dw_i2c_remove(struct platform_device *pdev)
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
 	struct resource *mem;
 
-	platform_set_drvdata(pdev, NULL);
 	i2c_del_adapter(&dev->adapter);
 	put_device(&pdev->dev);
 
@@ -188,6 +187,7 @@ static int __devexit dw_i2c_remove(struct platform_device *pdev)
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	release_mem_region(mem->start, resource_size(mem));
+	platform_set_drvdata(pdev, NULL);
 	return 0;
 }
 
-- 
1.7.5.4

[PATCH 4/6] i2c: at91: Fix a possible NULL pointer access

[Shubhrajyoti D]

platform_get_resource uses pdev so move the function
platform_set_drvdata(pdev, NULL) after the get_resource.

Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
untested

 drivers/i2c/busses/i2c-at91.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index e24484b..f748e87 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -263,7 +263,6 @@ static int __devexit at91_i2c_remove(struct platform_device *pdev)
 	int rc;
 
 	rc = i2c_del_adapter(adapter);
-	platform_set_drvdata(pdev, NULL);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	iounmap(twi_base);
@@ -271,6 +270,7 @@ static int __devexit at91_i2c_remove(struct platform_device *pdev)
 
 	clk_disable(twi_clk);		/* disable peripheral clock */
 	clk_put(twi_clk);
+	platform_set_drvdata(pdev, NULL);
 
 	return rc;
 }
-- 
1.7.5.4

[PATCH 5/6] i2c-puv3: Fix a possible NULL pointer access

[Shubhrajyoti D]

platform_get_resource uses pdev so move the function
platform_set_drvdata(pdev, NULL) after the get_resource.

Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
Untested
 drivers/i2c/busses/i2c-puv3.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-puv3.c b/drivers/i2c/busses/i2c-puv3.c
index d8515be..38e67b4 100644
--- a/drivers/i2c/busses/i2c-puv3.c
+++ b/drivers/i2c/busses/i2c-puv3.c
@@ -245,10 +245,10 @@ static int __devexit puv3_i2c_remove(struct platform_device *pdev)
 	}
 
 	put_device(&pdev->dev);
-	platform_set_drvdata(pdev, NULL);
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	release_mem_region(mem->start, resource_size(mem));
+	platform_set_drvdata(pdev, NULL);
 
 	return rc;
 }
-- 
1.7.5.4

[PATCH 6/6] i2c: davinci: Fix a possible NULL pointer access

[Shubhrajyoti D]

platform_get_resource uses pdev so move the function
platform_set_drvdata(pdev, NULL) after the get_resource.

Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
Untested

 drivers/i2c/busses/i2c-davinci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 79b4bcb..9666454 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -746,7 +746,6 @@ static int davinci_i2c_remove(struct platform_device *pdev)
 
 	i2c_davinci_cpufreq_deregister(dev);
 
-	platform_set_drvdata(pdev, NULL);
 	i2c_del_adapter(&dev->adapter);
 	put_device(&pdev->dev);
 
@@ -761,6 +760,7 @@ static int davinci_i2c_remove(struct platform_device *pdev)
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	release_mem_region(mem->start, resource_size(mem));
+	platform_set_drvdata(pdev, NULL);
 	return 0;
 }
 
-- 
1.7.5.4