[PATCH] staging:iio:ad7291: add missing .driver_module to struct iio_info

[PATCH] staging:iio:ad7291: add missing .driver_module to struct iio_info

[Wei Yongjun]

From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>

Add missing .driver_module of struct iio_info. This prevents the
module from being removed from underneath its users.

Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
 drivers/staging/iio/adc/ad7291.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/iio/adc/ad7291.c b/drivers/staging/iio/adc/ad7291.c
index 3fc79e5..a2e61c2 100644
--- a/drivers/staging/iio/adc/ad7291.c
+++ b/drivers/staging/iio/adc/ad7291.c
@@ -517,6 +517,7 @@ static const struct iio_info ad7291_info = {
 	.read_event_value = &ad7291_read_event_value,
 	.write_event_value = &ad7291_write_event_value,
 	.event_attrs = &ad7291_event_attribute_group,
+	.driver_module = THIS_MODULE,
 };
 
 static int ad7291_probe(struct i2c_client *client,

Re: [PATCH] staging:iio:ad7291: add missing .driver_module to struct iio_info

[Greg KH]

On Thu, Jul 04, 2013 at 09:46:34PM +0800, Wei Yongjun wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> 
> Add missing .driver_module of struct iio_info. This prevents the
> module from being removed from underneath its users.
> 
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> ---
>  drivers/staging/iio/adc/ad7291.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/iio/adc/ad7291.c b/drivers/staging/iio/adc/ad7291.c
> index 3fc79e5..a2e61c2 100644
> --- a/drivers/staging/iio/adc/ad7291.c
> +++ b/drivers/staging/iio/adc/ad7291.c
> @@ -517,6 +517,7 @@ static const struct iio_info ad7291_info = {
>  	.read_event_value = &ad7291_read_event_value,
>  	.write_event_value = &ad7291_write_event_value,
>  	.event_attrs = &ad7291_event_attribute_group,
> +	.driver_module = THIS_MODULE,
>  };

That's a hint that the register function should be changed so that no
module ever has to set this value.  Look at how PCI and USB do their
driver registration function for how to do this properly.

thanks,

greg k-h

Re: [PATCH] staging:iio:ad7291: add missing .driver_module to struct iio_info

[Lars-Peter Clausen]

On 07/04/2013 06:01 PM, Greg KH wrote:
> On Thu, Jul 04, 2013 at 09:46:34PM +0800, Wei Yongjun wrote:
>> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>>
>> Add missing .driver_module of struct iio_info. This prevents the
>> module from being removed from underneath its users.
>>
>> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>> ---
>>  drivers/staging/iio/adc/ad7291.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/staging/iio/adc/ad7291.c b/drivers/staging/iio/adc/ad7291.c
>> index 3fc79e5..a2e61c2 100644
>> --- a/drivers/staging/iio/adc/ad7291.c
>> +++ b/drivers/staging/iio/adc/ad7291.c
>> @@ -517,6 +517,7 @@ static const struct iio_info ad7291_info = {
>>  	.read_event_value = &ad7291_read_event_value,
>>  	.write_event_value = &ad7291_write_event_value,
>>  	.event_attrs = &ad7291_event_attribute_group,
>> +	.driver_module = THIS_MODULE,
>>  };
> 
> That's a hint that the register function should be changed so that no
> module ever has to set this value.  Look at how PCI and USB do their
> driver registration function for how to do this properly.

I don't think that the same approach as used for PCI, USB and others works
as nicely here. For one we do not have a registration function for the info
struct, it would probably be possible to rework things a bit, but not
without some code churn. And secondly we'd also have to make all the
iio_info structs non-const, which is something I don't like. And thirdly we
do have the proper tools to auto-detect such missing initializations (e.g.
using coccinelle), so we can catch these errors early on and they won't show
up in new drivers.

- Lars

Re: [PATCH] staging:iio:ad7291: add missing .driver_module to struct iio_info

[Wei Yongjun]

On 07/05/2013 05:55 PM, Lars-Peter Clausen wrote:
> On 07/04/2013 06:01 PM, Greg KH wrote:
>> On Thu, Jul 04, 2013 at 09:46:34PM +0800, Wei Yongjun wrote:
>> That's a hint that the register function should be changed so that no module ever has to set this value. Look at how PCI and USB do their driver registration function for how to do this properly. 
> I don't think that the same approach as used for PCI, USB and others works
> as nicely here. For one we do not have a registration function for the info
> struct, it would probably be possible to rework things a bit, but not
> without some code churn. And secondly we'd also have to make all the
> iio_info structs non-const, which is something I don't like. And thirdly we
> do have the proper tools to auto-detect such missing initializations (e.g.
> using coccinelle), so we can catch these errors early on and they won't show
> up in new drivers.

Agree, with the follow coccinelle script, we can found this easily.

@r__iio_info@
identifier nm;
position p;
@@
struct iio_info nm@p = {
...,
  .driver_module = THIS_MODULE,
...
};

@@
identifier nm;
position p != {r__iio_info.p};
@@
struct iio_info nm@p = {
...,
+ .driver_module = THIS_MODULE,
};

Re: [PATCH] staging:iio:ad7291: add missing .driver_module to struct iio_info

[Jonathan Cameron]

On 07/04/2013 02:46 PM, Wei Yongjun wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> 
> Add missing .driver_module of struct iio_info. This prevents the
> module from being removed from underneath its users.
> 
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Applied to the fixes-togreg branch of iio.git.

Obviously we want this fix whatever the outcome of the discussion started
by Greg as any changes will be in the next cycle, not this one.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/adc/ad7291.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/iio/adc/ad7291.c b/drivers/staging/iio/adc/ad7291.c
> index 3fc79e5..a2e61c2 100644
> --- a/drivers/staging/iio/adc/ad7291.c
> +++ b/drivers/staging/iio/adc/ad7291.c
> @@ -517,6 +517,7 @@ static const struct iio_info ad7291_info = {
>  	.read_event_value = &ad7291_read_event_value,
>  	.write_event_value = &ad7291_write_event_value,
>  	.event_attrs = &ad7291_event_attribute_group,
> +	.driver_module = THIS_MODULE,
>  };
>  
>  static int ad7291_probe(struct i2c_client *client,
>