Patchwork [v1,1/5] block/nand: Factor out common code

login
register
mail settings
Submitter Peter Crosthwaite
Date June 18, 2013, 11:08 a.m.
Message ID <f65ba24ae67c04af46940c8cc67093894af97871.1371553360.git.peter.crosthwaite@xilinx.com>
Download mbox | patch
Permalink /patch/252230/
State New
Headers show

Comments

Peter Crosthwaite - June 18, 2013, 11:08 a.m.
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(-)
Michael Tokarev - June 19, 2013, 8:09 a.m.
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
Peter Crosthwaite - June 19, 2013, 8:13 a.m.
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
>
Michael Tokarev - June 19, 2013, 8:47 a.m.
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
Peter Maydell - June 19, 2013, 8:54 a.m.
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
Peter Crosthwaite - July 31, 2013, 11:49 p.m.
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
>

Patch

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: