[PATCH] regulator: palmas: fix pdata ptr not be updated after it has been allocated

[PATCH] regulator: palmas: fix pdata ptr not be updated after it has been allocated

[Rhyland Klein]

The pmic platform data wasn't updated to dev.platform_data after it had
been allocated. That can cause the driver crash when using it.
This patch fixes the issue.

We need to update the pdata pointer at the end of probe because if
probe should fail for some reason, for instance if an in-supply
isn't ready yet, then probe will defer. However, the pdata is allocated
with devm, and so is freed when probe is deferred.

Based on work by Joseph Lo.

Cc: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Rhyland Klein <rklein-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/regulator/palmas-regulator.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c
index d0c8785..e11b762 100644
--- a/drivers/regulator/palmas-regulator.c
+++ b/drivers/regulator/palmas-regulator.c
@@ -1027,6 +1027,8 @@ static int palmas_regulators_probe(struct platform_device *pdev)
 		}
 	}
 
+	/* Store the pdata pointer after everything else passes */
+	pdev->dev.platform_data = pdata;
 
 	return 0;
 
-- 
1.7.9.5

[PATCH] regulator: palmas: fix pdata ptr not be updated after it has been allocated

[Rhyland Klein]

The pmic platform data wasn't updated to dev.platform_data after it had
been allocated. That can cause the driver crash when using it.
This patch fixes the issue.

We need to update the pdata pointer at the end of probe because if
probe should fail for some reason, for instance if an in-supply
isn't ready yet, then probe will defer. However, the pdata is allocated
with devm, and so is freed when probe is deferred.

Based on work by Joseph Lo.

Cc: Joseph Lo <josephl@nvidia.com>
Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
 drivers/regulator/palmas-regulator.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c
index d0c8785..e11b762 100644
--- a/drivers/regulator/palmas-regulator.c
+++ b/drivers/regulator/palmas-regulator.c
@@ -1027,6 +1027,8 @@ static int palmas_regulators_probe(struct platform_device *pdev)
 		}
 	}
 
+	/* Store the pdata pointer after everything else passes */
+	pdev->dev.platform_data = pdata;
 
 	return 0;
 
-- 
1.7.9.5

Re: [PATCH] regulator: palmas: fix pdata ptr not be updated after it has been allocated

[Mark Brown]

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

On Wed, Jul 24, 2013 at 12:28:02PM -0400, Rhyland Klein wrote:

> +	/* Store the pdata pointer after everything else passes */
> +	pdev->dev.platform_data = pdata;

No, this is bad - the driver should not be modifying the platform_data
pointer, it's owned by the thing that created the device.  If the
platform data needs to be modified it should be copied somewhere in the
driver data.

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

Re: [PATCH] regulator: palmas: fix pdata ptr not be updated after it has been allocated

[Mark Brown]

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

On Wed, Jul 24, 2013 at 12:28:02PM -0400, Rhyland Klein wrote:

> +	/* Store the pdata pointer after everything else passes */
> +	pdev->dev.platform_data = pdata;

No, this is bad - the driver should not be modifying the platform_data
pointer, it's owned by the thing that created the device.  If the
platform data needs to be modified it should be copied somewhere in the
driver data.

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

Re: [PATCH] regulator: palmas: fix pdata ptr not be updated after it has been allocated

[Rhyland Klein]

On 7/24/2013 1:02 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Jul 24, 2013 at 12:28:02PM -0400, Rhyland Klein wrote:
> 
>> +	/* Store the pdata pointer after everything else passes */
>> +	pdev->dev.platform_data = pdata;
> 
> No, this is bad - the driver should not be modifying the platform_data
> pointer, it's owned by the thing that created the device.  If the
> platform data needs to be modified it should be copied somewhere in the
> driver data.

Alright, thanks.

-rhyland

> 
> * Unknown Key
> * 0x7EA229BD
> 


-- 
nvpublic

Re: [PATCH] regulator: palmas: fix pdata ptr not be updated after it has been allocated

[Rhyland Klein]

On 7/24/2013 1:02 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Jul 24, 2013 at 12:28:02PM -0400, Rhyland Klein wrote:
> 
>> +	/* Store the pdata pointer after everything else passes */
>> +	pdev->dev.platform_data = pdata;
> 
> No, this is bad - the driver should not be modifying the platform_data
> pointer, it's owned by the thing that created the device.  If the
> platform data needs to be modified it should be copied somewhere in the
> driver data.

Alright, thanks.

-rhyland

> 
> * Unknown Key
> * 0x7EA229BD
> 


-- 
nvpublic

Re: [PATCH] regulator: palmas: fix pdata ptr not be updated after it has been allocated

[Stephen Warren]

On 07/24/2013 09:28 AM, Rhyland Klein wrote:
> The pmic platform data wasn't updated to dev.platform_data after it had
> been allocated. That can cause the driver crash when using it.
> This patch fixes the issue.
> 
> We need to update the pdata pointer at the end of probe because if
> probe should fail for some reason, for instance if an in-supply
> isn't ready yet, then probe will defer. However, the pdata is allocated
> with devm, and so is freed when probe is deferred.

This looks wrong. If the module is removed and re-probed, then the pdata
pointer will still be set to a stale (free'd) value and hence not
re-allocated, but it will still be used.

This function shouldn't save the pdata in dev.platform_data, but rather
some data structure that has the same scope as probe()..remove(), for
example as a field in the drvdata struct, as set by dev_set_drvdata().