[PATCH 1/2] staging: iio_simple_dummy: fix init

[PATCH 1/2] staging: iio_simple_dummy: fix init

[Vladimirs Ambrosovs]

This patch fixes the init function for the iio_simple_dummy driver.
The main issues were absence of kfree for the allocated array, and no
devices being removed in case the probe function fails, running in a loop.

The iio_dummy_remove function was also changed:
	* The return value was changed to void
	* The check for return value of iio_simple_dummy_events_unregister()
The reason for this changes is that, as per implementation,
events_unregister function always returns 0, so we are safe not to check
return value. As a result the return value for iio_dummy_remove function
becomes useless as well, hence return value type change.

Signed-off-by: Vladimirs Ambrosovs <rodriguez.twister@gmail.com>
---
 drivers/staging/iio/iio_simple_dummy.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
index b47bf9f..88fbb4f 100644
--- a/drivers/staging/iio/iio_simple_dummy.c
+++ b/drivers/staging/iio/iio_simple_dummy.c
@@ -665,9 +665,8 @@ error_ret:
  *
  * Parameters follow those of iio_dummy_probe for buses.
  */
-static int iio_dummy_remove(int index)
+static void iio_dummy_remove(int index)
 {
-	int ret;
 	/*
 	 * Get a pointer to the device instance iio_dev structure
 	 * from the bus subsystem. E.g.
@@ -685,15 +684,14 @@ static int iio_dummy_remove(int index)
 	/* Buffered capture related cleanup */
 	iio_simple_dummy_unconfigure_buffer(indio_dev);
 
-	ret = iio_simple_dummy_events_unregister(indio_dev);
-	if (ret)
-		goto error_ret;
+	/*
+	 * Tidy up interrupt handling
+	 * Always returns 0, so not checking for return value
+	 */
+	iio_simple_dummy_events_unregister(indio_dev);
 
 	/* Free all structures */
 	iio_device_free(indio_dev);
-
-error_ret:
-	return ret;
 }
 
 /**
@@ -722,9 +720,17 @@ static __init int iio_dummy_init(void)
 	for (i = 0; i < instances; i++) {
 		ret = iio_dummy_probe(i);
 		if (ret < 0)
-			return ret;
+			goto error_probe;
 	}
 	return 0;
+
+error_probe:
+	/* Free devices registered before error */
+	while (i--)
+		iio_dummy_remove(i);
+
+	kfree(iio_dummy_devs);
+	return ret;
 }
 module_init(iio_dummy_init);
 
-- 
2.4.1

[PATCH 2/2] staging: iio_simple_dummy: zero check param

[Vladimirs Ambrosovs]

Check for zero was added to the module parameter "instances" to
avoid the allocation of array of zero values. Although it is a valid call,
we don't want to allocate ZERO_SIZE_PTR, so need to disallow this case.
The type of variables which are compared to "instances" were also changed
to unsigned int so that no compiler complaints occur.

Signed-off-by: Vladimirs Ambrosovs <rodriguez.twister@gmail.com>
---
 drivers/staging/iio/iio_simple_dummy.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
index 88fbb4f..2744a1b 100644
--- a/drivers/staging/iio/iio_simple_dummy.c
+++ b/drivers/staging/iio/iio_simple_dummy.c
@@ -30,7 +30,7 @@
  * dummy devices are registered.
  */
 static unsigned instances = 1;
-module_param(instances, int, 0);
+module_param(instances, uint, 0);
 
 /* Pointer array used to fake bus elements */
 static struct iio_dev **iio_dummy_devs;
@@ -706,9 +706,10 @@ static void iio_dummy_remove(int index)
  */
 static __init int iio_dummy_init(void)
 {
-	int i, ret;
+	unsigned int i;
+	int ret;
 
-	if (instances > 10) {
+	if (instances == 0 || instances > 10) {
 		instances = 1;
 		return -EINVAL;
 	}
@@ -742,7 +743,7 @@ module_init(iio_dummy_init);
  */
 static __exit void iio_dummy_exit(void)
 {
-	int i;
+	unsigned int i;
 
 	for (i = 0; i < instances; i++)
 		iio_dummy_remove(i);
-- 
2.4.1

Re: [PATCH 1/2] staging: iio_simple_dummy: fix init

[Daniel Baluta]

Hi,

On Wed, May 27, 2015 at 1:19 AM, Vladimirs Ambrosovs
<rodriguez.twister@gmail.com> wrote:
> This patch fixes the init function for the iio_simple_dummy driver.
> The main issues were absence of kfree for the allocated array, and no
> devices being removed in case the probe function fails, running in a loop.
>
> The iio_dummy_remove function was also changed:
>         * The return value was changed to void
>         * The check for return value of iio_simple_dummy_events_unregister()
> The reason for this changes is that, as per implementation,
> events_unregister function always returns 0, so we are safe not to check
> return value. As a result the return value for iio_dummy_remove function
> becomes useless as well, hence return value type change.

While at it I think we can also make
iio_simple_dummy_events_unregister return type void.
Nice to see that people pay attention to the dummy module :).

As part of Outreachy program, Cristina (CC'ed) will work on making the
IIO dummy driver
more useful with the final goal of moving it out of staging.

http://kernelnewbies.org/OutreachyIntro


>
> Signed-off-by: Vladimirs Ambrosovs <rodriguez.twister@gmail.com>
> ---
>  drivers/staging/iio/iio_simple_dummy.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
> index b47bf9f..88fbb4f 100644
> --- a/drivers/staging/iio/iio_simple_dummy.c
> +++ b/drivers/staging/iio/iio_simple_dummy.c
> @@ -665,9 +665,8 @@ error_ret:
>   *
>   * Parameters follow those of iio_dummy_probe for buses.
>   */
> -static int iio_dummy_remove(int index)
> +static void iio_dummy_remove(int index)
>  {
> -       int ret;
>         /*
>          * Get a pointer to the device instance iio_dev structure
>          * from the bus subsystem. E.g.
> @@ -685,15 +684,14 @@ static int iio_dummy_remove(int index)
>         /* Buffered capture related cleanup */
>         iio_simple_dummy_unconfigure_buffer(indio_dev);
>
> -       ret = iio_simple_dummy_events_unregister(indio_dev);
> -       if (ret)
> -               goto error_ret;
> +       /*
> +        * Tidy up interrupt handling
> +        * Always returns 0, so not checking for return value
> +        */
> +       iio_simple_dummy_events_unregister(indio_dev);
>
>         /* Free all structures */
>         iio_device_free(indio_dev);
> -
> -error_ret:
> -       return ret;
>  }
>
>  /**
> @@ -722,9 +720,17 @@ static __init int iio_dummy_init(void)
>         for (i = 0; i < instances; i++) {
>                 ret = iio_dummy_probe(i);
>                 if (ret < 0)
> -                       return ret;
> +                       goto error_probe;
>         }
>         return 0;
> +
> +error_probe:
> +       /* Free devices registered before error */
> +       while (i--)
> +               iio_dummy_remove(i);
> +
> +       kfree(iio_dummy_devs);
> +       return ret;
>  }
>  module_init(iio_dummy_init);
>
> --
> 2.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] staging: iio_simple_dummy: fix init

[Dan Carpenter]

On Wed, May 27, 2015 at 01:19:57AM +0300, Vladimirs Ambrosovs wrote:
> @@ -685,15 +684,14 @@ static int iio_dummy_remove(int index)
>  	/* Buffered capture related cleanup */
>  	iio_simple_dummy_unconfigure_buffer(indio_dev);
>  
> -	ret = iio_simple_dummy_events_unregister(indio_dev);
> -	if (ret)
> -		goto error_ret;
> +	/*
> +	 * Tidy up interrupt handling
> +	 * Always returns 0, so not checking for return value
> +	 */

Even though this is a dummy driver, this comment should be obvious to
everyone.  Just leave it out.

> +	iio_simple_dummy_events_unregister(indio_dev);
>  
>  	/* Free all structures */
>  	iio_device_free(indio_dev);
> -
> -error_ret:
> -	return ret;
>  }
>  
>  /**
> @@ -722,9 +720,17 @@ static __init int iio_dummy_init(void)
>  	for (i = 0; i < instances; i++) {
>  		ret = iio_dummy_probe(i);
>  		if (ret < 0)
> -			return ret;
> +			goto error_probe;
>  	}
>  	return 0;
> +
> +error_probe:

The label actually removes...  It's better to name labels after the
label location instead of the goto location.

> +	/* Free devices registered before error */

Leave out the obvious commment.

> +	while (i--)
> +		iio_dummy_remove(i);
> +
> +	kfree(iio_dummy_devs);
> +	return ret;
>  }
>  module_init(iio_dummy_init);

regards,
dan carpenter

Re: [PATCH 2/2] staging: iio_simple_dummy: zero check param

[Dan Carpenter]

On Wed, May 27, 2015 at 01:19:58AM +0300, Vladimirs Ambrosovs wrote:
> Check for zero was added to the module parameter "instances" to
> avoid the allocation of array of zero values. Although it is a valid call,
> we don't want to allocate ZERO_SIZE_PTR, so need to disallow this case.
> The type of variables which are compared to "instances" were also changed
> to unsigned int so that no compiler complaints occur.

Which compiler is that?

You should get a different compiler if you compiler complains about
stupid stuff like that.  Making everything unsigned int is a common
cause of problems.  I fixed or reported several of those bugs yesterday.

"instances" should be unsigned int, though, you're correct about that.

> 
> Signed-off-by: Vladimirs Ambrosovs <rodriguez.twister@gmail.com>
> ---
>  drivers/staging/iio/iio_simple_dummy.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
> index 88fbb4f..2744a1b 100644
> --- a/drivers/staging/iio/iio_simple_dummy.c
> +++ b/drivers/staging/iio/iio_simple_dummy.c
> @@ -30,7 +30,7 @@
>   * dummy devices are registered.
>   */
>  static unsigned instances = 1;
> -module_param(instances, int, 0);
> +module_param(instances, uint, 0);
>  
>  /* Pointer array used to fake bus elements */
>  static struct iio_dev **iio_dummy_devs;
> @@ -706,9 +706,10 @@ static void iio_dummy_remove(int index)
>   */
>  static __init int iio_dummy_init(void)
>  {
> -	int i, ret;
> +	unsigned int i;
> +	int ret;

No.

>  
> -	if (instances > 10) {
> +	if (instances == 0 || instances > 10) {
>  		instances = 1;
>  		return -EINVAL;

Allocating zero size arrays is a totally valid thing the kernel and it
doesn't cause a problem unless there are other existing serious bugs in
the code.  In this case instances == 0 is fine.

Setting "instances = 1" is bogus though.

>  	}
> @@ -742,7 +743,7 @@ module_init(iio_dummy_init);
>   */
>  static __exit void iio_dummy_exit(void)
>  {
> -	int i;
> +	unsigned int i;

No.

regards,
dan carpenter

Re: [PATCH 1/2] staging: iio_simple_dummy: fix init

[Vladimirs Ambrosovs]

On Wed, May 27, 2015 at 09:21:28AM +0300, Daniel Baluta wrote:
> Hi,
> 
> On Wed, May 27, 2015 at 1:19 AM, Vladimirs Ambrosovs
> <rodriguez.twister@gmail.com> wrote:
> > This patch fixes the init function for the iio_simple_dummy driver.
> > The main issues were absence of kfree for the allocated array, and no
> > devices being removed in case the probe function fails, running in a loop.
> >
> > The iio_dummy_remove function was also changed:
> >         * The return value was changed to void
> >         * The check for return value of iio_simple_dummy_events_unregister()
> > The reason for this changes is that, as per implementation,
> > events_unregister function always returns 0, so we are safe not to check
> > return value. As a result the return value for iio_dummy_remove function
> > becomes useless as well, hence return value type change.
> 
> While at it I think we can also make
> iio_simple_dummy_events_unregister return type void.
> Nice to see that people pay attention to the dummy module :).
> 
> As part of Outreachy program, Cristina (CC'ed) will work on making the
> IIO dummy driver
> more useful with the final goal of moving it out of staging.
> 
> http://kernelnewbies.org/OutreachyIntro
> 
> 
Thanks, that's a good point. Should I re-submit the patch, or better
reject the changes, and leave it to Cristina to address in scope of
Outreachy project?

> >
> > Signed-off-by: Vladimirs Ambrosovs <rodriguez.twister@gmail.com>
> > ---
> >  drivers/staging/iio/iio_simple_dummy.c | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
> > index b47bf9f..88fbb4f 100644
> > --- a/drivers/staging/iio/iio_simple_dummy.c
> > +++ b/drivers/staging/iio/iio_simple_dummy.c
> > @@ -665,9 +665,8 @@ error_ret:
> >   *
> >   * Parameters follow those of iio_dummy_probe for buses.
> >   */
> > -static int iio_dummy_remove(int index)
> > +static void iio_dummy_remove(int index)
> >  {
> > -       int ret;
> >         /*
> >          * Get a pointer to the device instance iio_dev structure
> >          * from the bus subsystem. E.g.
> > @@ -685,15 +684,14 @@ static int iio_dummy_remove(int index)
> >         /* Buffered capture related cleanup */
> >         iio_simple_dummy_unconfigure_buffer(indio_dev);
> >
> > -       ret = iio_simple_dummy_events_unregister(indio_dev);
> > -       if (ret)
> > -               goto error_ret;
> > +       /*
> > +        * Tidy up interrupt handling
> > +        * Always returns 0, so not checking for return value
> > +        */
> > +       iio_simple_dummy_events_unregister(indio_dev);
> >
> >         /* Free all structures */
> >         iio_device_free(indio_dev);
> > -
> > -error_ret:
> > -       return ret;
> >  }
> >
> >  /**
> > @@ -722,9 +720,17 @@ static __init int iio_dummy_init(void)
> >         for (i = 0; i < instances; i++) {
> >                 ret = iio_dummy_probe(i);
> >                 if (ret < 0)
> > -                       return ret;
> > +                       goto error_probe;
> >         }
> >         return 0;
> > +
> > +error_probe:
> > +       /* Free devices registered before error */
> > +       while (i--)
> > +               iio_dummy_remove(i);
> > +
> > +       kfree(iio_dummy_devs);
> > +       return ret;
> >  }
> >  module_init(iio_dummy_init);
> >
> > --
> > 2.4.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

BR,
Vladimirs.

Re: [PATCH 1/2] staging: iio_simple_dummy: fix init

[Dan Carpenter]

On Wed, May 27, 2015 at 08:24:18PM +0300, Vladimirs Ambrosovs wrote:
> On Wed, May 27, 2015 at 09:21:28AM +0300, Daniel Baluta wrote:
> > Hi,
> > 
> > On Wed, May 27, 2015 at 1:19 AM, Vladimirs Ambrosovs
> > <rodriguez.twister@gmail.com> wrote:
> > > This patch fixes the init function for the iio_simple_dummy driver.
> > > The main issues were absence of kfree for the allocated array, and no
> > > devices being removed in case the probe function fails, running in a loop.
> > >
> > > The iio_dummy_remove function was also changed:
> > >         * The return value was changed to void
> > >         * The check for return value of iio_simple_dummy_events_unregister()
> > > The reason for this changes is that, as per implementation,
> > > events_unregister function always returns 0, so we are safe not to check
> > > return value. As a result the return value for iio_dummy_remove function
> > > becomes useless as well, hence return value type change.
> > 
> > While at it I think we can also make
> > iio_simple_dummy_events_unregister return type void.
> > Nice to see that people pay attention to the dummy module :).
> > 
> > As part of Outreachy program, Cristina (CC'ed) will work on making the
> > IIO dummy driver
> > more useful with the final goal of moving it out of staging.
> > 
> > http://kernelnewbies.org/OutreachyIntro
> > 
> > 
> Thanks, that's a good point. Should I re-submit the patch, or better
> reject the changes, and leave it to Cristina to address in scope of
> Outreachy project?

No no.  Please resend the patches.

regards,
dan carpenter

Re: [PATCH 2/2] staging: iio_simple_dummy: zero check param

[Vladimirs Ambrosovs]

On Wed, May 27, 2015 at 11:25:07AM +0300, Dan Carpenter wrote:
> On Wed, May 27, 2015 at 01:19:58AM +0300, Vladimirs Ambrosovs wrote:
> > Check for zero was added to the module parameter "instances" to
> > avoid the allocation of array of zero values. Although it is a valid call,
> > we don't want to allocate ZERO_SIZE_PTR, so need to disallow this case.
> > The type of variables which are compared to "instances" were also changed
> > to unsigned int so that no compiler complaints occur.
> 
> Which compiler is that?
> 
> You should get a different compiler if you compiler complains about
> stupid stuff like that.  Making everything unsigned int is a common
> cause of problems.  I fixed or reported several of those bugs yesterday.
> 
> "instances" should be unsigned int, though, you're correct about that.
> 
Mine is fine - not complaining ;). 

Got your point, although, in some cases, I think, these warnings not a
stupid stuff, and could get some junior out of trouble.

But anyway, will keep in mind to stay away from unsigned ints. 
> > 
> > Signed-off-by: Vladimirs Ambrosovs <rodriguez.twister@gmail.com>
> > ---
> >  drivers/staging/iio/iio_simple_dummy.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
> > index 88fbb4f..2744a1b 100644
> > --- a/drivers/staging/iio/iio_simple_dummy.c
> > +++ b/drivers/staging/iio/iio_simple_dummy.c
> > @@ -30,7 +30,7 @@
> >   * dummy devices are registered.
> >   */
> >  static unsigned instances = 1;
> > -module_param(instances, int, 0);
> > +module_param(instances, uint, 0);
> >  
> >  /* Pointer array used to fake bus elements */
> >  static struct iio_dev **iio_dummy_devs;
> > @@ -706,9 +706,10 @@ static void iio_dummy_remove(int index)
> >   */
> >  static __init int iio_dummy_init(void)
> >  {
> > -	int i, ret;
> > +	unsigned int i;
> > +	int ret;
> 
> No.
> 
> >  
> > -	if (instances > 10) {
> > +	if (instances == 0 || instances > 10) {
> >  		instances = 1;
> >  		return -EINVAL;
> 
> Allocating zero size arrays is a totally valid thing the kernel and it
> doesn't cause a problem unless there are other existing serious bugs in
> the code.  In this case instances == 0 is fine.
> 
Sorry, got a bit confused - is it fine to be in the code, or the 0
value is valid, and shouldn't be checked for? The idea behind this
change was not the allocation of zero size array, but the
use of the module with 0 instances. However, maybe that actually
addresses some usecase, so probably would be better to leave it as it
was before.

> Setting "instances = 1" is bogus though.
> 
> >  	}
> > @@ -742,7 +743,7 @@ module_init(iio_dummy_init);
> >   */
> >  static __exit void iio_dummy_exit(void)
> >  {
> > -	int i;
> > +	unsigned int i;
> 
> No.
> 
> regards,
> dan carpenter
> 

Thanks for all the comments. Will send out the updated patches soon.

BR,
Vladimirs

Re: [PATCH 1/2] staging: iio_simple_dummy: fix init

[Daniel Baluta]

On Wed, May 27, 2015 at 8:29 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, May 27, 2015 at 08:24:18PM +0300, Vladimirs Ambrosovs wrote:
>> On Wed, May 27, 2015 at 09:21:28AM +0300, Daniel Baluta wrote:
>> > Hi,
>> >
>> > On Wed, May 27, 2015 at 1:19 AM, Vladimirs Ambrosovs
>> > <rodriguez.twister@gmail.com> wrote:
>> > > This patch fixes the init function for the iio_simple_dummy driver.
>> > > The main issues were absence of kfree for the allocated array, and no
>> > > devices being removed in case the probe function fails, running in a loop.
>> > >
>> > > The iio_dummy_remove function was also changed:
>> > >         * The return value was changed to void
>> > >         * The check for return value of iio_simple_dummy_events_unregister()
>> > > The reason for this changes is that, as per implementation,
>> > > events_unregister function always returns 0, so we are safe not to check
>> > > return value. As a result the return value for iio_dummy_remove function
>> > > becomes useless as well, hence return value type change.
>> >
>> > While at it I think we can also make
>> > iio_simple_dummy_events_unregister return type void.
>> > Nice to see that people pay attention to the dummy module :).
>> >
>> > As part of Outreachy program, Cristina (CC'ed) will work on making the
>> > IIO dummy driver
>> > more useful with the final goal of moving it out of staging.
>> >
>> > http://kernelnewbies.org/OutreachyIntro
>> >
>> >
>> Thanks, that's a good point. Should I re-submit the patch, or better
>> reject the changes, and leave it to Cristina to address in scope of
>> Outreachy project?
>
> No no.  Please resend the patches.

Agree.

Re: [PATCH 2/2] staging: iio_simple_dummy: zero check param

[Dan Carpenter]

On Thu, May 28, 2015 at 01:12:40AM +0300, Vladimirs Ambrosovs wrote:
> On Wed, May 27, 2015 at 11:25:07AM +0300, Dan Carpenter wrote:
> > On Wed, May 27, 2015 at 01:19:58AM +0300, Vladimirs Ambrosovs wrote:
> > > Check for zero was added to the module parameter "instances" to
> > > avoid the allocation of array of zero values. Although it is a valid call,
> > > we don't want to allocate ZERO_SIZE_PTR, so need to disallow this case.
> > > The type of variables which are compared to "instances" were also changed
> > > to unsigned int so that no compiler complaints occur.
> > 
> > Which compiler is that?
> > 
> > You should get a different compiler if you compiler complains about
> > stupid stuff like that.  Making everything unsigned int is a common
> > cause of problems.  I fixed or reported several of those bugs yesterday.
> > 
> > "instances" should be unsigned int, though, you're correct about that.
> > 
> Mine is fine - not complaining ;). 
> 
> Got your point, although, in some cases, I think, these warnings not a
> stupid stuff, and could get some junior out of trouble.
> 
> But anyway, will keep in mind to stay away from unsigned ints. 

It's not a matter of staying away from unsigned ints, it's that some
people make everything unsigned by default.  That causes problems for
two reasons.  1) The kernel uses negative error codes.  2) int is the
default datatype when you want a "number" in C.  If you want a special
number then you make it unsigned int, u32, or unsigned long or whatever.
All those types mean something.  An unsigned int and a u32 are the same
to a computer but to a human they mean something different.  People who
use complicated datatypes all the time instead of just plain old int are
making the code complicated.


> > > 
> > > Signed-off-by: Vladimirs Ambrosovs <rodriguez.twister@gmail.com>
> > > ---
> > >  drivers/staging/iio/iio_simple_dummy.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
> > > index 88fbb4f..2744a1b 100644
> > > --- a/drivers/staging/iio/iio_simple_dummy.c
> > > +++ b/drivers/staging/iio/iio_simple_dummy.c
> > > @@ -30,7 +30,7 @@
> > >   * dummy devices are registered.
> > >   */
> > >  static unsigned instances = 1;
> > > -module_param(instances, int, 0);
> > > +module_param(instances, uint, 0);
> > >  
> > >  /* Pointer array used to fake bus elements */
> > >  static struct iio_dev **iio_dummy_devs;
> > > @@ -706,9 +706,10 @@ static void iio_dummy_remove(int index)
> > >   */
> > >  static __init int iio_dummy_init(void)
> > >  {
> > > -	int i, ret;
> > > +	unsigned int i;
> > > +	int ret;
> > 
> > No.
> > 
> > >  
> > > -	if (instances > 10) {
> > > +	if (instances == 0 || instances > 10) {
> > >  		instances = 1;
> > >  		return -EINVAL;
> > 
> > Allocating zero size arrays is a totally valid thing the kernel and it
> > doesn't cause a problem unless there are other existing serious bugs in
> > the code.  In this case instances == 0 is fine.
> > 
> Sorry, got a bit confused - is it fine to be in the code, or the 0
> value is valid, and shouldn't be checked for? The idea behind this
> change was not the allocation of zero size array, but the
> use of the module with 0 instances.

The changelog specifically mentioned a ZERO_SIZE_ARRAY.  If you had
said, "It doesn't make sense to load a module with 0 instances" then I
would have allowed the patch.  I don't care if you make this change or
not, but the changelog had wrong motivation.

regards,
dan carpenter

Re: [PATCH 2/2] staging: iio_simple_dummy: zero check param

[Vladimirs Ambrosovs]

On Thu, May 28, 2015 at 09:59:34AM +0300, Dan Carpenter wrote:
> On Thu, May 28, 2015 at 01:12:40AM +0300, Vladimirs Ambrosovs wrote:
> > On Wed, May 27, 2015 at 11:25:07AM +0300, Dan Carpenter wrote:
> > > On Wed, May 27, 2015 at 01:19:58AM +0300, Vladimirs Ambrosovs wrote:
> > > > Check for zero was added to the module parameter "instances" to
> > > > avoid the allocation of array of zero values. Although it is a valid call,
> > > > we don't want to allocate ZERO_SIZE_PTR, so need to disallow this case.
> > > > The type of variables which are compared to "instances" were also changed
> > > > to unsigned int so that no compiler complaints occur.
> > > 
> > > Which compiler is that?
> > > 
> > > You should get a different compiler if you compiler complains about
> > > stupid stuff like that.  Making everything unsigned int is a common
> > > cause of problems.  I fixed or reported several of those bugs yesterday.
> > > 
> > > "instances" should be unsigned int, though, you're correct about that.
> > > 
> > Mine is fine - not complaining ;). 
> > 
> > Got your point, although, in some cases, I think, these warnings not a
> > stupid stuff, and could get some junior out of trouble.
> > 
> > But anyway, will keep in mind to stay away from unsigned ints. 
> 
> It's not a matter of staying away from unsigned ints, it's that some
> people make everything unsigned by default.  That causes problems for
> two reasons.  1) The kernel uses negative error codes.  2) int is the
> default datatype when you want a "number" in C.  If you want a special
> number then you make it unsigned int, u32, or unsigned long or whatever.
> All those types mean something.  An unsigned int and a u32 are the same
> to a computer but to a human they mean something different.  People who
> use complicated datatypes all the time instead of just plain old int are
> making the code complicated.
> 
> 
Thanks for the explanation.

> > > > 
> > > > Signed-off-by: Vladimirs Ambrosovs <rodriguez.twister@gmail.com>
> > > > ---
> > > >  drivers/staging/iio/iio_simple_dummy.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
> > > > index 88fbb4f..2744a1b 100644
> > > > --- a/drivers/staging/iio/iio_simple_dummy.c
> > > > +++ b/drivers/staging/iio/iio_simple_dummy.c
> > > > @@ -30,7 +30,7 @@
> > > >   * dummy devices are registered.
> > > >   */
> > > >  static unsigned instances = 1;
> > > > -module_param(instances, int, 0);
> > > > +module_param(instances, uint, 0);
> > > >  
> > > >  /* Pointer array used to fake bus elements */
> > > >  static struct iio_dev **iio_dummy_devs;
> > > > @@ -706,9 +706,10 @@ static void iio_dummy_remove(int index)
> > > >   */
> > > >  static __init int iio_dummy_init(void)
> > > >  {
> > > > -	int i, ret;
> > > > +	unsigned int i;
> > > > +	int ret;
> > > 
> > > No.
> > > 
> > > >  
> > > > -	if (instances > 10) {
> > > > +	if (instances == 0 || instances > 10) {
> > > >  		instances = 1;
> > > >  		return -EINVAL;
> > > 
> > > Allocating zero size arrays is a totally valid thing the kernel and it
> > > doesn't cause a problem unless there are other existing serious bugs in
> > > the code.  In this case instances == 0 is fine.
> > > 
> > Sorry, got a bit confused - is it fine to be in the code, or the 0
> > value is valid, and shouldn't be checked for? The idea behind this
> > change was not the allocation of zero size array, but the
> > use of the module with 0 instances.
> 
> The changelog specifically mentioned a ZERO_SIZE_ARRAY.  If you had
> said, "It doesn't make sense to load a module with 0 instances" then I
> would have allowed the patch.  I don't care if you make this change or
> not, but the changelog had wrong motivation.
> 
Fair enough.

> regards,
> dan carpenter

BR,
Vladimirs