Re: [PATCH] Staging: speakup: Fix getting port information

[Dan Carpenter]

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@xxxxxxxxxxxx>
> 
> --- 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/