Fwd: Re: speakup issues

[William Hubbs <w.d.hubbs@gmail.com>]

----- Forwarded message from Dan Carpenter <dan.carpenter@oracle.com> -----

> Date: Fri, 10 May 2013 01:49:12 +0300
> From: Dan Carpenter <dan.carpenter@oracle.com>
> To: William Hubbs <w.d.hubbs@gmail.com>
> Cc: devel@driverdev.osuosl.org
> Subject: Re: speakup issues
> 
> Btw, this code isn't horrible.  It uses small functions and it's
> readable.  It's just needs small cleanups throughout...
> 
> Get rid of the global variable called "synth".  That's a bad name
> for a global and it's shadowed by local variables named "synth" so
> it creates confusion.
> 
> Don't do "pr_warn("synth probe\n");" on the success path.
> 
> As much as possible get rid of forward declarations.
> 
> Some of the 80 character line breaks were done in a funny way.
> 
> What is this code?
> #if defined(__powerpc__) || defined(__alpha__)
> 	cval >>= 8;
> #else /* !__powerpc__ && !__alpha__ */
> 	cval >>= 4;
> #endif /* !__powerpc__ && !__alpha__ */
> 
> Delete commented out code.
> 
> The file scoped variable "timeouts" is only used in one function.
> It could be declared as a static variable locally in that function.
> 
> /*
>  * this is cut&paste from 8250.h. Get rid of the structure, the definitions
>  * and this whole broken driver.
>  */
> 
> We go to a lot of effort to print out this message which just tells
> you that adds no information:
> 
> 	if (failed) {
> 		pr_info("%s: not found\n", synth->long_name);
> 		return -ENODEV;
> 	}
> 
> synth_add() has a memory corrupting off-by-one bug.
> 
> The code returns -1 (which means "permission denied") instead of
> returning standard error codes.
> 
> The author of this code doesn't like break statements and uses
> compound conditions to avoid them.
> 
>    426          for (i = 0; i < MAXSYNTHS && synths[i] != NULL; i++)
>    427                  /* synth_remove() is responsible for rotating the array down */
>    428                  if (in_synth == synths[i]) {
>    429                          mutex_unlock(&spk_mutex);
>    430                          return 0;
>    431                  }
>    432          if (i == MAXSYNTHS) {
>    433                  pr_warn("Error: attempting to add a synth past end of array\n");
>    434                  mutex_unlock(&spk_mutex);
>    435                  return -1;
>    436          }
> 
> Unless there is a special reason then for loops should be written
> in the canonical format.  Use curly braces for readability when
> there is a multi-line loop even if they are not needed
> syntactically.
> 
> 	for (i = 0; i < ARRAY_SIZE(synths); i++) {
> 		/* synth_remove() is responsible for rotating the array down */
> 		if (synths[i] == in_synth) {
> 			mutex_unlock(&spk_mutex);
> 			return 0;
> 		}
> 		if (synths[i] == NULL)
> 			break;
> 	}
> 	if (i == ARRAY_SIZE(synths)) {
> 		pr_warn("Error: attempting to add a synth past end of array\n");
> 		mutex_unlock(&spk_mutex);
> 		return -ENOMEM;
> 	}
> 
> Pretty much where ever you look there is something to clean up.
> Fortunately, it's mostly small things.
> 
> Sparse has a few things it complains about.
> 
> regards,
> dan carpenter

----- End forwarded message -----