[PATCH] si4713: Fix oops when si4713_platform_data is marked as __initdata

[PATCH] si4713: Fix oops when si4713_platform_data is marked as __initdata

[Jarkko Nikula]

This driver can cause an oops if si4713_platform_data holding pointer to
set_power function is marked as __initdata and when trying to power up the
chip after booting e.g. with 'v4l2-ctl -d /dev/radio0 --set-ctrl=mute=0'.

This happens because the sdev->platform_data doesn't point to valid data
anymore after kernel is initialized.

Fix this by taking local copy of si4713_platform_data->set_power. Add also
NULL check for this function pointer.

Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 drivers/media/radio/si4713-i2c.c |   15 +++++++++------
 drivers/media/radio/si4713-i2c.h |    2 +-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/media/radio/si4713-i2c.c b/drivers/media/radio/si4713-i2c.c
index ab63dd5..cf9858d 100644
--- a/drivers/media/radio/si4713-i2c.c
+++ b/drivers/media/radio/si4713-i2c.c
@@ -369,7 +369,8 @@ static int si4713_powerup(struct si4713_device *sdev)
 	if (sdev->power_state)
 		return 0;
 
-	sdev->platform_data->set_power(1);
+	if (sdev->set_power)
+		sdev->set_power(1);
 	err = si4713_send_command(sdev, SI4713_CMD_POWER_UP,
 					args, ARRAY_SIZE(args),
 					resp, ARRAY_SIZE(resp),
@@ -383,8 +384,8 @@ static int si4713_powerup(struct si4713_device *sdev)
 
 		err = si4713_write_property(sdev, SI4713_GPO_IEN,
 						SI4713_STC_INT | SI4713_CTS);
-	} else {
-		sdev->platform_data->set_power(0);
+	} else if (sdev->set_power) {
+		sdev->set_power(0);
 	}
 
 	return err;
@@ -411,7 +412,8 @@ static int si4713_powerdown(struct si4713_device *sdev)
 		v4l2_dbg(1, debug, &sdev->sd, "Power down response: 0x%02x\n",
 				resp[0]);
 		v4l2_dbg(1, debug, &sdev->sd, "Device in reset mode\n");
-		sdev->platform_data->set_power(0);
+		if (sdev->set_power)
+			sdev->set_power(0);
 		sdev->power_state = POWER_OFF;
 	}
 
@@ -1959,6 +1961,7 @@ static int si4713_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
 {
 	struct si4713_device *sdev;
+	struct si4713_platform_data *pdata = client->dev.platform_data;
 	int rval;
 
 	sdev = kzalloc(sizeof *sdev, GFP_KERNEL);
@@ -1968,12 +1971,12 @@ static int si4713_probe(struct i2c_client *client,
 		goto exit;
 	}
 
-	sdev->platform_data = client->dev.platform_data;
-	if (!sdev->platform_data) {
+	if (!pdata) {
 		v4l2_err(&sdev->sd, "No platform data registered.\n");
 		rval = -ENODEV;
 		goto free_sdev;
 	}
+	sdev->set_power = pdata->set_power;
 
 	v4l2_i2c_subdev_init(&sdev->sd, client, &si4713_subdev_ops);
 
diff --git a/drivers/media/radio/si4713-i2c.h b/drivers/media/radio/si4713-i2c.h
index faf8cff..d1af889 100644
--- a/drivers/media/radio/si4713-i2c.h
+++ b/drivers/media/radio/si4713-i2c.h
@@ -220,7 +220,7 @@ struct si4713_device {
 	/* private data structures */
 	struct mutex mutex;
 	struct completion work;
-	struct si4713_platform_data *platform_data;
+	int (*set_power)(int power);
 	struct rds_info rds_info;
 	struct limiter_info limiter_info;
 	struct pilot_info pilot_info;
-- 
1.7.1

Re: [PATCH] si4713: Fix oops when si4713_platform_data is marked as __initdata

[Eduardo Valentin]

Hello,

On Sun, May 16, 2010 at 07:04:26PM +0200, Jarkko Nikula wrote:
> This driver can cause an oops if si4713_platform_data holding pointer to
> set_power function is marked as __initdata and when trying to power up the
> chip after booting e.g. with 'v4l2-ctl -d /dev/radio0 --set-ctrl=mute=0'.
> 
> This happens because the sdev->platform_data doesn't point to valid data
> anymore after kernel is initialized.
> 
> Fix this by taking local copy of si4713_platform_data->set_power. Add also
> NULL check for this function pointer.

I'm probably fine with this patch, and the driver must check for the pointer
before using it, indeed.

But, I'm a bit skeptic about marking its platform data as __initdata. Would it make sense?
What happens if driver is built as module and loaded / unload / loaded again?

Maybe the initdata flag does not apply in this case. Not sure (and not tested the above case).

BR,

> 
> Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> Cc: Eduardo Valentin <eduardo.valentin@nokia.com>
> ---
>  drivers/media/radio/si4713-i2c.c |   15 +++++++++------
>  drivers/media/radio/si4713-i2c.h |    2 +-
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/radio/si4713-i2c.c b/drivers/media/radio/si4713-i2c.c
> index ab63dd5..cf9858d 100644
> --- a/drivers/media/radio/si4713-i2c.c
> +++ b/drivers/media/radio/si4713-i2c.c
> @@ -369,7 +369,8 @@ static int si4713_powerup(struct si4713_device *sdev)
>  	if (sdev->power_state)
>  		return 0;
>  
> -	sdev->platform_data->set_power(1);
> +	if (sdev->set_power)
> +		sdev->set_power(1);
>  	err = si4713_send_command(sdev, SI4713_CMD_POWER_UP,
>  					args, ARRAY_SIZE(args),
>  					resp, ARRAY_SIZE(resp),
> @@ -383,8 +384,8 @@ static int si4713_powerup(struct si4713_device *sdev)
>  
>  		err = si4713_write_property(sdev, SI4713_GPO_IEN,
>  						SI4713_STC_INT | SI4713_CTS);
> -	} else {
> -		sdev->platform_data->set_power(0);
> +	} else if (sdev->set_power) {
> +		sdev->set_power(0);
>  	}
>  
>  	return err;
> @@ -411,7 +412,8 @@ static int si4713_powerdown(struct si4713_device *sdev)
>  		v4l2_dbg(1, debug, &sdev->sd, "Power down response: 0x%02x\n",
>  				resp[0]);
>  		v4l2_dbg(1, debug, &sdev->sd, "Device in reset mode\n");
> -		sdev->platform_data->set_power(0);
> +		if (sdev->set_power)
> +			sdev->set_power(0);
>  		sdev->power_state = POWER_OFF;
>  	}
>  
> @@ -1959,6 +1961,7 @@ static int si4713_probe(struct i2c_client *client,
>  					const struct i2c_device_id *id)
>  {
>  	struct si4713_device *sdev;
> +	struct si4713_platform_data *pdata = client->dev.platform_data;
>  	int rval;
>  
>  	sdev = kzalloc(sizeof *sdev, GFP_KERNEL);
> @@ -1968,12 +1971,12 @@ static int si4713_probe(struct i2c_client *client,
>  		goto exit;
>  	}
>  
> -	sdev->platform_data = client->dev.platform_data;
> -	if (!sdev->platform_data) {
> +	if (!pdata) {
>  		v4l2_err(&sdev->sd, "No platform data registered.\n");
>  		rval = -ENODEV;
>  		goto free_sdev;
>  	}
> +	sdev->set_power = pdata->set_power;
>  
>  	v4l2_i2c_subdev_init(&sdev->sd, client, &si4713_subdev_ops);
>  
> diff --git a/drivers/media/radio/si4713-i2c.h b/drivers/media/radio/si4713-i2c.h
> index faf8cff..d1af889 100644
> --- a/drivers/media/radio/si4713-i2c.h
> +++ b/drivers/media/radio/si4713-i2c.h
> @@ -220,7 +220,7 @@ struct si4713_device {
>  	/* private data structures */
>  	struct mutex mutex;
>  	struct completion work;
> -	struct si4713_platform_data *platform_data;
> +	int (*set_power)(int power);
>  	struct rds_info rds_info;
>  	struct limiter_info limiter_info;
>  	struct pilot_info pilot_info;
> -- 
> 1.7.1

Re: [PATCH] si4713: Fix oops when si4713_platform_data is marked as __initdata

[Jarkko Nikula]

On Tue, 18 May 2010 15:55:27 +0300
Eduardo Valentin <eduardo.valentin@nokia.com> wrote:

> I'm probably fine with this patch, and the driver must check for the pointer
> before using it, indeed.
> 
> But, I'm a bit skeptic about marking its platform data as __initdata. Would it make sense?
> What happens if driver is built as module and loaded / unload / loaded again?
> 
> Maybe the initdata flag does not apply in this case. Not sure (and not tested the above case).
> 
Yep, it doesn't work or make sense for modules if platform data is
marked as __initdata but with built in case it can save some bytes which
are not needed after kernel is initialized.

Like with this driver the i2c_bus number and i2_board_info data are not
needed after probing but only pointer to set_power must be preserved.


-- 
Jarkko

Re: [PATCH] si4713: Fix oops when si4713_platform_data is marked as __initdata

[Mauro Carvalho Chehab]

Em 18-05-2010 10:24, Jarkko Nikula escreveu:
> On Tue, 18 May 2010 15:55:27 +0300
> Eduardo Valentin <eduardo.valentin@nokia.com> wrote:
> 
>> I'm probably fine with this patch, and the driver must check for the pointer
>> before using it, indeed.
>>
>> But, I'm a bit skeptic about marking its platform data as __initdata. Would it make sense?
>> What happens if driver is built as module and loaded / unload / loaded again?
>>
>> Maybe the initdata flag does not apply in this case. Not sure (and not tested the above case).
>>
> Yep, it doesn't work or make sense for modules if platform data is
> marked as __initdata but with built in case it can save some bytes which
> are not needed after kernel is initialized.
> 
> Like with this driver the i2c_bus number and i2_board_info data are not
> needed after probing but only pointer to set_power must be preserved.
> 

Hi Eduardo,

This patch is still on my queue. It is not clear to me what "proably fine" means...
Please ack or nack on it for me to move ahead ;)

Cheers,
Mauro

Re: [PATCH] si4713: Fix oops when si4713_platform_data is marked as __initdata

[Jarkko Nikula]

On Mon, 05 Jul 2010 13:09:22 -0300
Mauro Carvalho Chehab <mchehab@redhat.com> wrote:

> Hi Eduardo,
> 
> This patch is still on my queue. It is not clear to me what "proably fine" means...
> Please ack or nack on it for me to move ahead ;)
> 
Ah, sorry, I should have nacked this myself after I sent the regulator
framework conversion patch [1] which removes the set_power callback and
thus null check need for it.


-- 
Jarkko
1. http://www.spinics.net/lists/linux-media/msg20200.html

Re: [PATCH] si4713: Fix oops when si4713_platform_data is marked as __initdata

[Mauro Carvalho Chehab]

Em 05-07-2010 13:48, Jarkko Nikula escreveu:
> On Mon, 05 Jul 2010 13:09:22 -0300
> Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> 
>> Hi Eduardo,
>>
>> This patch is still on my queue. It is not clear to me what "proably fine" means...
>> Please ack or nack on it for me to move ahead ;)
>>
> Ah, sorry, I should have nacked this myself

Thanks for the nack !

> after I sent the regulator
> framework conversion patch [1] which removes the set_power callback and
> thus null check need for it.

Ok, it is on my pending list. I'll analyze it later likely today.

Cheers,
Mauro.