Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1300147 > unrolled thread

[PATCH] Staging: speakup: Fix getting port information

Back to article view | Back to linux.kernel

Contents

  [PATCH] Staging: speakup: Fix getting port information Samuel Thibault <samuel.thibault@ens-lyon.org> - 2016-01-03 00:40 +0100
    Re: [PATCH] Staging: speakup: Fix getting port information Samuel Thibault <samuel.thibault@ens-lyon.org> - 2016-01-03 00:50 +0100
    Re: [PATCH] Staging: speakup: Fix getting port information covici@ccs.covici.com - 2016-01-03 01:50 +0100
      Re: [PATCH] Staging: speakup: Fix getting port information Samuel Thibault <samuel.thibault@ens-lyon.org> - 2016-01-03 02:10 +0100
        Re: [PATCH] Staging: speakup: Fix getting port information covici@ccs.covici.com - 2016-01-03 02:40 +0100
        Re: [PATCH] Staging: speakup: Fix getting port information Dan Carpenter <dan.carpenter@oracle.com> - 2016-01-04 13:30 +0100
          Re: [PATCH] Staging: speakup: Fix getting port information Samuel Thibault <samuel.thibault@ens-lyon.org> - 2016-01-05 02:30 +0100
        Re: [PATCH] Staging: speakup: Fix getting port information Dan Carpenter <dan.carpenter@oracle.com> - 2016-01-04 13:30 +0100
    Re: [PATCH] Staging: speakup: Fix getting port information Dan Carpenter <dan.carpenter@oracle.com> - 2016-01-04 13:30 +0100

#1300147 — [PATCH] Staging: speakup: Fix getting port information

5e6dc54 broke the port information in the speakup driver:
SERIAL_PORT_DFNS only gets defined if asm/serial.h is included.

Along the way, make sure that we do have information for the requested
serial port number (index)

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

--- a/drivers/staging/speakup/serialio.c
+++ b/drivers/staging/speakup/serialio.c
@@ -6,6 +6,9 @@
 #include "spk_priv.h"
 #include "serialio.h"
 
+#include <linux/serial_core.h>
+#include <asm/serial.h>
+
 #ifndef SERIAL_PORT_DFNS
 #define SERIAL_PORT_DFNS
 #endif
@@ -26,6 +29,11 @@ const struct old_serial_port *spk_serial
 	const struct old_serial_port *ser = rs_table + index;
 	int err;
 
+	if (index > sizeof(rs_table) / sizeof(*rs_table)) {
+		pr_info("no port info for ttyS%d\n", index);
+		return NULL;
+	}
+
 	/*	Divisor, bytesize and parity */
 	quot = ser->baud_base / baud;
 	cval = cflag & (CSIZE | CSTOPB);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1300148

Samuel Thibault, on Sun 03 Jan 2016 00:25:29 +0100, wrote:
> 5e6dc54 broke the port information in the speakup driver:
> SERIAL_PORT_DFNS only gets defined if asm/serial.h is included.
> 
> Along the way, make sure that we do have information for the requested
> serial port number (index)

(It'd be good to get this into 4.4 and 4.3.4).

> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> --- a/drivers/staging/speakup/serialio.c
> +++ b/drivers/staging/speakup/serialio.c
> @@ -6,6 +6,9 @@
>  #include "spk_priv.h"
>  #include "serialio.h"
>  
> +#include <linux/serial_core.h>
> +#include <asm/serial.h>
> +
>  #ifndef SERIAL_PORT_DFNS
>  #define SERIAL_PORT_DFNS
>  #endif
> @@ -26,6 +29,11 @@ const struct old_serial_port *spk_serial
>  	const struct old_serial_port *ser = rs_table + index;
>  	int err;
>  
> +	if (index > sizeof(rs_table) / sizeof(*rs_table)) {
> +		pr_info("no port info for ttyS%d\n", index);
> +		return NULL;
> +	}
> +
>  	/*	Divisor, bytesize and parity */
>  	quot = ser->baud_base / baud;
>  	cval = cflag & (CSIZE | CSTOPB);

-- 
Samuel
AUTHOR
     FvwmM4 is the result of a random  bit  mutation  on  a  hard
     disk,  presumably  a  result  of  a  cosmic-ray or some such
     thing.
(extrait de la page de man de FvwmM4)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300157

I had a patch which also worked, but yours may be better -- I enclose it
here for your information.

--- drivers/staging/speakup/serialio.h.old	2015-08-30 14:34:09.000000000 -0400
+++ drivers/staging/speakup/serialio.h	2015-10-07 06:27:04.880829874 -0400
@@ -1,22 +1,24 @@
 #ifndef _SPEAKUP_SERIAL_H
 #define _SPEAKUP_SERIAL_H
 
+#include <linux/serial_core.h>
 #include <linux/serial.h>	/* for rs_table, serial constants */
 #include <linux/serial_reg.h>	/* for more serial constants */
 #ifndef __sparc__
-#include <linux/serial.h>
+#include <asm/serial.h>
 #endif
 
 /*
  * this is cut&paste from 8250.h. Get rid of the structure, the definitions
  * and this whole broken driver.
  */
+
 struct old_serial_port {
 	unsigned int uart; /* unused */
 	unsigned int baud_base;
 	unsigned int port;
 	unsigned int irq;
-	unsigned int flags; /* unused */
+  upf_t        flags; /*unused*/
 };
 
 /* countdown values for serial timeouts in us */
@@ -34,7 +36,6 @@
 #define SPK_TIMEOUT 100
 #define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
 
-#define spk_serial_tx_busy() \
-	((inb(speakup_info.port_tts + UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
+#define spk_serial_tx_busy() ((inb(speakup_info.port_tts + UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
 
 #endif

Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:

> 5e6dc54 broke the port information in the speakup driver:
> SERIAL_PORT_DFNS only gets defined if asm/serial.h is included.
> 
> Along the way, make sure that we do have information for the requested
> serial port number (index)
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> --- a/drivers/staging/speakup/serialio.c
> +++ b/drivers/staging/speakup/serialio.c
> @@ -6,6 +6,9 @@
>  #include "spk_priv.h"
>  #include "serialio.h"
>  
> +#include <linux/serial_core.h>
> +#include <asm/serial.h>
> +
>  #ifndef SERIAL_PORT_DFNS
>  #define SERIAL_PORT_DFNS
>  #endif
> @@ -26,6 +29,11 @@ const struct old_serial_port *spk_serial
>  	const struct old_serial_port *ser = rs_table + index;
>  	int err;
>  
> +	if (index > sizeof(rs_table) / sizeof(*rs_table)) {
> +		pr_info("no port info for ttyS%d\n", index);
> +		return NULL;
> +	}
> +
>  	/*	Divisor, bytesize and parity */
>  	quot = ser->baud_base / baud;
>  	cval = cflag & (CSIZE | CSTOPB);
> _______________________________________________
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300158

covici@ccs.covici.com, on Sat 02 Jan 2016 19:10:36 -0500, wrote:
> I had a patch which also worked, but yours may be better -- I enclose it
> here for your information.

Well, it's not up to serialio.h to include things for serialio.c. That
however makes me realize that the culprit is actually
f79b0d9 (which actually doesn't make much sense since linux/serial.h is
getting included a couple of lines above...).

I don't know what this "use <linux/serial.h> instead <asm/serial.h>"
warning is about, but *no* header in include/ includes asm/serial.h, so
there is no way to get the SERIAL_PORT_DFNS definition just by including
linux/serial.h, we really need asm/serial.h, just like 8250*.c do.

So we really need serialio.c to include linux/serial_core.h then
asm/serial.h, as my patch does.

Samuel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300164

Well, OK with me, I will use yours instead because I don't know if they
will backport the thing, thanks so much for doing this.

Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:

> covici@ccs.covici.com, on Sat 02 Jan 2016 19:10:36 -0500, wrote:
> > I had a patch which also worked, but yours may be better -- I enclose it
> > here for your information.
> 
> Well, it's not up to serialio.h to include things for serialio.c. That
> however makes me realize that the culprit is actually
> f79b0d9 (which actually doesn't make much sense since linux/serial.h is
> getting included a couple of lines above...).
> 
> I don't know what this "use <linux/serial.h> instead <asm/serial.h>"
> warning is about, but *no* header in include/ includes asm/serial.h, so
> there is no way to get the SERIAL_PORT_DFNS definition just by including
> linux/serial.h, we really need asm/serial.h, just like 8250*.c do.
> 
> So we really need serialio.c to include linux/serial_core.h then
> asm/serial.h, as my patch does.
> 
> Samuel
> _______________________________________________
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300699

On Mon, Jan 04, 2016 at 03:22:49PM +0300, Dan Carpenter wrote:
> On Sun, Jan 03, 2016 at 01:56:20AM +0100, Samuel Thibault wrote:
> > covici@ccs.covici.com, on Sat 02 Jan 2016 19:10:36 -0500, wrote:
> > > I had a patch which also worked, but yours may be better -- I enclose it
> > > here for your information.
> > 
> > Well, it's not up to serialio.h to include things for serialio.c. That
> > however makes me realize that the culprit is actually
> > f79b0d9 (which actually doesn't make much sense since linux/serial.h is
> > getting included a couple of lines above...).
> 

Btw, the patch title of that patch f79b0d9c223a ('staging: speakup:
Fixed warning <linux/serial.h> instead of <asm/serial.h>') describes
exactly everything about the patch.  So including the title really does
help save time for everyone.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301222

Mmm, sorry.  I don't submit patches often enough, so that each time I do
it, there are new things to know about it :)

Thanks for the comments,
Samuel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300708

On Sun, Jan 03, 2016 at 01:56:20AM +0100, Samuel Thibault wrote:
> covici@ccs.covici.com, on Sat 02 Jan 2016 19:10:36 -0500, wrote:
> > I had a patch which also worked, but yours may be better -- I enclose it
> > here for your information.
> 
> Well, it's not up to serialio.h to include things for serialio.c. That
> however makes me realize that the culprit is actually
> f79b0d9 (which actually doesn't make much sense since linux/serial.h is
> getting included a couple of lines above...).

CC the guilty.

Also we have a Fixes tag.  Please use it.

> 
> I don't know what this "use <linux/serial.h> instead <asm/serial.h>"
> warning is about, but *no* header in include/ includes asm/serial.h, so
> there is no way to get the SERIAL_PORT_DFNS definition just by including
> linux/serial.h, we really need asm/serial.h, just like 8250*.c do.
> 
> So we really need serialio.c to include linux/serial_core.h then
> asm/serial.h, as my patch does.

Ah...  Ok.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300706

On Sun, Jan 03, 2016 at 12:25:29AM +0100, Samuel Thibault wrote:
> 5e6dc54 broke the port information in the speakup driver:

There is a correct format for this.

Patch 5e6dc548e453 ('drivers: staging: speakup: serialio: only use
platform specific SERIAL_PORT_DFNS.') broke the port information ...

If you specify fewer than 12 numbers from the git hash it might not be
unique next year.  If you leave out the patch title then no one
knows what you are talking about because we are not robots and we are
better at remembering text instead if hex numbers.  Also CC the guilty
party instead of discussing them behind their backs.

> SERIAL_PORT_DFNS only gets defined if asm/serial.h is included.

No, that's not true.  There is a #define SERIAL_PORT_DFN at the start of
the file.  I am confused.

> 
> Along the way, make sure that we do have information for the requested
> serial port number (index)
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> --- a/drivers/staging/speakup/serialio.c
> +++ b/drivers/staging/speakup/serialio.c
> @@ -6,6 +6,9 @@
>  #include "spk_priv.h"
>  #include "serialio.h"
>  
> +#include <linux/serial_core.h>
> +#include <asm/serial.h>

This should be: <linux/serial.h> probably.

> +
>  #ifndef SERIAL_PORT_DFNS
>  #define SERIAL_PORT_DFNS
>  #endif
> @@ -26,6 +29,11 @@ const struct old_serial_port *spk_serial
>  	const struct old_serial_port *ser = rs_table + index;
>  	int err;
>  
> +	if (index > sizeof(rs_table) / sizeof(*rs_table)) {

This has an off-by-one bug > vs >=.  Also use the ARRAY_SIZE() macro.

	if (index >= ARRAY_SIZE(rs_table)) {

Could you move the use of index below the check?  Current static
analysis tools are deficient and prefer "check first and then use" order.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web