[PATCH v3 0/2] Tegra ADMA fixes

[PATCH v3 0/2] Tegra ADMA fixes

[Mohan Kumar D]

- Fix build error due to 64-by-32 division
- Additional check for adma max page

Mohan Kumar D (2):
  dmaengine: tegra210-adma: Fix build error due to 64-by-32 division
  dmaengine: tegra210-adma: check for adma max page

 drivers/dma/tegra210-adma.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

-- 
2.25.1

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

[Mohan Kumar D]

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)))
+				return -EINVAL;
+
+			page_offset = lower_32_bits(res_page->start) -
+						lower_32_bits(res_base->start);
+			page_no = page_offset / cdata->ch_base_offset;
+			if (page_no == 0)
 				return -EINVAL;
+
 			tdma->ch_page_no = page_no - 1;
 			tdma->base_addr = devm_ioremap_resource(&pdev->dev, res_base);
 			if (IS_ERR(tdma->base_addr))
-- 
2.25.1

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

[Jon Hunter]

On 16/01/2025 16:20, 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)))
> +				return -EINVAL;
> +
> +			page_offset = lower_32_bits(res_page->start) -
> +						lower_32_bits(res_base->start);
> +			page_no = page_offset / cdata->ch_base_offset;
> +			if (page_no == 0)
>   				return -EINVAL;
> +
>   			tdma->ch_page_no = page_no - 1;
>   			tdma->base_addr = devm_ioremap_resource(&pdev->dev, res_base);
>   			if (IS_ERR(tdma->base_addr))


Reviewed-by: Jon Hunter <jonathanh@nvidia.com>

Thanks!
Jon

-- 
nvpublic

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2634 bytes --]

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

> +				return -EINVAL;
> +
> +			page_offset = lower_32_bits(res_page->start) -
> +						lower_32_bits(res_base->start);
> +			page_no = page_offset / cdata->ch_base_offset;
> +			if (page_no == 0)
>  				return -EINVAL;
> +
>  			tdma->ch_page_no = page_no - 1;
>  			tdma->base_addr = devm_ioremap_resource(&pdev->dev, res_base);
>  			if (IS_ERR(tdma->base_addr))
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Mohan Kumar D]

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.
>
>> +				return -EINVAL;
>> +
>> +			page_offset = lower_32_bits(res_page->start) -
>> +						lower_32_bits(res_base->start);
>> +			page_no = page_offset / cdata->ch_base_offset;
>> +			if (page_no == 0)
>>   				return -EINVAL;
>> +
>>   			tdma->ch_page_no = page_no - 1;
>>   			tdma->base_addr = devm_ioremap_resource(&pdev->dev, res_base);
>>   			if (IS_ERR(tdma->base_addr))
>> -- 
>> 2.25.1
>>

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4268 bytes --]

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?

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);

	if (WARN_ON(page == 0 || page > cdata->max_page))
		return -EINVAL;

Thierry

> > 
> > > +				return -EINVAL;
> > > +
> > > +			page_offset = lower_32_bits(res_page->start) -
> > > +						lower_32_bits(res_base->start);
> > > +			page_no = page_offset / cdata->ch_base_offset;
> > > +			if (page_no == 0)
> > >   				return -EINVAL;
> > > +
> > >   			tdma->ch_page_no = page_no - 1;
> > >   			tdma->base_addr = devm_ioremap_resource(&pdev->dev, res_base);
> > >   			if (IS_ERR(tdma->base_addr))
> > > -- 
> > > 2.25.1
> > > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jon Hunter]

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!

> 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.

Jon

--
nvpublic

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5640 bytes --]

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.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Mohan Kumar D]

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

[PATCH v3 2/2] dmaengine: tegra210-adma: check for adma max page

[Mohan Kumar D]

Have additional check for max channel page during the probe
to cover if any offset overshoot happens due to wrong DT
configuration.

Fixes: 68811c928f88 ("dmaengine: tegra210-adma: Support channel page")
Cc: stable@vger.kernel.org
Signed-off-by: Mohan Kumar D <mkumard@nvidia.com>
---
 drivers/dma/tegra210-adma.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
index 258220c9cb50..393e8a8a5bc1 100644
--- a/drivers/dma/tegra210-adma.c
+++ b/drivers/dma/tegra210-adma.c
@@ -83,7 +83,9 @@ struct tegra_adma;
  * @nr_channels: Number of DMA channels available.
  * @ch_fifo_size_mask: Mask for FIFO size field.
  * @sreq_index_offset: Slave channel index offset.
+ * @max_page: Maximum ADMA Channel Page.
  * @has_outstanding_reqs: If DMA channel can have outstanding requests.
+ * @set_global_pg_config: Global page programming.
  */
 struct tegra_adma_chip_data {
 	unsigned int (*adma_get_burst_config)(unsigned int burst_size);
@@ -99,6 +101,7 @@ struct tegra_adma_chip_data {
 	unsigned int nr_channels;
 	unsigned int ch_fifo_size_mask;
 	unsigned int sreq_index_offset;
+	unsigned int max_page;
 	bool has_outstanding_reqs;
 	void (*set_global_pg_config)(struct tegra_adma *tdma);
 };
@@ -854,6 +857,7 @@ static const struct tegra_adma_chip_data tegra210_chip_data = {
 	.nr_channels		= 22,
 	.ch_fifo_size_mask	= 0xf,
 	.sreq_index_offset	= 2,
+	.max_page		= 0,
 	.has_outstanding_reqs	= false,
 	.set_global_pg_config	= NULL,
 };
@@ -871,6 +875,7 @@ static const struct tegra_adma_chip_data tegra186_chip_data = {
 	.nr_channels		= 32,
 	.ch_fifo_size_mask	= 0x1f,
 	.sreq_index_offset	= 4,
+	.max_page		= 4,
 	.has_outstanding_reqs	= true,
 	.set_global_pg_config	= tegra186_adma_global_page_config,
 };
@@ -922,7 +927,7 @@ static int tegra_adma_probe(struct platform_device *pdev)
 			page_offset = lower_32_bits(res_page->start) -
 						lower_32_bits(res_base->start);
 			page_no = page_offset / cdata->ch_base_offset;
-			if (page_no == 0)
+			if (page_no == 0 || page_no > cdata->max_page)
 				return -EINVAL;
 
 			tdma->ch_page_no = page_no - 1;
-- 
2.25.1

Re: [PATCH v3 2/2] dmaengine: tegra210-adma: check for adma max page

[Jon Hunter]

On 16/01/2025 16:20, Mohan Kumar D wrote:
> Have additional check for max channel page during the probe
> to cover if any offset overshoot happens due to wrong DT
> configuration.
> 
> Fixes: 68811c928f88 ("dmaengine: tegra210-adma: Support channel page")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mohan Kumar D <mkumard@nvidia.com>
> ---
>   drivers/dma/tegra210-adma.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
> index 258220c9cb50..393e8a8a5bc1 100644
> --- a/drivers/dma/tegra210-adma.c
> +++ b/drivers/dma/tegra210-adma.c
> @@ -83,7 +83,9 @@ struct tegra_adma;
>    * @nr_channels: Number of DMA channels available.
>    * @ch_fifo_size_mask: Mask for FIFO size field.
>    * @sreq_index_offset: Slave channel index offset.
> + * @max_page: Maximum ADMA Channel Page.
>    * @has_outstanding_reqs: If DMA channel can have outstanding requests.
> + * @set_global_pg_config: Global page programming.
>    */
>   struct tegra_adma_chip_data {
>   	unsigned int (*adma_get_burst_config)(unsigned int burst_size);
> @@ -99,6 +101,7 @@ struct tegra_adma_chip_data {
>   	unsigned int nr_channels;
>   	unsigned int ch_fifo_size_mask;
>   	unsigned int sreq_index_offset;
> +	unsigned int max_page;
>   	bool has_outstanding_reqs;
>   	void (*set_global_pg_config)(struct tegra_adma *tdma);
>   };
> @@ -854,6 +857,7 @@ static const struct tegra_adma_chip_data tegra210_chip_data = {
>   	.nr_channels		= 22,
>   	.ch_fifo_size_mask	= 0xf,
>   	.sreq_index_offset	= 2,
> +	.max_page		= 0,
>   	.has_outstanding_reqs	= false,
>   	.set_global_pg_config	= NULL,
>   };
> @@ -871,6 +875,7 @@ static const struct tegra_adma_chip_data tegra186_chip_data = {
>   	.nr_channels		= 32,
>   	.ch_fifo_size_mask	= 0x1f,
>   	.sreq_index_offset	= 4,
> +	.max_page		= 4,
>   	.has_outstanding_reqs	= true,
>   	.set_global_pg_config	= tegra186_adma_global_page_config,
>   };
> @@ -922,7 +927,7 @@ static int tegra_adma_probe(struct platform_device *pdev)
>   			page_offset = lower_32_bits(res_page->start) -
>   						lower_32_bits(res_base->start);
>   			page_no = page_offset / cdata->ch_base_offset;
> -			if (page_no == 0)
> +			if (page_no == 0 || page_no > cdata->max_page)
>   				return -EINVAL;
>   
>   			tdma->ch_page_no = page_no - 1;


Reviewed-by: Jon Hunter <jonathanh@nvidia.com>

Thanks!
Jon

-- 
nvpublic

Re: [PATCH v3 0/2] Tegra ADMA fixes

[Jakub Kicinski]

On Thu, 16 Jan 2025 21:50:31 +0530 Mohan Kumar D wrote:
> - Fix build error due to 64-by-32 division
> - Additional check for adma max page

Hi!

What's happening with this series? The buggy commit reached Linus's
tree and broke 32b builds for everybody. The patches look 2 weeks old.