From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755759AbbEUIfO (ORCPT ); Thu, 21 May 2015 04:35:14 -0400 Received: from mail-wi0-f178.google.com ([209.85.212.178]:33395 "EHLO mail-wi0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755273AbbEUIdv (ORCPT ); Thu, 21 May 2015 04:33:51 -0400 MIME-Version: 1.0 In-Reply-To: <20150520234525.GY11598@ld-irv-0074> References: <02f7510acb9b6dbb3a5a6cd5bb287762eb4d22c1.1430430153.git.hramrach@gmail.com> <20150520234525.GY11598@ld-irv-0074> From: Michal Suchanek Date: Thu, 21 May 2015 10:33:09 +0200 Message-ID: Subject: Re: [PATCH 1/3] MTD: m25p80: fix write return value. To: Brian Norris Cc: David Woodhouse , "Rafa?? Mi??ecki" , Marek Vasut , Alison Chaiken , Ben Hutchings , Geert Uytterhoeven , "Bean Huo (beanhuo)" , "grmoore@altera.com" , linux-mtd@lists.infradead.org, Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, On 21 May 2015 at 01:45, Brian Norris wrote: > On Thu, Apr 30, 2015 at 03:33:47PM +0200, Michal Suchanek wrote: >> The 'retlen' points to a variable representing the number of data bytes >> written/read (see include/linux/mtd/mtd.h) by the current invocation of >> the function. This variable must be set, not incremented. >> >> v2: clearer commit message >> >> Signed-off-by: Michal Suchanek >> Acked-by: Marek Vasut >> --- >> drivers/mtd/devices/m25p80.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c >> index 7c8b169..0b2bc21 100644 >> --- a/drivers/mtd/devices/m25p80.c >> +++ b/drivers/mtd/devices/m25p80.c >> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len, >> >> spi_sync(spi, &m); >> >> - *retlen += m.actual_length - cmd_sz; >> + *retlen = m.actual_length - cmd_sz; > > This is very wrong. It gets a little better once you add your next > patches (which are also not good) since those patches reinterpret the > usage of retlen, but this one definitely does *not* stand a lone. > > I'll admit the API is a little odd here, but the callers of this > function (see spi_nor_write()) actually depend on calling this multiple > times, with the value incrementing each time so we get a summary total. > You're now clobbering this value. > > I'm willing to accept patches to improve this API, if you think that > would help. Or to add comments that document this. Yes, the only user of the retlen value ignores it but passes it on so without the following patch this one makes the passed on value different from before. For m25p80 this would be fixed by truncating the write in the driver and setting actual_length appropriately rather than returning an error when the message is too long. It might possibly break other drivers, though. Thanks Michal