Re: [bug report] drm/panthor: Call panthor_gpu_coherency_init() after PM resume()

[Boris Brezillon <boris.brezillon@collabora.com>]

On Sat, 12 Apr 2025 17:39:58 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> Hello Boris Brezillon,
> 
> Commit 7d5a3b22f5b5 ("drm/panthor: Call panthor_gpu_coherency_init()
> after PM resume()") from Apr 4, 2025 (linux-next), leads to the
> following Smatch static checker warning:
> 
> 	drivers/gpu/drm/panthor/panthor_device.c:248 panthor_device_init()
> 	warn: missing unwind goto?
> 
> drivers/gpu/drm/panthor/panthor_device.c
>     167 int panthor_device_init(struct panthor_device *ptdev)
>     168 {
>     169         u32 *dummy_page_virt;
>     170         struct resource *res;
>     171         struct page *p;
>     172         int ret;
>     173 
>     174         init_completion(&ptdev->unplug.done);
>     175         ret = drmm_mutex_init(&ptdev->base, &ptdev->unplug.lock);
>     176         if (ret)
>     177                 return ret;
>     178 
>     179         ret = drmm_mutex_init(&ptdev->base, &ptdev->pm.mmio_lock);
>     180         if (ret)
>     181                 return ret;
>     182 
>     183         atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDED);
>     184         p = alloc_page(GFP_KERNEL | __GFP_ZERO);
>     185         if (!p)
>     186                 return -ENOMEM;
>     187 
>     188         ptdev->pm.dummy_latest_flush = p;
>     189         dummy_page_virt = page_address(p);
>     190         ret = drmm_add_action_or_reset(&ptdev->base, panthor_device_free_page,
>     191                                        ptdev->pm.dummy_latest_flush);
>     192         if (ret)
>     193                 return ret;
>     194 
>     195         /*
>     196          * Set the dummy page holding the latest flush to 1. This will cause the
>     197          * flush to avoided as we know it isn't necessary if the submission
>     198          * happens while the dummy page is mapped. Zero cannot be used because
>     199          * that means 'always flush'.
>     200          */
>     201         *dummy_page_virt = 1;
>     202 
>     203         INIT_WORK(&ptdev->reset.work, panthor_device_reset_work);
>     204         ptdev->reset.wq = alloc_ordered_workqueue("panthor-reset-wq", 0);
>     205         if (!ptdev->reset.wq)
>     206                 return -ENOMEM;
>     207 
>     208         ret = drmm_add_action_or_reset(&ptdev->base, panthor_device_reset_cleanup, NULL);
>     209         if (ret)
>     210                 return ret;
>     211 
>     212         ret = panthor_clk_init(ptdev);
>     213         if (ret)
>     214                 return ret;
>     215 
>     216         ret = panthor_devfreq_init(ptdev);
>     217         if (ret)
>     218                 return ret;
>     219 
>     220         ptdev->iomem = devm_platform_get_and_ioremap_resource(to_platform_device(ptdev->base.dev),
>     221                                                               0, &res);
>     222         if (IS_ERR(ptdev->iomem))
>     223                 return PTR_ERR(ptdev->iomem);
>     224 
>     225         ptdev->phys_addr = res->start;
>     226 
>     227         ret = devm_pm_runtime_enable(ptdev->base.dev);
>     228         if (ret)
>     229                 return ret;
>     230 
>     231         ret = pm_runtime_resume_and_get(ptdev->base.dev);
>     232         if (ret)
>     233                 return ret;
>     234 
>     235         /* If PM is disabled, we need to call panthor_device_resume() manually. */
>     236         if (!IS_ENABLED(CONFIG_PM)) {
>     237                 ret = panthor_device_resume(ptdev->base.dev);
>     238                 if (ret)
>     239                         return ret;
>     240         }
>     241 
>     242         ret = panthor_gpu_init(ptdev);
>     243         if (ret)
>     244                 goto err_rpm_put;
>     245 
>     246         ret = panthor_gpu_coherency_init(ptdev);
>     247         if (ret)
> --> 248                 return ret;  
>                         ^^^^^^^^^^^
> Missing cleanup?

Thanks for the report, it should definitely be

			goto err_unplug_gpu;

Do you plan to send a patch, or should I do it?

> 
>     249 
>     250         ret = panthor_mmu_init(ptdev);
>     251         if (ret)
>     252                 goto err_unplug_gpu;
>     253 
>     254         ret = panthor_fw_init(ptdev);
>     255         if (ret)
>     256                 goto err_unplug_mmu;
>     257 
>     258         ret = panthor_sched_init(ptdev);
>     259         if (ret)
>     260                 goto err_unplug_fw;
>     261 
>     262         /* ~3 frames */
>     263         pm_runtime_set_autosuspend_delay(ptdev->base.dev, 50);
>     264         pm_runtime_use_autosuspend(ptdev->base.dev);
>     265 
>     266         ret = drm_dev_register(&ptdev->base, 0);
>     267         if (ret)
>     268                 goto err_disable_autosuspend;
>     269 
>     270         pm_runtime_put_autosuspend(ptdev->base.dev);
>     271         return 0;
>     272 
>     273 err_disable_autosuspend:
>     274         pm_runtime_dont_use_autosuspend(ptdev->base.dev);
>     275         panthor_sched_unplug(ptdev);
>     276 
>     277 err_unplug_fw:
>     278         panthor_fw_unplug(ptdev);
>     279 
>     280 err_unplug_mmu:
>     281         panthor_mmu_unplug(ptdev);
>     282 
>     283 err_unplug_gpu:
>     284         panthor_gpu_unplug(ptdev);
>     285 
>     286 err_rpm_put:
>     287         pm_runtime_put_sync_suspend(ptdev->base.dev);
>     288         return ret;
>     289 }
> 
> regards,
> dan carpenter