On 8/15/2025 7:59 PM, Rob Clark wrote: > On Thu, Aug 14, 2025 at 11:05 PM Dan Carpenter <[email protected]> > wrote: >> >> On Thu, Aug 14, 2025 at 06:57:35AM -0700, Rob Clark wrote: >>> On Thu, Aug 14, 2025 at 12:06 AM Dan Carpenter <[email protected]> >>> wrote: >>>> >>>> On Thu, Aug 14, 2025 at 12:28:31AM +0530, Akhil P Oommen wrote: >>>>> On 8/13/2025 11:18 AM, Dan Carpenter wrote: >>>>>> On Fri, Aug 08, 2025 at 10:28:38PM +0530, Akhil P Oommen wrote: >>>>>>> On 8/7/2025 9:23 PM, Dan Carpenter wrote: >>>>>>>> Hello Akhil P Oommen, >>>>>>>> >>>>>>>> Commit b733fe7bff8b ("drm/msm/adreno: Add support for ACD") from Apr >>>>>>>> 19, 2025 (linux-next), leads to the following Smatch static checker >>>>>>>> warning: >>>>>>>> >>>>>>>> drivers/gpu/drm/msm/adreno/a6xx_gmu.c:1700 a6xx_gmu_acd_probe() >>>>>>>> error: 'opp' dereferencing possible ERR_PTR() >>>>>>>> >>>>>>>> drivers/gpu/drm/msm/adreno/a6xx_gmu.c >>>>>>>> 1668 static int a6xx_gmu_acd_probe(struct a6xx_gmu *gmu) >>>>>>>> 1669 { >>>>>>>> 1670 struct a6xx_gpu *a6xx_gpu = container_of(gmu, struct >>>>>>>> a6xx_gpu, gmu); >>>>>>>> 1671 struct a6xx_hfi_acd_table *cmd = &gmu->acd_table; >>>>>>>> 1672 struct adreno_gpu *adreno_gpu = &a6xx_gpu->base; >>>>>>>> 1673 struct msm_gpu *gpu = &adreno_gpu->base; >>>>>>>> 1674 int ret, i, cmd_idx = 0; >>>>>>>> 1675 extern bool disable_acd; >>>>>>>> 1676 >>>>>>>> 1677 /* Skip ACD probe if requested via module param */ >>>>>>>> 1678 if (disable_acd) { >>>>>>>> 1679 DRM_DEV_ERROR(gmu->dev, "Skipping GPU ACD >>>>>>>> probe\n"); >>>>>>>> 1680 return 0; >>>>>>>> 1681 } >>>>>>>> 1682 >>>>>>>> 1683 cmd->version = 1; >>>>>>>> 1684 cmd->stride = 1; >>>>>>>> 1685 cmd->enable_by_level = 0; >>>>>>>> 1686 >>>>>>>> 1687 /* Skip freq = 0 and parse acd-level for rest of the >>>>>>>> OPPs */ >>>>>>>> 1688 for (i = 1; i < gmu->nr_gpu_freqs; i++) { >>>>>>>> 1689 struct dev_pm_opp *opp; >>>>>>>> 1690 struct device_node *np; >>>>>>>> 1691 unsigned long freq; >>>>>>>> 1692 u32 val; >>>>>>>> 1693 >>>>>>>> 1694 freq = gmu->gpu_freqs[i]; >>>>>>>> 1695 opp = >>>>>>>> dev_pm_opp_find_freq_exact(&gpu->pdev->dev, freq, true); >>>>>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>>>>> No error checking. >>>>>>> >>>>>>> We are passing back a freq which we pulled out from the opp_table a few >>>>>>> lines before this. So it is unlikely that this call would fail. >>>>>>> >>>>>>> But it is okay to add a check here if that would make Smatch checker >>>>>>> happy. >>>>>>> >>>>>> >>>>>> No, no, just ignore it, if it can't fail. >>>>>> >>>>>> Or I can add dev_pm_opp_find_freq_exact() to the "no need to check" list. >>>>>> That's easy to do. >>>>> >>>>> Would that make Smatch ignore usage of "dev_pm_opp_find_freq_exact()" in >>>>> other code/drivers? If yes, we may not want that. >>>> >>>> It just wouldn't print this warning if people left off the error handling. >>>> >>>> I'm going to ignore it anyway, right? I recently had a case where I got >>>> mixed up which functions needed error handling and I ignored the wrong one. >>>> We still caught it in testing, but I'm also going through and marking >>>> which ones >>>> to ignore or not. >>> >>> drive-by comment: Would it be useful to have a comment that smatch >>> could look for in cases like this.. similar to how rust has a practice >>> of adding a comment describing unsafe blocks? It could be a useful >>> way to document "safe because: this isn't expected to fail" cases, >>> both for humans and tools. >>> >> >> I don't want to litter the code with comments silencing Smatch warnings. >> >> Adding a comment for humans would be enough. I hand review all these >> warnings so I'd see the comment. Otherwise, generally, I try to only >> send warnings once. We fix all the real bugs so all old warnings are >> false positives. > > ok, well I think we should at least add a comment here for humans
Yeah. I will add this to my list. -Akhil > > BR, > -R > >> regards, >> dan carpenter
