| Message ID | f65ba24ae67c04af46940c8cc67093894af97871.1371553360.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
18.06.2013 15:08, peter.crosthwaite@xilinx.com wrote: > From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > 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 <peter.crosthwaite@xilinx.com> > --- > > 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<<shift), instead of directly (except of erase_shift). Thanks, /mjt
Hi Michael, On Wed, Jun 19, 2013 at 6:09 PM, Michael Tokarev <mjt@tls.msk.ru> wrote: > 18.06.2013 15:08, peter.crosthwaite@xilinx.com wrote: >> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> >> 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 <peter.crosthwaite@xilinx.com> >> --- >> >> 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<<shift), instead of directly (except of erase_shift). > All sounds fair. Ill have another look in the context of type changing, and factoring out some commonality. Your able to take P2 without conflict? :) Regards, Peter > Thanks, > > /mjt >
19.06.2013 12:13, Peter Crosthwaite wrote: [] > All sounds fair. Ill have another look in the context of type > changing, and factoring out some commonality. Your able to take P2 > without conflict? :) Thanks! And sure, either with or without your next reformatting (block/nand: Formatting sweep) -- either on top of it or without it -- is okay. /mjt
On 18 June 2013 12:08, <peter.crosthwaite@xilinx.com> wrote: > From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > 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 Please don't cc qemu-trivial on half a patchset: it makes it awkward to commit to a submaintainer tree if half of it has already gone somewhere else. thanks -- PMM
On Wed, Jun 19, 2013 at 6:09 PM, Michael Tokarev <mjt@tls.msk.ru> wrote: > 18.06.2013 15:08, peter.crosthwaite@xilinx.com wrote: >> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> >> 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 <peter.crosthwaite@xilinx.com> >> --- >> >> 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? :) > Factored this out. Just added it to nand_blk_load_XXX(). Regards, Peter > 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<<shift), instead of directly (except of erase_shift). > > Thanks, > > /mjt >
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; + } break; case NAND_CMD_RESET: