Re: [Qemu-devel] [PATCH V2 01/10] hw/sd.c: convert wp_groups in SDState to bitfield

[Igor Mitsyanko]

On 04/11/2012 02:12 PM, Peter Maydell wrote:
> On 5 April 2012 16:48, Igor Mitsyanko<address@hidden>  wrote:
> > @@ -536,8 +541,8 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
> >
> >   static inline int sd_wp_addr(SDState *sd, uint32_t addr)
> >   {

I've just noticed that it truncates addr to 32 bits... And test_bit() 
and bitmap_new() takes int argument. Do we care about this?

> > -    return sd->wp_groups[addr>>
> > -            (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)];
> > +    return test_bit(addr>>  (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT),
> > +            sd->wp_groups);
>
> Looking at how often the expression "addr>>  (HWBLOCK_SHIFT +
> SECTOR_SHIFT + WPGROUP_SHIFT)"
> turns up in this file, I suspect that it would be helpful to have
> a function
> static uint32_t addr_to_wpnum(uint64_t addr) {
>      return addr>>  (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
> }

This implicitly limits max address to 0xFFFFFFFF << (HWBLOCK_SHIFT + 
SECTOR_SHIFT + WPGROUP_SHIFT), have you done this on purpose?

> But that should be in a separate patch, and it's optional.

OK, I like it.

> >   }
> >
> >   static void sd_lock_command(SDState *sd)
> > @@ -560,8 +565,8 @@ static void sd_lock_command(SDState *sd)
> >              sd->card_status |= LOCK_UNLOCK_FAILED;
> >              return;
> >          }
> > -        memset(sd->wp_groups, 0, sizeof(int) * (sd->size>>
> > -                        (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)));
> > +        bitmap_zero(sd->wp_groups, BITS_TO_LONGS((sd->size>>  (HWBLOCK_SHIFT +
> > +                SECTOR_SHIFT + WPGROUP_SHIFT)) + 1));
>
> This is wrong -- bitmap_zero() takes a bit count, so you don't need
> to do BITS_TO_LONGS. Also where has the + 1 come from?
Will fix it.
> Otherwise looks good.
>
> -- PMM


--
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: address@hidden