diff mbox

[06/15] Use range_covers_byte

Message ID AANLkTimx6qrD4aqNgfuQ8purhxafeTv+G=1RjzLpjoC6@mail.gmail.com
State New
Headers show

Commit Message

Blue Swirl Sept. 5, 2010, 3:06 p.m. UTC 
Use range_covers_byte() instead of comparisons.

This also fixes some warnings with GCC flag -Wtype-limits.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 hw/omap1.c |   21 +++++++++++++++------
 hw/sm501.c |    5 +++--
 2 files changed, 18 insertions(+), 8 deletions(-)


@@ -828,7 +829,7 @@ static void sm501_palette_write(void *opaque,
     /* TODO : consider BYTE/WORD access */
     /* TODO : consider endian */

-    assert(0 <= addr && addr < 0x400 * 3);
+    assert(range_covers_byte(0, 0x400 * 3, addr));
     *(uint32_t*)&s->dc_palette[addr] = value;
 }

Comments

Michael S. Tsirkin Sept. 5, 2010, 6:03 p.m. UTC | #1 
On Sun, Sep 05, 2010 at 03:06:07PM +0000, Blue Swirl wrote:
> Use range_covers_byte() instead of comparisons.
> 
> This also fixes some warnings with GCC flag -Wtype-limits.
> 
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>

To me (not a native english speaker)
this comment implies that there's a bugfix here. Is there?

> ---
>  hw/omap1.c |   21 +++++++++++++++------
>  hw/sm501.c |    5 +++--
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/omap1.c b/hw/omap1.c
> index 06370b6..2dd62ec 100644
> --- a/hw/omap1.c
> +++ b/hw/omap1.c
> @@ -26,6 +26,7 @@
>  /* We use pc-style serial ports.  */
>  #include "pc.h"
>  #include "blockdev.h"
> +#include "range.h"
> 
>  /* Should signal the TCMI/GPMC */
>  uint32_t omap_badwidth_read8(void *opaque, target_phys_addr_t addr)
> @@ -3669,37 +3670,45 @@ static const struct dma_irq_map omap1_dma_irq_map[] = {
>  static int omap_validate_emiff_addr(struct omap_mpu_state_s *s,
>                  target_phys_addr_t addr)
>  {
> -    return addr >= OMAP_EMIFF_BASE && addr < OMAP_EMIFF_BASE + s->sdram_size;
> +    return range_covers_byte(OMAP_EMIFF_BASE,
> +                             OMAP_EMIFF_BASE + s->sdram_size - OMAP_EMIFF_BASE,
> +                             addr);


same as

    return range_covers_byte(OMAP_EMIFF_BASE,
                             s->sdram_size,
                             addr);

>  }
> 
>  static int omap_validate_emifs_addr(struct omap_mpu_state_s *s,
>                  target_phys_addr_t addr)
>  {
> -    return addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE;
> +    return range_covers_byte(OMAP_EMIFS_BASE, OMAP_EMIFF_BASE -
> OMAP_EMIFS_BASE,
> +                             addr);
>  }
> 
>  static int omap_validate_imif_addr(struct omap_mpu_state_s *s,
>                  target_phys_addr_t addr)
>  {
> -    return addr >= OMAP_IMIF_BASE && addr < OMAP_IMIF_BASE + s->sram_size;
> +    return range_covers_byte(OMAP_IMIF_BASE,
> +                             OMAP_IMIF_BASE + s->sram_size - OMAP_IMIF_BASE,
> +                             addr);


same as 
    return range_covers_byte(OMAP_IMIF_BASE,
                             s->sram_size,
                             addr);
?

>  }
> 
>  static int omap_validate_tipb_addr(struct omap_mpu_state_s *s,
>                  target_phys_addr_t addr)
>  {
> -    return addr >= 0xfffb0000 && addr < 0xffff0000;
> +    return range_covers_byte(0xfffb0000, 0xffff0000 - 0xfffb0000, addr);
>  }
> 

repeating the constant 0xfffb0000 is a bit ugly ... give them names?

>  static int omap_validate_local_addr(struct omap_mpu_state_s *s,
>                  target_phys_addr_t addr)
>  {
> -    return addr >= OMAP_LOCALBUS_BASE && addr < OMAP_LOCALBUS_BASE + 0x1000000;
> +    return range_covers_byte(OMAP_LOCALBUS_BASE,
> +                             OMAP_LOCALBUS_BASE + 0x1000000 -
> +                             OMAP_LOCALBUS_BASE,
> +                             addr);


Same as 
    return range_covers_byte(OMAP_LOCALBUS_BASE,
                             0x1000000,
                             addr);

?

>  }
> 
>  static int omap_validate_tipb_mpui_addr(struct omap_mpu_state_s *s,
>                  target_phys_addr_t addr)
>  {
> -    return addr >= 0xe1010000 && addr < 0xe1020004;
> +    return range_covers_byte(0xe1010000, 0xe1020004 - 0xe1010000, addr);
>  }

repeating the constants is a bit ugly ... give them names?

> 
>  struct omap_mpu_state_s *omap310_mpu_init(unsigned long sdram_size,
> diff --git a/hw/sm501.c b/hw/sm501.c
> index 8e6932d..705e0a5 100644
> --- a/hw/sm501.c
> +++ b/hw/sm501.c
> @@ -29,6 +29,7 @@
>  #include "devices.h"
>  #include "sysbus.h"
>  #include "qdev-addr.h"
> +#include "range.h"
> 
>  /*
>   * Status: 2010/05/07
> @@ -814,7 +815,7 @@ static uint32_t sm501_palette_read(void *opaque,
> target_phys_addr_t addr)
>      /* TODO : consider BYTE/WORD access */
>      /* TODO : consider endian */
> 
> -    assert(0 <= addr && addr < 0x400 * 3);
> +    assert(range_covers_byte(0, 0x400 * 3, addr));
>      return *(uint32_t*)&s->dc_palette[addr];
>  }
> 
> @@ -828,7 +829,7 @@ static void sm501_palette_write(void *opaque,
>      /* TODO : consider BYTE/WORD access */
>      /* TODO : consider endian */
> 
> -    assert(0 <= addr && addr < 0x400 * 3);
> +    assert(range_covers_byte(0, 0x400 * 3, addr));
>      *(uint32_t*)&s->dc_palette[addr] = value;
>  }
> 
> -- 
> 1.6.2.4
Blue Swirl Sept. 5, 2010, 7:46 p.m. UTC | #2 
On Sun, Sep 5, 2010 at 6:03 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Sep 05, 2010 at 03:06:07PM +0000, Blue Swirl wrote:
>> Use range_covers_byte() instead of comparisons.
>>
>> This also fixes some warnings with GCC flag -Wtype-limits.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>
> To me (not a native english speaker)
> this comment implies that there's a bugfix here. Is there?

No, perhaps 'avoids warnings' would be a better description.

>
>> ---
>>  hw/omap1.c |   21 +++++++++++++++------
>>  hw/sm501.c |    5 +++--
>>  2 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/omap1.c b/hw/omap1.c
>> index 06370b6..2dd62ec 100644
>> --- a/hw/omap1.c
>> +++ b/hw/omap1.c
>> @@ -26,6 +26,7 @@
>>  /* We use pc-style serial ports.  */
>>  #include "pc.h"
>>  #include "blockdev.h"
>> +#include "range.h"
>>
>>  /* Should signal the TCMI/GPMC */
>>  uint32_t omap_badwidth_read8(void *opaque, target_phys_addr_t addr)
>> @@ -3669,37 +3670,45 @@ static const struct dma_irq_map omap1_dma_irq_map[] = {
>>  static int omap_validate_emiff_addr(struct omap_mpu_state_s *s,
>>                  target_phys_addr_t addr)
>>  {
>> -    return addr >= OMAP_EMIFF_BASE && addr < OMAP_EMIFF_BASE + s->sdram_size;
>> +    return range_covers_byte(OMAP_EMIFF_BASE,
>> +                             OMAP_EMIFF_BASE + s->sdram_size - OMAP_EMIFF_BASE,
>> +                             addr);
>
>
> same as
>
>    return range_covers_byte(OMAP_EMIFF_BASE,
>                             s->sdram_size,
>                             addr);

Ok. I made the conversion a bit too mechanically.

>
>>  }
>>
>>  static int omap_validate_emifs_addr(struct omap_mpu_state_s *s,
>>                  target_phys_addr_t addr)
>>  {
>> -    return addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE;
>> +    return range_covers_byte(OMAP_EMIFS_BASE, OMAP_EMIFF_BASE -
>> OMAP_EMIFS_BASE,
>> +                             addr);
>>  }
>>
>>  static int omap_validate_imif_addr(struct omap_mpu_state_s *s,
>>                  target_phys_addr_t addr)
>>  {
>> -    return addr >= OMAP_IMIF_BASE && addr < OMAP_IMIF_BASE + s->sram_size;
>> +    return range_covers_byte(OMAP_IMIF_BASE,
>> +                             OMAP_IMIF_BASE + s->sram_size - OMAP_IMIF_BASE,
>> +                             addr);
>
>
> same as
>    return range_covers_byte(OMAP_IMIF_BASE,
>                             s->sram_size,
>                             addr);
> ?
>
>>  }
>>
>>  static int omap_validate_tipb_addr(struct omap_mpu_state_s *s,
>>                  target_phys_addr_t addr)
>>  {
>> -    return addr >= 0xfffb0000 && addr < 0xffff0000;
>> +    return range_covers_byte(0xfffb0000, 0xffff0000 - 0xfffb0000, addr);
>>  }
>>
>
> repeating the constant 0xfffb0000 is a bit ugly ... give them names?

I agree, but I'd rather not fix all problems with this patch set.

>
>>  static int omap_validate_local_addr(struct omap_mpu_state_s *s,
>>                  target_phys_addr_t addr)
>>  {
>> -    return addr >= OMAP_LOCALBUS_BASE && addr < OMAP_LOCALBUS_BASE + 0x1000000;
>> +    return range_covers_byte(OMAP_LOCALBUS_BASE,
>> +                             OMAP_LOCALBUS_BASE + 0x1000000 -
>> +                             OMAP_LOCALBUS_BASE,
>> +                             addr);
>
>
> Same as
>    return range_covers_byte(OMAP_LOCALBUS_BASE,
>                             0x1000000,
>                             addr);
>
> ?
>
>>  }
>>
>>  static int omap_validate_tipb_mpui_addr(struct omap_mpu_state_s *s,
>>                  target_phys_addr_t addr)
>>  {
>> -    return addr >= 0xe1010000 && addr < 0xe1020004;
>> +    return range_covers_byte(0xe1010000, 0xe1020004 - 0xe1010000, addr);
>>  }
>
> repeating the constants is a bit ugly ... give them names?
>
>>
>>  struct omap_mpu_state_s *omap310_mpu_init(unsigned long sdram_size,
>> diff --git a/hw/sm501.c b/hw/sm501.c
>> index 8e6932d..705e0a5 100644
>> --- a/hw/sm501.c
>> +++ b/hw/sm501.c
>> @@ -29,6 +29,7 @@
>>  #include "devices.h"
>>  #include "sysbus.h"
>>  #include "qdev-addr.h"
>> +#include "range.h"
>>
>>  /*
>>   * Status: 2010/05/07
>> @@ -814,7 +815,7 @@ static uint32_t sm501_palette_read(void *opaque,
>> target_phys_addr_t addr)
>>      /* TODO : consider BYTE/WORD access */
>>      /* TODO : consider endian */
>>
>> -    assert(0 <= addr && addr < 0x400 * 3);
>> +    assert(range_covers_byte(0, 0x400 * 3, addr));
>>      return *(uint32_t*)&s->dc_palette[addr];
>>  }
>>
>> @@ -828,7 +829,7 @@ static void sm501_palette_write(void *opaque,
>>      /* TODO : consider BYTE/WORD access */
>>      /* TODO : consider endian */
>>
>> -    assert(0 <= addr && addr < 0x400 * 3);
>> +    assert(range_covers_byte(0, 0x400 * 3, addr));
>>      *(uint32_t*)&s->dc_palette[addr] = value;
>>  }
>>
>> --
>> 1.6.2.4
>

Thanks for the review.
diff mbox

Patch

diff --git a/hw/omap1.c b/hw/omap1.c
index 06370b6..2dd62ec 100644
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -26,6 +26,7 @@ 
 /* We use pc-style serial ports.  */
 #include "pc.h"
 #include "blockdev.h"
+#include "range.h"

 /* Should signal the TCMI/GPMC */
 uint32_t omap_badwidth_read8(void *opaque, target_phys_addr_t addr)
@@ -3669,37 +3670,45 @@  static const struct dma_irq_map omap1_dma_irq_map[] = {
 static int omap_validate_emiff_addr(struct omap_mpu_state_s *s,
                 target_phys_addr_t addr)
 {
-    return addr >= OMAP_EMIFF_BASE && addr < OMAP_EMIFF_BASE + s->sdram_size;
+    return range_covers_byte(OMAP_EMIFF_BASE,
+                             OMAP_EMIFF_BASE + s->sdram_size - OMAP_EMIFF_BASE,
+                             addr);
 }

 static int omap_validate_emifs_addr(struct omap_mpu_state_s *s,
                 target_phys_addr_t addr)
 {
-    return addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE;
+    return range_covers_byte(OMAP_EMIFS_BASE, OMAP_EMIFF_BASE -
OMAP_EMIFS_BASE,
+                             addr);
 }

 static int omap_validate_imif_addr(struct omap_mpu_state_s *s,
                 target_phys_addr_t addr)
 {
-    return addr >= OMAP_IMIF_BASE && addr < OMAP_IMIF_BASE + s->sram_size;
+    return range_covers_byte(OMAP_IMIF_BASE,
+                             OMAP_IMIF_BASE + s->sram_size - OMAP_IMIF_BASE,
+                             addr);
 }

 static int omap_validate_tipb_addr(struct omap_mpu_state_s *s,
                 target_phys_addr_t addr)
 {
-    return addr >= 0xfffb0000 && addr < 0xffff0000;
+    return range_covers_byte(0xfffb0000, 0xffff0000 - 0xfffb0000, addr);
 }

 static int omap_validate_local_addr(struct omap_mpu_state_s *s,
                 target_phys_addr_t addr)
 {
-    return addr >= OMAP_LOCALBUS_BASE && addr < OMAP_LOCALBUS_BASE + 0x1000000;
+    return range_covers_byte(OMAP_LOCALBUS_BASE,
+                             OMAP_LOCALBUS_BASE + 0x1000000 -
+                             OMAP_LOCALBUS_BASE,
+                             addr);
 }

 static int omap_validate_tipb_mpui_addr(struct omap_mpu_state_s *s,
                 target_phys_addr_t addr)
 {
-    return addr >= 0xe1010000 && addr < 0xe1020004;
+    return range_covers_byte(0xe1010000, 0xe1020004 - 0xe1010000, addr);
 }

 struct omap_mpu_state_s *omap310_mpu_init(unsigned long sdram_size,
diff --git a/hw/sm501.c b/hw/sm501.c
index 8e6932d..705e0a5 100644
--- a/hw/sm501.c
+++ b/hw/sm501.c
@@ -29,6 +29,7 @@ 
 #include "devices.h"
 #include "sysbus.h"
 #include "qdev-addr.h"
+#include "range.h"

 /*
  * Status: 2010/05/07
@@ -814,7 +815,7 @@  static uint32_t sm501_palette_read(void *opaque,
target_phys_addr_t addr)
     /* TODO : consider BYTE/WORD access */
     /* TODO : consider endian */

-    assert(0 <= addr && addr < 0x400 * 3);
+    assert(range_covers_byte(0, 0x400 * 3, addr));
     return *(uint32_t*)&s->dc_palette[addr];
 }