On 6 July 2012 20:26, Dave Martin <dave.mar...@linaro.org> wrote:
> On Fri, Jul 06, 2012 at 07:07:35PM +0100, Peter Maydell wrote:
>> On 6 July 2012 00:27, Rob Herring <robherri...@gmail.com> wrote:
>> > I would just change arm_add_memory to use phys_addr_t for the size
>> > param. This ultimately calls memblock functions which use phys_addr_t
>> > for sizes.
>>
>> So I have a patch that does this which basically works. However
>> there is a bit I'm not sure about. arm_add_memory() does this:
>>    bank->size = size & PAGE_MASK;
>>
>> in an attempt to clear the bottom bits of the size. However,
>> since PAGE_MASK is defined as:
>>  #define PAGE_SIZE               (_AC(1,UL) << PAGE_SHIFT)
>>  #define PAGE_MASK               (~(PAGE_SIZE-1))
>>
>> PAGE_MASK is a 32 bit unsigned constant and so this & will
>> clear the top 32 bits of bank->size.
>>
>> I'm really not sure what the best way to fix this is; suggestions?
>
> Maybe something like
>
>         ~(phys_addr_t)~PAGE_MASK
>
> or
>
>         ~(phys_addr_t)(PAGE_SIZE - 1)
>
> would work.

Mmm. It feels a bit unsatisfactory that an "obviously correct"
line of code like "size &= ~PAGE_MASK" could actually be subtly
wrong (seems like the kind of thing that could easily slip
through code review), so I was wondering if maybe redefining
PAGE_MASK so it didn't do the wrong thing on 64 bit types
would be better. (That's definitely way out of my depth though.)

-- PMM

_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Reply via email to