Re: [PATCH v3 1/2] dmaengine: tegra210-adma: Fix build error due to 64-by-32 division

[Mohan Kumar D <mkumard@nvidia.com>]

On 04-02-2025 23:28, Thierry Reding wrote:
> On Tue, Feb 04, 2025 at 05:18:46PM +0000, Jon Hunter wrote:
>> On 04/02/2025 17:03, Thierry Reding wrote:
>>> On Tue, Feb 04, 2025 at 10:13:09PM +0530, Mohan Kumar D wrote:
>>>> On 04-02-2025 21:06, Thierry Reding wrote:
>>>>> On Thu, Jan 16, 2025 at 09:50:32PM +0530, Mohan Kumar D wrote:
>>>>>> Kernel test robot reported the build errors on 32-bit platforms due to
>>>>>> plain 64-by-32 division. Following build erros were reported.
>>>>>>
>>>>>>       "ERROR: modpost: "__udivdi3" [drivers/dma/tegra210-adma.ko] undefined!
>>>>>>        ld: drivers/dma/tegra210-adma.o: in function `tegra_adma_probe':
>>>>>>        tegra210-adma.c:(.text+0x12cf): undefined reference to `__udivdi3'"
>>>>>>
>>>>>> This can be fixed by using lower_32_bits() for the adma address space as
>>>>>> the offset is constrained to the lower 32 bits
>>>>>>
>>>>>> Fixes: 68811c928f88 ("dmaengine: tegra210-adma: Support channel page")
>>>>>> Cc: stable@vger.kernel.org
>>>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202412250204.GCQhdKe3-lkp@intel.com/
>>>>>> Signed-off-by: Mohan Kumar D <mkumard@nvidia.com>
>>>>>> ---
>>>>>>     drivers/dma/tegra210-adma.c | 14 +++++++++++---
>>>>>>     1 file changed, 11 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
>>>>>> index 6896da8ac7ef..258220c9cb50 100644
>>>>>> --- a/drivers/dma/tegra210-adma.c
>>>>>> +++ b/drivers/dma/tegra210-adma.c
>>>>>> @@ -887,7 +887,8 @@ static int tegra_adma_probe(struct platform_device *pdev)
>>>>>>     	const struct tegra_adma_chip_data *cdata;
>>>>>>     	struct tegra_adma *tdma;
>>>>>>     	struct resource *res_page, *res_base;
>>>>>> -	int ret, i, page_no;
>>>>>> +	unsigned int page_no, page_offset;
>>>>>> +	int ret, i;
>>>>>>     	cdata = of_device_get_match_data(&pdev->dev);
>>>>>>     	if (!cdata) {
>>>>>> @@ -914,9 +915,16 @@ static int tegra_adma_probe(struct platform_device *pdev)
>>>>>>     		res_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "global");
>>>>>>     		if (res_base) {
>>>>>> -			page_no = (res_page->start - res_base->start) / cdata->ch_base_offset;
>>>>>> -			if (page_no <= 0)
>>>>>> +			if (WARN_ON(lower_32_bits(res_page->start) <=
>>>>>> +						lower_32_bits(res_base->start)))
>>>>> Don't we technically also want to check that
>>>>>
>>>>> 	res_page->start <= res_base->start
>>>>>
>>>>> because otherwise people might put in something that's completely out of
>>>>> range? I guess maybe you could argue that the DT is then just broken,
>>>>> but since we're checking anyway, might as well check for all corner
>>>>> cases.
>>>>>
>>>>> Thierry
>>>> ADMA Address range for all Tegra chip falls within 32bit range. Do you think
>>>> still we need to have this extra check which seems like redundant for now.
>>> No, you're right. If this is all within the lower 32 bit range, this
>>> should be plenty enough. It might be worth to make it a bit more
>>> explicit and store these values in variables and add a comment as to
>>> why we only need the 32 bits. That would also make the code a bit
>>> easier to read by making the lines shorter.
>>>
>>> 	// memory regions are guaranteed to be within the lower 4 GiB
>>> 	u32 base = lower_32_bits(res_base->start);
>>> 	u32 page = lower_32_bits(res_page->start);
>>>
>>> 	if (WARN_ON(page <= base))
>>> 		...
>>>
>>> etc.
>>>
>>> Hm... on the other hand. Do we know that it's always going to stay that
>>> way? What if we ever get a chip that has a very different address map?
>> You mean a DMA register space that crosses a 4GB address boundary? I would
>> hope not but maybe I should not assume that!
> Not cross the boundary, but simply be beyond that boundary. The current
> check will falsely succeed if you've got something like this:
>
> 	base: 0x00_44000000
> 	page: 0x01_45000000
>
> or:
>
> 	base: 0x01_44000000
> 	page: 0x00_45000000
>
> For both of them the page > base condition is true, but they are clearly
> not related. Of course this would only happen in the hypothetical case
> where there are multiple instances, which is not the case for ADMA, but
> for other devices this could happen.
>
> So I think it's always good to be prepared for those cases and do the
> right thing regardless.
>
>>> Maybe we can do a combination of Arnd's patch and this. In conjunction
>>> with your second patch here, this could become something along these
>>> lines:
>>>
>>> 	u64 offset, page;
>>>
>>> 	if (WARN_ON(res_page->start <= res_base->start))
>>> 		return -EINVAL;
>>>
>>> 	offset = res_page->start - res_base->start;
>>> 	page = div_u64(offset, cdata->ch_base_offset);
>>
>> We were trying to avoid the div_u64 because at some point we want to convert
>> the result to 32-bits to avoid any further 64-bit math and we really don't
>> need 64-bits for the page number.
> Well, we can always safely cast page to u32 after this, or after
> checking (in the second patch) that it's within an expected range. But
> then again, do we really need to do 64-bit divisions using these numbers
> again? As far as I can tell this is only used in
> tegra186_adma_global_page_config(), where it's multiplied by 4, and that
> should work just fine with a 64-bit variable. But it's also fine to just
> cast to whatever ch_page_no is (unsigned int). That's ultimately what
> lower_32_bits() ends up doing anyway.
Thanks!, will resend the series with the update.
>
> Thierry