From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38534) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UpDSr-0001LS-QD for qemu-devel@nongnu.org; Wed, 19 Jun 2013 04:10:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UpDSq-0007mC-DK for qemu-devel@nongnu.org; Wed, 19 Jun 2013 04:10:01 -0400 Message-ID: <51C16755.9050700@msgid.tls.msk.ru> Date: Wed, 19 Jun 2013 12:09:57 +0400 From: Michael Tokarev MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/5] block/nand: Factor out common code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: peter.crosthwaite@xilinx.com Cc: Kevin Wolf , peter.maydell@linaro.org, qemu-trivial@nongnu.org, qemu-devel@nongnu.org, Stefan Hajnoczi , edgar.iglesias@gmail.com 18.06.2013 15:08, peter.crosthwaite@xilinx.com wrote: > From: Peter Crosthwaite > > Most of this computation of s->iolen is the same for both the if and > else paths here. Factor out the common parts outside the if. > > Cc: qemu-trivial@nongnu.org > > Signed-off-by: Peter Crosthwaite > --- > > hw/block/nand.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/hw/block/nand.c b/hw/block/nand.c > index 087ca14..6309f93 100644 > --- a/hw/block/nand.c > +++ b/hw/block/nand.c > @@ -272,10 +272,10 @@ static void nand_command(NANDFlashState *s) > break; > offset = s->addr & ((1 << s->addr_shift) - 1); > s->blk_load(s, s->addr, offset); > - if (s->gnd) > - s->iolen = (1 << s->page_shift) - offset; > - else > - s->iolen = (1 << s->page_shift) + (1 << s->oob_shift) - offset; > + s->iolen = (1 << s->page_shift) - offset; > + if (!s->gnd) { > + s->iolen += 1 << s->oob_shift; > + } Hm. Can s->iolen become negative here? addr_shift can be either less or greather than page_shift. When addr_shift is larger than page_shift (addr_shift=16, page_shift=11), offset may be up to 65535, in which case s->iolen may be -63487, even without additional oob_shift. I dunno if this is a concern, just.. asking. Besides, exactly the same expression is used in nand_getio() down this file, maybe a common function is in order? :) And looking at this file, I think it deserves some good type changes. For example, all these _shift are unsigned, and most are actually used as masks, in form (1<