On 11 April 2012 12:57, Igor Mitsyanko <i.mitsya...@samsung.com> wrote:
> On 04/11/2012 02:12 PM, Peter Maydell wrote:
>>
>> On 5 April 2012 16:48, Igor Mitsyanko<i.mitsya...@samsung.com> 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?
We definitely don't want to restrict the SD card image size to
32 bits, so any byte addresses must be uint64_t, and you're
correct that sd_wp_addr is wrong here.
I don't think we care that the bitmap size is limited to
possibly be 32 bits only on a 32 bit system, because there's
only one bit per wp group, and so it doesn't impose much of a
restriction on the file size.
>>> - 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?
You could argue for uint64_t return type, but we have to pass
the result to the bitmap functions anyway, so there is implicitly
a limit. I don't think it's a particularly severe one.
If you'd rather uint64_t return here I'm happy with that: I guess
that means the sd.c code isn't putting any extra limits beyond
what the bitmap code is, so it's probably better.
-- PMM
