[bug report] drm/msm/adreno: Add support for ACD

[Rob Clark]

On Thu, Aug 14, 2025 at 11:05 PM Dan Carpenter <dan.carpenter at linaro.org> 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 <dan.carpenter at linaro.org> 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

BR,
-R

> regards,
> dan carpenter