On 31.10.2016 19:51, Andrew Jones wrote:
> Add some asserts to make sure phys_alloc_init is called only once
> and before any other alloc calls.
>
> Signed-off-by: Andrew Jones <drjones@xxxxxxxxxx>
> ---
> lib/alloc.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/lib/alloc.c b/lib/alloc.c
> index e1d7b8a380ff..a83c8c5db850 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -42,12 +42,13 @@ void phys_alloc_show(void)
>
> void phys_alloc_init(phys_addr_t base_addr, phys_addr_t size)
> {
> - spin_lock(&lock);
> + /* we should only be called once by one processor */
> + assert(!top);
> + assert(size);
If you remove the spinlock, isn't there a potential race between the
assert(!top) and the "top = ..." below? E.g. if two CPUs enter this
function at the same time, both could check and passs assert(!top)
independently, and then do the "top = ..." in parallel?
> base = base_addr;
> top = base + size;
> align_min = DEFAULT_MINIMUM_ALIGNMENT;
> - nr_regions = 0;
> - spin_unlock(&lock);
> }
>
> void phys_alloc_set_minimum_alignment(phys_addr_t align)
> @@ -63,14 +64,14 @@ static phys_addr_t phys_alloc_aligned_safe(phys_addr_t size,
> {
> static bool warned = false;
> phys_addr_t addr, size_orig = size;
> - u64 top_safe;
> + u64 top_safe = top;
>
> - spin_lock(&lock);
> + if (safe && sizeof(long) == 4)
> + top_safe = MIN(top, 1ULL << 32);
>
> - top_safe = top;
> + assert(base < top_safe);
>
> - if (safe && sizeof(long) == 4)
> - top_safe = MIN(top_safe, 1ULL << 32);
> + spin_lock(&lock);
>
> align = MAX(align, align_min);
This hunk rather looks like a code re-arrangement to me which is not
directly related to the patch description - maybe you should put this
into a separate patch?
Thomas
Attachment:
signature.asc
Description: OpenPGP digital signature
