Re: [PATCH v3 1/2] powerpc/mm: Cleanup memory block size probing

[Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>]

On 7/29/23 3:25 AM, Reza Arbab wrote:
> On Fri, Jul 28, 2023 at 04:05:55PM +0530, Aneesh Kumar K.V wrote:
>> --- a/arch/powerpc/mm/init_64.c
>> +++ b/arch/powerpc/mm/init_64.c
> [snip]
>> +    /*
>> +     * "ibm,coherent-device-memory with linux,usable-memory = 0
>> +     * Force 256MiB block size. Work around for GPUs on P9 PowerNV
>> +     * linux,usable-memory == 0 implies driver managed memory and
>> +     * we can't use large memory block size due to hotplug/unplug
>> +     * limitations.
>> +     */
>> +    compatible = of_get_flat_dt_prop(node, "compatible", NULL);
>> +    if (compatible && !strcmp(compatible, "ibm,coherent-device-memory")) {
>> +        int len = 0;
>> +        const __be32 *usm;
>> +
>> +        usm = of_get_flat_dt_prop(node, "linux,drconf-usable-memory", &len);
> 
> I think this should be "linux,usable-memory".
> 
>> +        if (usm && !len) {
>> +            *block_size = SZ_256M;
>> +            return 1;
>> +        }
> 
> This isn't quite right. The criteria is not that the property itself has no registers, it's that the base/size combo has size zero.
> 
> If you fold in the patch appended to the end of this mail, things worked for me.
> 
>> +    }
>> +
>> +    reg = of_get_flat_dt_prop(node, "reg", &l);
>> +    endp = reg + (l / sizeof(__be32));
>> +
>> +    while ((endp - reg) >= (dt_root_addr_cells + dt_root_size_cells)) {
>> +        u64 base, size;
>> +
>> +        base = dt_mem_next_cell(dt_root_addr_cells, &reg);
>> +        size = dt_mem_next_cell(dt_root_size_cells, &reg);
>> +
>> +        if (size == 0)
>> +            continue;
>> +
>> +        update_memory_block_size(block_size, size);
>> +    }
>> +    /* continue looking for other memory device types */
>> +    return 0;
>> +}
>> +
>> +/*
>> + * start with 1G memory block size. Early init will
>> + * fix this with correct value.
>> + */
>> +unsigned long memory_block_size __ro_after_init = 1UL << 30;
> 
> Could use SZ_1G here.
> 
> With the following fixup, I got 256MiB blocks on a system with
> "ibm,coherent-device-memory" nodes.
> 
> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
> index dbed37d6cffb..1ac58e72a885 100644
> --- a/arch/powerpc/mm/init_64.c
> +++ b/arch/powerpc/mm/init_64.c
> @@ -487,7 +487,6 @@ static int __init probe_memory_block_size(unsigned long node, const char *uname,
>                        depth, void *data)
>  {
>      const char *type;
> -    const char *compatible;
>      unsigned long *block_size = (unsigned long *)data;
>      const __be32 *reg, *endp;
>      int l;
> @@ -532,38 +531,38 @@ static int __init probe_memory_block_size(unsigned long node, const char *uname,
>      if (type == NULL || strcmp(type, "memory") != 0)
>          return 0;
>  
> -    /*
> -     * "ibm,coherent-device-memory with linux,usable-memory = 0
> -     * Force 256MiB block size. Work around for GPUs on P9 PowerNV
> -     * linux,usable-memory == 0 implies driver managed memory and
> -     * we can't use large memory block size due to hotplug/unplug
> -     * limitations.
> -     */
> -    compatible = of_get_flat_dt_prop(node, "compatible", NULL);
> -    if (compatible && !strcmp(compatible, "ibm,coherent-device-memory")) {
> -        int len = 0;
> -        const __be32 *usm;
> -
> -        usm = of_get_flat_dt_prop(node, "linux,drconf-usable-memory", &len);
> -        if (usm && !len) {
> -            *block_size = SZ_256M;
> -            return 1;
> -        }
> -    }
> +    reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
> +    if (!reg)
> +        reg = of_get_flat_dt_prop(node, "reg", &l);
> +    if (!reg)
> +        return 0;
>  
> -    reg = of_get_flat_dt_prop(node, "reg", &l);
>      endp = reg + (l / sizeof(__be32));
>  
>      while ((endp - reg) >= (dt_root_addr_cells + dt_root_size_cells)) {
> +        const char *compatible;
>          u64 base, size;
>  
>          base = dt_mem_next_cell(dt_root_addr_cells, &reg);
>          size = dt_mem_next_cell(dt_root_size_cells, &reg);
>  
> -        if (size == 0)
> +        if (size) {
> +            update_memory_block_size(block_size, size);
>              continue;
> +        }
>  
> -        update_memory_block_size(block_size, size);
> +        /*
> +         * ibm,coherent-device-memory with linux,usable-memory = 0
> +         * Force 256MiB block size. Work around for GPUs on P9 PowerNV
> +         * linux,usable-memory == 0 implies driver managed memory and
> +         * we can't use large memory block size due to hotplug/unplug
> +         * limitations.
> +         */
> +        compatible = of_get_flat_dt_prop(node, "compatible", NULL);
> +        if (compatible && !strcmp(compatible, "ibm,coherent-device-memory")) {
> +            *block_size = SZ_256M;
> +            return 1;
> +        }
>      }
>      /* continue looking for other memory device types */
>      return 0;

Thanks for correcting the right device tree node and testing the changes. Can I add

Co-authored-by: Reza Arbab <arbab@linux.ibm.com>

-aneesh