Re: [Qemu-devel] [PATCH v4 03/14] nand: Switch to byte-based block access

2016-05-03 Thread Kevin Wolf 
Am 02.05.2016 um 23:09 hat Eric Blake geschrieben:
> On 05/02/2016 09:35 AM, Kevin Wolf wrote:
> > Am 29.04.2016 um 22:08 hat Eric Blake geschrieben:
> >> Sector-based blk_write() should die; switch to byte-based
> >> blk_pwrite() instead.  Likewise for blk_read().
> >>
> >> This file is doing some complex computations to map various
> >> flash page sizes (256, 512, and 2048) atop generic uses of
> >> 512-byte sector operations.  Perhaps someone will want to tidy
> >> up the file for fewer gymnastics in managing addresses and
> >> offsets, and less wasteful visits of 256-byte pages, but it
> >> was out of scope for this series, where I just went with the
> >> mechanical conversion.
> >>
> >> Signed-off-by: Eric Blake 
> >> ---
> >>  hw/block/nand.c | 36 +++-
> >>  1 file changed, 23 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/block/nand.c b/hw/block/nand.c
> >> index 29c6596..2703ff4 100644
> >> --- a/hw/block/nand.c
> 
> >>  } else {
> >>  off = PAGE_START(s->addr) + (s->addr & PAGE_MASK) + s->offset;
> >>  sector = off >> 9;
> >>  soff = off & 0x1ff;
> >> -if (blk_read(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
> >> +if (blk_pread(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> >> + (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {
> 
> Botched indentation here, too.
> 
> >>  printf("%s: read error in sector %" PRIu64 "\n", __func__, 
> >> sector);
> >>  return;
> >>  }
> >>
> >>  mem_and(iobuf + soff, s->io, s->iolen);
> >>
> >> -if (blk_write(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
> >> +if (blk_write(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> >> +  (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS, 0) < 0) {
> > 
> > You forgot to actually change which function is called here.
> 
> I _did_ warn that some of my patches were not compile tested (wonder why
> I forgot to warn on this one, when I did warn on 4/14).  Of course,
> patch 14/14 would have caused a compile error if I had been compiling
> this. So, what exactly do I need to start compiling these files, and
> ensure I'm not doing dead code edits?

I just did a full compile (i.e. no --target-list), that covered it. This
is the only thing I saw, though maybe I missed a warning because somehow
-Werror didn't seem to be in effect. I did the test build with clang,
maybe configure behaves different there.

Kevin


pgpU8Pjr5C7fp.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH v4 03/14] nand: Switch to byte-based block access

2016-05-02 Thread Eric Blake 
On 05/02/2016 09:35 AM, Kevin Wolf wrote:
> Am 29.04.2016 um 22:08 hat Eric Blake geschrieben:
>> Sector-based blk_write() should die; switch to byte-based
>> blk_pwrite() instead.  Likewise for blk_read().
>>
>> This file is doing some complex computations to map various
>> flash page sizes (256, 512, and 2048) atop generic uses of
>> 512-byte sector operations.  Perhaps someone will want to tidy
>> up the file for fewer gymnastics in managing addresses and
>> offsets, and less wasteful visits of 256-byte pages, but it
>> was out of scope for this series, where I just went with the
>> mechanical conversion.
>>
>> Signed-off-by: Eric Blake 
>> ---
>>  hw/block/nand.c | 36 +++-
>>  1 file changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/block/nand.c b/hw/block/nand.c
>> index 29c6596..2703ff4 100644
>> --- a/hw/block/nand.c

>>  } else {
>>  off = PAGE_START(s->addr) + (s->addr & PAGE_MASK) + s->offset;
>>  sector = off >> 9;
>>  soff = off & 0x1ff;
>> -if (blk_read(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
>> +if (blk_pread(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
>> + (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {

Botched indentation here, too.

>>  printf("%s: read error in sector %" PRIu64 "\n", __func__, 
>> sector);
>>  return;
>>  }
>>
>>  mem_and(iobuf + soff, s->io, s->iolen);
>>
>> -if (blk_write(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
>> +if (blk_write(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
>> +  (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS, 0) < 0) {
> 
> You forgot to actually change which function is called here.

I _did_ warn that some of my patches were not compile tested (wonder why
I forgot to warn on this one, when I did warn on 4/14).  Of course,
patch 14/14 would have caused a compile error if I had been compiling
this. So, what exactly do I need to start compiling these files, and
ensure I'm not doing dead code edits?

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 03/14] nand: Switch to byte-based block access

2016-05-02 Thread Kevin Wolf 
Am 29.04.2016 um 22:08 hat Eric Blake geschrieben:
> Sector-based blk_write() should die; switch to byte-based
> blk_pwrite() instead.  Likewise for blk_read().
> 
> This file is doing some complex computations to map various
> flash page sizes (256, 512, and 2048) atop generic uses of
> 512-byte sector operations.  Perhaps someone will want to tidy
> up the file for fewer gymnastics in managing addresses and
> offsets, and less wasteful visits of 256-byte pages, but it
> was out of scope for this series, where I just went with the
> mechanical conversion.
> 
> Signed-off-by: Eric Blake 
> ---
>  hw/block/nand.c | 36 +++-
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/block/nand.c b/hw/block/nand.c
> index 29c6596..2703ff4 100644
> --- a/hw/block/nand.c
> +++ b/hw/block/nand.c
> @@ -663,7 +663,8 @@ static void glue(nand_blk_write_, 
> PAGE_SIZE)(NANDFlashState *s)
>  sector = SECTOR(s->addr);
>  off = (s->addr & PAGE_MASK) + s->offset;
>  soff = SECTOR_OFFSET(s->addr);
> -if (blk_read(s->blk, sector, iobuf, PAGE_SECTORS) < 0) {
> +if (blk_pread(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> +  PAGE_SECTORS << BDRV_SECTOR_BITS) < 0) {
>  printf("%s: read error in sector %" PRIu64 "\n", __func__, 
> sector);
>  return;
>  }
> @@ -675,21 +676,24 @@ static void glue(nand_blk_write_, 
> PAGE_SIZE)(NANDFlashState *s)
>  MIN(OOB_SIZE, off + s->iolen - PAGE_SIZE));
>  }
> 
> -if (blk_write(s->blk, sector, iobuf, PAGE_SECTORS) < 0) {
> +if (blk_pwrite(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> +   PAGE_SECTORS << BDRV_SECTOR_BITS, 0) < 0) {
>  printf("%s: write error in sector %" PRIu64 "\n", __func__, 
> sector);
>  }
>  } else {
>  off = PAGE_START(s->addr) + (s->addr & PAGE_MASK) + s->offset;
>  sector = off >> 9;
>  soff = off & 0x1ff;
> -if (blk_read(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
> +if (blk_pread(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> + (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {
>  printf("%s: read error in sector %" PRIu64 "\n", __func__, 
> sector);
>  return;
>  }
> 
>  mem_and(iobuf + soff, s->io, s->iolen);
> 
> -if (blk_write(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
> +if (blk_write(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
> +  (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS, 0) < 0) {

You forgot to actually change which function is called here.

Kevin

[Qemu-devel] [PATCH v4 03/14] nand: Switch to byte-based block access

2016-04-29 Thread Eric Blake 
Sector-based blk_write() should die; switch to byte-based
blk_pwrite() instead.  Likewise for blk_read().

This file is doing some complex computations to map various
flash page sizes (256, 512, and 2048) atop generic uses of
512-byte sector operations.  Perhaps someone will want to tidy
up the file for fewer gymnastics in managing addresses and
offsets, and less wasteful visits of 256-byte pages, but it
was out of scope for this series, where I just went with the
mechanical conversion.

Signed-off-by: Eric Blake 
---
 hw/block/nand.c | 36 +++-
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 29c6596..2703ff4 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -663,7 +663,8 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState 
*s)
 sector = SECTOR(s->addr);
 off = (s->addr & PAGE_MASK) + s->offset;
 soff = SECTOR_OFFSET(s->addr);
-if (blk_read(s->blk, sector, iobuf, PAGE_SECTORS) < 0) {
+if (blk_pread(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
+  PAGE_SECTORS << BDRV_SECTOR_BITS) < 0) {
 printf("%s: read error in sector %" PRIu64 "\n", __func__, sector);
 return;
 }
@@ -675,21 +676,24 @@ static void glue(nand_blk_write_, 
PAGE_SIZE)(NANDFlashState *s)
 MIN(OOB_SIZE, off + s->iolen - PAGE_SIZE));
 }

-if (blk_write(s->blk, sector, iobuf, PAGE_SECTORS) < 0) {
+if (blk_pwrite(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
+   PAGE_SECTORS << BDRV_SECTOR_BITS, 0) < 0) {
 printf("%s: write error in sector %" PRIu64 "\n", __func__, 
sector);
 }
 } else {
 off = PAGE_START(s->addr) + (s->addr & PAGE_MASK) + s->offset;
 sector = off >> 9;
 soff = off & 0x1ff;
-if (blk_read(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
+if (blk_pread(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
+ (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {
 printf("%s: read error in sector %" PRIu64 "\n", __func__, sector);
 return;
 }

 mem_and(iobuf + soff, s->io, s->iolen);

-if (blk_write(s->blk, sector, iobuf, PAGE_SECTORS + 2) < 0) {
+if (blk_write(s->blk, sector << BDRV_SECTOR_BITS, iobuf,
+  (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS, 0) < 0) {
 printf("%s: write error in sector %" PRIu64 "\n", __func__, 
sector);
 }
 }
@@ -716,17 +720,20 @@ static void glue(nand_blk_erase_, 
PAGE_SIZE)(NANDFlashState *s)
 i = SECTOR(addr);
 page = SECTOR(addr + (1 << (ADDR_SHIFT + s->erase_shift)));
 for (; i < page; i ++)
-if (blk_write(s->blk, i, iobuf, 1) < 0) {
+if (blk_pwrite(s->blk, i << BDRV_SECTOR_BITS, iobuf,
+   BDRV_SECTOR_SIZE, 0) < 0) {
 printf("%s: write error in sector %" PRIu64 "\n", __func__, i);
 }
 } else {
 addr = PAGE_START(addr);
 page = addr >> 9;
-if (blk_read(s->blk, page, iobuf, 1) < 0) {
+if (blk_pread(s->blk, page << BDRV_SECTOR_BITS, iobuf,
+  BDRV_SECTOR_SIZE) < 0) {
 printf("%s: read error in sector %" PRIu64 "\n", __func__, page);
 }
 memset(iobuf + (addr & 0x1ff), 0xff, (~addr & 0x1ff) + 1);
-if (blk_write(s->blk, page, iobuf, 1) < 0) {
+if (blk_pwrite(s->blk, page << BDRV_SECTOR_BITS, iobuf,
+   BDRV_SECTOR_SIZE, 0) < 0) {
 printf("%s: write error in sector %" PRIu64 "\n", __func__, page);
 }

@@ -734,18 +741,20 @@ static void glue(nand_blk_erase_, 
PAGE_SIZE)(NANDFlashState *s)
 i = (addr & ~0x1ff) + 0x200;
 for (addr += ((PAGE_SIZE + OOB_SIZE) << s->erase_shift) - 0x200;
 i < addr; i += 0x200) {
-if (blk_write(s->blk, i >> 9, iobuf, 1) < 0) {
+if (blk_pwrite(s->blk, i, iobuf, BDRV_SECTOR_SIZE, 0) < 0) {
 printf("%s: write error in sector %" PRIu64 "\n",
__func__, i >> 9);
 }
 }

 page = i >> 9;
-if (blk_read(s->blk, page, iobuf, 1) < 0) {
+if (blk_pread(s->blk, page << BDRV_SECTOR_BITS, iobuf,
+  BDRV_SECTOR_SIZE) < 0) {
 printf("%s: read error in sector %" PRIu64 "\n", __func__, page);
 }
 memset(iobuf, 0xff, ((addr - 1) & 0x1ff) + 1);
-if (blk_write(s->blk, page, iobuf, 1) < 0) {
+if (blk_pwrite(s->blk, page << BDRV_SECTOR_BITS, iobuf,
+   BDRV_SECTOR_SIZE, 0) < 0) {
 printf("%s: write error in sector %" PRIu64 "\n", __func__, page);
 }
 }
@@ -760,7 +769,8 @@ static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState 
*s,

 if (s->blk) {
 if (s->mem_oob) {
-