[PATCH 0/2] Accelerometer driver unload

[PATCH 0/2] Accelerometer driver unload

[Yuri Ershov]

Hello,

I2C part of lis3 accelerometer driver can be unloaded while in use which may
cause kernel oops.

These patches allow to change /dev/freefall file ops owner to prevent driver
unloading while device in use.

Please review.

Best regards,
Yuri

Yuri Kululin (2):
  lis3: Add device owner
  lis3: Set device owner

 drivers/hwmon/lis3lv02d.c     |    8 ++++++--
 drivers/hwmon/lis3lv02d.h     |    2 ++
 drivers/hwmon/lis3lv02d_i2c.c |    2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

[PATCH 1/2] lis3: Add device owner

[Yuri Ershov]

Add device owner and change /dev/freefall file operations owner
according to the used driver

Signed-off-by: Yuri Kululin <ext-yuri.kululin@nokia.com>
---
 drivers/hwmon/lis3lv02d.c |    8 ++++++--
 drivers/hwmon/lis3lv02d.h |    2 ++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index e278f0e..0f1bd36 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -591,8 +591,7 @@ static int lis3lv02d_misc_fasync(int fd, struct file *file, int on)
 	return fasync_helper(fd, file, on, &lis3_dev.async_queue);
 }
 
-static const struct file_operations lis3lv02d_misc_fops = {
-	.owner   = THIS_MODULE,
+static struct file_operations lis3lv02d_misc_fops = {
 	.llseek  = no_llseek,
 	.read    = lis3lv02d_misc_read,
 	.open    = lis3lv02d_misc_open,
@@ -1007,6 +1006,11 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
 		goto out;
 	}
 
+	if (dev->owner)
+		lis3lv02d_misc_fops.owner = dev->owner;
+	else
+		lis3lv02d_misc_fops.owner = THIS_MODULE;
+
 	if (misc_register(&lis3lv02d_misc_device))
 		printk(KERN_ERR DRIVER_NAME ": misc_register failed\n");
 out:
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index 1f0a66f..e63dd3d 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -258,6 +258,8 @@ struct lis3lv02d {
 	unsigned char		irq_cfg;
 	struct lis3lv02d_platform_data *pdata;	/* for passing board config */
 	struct mutex		mutex;     /* Serialize poll and selftest */
+
+	struct module		*owner;    /* Device owner */
 };
 
 int lis3lv02d_init_device(struct lis3lv02d *lis3);
-- 
1.7.1.1

[PATCH 2/2] lis3: Set device owner

[Yuri Ershov]

Set device owner for i2c part.

Signed-off-by: Yuri Kululin <ext-yuri.kululin@nokia.com>
---
 drivers/hwmon/lis3lv02d_i2c.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
index b2f8bae..1a55bb3 100644
--- a/drivers/hwmon/lis3lv02d_i2c.c
+++ b/drivers/hwmon/lis3lv02d_i2c.c
@@ -129,6 +129,7 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 	lis3_dev.reg_ctrl = lis3_reg_ctrl;
 	lis3_dev.irq	  = client->irq;
 	lis3_dev.ac	  = lis3lv02d_axis_map;
+	lis3_dev.owner	  = THIS_MODULE;
 
 	i2c_set_clientdata(client, &lis3_dev);
 
@@ -149,6 +150,7 @@ static int __devexit lis3lv02d_i2c_remove(struct i2c_client *client)
 
 	lis3lv02d_joystick_disable();
 	lis3lv02d_remove_fs(&lis3_dev);
+	lis3_dev.owner = NULL;
 
 	regulator_bulk_free(ARRAY_SIZE(lis3_dev.regulators),
 			lis3_dev.regulators);
-- 
1.7.1.1

Re: [1/2] lis3: Add device owner

[Guenter Roeck]

On Fri, Aug 27, 2010 at 02:33:20PM -0000, Yuri Ershov wrote:
> Add device owner and change /dev/freefall file operations owner
> according to the used driver
> 
> Signed-off-by: Yuri Kululin <ext-yuri.kululin@nokia.com>
> 
> ---
> drivers/hwmon/lis3lv02d.c |    8 ++++++--
>  drivers/hwmon/lis3lv02d.h |    2 ++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
> index e278f0e..0f1bd36 100644
> --- a/drivers/hwmon/lis3lv02d.c
> +++ b/drivers/hwmon/lis3lv02d.c
> @@ -591,8 +591,7 @@ static int lis3lv02d_misc_fasync(int fd, struct file *file, int on)
>  	return fasync_helper(fd, file, on, &lis3_dev.async_queue);
>  }
>  
> -static const struct file_operations lis3lv02d_misc_fops = {
> -	.owner   = THIS_MODULE,
> +static struct file_operations lis3lv02d_misc_fops = {
>  	.llseek  = no_llseek,
>  	.read    = lis3lv02d_misc_read,
>  	.open    = lis3lv02d_misc_open,
> @@ -1007,6 +1006,11 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
>  		goto out;
>  	}
>  
> +	if (dev->owner)
> +		lis3lv02d_misc_fops.owner = dev->owner;
> +	else
> +		lis3lv02d_misc_fops.owner = THIS_MODULE;
> +
If you retained the static assignment to .owner above, you would not need
the else part here.

Also, this depends on patch#2, which actually sets dev->owner.
So the problem isn't really addressed w/o patch #2. Given that,
I am not sure if it makes sense to have two separate patches.

If both patches were applied as one, you might not need the if ()
above in the first place. And if you do, you might still not have solved
the problem completely since .owner would not be set correctly
if dev->owner is NULL. 

On a higher level, reassigning the owner like this seems to be quite uncommon,
at least in hwmon. I would like to see an Acked-by from Eric to ensure
that the fix is correct.

Thanks,
Guenter

Re: [2/2] lis3: Set device owner

[Guenter Roeck]

On Fri, Aug 27, 2010 at 02:33:21PM -0000, Yuri Ershov wrote:
> Set device owner for i2c part.
> 
> Signed-off-by: Yuri Kululin <ext-yuri.kululin@nokia.com>
> 
> ---
> drivers/hwmon/lis3lv02d_i2c.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
> index b2f8bae..1a55bb3 100644
> --- a/drivers/hwmon/lis3lv02d_i2c.c
> +++ b/drivers/hwmon/lis3lv02d_i2c.c
> @@ -129,6 +129,7 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
>  	lis3_dev.reg_ctrl = lis3_reg_ctrl;
>  	lis3_dev.irq	  = client->irq;
>  	lis3_dev.ac	  = lis3lv02d_axis_map;
> +	lis3_dev.owner	  = THIS_MODULE;
>  
>  	i2c_set_clientdata(client, &lis3_dev);
>  
> @@ -149,6 +150,7 @@ static int __devexit lis3lv02d_i2c_remove(struct i2c_client *client)
>  
>  	lis3lv02d_joystick_disable();
>  	lis3lv02d_remove_fs(&lis3_dev);
> +	lis3_dev.owner = NULL;
>  
Should not be necessary, since you are about to unload the driver.
Unless I am missing something, of course.

Guenter

Re: [1/2] lis3: Add device owner

[Yuri Kululin]

ext Guenter Roeck wrote:
> On Fri, Aug 27, 2010 at 02:33:20PM -0000, Yuri Ershov wrote:
>> Add device owner and change /dev/freefall file operations owner
>> according to the used driver
>>
>> Signed-off-by: Yuri Kululin <ext-yuri.kululin@nokia.com>
>>
>> ---
>> drivers/hwmon/lis3lv02d.c |    8 ++++++--
>>  drivers/hwmon/lis3lv02d.h |    2 ++
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
>> index e278f0e..0f1bd36 100644
>> --- a/drivers/hwmon/lis3lv02d.c
>> +++ b/drivers/hwmon/lis3lv02d.c
>> @@ -591,8 +591,7 @@ static int lis3lv02d_misc_fasync(int fd, struct file *file, int on)
>>  	return fasync_helper(fd, file, on, &lis3_dev.async_queue);
>>  }
>>  
>> -static const struct file_operations lis3lv02d_misc_fops = {
>> -	.owner   = THIS_MODULE,
>> +static struct file_operations lis3lv02d_misc_fops = {
>>  	.llseek  = no_llseek,
>>  	.read    = lis3lv02d_misc_read,
>>  	.open    = lis3lv02d_misc_open,
>> @@ -1007,6 +1006,11 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
>>  		goto out;
>>  	}
>>  
>> +	if (dev->owner)
>> +		lis3lv02d_misc_fops.owner = dev->owner;
>> +	else
>> +		lis3lv02d_misc_fops.owner = THIS_MODULE;
>> +
> If you retained the static assignment to .owner above, you would not need
> the else part here.

Yes, you are right. I can change this.

> Also, this depends on patch#2, which actually sets dev->owner.
> So the problem isn't really addressed w/o patch #2. Given that,
> I am not sure if it makes sense to have two separate patches.

Ok, I can merge my patches.

> If both patches were applied as one, you might not need the if ()
> above in the first place. And if you do, you might still not have solved
> the problem completely since .owner would not be set correctly
> if dev->owner is NULL. 

I've added "if" here to keep the situation "as is" for all other drivers
(HP driver - hp_accel.c and spi part - lis3lv02d_spi.c. I don't have the HW to 
test) which are using the lis3 core. If we want to avoid the issues with 
unloading completely all parts should set the dev->owner. Not a problem to add 
this to HP driver and lis3lv02d_spi.c.

> On a higher level, reassigning the owner like this seems to be quite uncommon,
> at least in hwmon. I would like to see an Acked-by from Eric to ensure
> that the fix is correct.

I tried to use the standard way of module usage control because all functions 
provided by lis3lv02d_i2c.c, lis3lv02d_spi.c or hp_accel.c can be called through 
  lis3lv02d_misc_fops file ops or joystick poll device.

I can propose another solution. We can use try_module_get(owner) during device 
registration (in lis3lv02d_add_fs()), release module by using module_put(owner) 
  after device unregister (in lis3lv02d_remove_fs()) and do not touch 
lis3lv02d_misc_fops. But anyway dev->owner should be set.

Thanks a lot for reviewing,
Yuri K
> 
> Thanks,
> Guenter

Re: [1/2] lis3: Add device owner

[Guenter Roeck]

On Tue, Sep 28, 2010 at 04:52:19AM -0400, Yuri Kululin wrote:
[ ... ]
> 
> > On a higher level, reassigning the owner like this seems to be quite uncommon,
> > at least in hwmon. I would like to see an Acked-by from Eric to ensure
> > that the fix is correct.
> 
> I tried to use the standard way of module usage control because all functions 
> provided by lis3lv02d_i2c.c, lis3lv02d_spi.c or hp_accel.c can be called through 
>   lis3lv02d_misc_fops file ops or joystick poll device.
> 
> I can propose another solution. We can use try_module_get(owner) during device 
> registration (in lis3lv02d_add_fs()), release module by using module_put(owner) 
>   after device unregister (in lis3lv02d_remove_fs()) and do not touch 
> lis3lv02d_misc_fops. But anyway dev->owner should be set.
> 
Key is really to get feedback from others, since I am neither familiar
with module ownership handling nor with this driver.

Thanks,
Guenter