On 05/02/2025 11:27, Jon Hunter wrote:

On 05/02/2025 03:31, 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 div_u64() for the adma address space

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 | 15 +++++++++++----   1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c index 6896da8ac7ef..a0bd4822ed80 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; +    u64 page_no, page_offset; +    int ret, i;       cdata = of_device_get_match_data(&pdev->dev);       if (!cdata) { @@ -914,10 +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(res_page->start <= res_base->start))                   return -EINVAL; -            tdma->ch_page_no = page_no - 1;

+            page_offset = res_page->start - res_base->start; +            page_no = div_u64(page_offset, cdata->ch_base_offset);

+            if (WARN_ON(page_no == 0)) +                return -EINVAL;

Sorry to be pedantic but should this now be ...

if (WARN_ON((page_no == 0) || (page_no > UINT_MAX)))

Nevermind, patch 2/2 addresses this.

Jon

Hi Mohan,

On 05-02-25, 09:01, 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.

Patch should describe the change! Please revise subject to describe that and not fix build error... This can come in changelog and below para is apt

"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 div_u64() for the adma address space

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 | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c index 6896da8ac7ef..a0bd4822ed80 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;

• u64 page_no, page_offset;
• int ret, i;

cdata = of_device_get_match_data(&pdev->dev); if (!cdata) { @@ -914,10 +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(res_page->start <= res_base->start))
  	return -EINVAL;
  

• 	tdma->ch_page_no = page_no - 1;
  

• 	page_offset = res_page->start - res_base->start;
  

• 	page_no = div_u64(page_offset, cdata->ch_base_offset);
  

• 	if (WARN_ON(page_no == 0))
  

• 		return -EINVAL;
  

• 	tdma->ch_page_no = lower_32_bits(page_no) - 1;
  tdma->base_addr = devm_ioremap_resource(&pdev->dev, res_base);
  if (IS_ERR(tdma->base_addr))
  	return PTR_ERR(tdma->base_addr);
  

-- 2.25.1

On 05/02/2025 03:31, 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 a0bd4822ed80..801740ad8e0d 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, };

@@ -921,7 +926,7 @@ static int tegra_adma_probe(struct platform_device *pdev) page_offset = res_page->start - res_base->start; page_no = div_u64(page_offset, cdata->ch_base_offset);

• 	if (WARN_ON(page_no == 0))
  

• 	if (WARN_ON(page_no == 0 || page_no > cdata->max_page))
  

So no one should ever specify the 'page' region for Tegra210, correct? If they did then this would always fail. I don't know if it is also worth checking if someone has a 'page' region for a device that has max_page == 0?

Jon

On Wed, Feb 05, 2025 at 06:28:34PM +0530, Mohan Kumar D wrote:

On 05-02-2025 17:10, Jon Hunter wrote:

...

On 05/02/2025 03:31, 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 a0bd4822ed80..801740ad8e0d 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,   }; @@ -921,7 +926,7 @@ static int tegra_adma_probe(struct platform_device *pdev)               page_offset = res_page->start - res_base->start;               page_no = div_u64(page_offset, cdata->ch_base_offset);   -            if (WARN_ON(page_no == 0)) +            if (WARN_ON(page_no == 0 || page_no > cdata->max_page))

So no one should ever specify the 'page' region for Tegra210, correct? If they did then this would always fail. I don't know if it is also worth checking if someone has a 'page' region for a device that has max_page == 0?

Yes, DT binding specifies "page" should not be used for Tegra210 and above conditions takes care. I believe above condition should suffice.

Yes, if you get this wrong, the DT validation should already catch it. We could always make it very clear by adding an explicit check and a corresponding error message, but I'm not sure it's really worth it in this case. If we do, then maybe we should add error messages to these other cases as well.

Thierry