Re: [PATCH v4] of: reserved_mem: Restructure call site for dma_contiguous_early_fixup()

[Oreoluwa Babatunde <oreoluwa.babatunde@oss.qualcomm.com>]

On 12/1/2025 10:54 PM, Oreoluwa Babatunde wrote:
> 
> 
> On 11/30/2025 11:51 PM, Marek Szyprowski wrote:
>> On 01.12.2025 07:31, Oreoluwa Babatunde wrote:
>>> On 11/28/2025 4:43 AM, Marek Szyprowski wrote:
>>>> On 26.11.2025 02:37, Ye Li wrote:
>>>>> On 8/11/2025 7:07 PM, Marek Szyprowski wrote:
>>>>>> On 06.08.2025 19:24, Oreoluwa Babatunde wrote:
>>>>>>> Restructure the call site for dma_contiguous_early_fixup() to
>>>>>>> where the reserved_mem nodes are being parsed from the DT so that
>>>>>>> dma_mmu_remap[] is populated before dma_contiguous_remap() is called.
>>>>>>>
>>>>>>> Fixes: 8a6e02d0c00e ("of: reserved_mem: Restructure how the reserved
>>>>>>> memory regions are processed")
>>>>>>> Signed-off-by: Oreoluwa Babatunde <oreoluwa.babatunde@oss.qualcomm.com>
>>>>>>> Tested-by: William Zhang <william.zhang@broadcom.com>
>>>>>> Thanks, applied to dma-mapping-fixes branch.
>>>>>>
>>>>>> Best regards
>>>>> Hi Oreoluwa,
>>>>>
>>>>> We observed this patch causing kernel boot hang on iMX6 (armv7)
>>>>> platforms if using "cma=" kernel parameter. It only happens when the
>>>>> size assigned in
>>>>> "cma=" parameter is smaller than cma default size in dts.
>>>>>
>>>>> For example, we use "cma=96M" in command line and below reserved
>>>>> memory node (160M) in dts.
>>>>>
>>>>>          reserved-memory {
>>>>>                  #address-cells = <1>;
>>>>>                  #size-cells = <1>;
>>>>>                  ranges;
>>>>>
>>>>>                  linux,cma {
>>>>>                          compatible = "shared-dma-pool";
>>>>>                          reusable;
>>>>>                          size = <0xa000000>;
>>>>>                          linux,cma-default;
>>>>>                  };
>>>>>          };
>>>>>
>>>>> The root cause is this patch moving the dma_contiguous_early_fixup
>>>>> from rmem_cma_setup to __reserved_mem_alloc_size. rmem_cma_setup can
>>>>> skip the cma reserved memory if command line has cma parameter.
>>>>> However, the __reserved_mem_alloc_size won't do it. So this leads to
>>>>> have two cma regions added to dma_mmu_remap, one from dts, the other
>>>>> from command line. But the reserved memory of memblock that only
>>>>> records the cma from command line is inconsistent with dma_mmu_remap.
>>>>> The dma_contiguous_remap clears the MMU paging for the region of
>>>>> dma_mmu_remap firstly, then create a new mapping by iotable_init. For
>>>>> the cma from dts, this causes incorrect memory mapping cleared. Then
>>>>> any allocation from memblock in iotable_init hitting to the area will
>>>>> meet MMU mapping issue.
>>>>>
>>> Hi Ye Li,
>>>
>>> Thanks for pointing this out. From what I see in the code, if "cma="
>>> kernel parameter is being used to configure the default cma region, then we
>>> should skip adding the DT defined region to dma_mmu_remap array.
>>>
>>> I will work on a fix which does this and share here when it is done.
>>
>> I wonder how to avoid adding more such checks to 
>> drivers/of/of_reserved_mem.c and making this code even more tangled and 
>> spaghetti-like... I've briefly scanned that code and it is already quite 
>> hard to follow, especially after commits 8a6e02d0c00e ("of: 
>> reserved_mem: Restructure how the reserved memory regions are 
>> processed") and 2c223f7239f3 ("of: reserved_mem: Restructure call site 
>> for dma_contiguous_early_fixup()")... I wonder how many reserved memory 
>> regions are used on real machines? Maybe instead of complicating this 
>> code even more it is enough to make this configurable via Kconfig and 
>> restore pre-8a6e02d0c00e version?
> 
> Hi Marek,
> 
> There was a change which attempted a simpler approach of increasing the
> size of the static array:
> https://lore.kernel.org/all/1650488954-26662-1-git-send-email-quic_pdaly@quicinc.com/
> 
> The comment from Rob at the time was to revive another thread which attempted
> to dynamically allocate the reserved_mem array like we are doing now.
> Dynamic allocation gives more flexibility because we only use the exact
> amount of memory that is needed. This can save some memory if that ends
> up being smaller than what is specified in MAX_RESERVED_REGIONS.
> 
> I do agree that adding another check in of_reserved_mem.c might not be the best
> in terms of code complexity, so I'm exploring other options on how to keep things
> simpler.
> 
> Regards,
> Oreoluwa
> 

Hi all,

Apologies for the delay in pushing a fix for this.
I've uploaded a fix now:
https://lore.kernel.org/all/20251210002027.1171519-1-oreoluwa.babatunde@oss.qualcomm.com/

I ended up adding some checks in the reserved_mem code to skip initializing
the default cma region when the "cma=" kernel param is being used. This seems
to be the most straight forward way of fixing this issue.

Also, this way makes sense because it allows the reserved_mem code to skip the
default cma node and not add it in the reserved_mem array since it will not be
used as a cma region eventually.

Please help me review and give some feedback.

Thanks,
Oreoluwa