Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753784AbbG2RDm (ORCPT ); Wed, 29 Jul 2015 13:03:42 -0400 Received: from mail-out.m-online.net ([212.18.0.9]:58188 "EHLO mail-out.m-online.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752488AbbG2RDk (ORCPT ); Wed, 29 Jul 2015 13:03:40 -0400 X-Auth-Info: esTSp6+PukjFDDVz841jEETSv8K/1AJhhFWPeR8F53I= From: Marek Vasut To: Michal Suchanek Subject: Re: [PATCH 1/2] mtd: spi-nor: rework spi nor read and write. Date: Wed, 29 Jul 2015 19:03:35 +0200 User-Agent: KMail/1.13.7 (Linux/3.14-2-amd64; KDE/4.13.1; x86_64; ; ) Cc: David Woodhouse , Brian Norris , Han Xu , =?utf-8?q?Rafa=C5=82_Mi=C5=82ecki?= , Alison Chaiken , Huang Shijie , Ben Hutchings , Gabor Juhos , "Bean Huo =?utf-8?q?=E9=9C=8D=E6=96=8C=E6=96=8C?= (beanhuo)" , MTD Maling List , Linux Kernel Mailing List References: <201507282015.10775.marex@denx.de> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <201507291903.36020.marex@denx.de> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3772 Lines: 101 On Wednesday, July 29, 2015 at 06:16:21 AM, Michal Suchanek wrote: > On 28 July 2015 at 20:15, Marek Vasut wrote: > > On Tuesday, July 28, 2015 at 11:23:02 AM, Michal Suchanek wrote: > >> The spi_nor read and write functions pass thru the mtd retlen to the > >> chip-specific read and write function. This makes it difficult to check > >> for errors in read and write functions and these errors are not checked. > >> This leads to silent data corruption. > >> > >> This patch styles the chip-specific read and write function as unix > >> read(2) and write(2) and updates the retlen in the spi-nor generic > >> driver. > >> > >> This also makes it possible to check for write errors. > >> When pl330 fails to transfer the flash data over SPI I get I/O error > >> instead of 4M of zeroes. > >> > >> I do not have sst and fsl hardware to test with. > >> > >> Signed-off-by: Michal Suchanek > >> --- > >> > >> drivers/mtd/devices/m25p80.c | 33 ++++++++++++++--------- > >> drivers/mtd/spi-nor/fsl-quadspi.c | 29 ++++++++++---------- > >> drivers/mtd/spi-nor/spi-nor.c | 57 > >> > >> ++++++++++++++++++++++++++++----------- include/linux/mtd/spi-nor.h > >> > >> | 8 +++--- > >> > >> 4 files changed, 81 insertions(+), 46 deletions(-) > >> > >> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > >> index d313f948b..d8f064b 100644 > >> --- a/drivers/mtd/devices/m25p80.c > >> +++ b/drivers/mtd/devices/m25p80.c > >> @@ -75,14 +75,15 @@ static int m25p80_write_reg(struct spi_nor *nor, u8 > >> opcode, u8 *buf, int len, return spi_write(spi, flash->command, len + > >> 1); > >> > >> } > >> > >> -static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len, > >> - size_t *retlen, const u_char *buf) > >> +static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len, > >> + const u_char *buf) > >> > >> { > >> > >> struct m25p *flash = nor->priv; > >> struct spi_device *spi = flash->spi; > >> struct spi_transfer t[2] = {}; > >> struct spi_message m; > >> int cmd_sz = m25p_cmdsz(nor); > >> > >> + ssize_t ret; > >> > >> spi_message_init(&m); > >> > >> @@ -100,9 +101,14 @@ static void m25p80_write(struct spi_nor *nor, > >> loff_t to, size_t len, t[1].len = len; > >> > >> spi_message_add_tail(&t[1], &m); > >> > >> - spi_sync(spi, &m); > >> + ret = spi_sync(spi, &m); > >> + if (ret) > >> + return ret; > >> > >> - *retlen += m.actual_length - cmd_sz; > >> + ret = m.actual_length - cmd_sz; > >> + if (ret < 0) > >> + return -EIO; > >> + return ret; > > > > I'd prefer to just add the return value and keep the retlen to keep error > > codes and transfer length separate. > > I prefer to not pass around retlen because passing it around > > - causes code duplication > - makes the code harder to understand Not sure I want to argue on this one, since it looks like a matter of taste. Adding the error return first and removing the retlen second might make the patch more readable though. > > btw. you change the transfer length from unsigned to signed type -- long > > transfer might get interpreted as an error. > > Note that ssize_t is supposed to be enough for write(2) so when it > does not suffice you have a real system-wide problem. > > That aside NOR flash sizes tend to be in the order of megabytes so > this concern is very theoretical. I do hope so :) -- 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/