On Sun, Sep 5, 2010 at 6:03 PM, Michael S. Tsirkin <m...@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 <blauwir...@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.
