From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753042AbcADMU3 (ORCPT <rfc822;w@1wt.eu>);
	Mon, 4 Jan 2016 07:20:29 -0500
Received: from aserp1040.oracle.com ([141.146.126.69]:24904 "EHLO
	aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751455AbcADMU0 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 4 Jan 2016 07:20:26 -0500
Date: Mon, 4 Jan 2016 15:20:05 +0300
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Samuel Thibault <samuel.thibault@ens-lyon.org>,
        Chen Gang <gang.chen@asianux.com>, William Hubbs <w.d.hubbs@gmail.com>,
        Chris Brannon <chris@the-brannons.com>, Kirk Reiser <kirk@reisers.ca>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        speakup@linux-speakup.org, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Staging: speakup: Fix getting port information
Message-ID: <20160104122005.GI5284@mwanda>
References: <20160102232529.GC2860@var.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160102232529.GC2860@var.home>
User-Agent: Mutt/1.5.21 (2010-09-15)
X-Source-IP: userv0022.oracle.com [156.151.31.74]
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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