Re: [PATCH v3] dmaengine: tegra210-adma: Add error logging on failure paths

[Sheetal .]

On 03-04-2026 08:33, Frank Li wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Mar 23, 2026 at 08:38:58AM +0000, Sheetal wrote:
> > Add dev_err/dev_err_probe logging across failure paths to improve
> > debuggability of DMA errors during runtime and probe.
> >
> > Signed-off-by: Sheetal <sheetal@xxxxxxxxxx>
> > ---
> > Changes in v3:
> > - Cast page_no to (unsigned long long) for %llu to fix -Wformat
> >    warning on 32-bit builds where resource_size_t is unsigned int
> > - Remove redundant dev_err for devm_ioremap_resource failures since
> >    the API already logs errors internally.
> >
> > Changes in v2:
> > - Fix format specifier for size_t: use %zu instead of %u for
> >    desc->num_periods to resolve -Wformat warning with W=1
> >
> >   drivers/dma/tegra210-adma.c | 37 +++++++++++++++++++++++++++-------
> >   1 file changed, 30 insertions(+), 7 deletions(-)
> ...
> > @@ -1047,38 +1058,45 @@ static int tegra_adma_probe(struct platform_device *pdev)
> >        res_page = platform_get_resource_byname(pdev, IORESOURCE_MEM, "page");
> >        if (res_page) {
> >                tdma->ch_base_addr = devm_ioremap_resource(&pdev->dev, res_page);
> >                if (IS_ERR(tdma->ch_base_addr))
> >                        return PTR_ERR(tdma->ch_base_addr);
> >
> >                res_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "global");
> >                if (res_base) {
> >                        resource_size_t page_offset, page_no;
> >                        unsigned int ch_base_offset;
> >
> > -                     if (res_page->start < res_base->start)
> > +                     if (res_page->start < res_base->start) {
> > +                             dev_err(&pdev->dev, "invalid page/global resource order\n");
> >                                return -EINVAL;
>
> It is in probe function, return dev_err_probe(, -EINVAL, ...);
> check other place

ACK

> > +                     }
> > +
> >                        page_offset = res_page->start - res_base->start;
> >                        ch_base_offset = cdata->ch_base_offset;
> >                        if (!ch_base_offset)
> >                                return -EINVAL;
> >
> >                        page_no = div_u64(page_offset, ch_base_offset);
> > -                     if (!page_no || page_no > INT_MAX)
> > +                     if (!page_no || page_no > INT_MAX) {
> > +                             dev_err(&pdev->dev, "invalid page number %llu\n",
> > +                                     (unsigned long long)page_no);
> >                                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))
> >                                return PTR_ERR(tdma->base_addr);
> >                }
> >        } else {
> >                /* If no 'page' property found, then reg DT binding would be legacy */
> >                res_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >                if (res_base) {
> >                        tdma->base_addr = devm_ioremap_resource(&pdev->dev, res_base);
> >                        if (IS_ERR(tdma->base_addr))
> >                                return PTR_ERR(tdma->base_addr);
> >                } else {
> > +                     dev_err(&pdev->dev, "failed to get memory resource\n");
> >                        return -ENODEV;
> >                }
> >
> > @@ -1130,6 +1147,7 @@ static int tegra_adma_probe(struct platform_device *pdev)
> >                tdc->irq = of_irq_get(pdev->dev.of_node, i);
> >                if (tdc->irq <= 0) {
> >                        ret = tdc->irq ?: -ENXIO;
> > +                     dev_err_probe(&pdev->dev, ret, "failed to get IRQ for channel %d\n", i);
> >                        goto irq_dispose;
> >                }
> >
> > @@ -1141,12 +1159,16 @@ static int tegra_adma_probe(struct platform_device *pdev)
> >        pm_runtime_enable(&pdev->dev);
> >
> >        ret = pm_runtime_resume_and_get(&pdev->dev);
> > -     if (ret < 0)
> > +     if (ret < 0) {
> > +             dev_err_probe(&pdev->dev, ret, "runtime PM resume failed\n");
> >                goto rpm_disable;
>
> can you change to use devm_ firtly to elimiate goto first, then change to
> use
>          return dev_err_probe() pattern


Thanks for the review, Frank.

For the devm_ conversion, it doesn't seem straightforward as the goto 
cleanup paths handle multiple resources. I'd like to send that as a 
separate follow-up patch.


> Frank
>
> > +     }
> >
> >        ret = tegra_adma_init(tdma);
> > -     if (ret)
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "failed to initialize ADMA: %d\n", ret);
> >                goto rpm_put;
> > +     }
> >
> >        dma_cap_set(DMA_SLAVE, tdma->dma_dev.cap_mask);
> >        dma_cap_set(DMA_PRIVATE, tdma->dma_dev.cap_mask);
> > --
> > 2.17.1