Fwd: Re: speakup issues

Fwd: Re: speakup issues

[William Hubbs]

----- Forwarded message from Greg KH <gregkh@linuxfoundation.org> -----

> Date: Thu, 9 May 2013 13:47:19 -0700
> From: Greg KH <gregkh@linuxfoundation.org>
> To: William Hubbs <w.d.hubbs@gmail.com>
> Cc: devel@driverdev.osuosl.org
> Subject: Re: speakup issues
> 
> On Thu, May 09, 2013 at 10:59:58AM -0500, William Hubbs wrote:
> > Hi Greg,
> 
> Hi.  Note my email change, you probably got the bounce from the old
> suse.de address already...
> 
> > I am writing to you and to this list because I'm not quite sure where
> > else to write to ask this.
> > 
> > For those on the list who do not know me, I am one of the developers
> > working on Speakup.
> > 
> > I am interested in collecting a list of things that would need to happen
> > to allow speakup to move from staging to mainline. Is this list the
> > place to get that kind of information?
> > 
> > Greg, if you decide when things can move, can you give me a list of
> > things that we need to look into fixing?
> > 
> > I'm sure the speakup serial i/o code is one of those things, but what
> > else needs to be done?
> 
> Dan has given you a great list of things to work on, start with that.
> 
> Running checkpatch.pl is a great place to start with.  The serial stuff
> can be worked on as well, there are better ways to resolve that problem.
> 
> Also, please remove all the BUG_ON() instances, no driver should ever be
> able to instantly stop a machine, with no chance of it to be revived,
> that's just not nice.
> 
> thanks,
> 
> greg k-h

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

Re: Fwd: Re: speakup issues

[covici]

Well, that does not sound so bad --   a lot of small things.  What is
checkpatch.pl ?  Also, Does Greg have a better idea for the serial
stuff?

William Hubbs <w.d.hubbs@gmail.com> wrote:

> ----- Forwarded message from Greg KH <gregkh@linuxfoundation.org> -----
> 
> > Date: Thu, 9 May 2013 13:47:19 -0700
> > From: Greg KH <gregkh@linuxfoundation.org>
> > To: William Hubbs <w.d.hubbs@gmail.com>
> > Cc: devel@driverdev.osuosl.org
> > Subject: Re: speakup issues
> > 
> > On Thu, May 09, 2013 at 10:59:58AM -0500, William Hubbs wrote:
> > > Hi Greg,
> > 
> > Hi.  Note my email change, you probably got the bounce from the old
> > suse.de address already...
> > 
> > > I am writing to you and to this list because I'm not quite sure where
> > > else to write to ask this.
> > > 
> > > For those on the list who do not know me, I am one of the developers
> > > working on Speakup.
> > > 
> > > I am interested in collecting a list of things that would need to happen
> > > to allow speakup to move from staging to mainline. Is this list the
> > > place to get that kind of information?
> > > 
> > > Greg, if you decide when things can move, can you give me a list of
> > > things that we need to look into fixing?
> > > 
> > > I'm sure the speakup serial i/o code is one of those things, but what
> > > else needs to be done?
> > 
> > Dan has given you a great list of things to work on, start with that.
> > 
> > Running checkpatch.pl is a great place to start with.  The serial stuff
> > can be worked on as well, there are better ways to resolve that problem.
> > 
> > Also, please remove all the BUG_ON() instances, no driver should ever be
> > able to instantly stop a machine, with no chance of it to be revived,
> > that's just not nice.
> > 
> > thanks,
> > 
> > greg k-h
> 
> ----- End forwarded message -----
> _______________________________________________
> Speakup mailing list
> Speakup@linux-speakup.org
> http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup

-- 
Your life is like a penny.  You're going to lose it.  The question is:
How do
you spend it?

         John Covici
         covici@ccs.covici.com

Re: Fwd: Re: speakup issues

[Jason White]

covici@ccs.covici.com <covici@ccs.covici.com> wrote:
> Well, that does not sound so bad --   a lot of small things.  What is
> checkpatch.pl ? 

I think it's a test script that looks for code which doesn't follow kernel
style and programming guidelines.

You should look for it in the kernel sources.

Fwd: Re: speakup issues

[William Hubbs]

----- 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 -----

Fwd: Re: speakup issues

[William Hubbs]

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

> Date: Thu, 9 May 2013 21:55:26 +0300
> From: Dan Carpenter <dan.carpenter@oracle.com>
> To: William Hubbs <w.d.hubbs@gmail.com>
> Cc: gregkh@suse.de, devel@driverdev.osuosl.org
> Subject: Re: speakup issues
> 
> It's like every function in speakup has something that's just a
> little bit ugly about it.
> 
> For example
> 1) No spaces around operations.  IE "if (x&4) {" vs "if (x & 4) {".
> 2) No blank lines between the variable declarations and the code.
> 3) The 'E' in E_DEFAULT stands for "enum" which is bogus.
> 4) Custom macros like spk_lock()
> 5) Using u_char instead of u8.  (Yes, there are thousands of places
>    in the kernel which do this but it's mostly horrid legacy code
>    like ISDN).
> 6) In synth_direct_store() the last 5 bytes of the tmp[] buf are not
>    used.
> 7) Unnecessary casts.
> 8) Things like this:
> 
> 	char *cp;
>         u_char *cp1;
> 
>    "cp1" is a poorly chosen variable name.
> 
> Anyway, there is a lot of stuff.  It just needs someone to go
> through it bit by bit and re-write it cleaner.
> 
> regards,
> dan carpenter
> 

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