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. regards, dan carpenter
