[bug report] drm/xe/psmi: Add debugfs interface for PSMI

[bug report] drm/xe/psmi: Add debugfs interface for PSMI

[Dan Carpenter]

Hello Lucas De Marchi,

Commit aaa0c1f50a3d ("drm/xe/psmi: Add debugfs interface for PSMI")
from Aug 21, 2025 (linux-next), leads to the following Smatch static
checker warning:

	drivers/gpu/drm/xe/xe_psmi.c:93 psmi_alloc_object()
	error: 'bo' dereferencing possible ERR_PTR()

drivers/gpu/drm/xe/xe_psmi.c
    68 static struct xe_bo *psmi_alloc_object(struct xe_device *xe,
    69                                        unsigned int id, size_t bo_size)
    70 {
    71         struct xe_bo *bo = NULL;
    72         struct xe_tile *tile;
    73         int err;
    74 
    75         if (!id || !bo_size)
    76                 return NULL;

I really encourage everyone to document why functions return both error
pointers and NULL.  Here "bo_size" can never actually be zero so that's
an impossible path.  Presumably id can be zero, but what's the point of
storing a NULL in xe->psmi.capture_obj[0]?  It feels like it complicates
things...

    77 
    78         tile = &xe->tiles[id - 1];
    79 
    80         /* VRAM: Allocate GEM object for the capture buffer */
    81         bo = xe_bo_create_locked(xe, tile, NULL, bo_size,
    82                                  ttm_bo_type_kernel,
    83                                  XE_BO_FLAG_VRAM_IF_DGFX(tile) |
    84                                  XE_BO_FLAG_PINNED |
    85                                  XE_BO_FLAG_PINNED_LATE_RESTORE |
    86                                  XE_BO_FLAG_NEEDS_CPU_ACCESS);
    87 
    88         if (!IS_ERR(bo)) {
                   ^^^^^^^^^^
Better to flip these around.  Always do error handling, never success
handling.

    89                 /* Buffer written by HW, ensure stays resident */
    90                 err = xe_bo_pin(bo);
    91                 if (err)
    92                         bo = ERR_PTR(err);
                               ^^^^^^^^^^^^^^^^^
bo set to error pointer.

--> 93                 xe_bo_unlock(bo);
                                    ^^
Dead.

Don't we need to call xe_bo_put() or something to free the bo before
returning?  Something like this?

	if (IS_ERR(bo))
		return ERR_CAST(bo);

	/* Buffer written by HW, ensure stays resident */
	err = xe_bo_pin(bo);
	xe_bo_unlock(bo);
	if (err) {
		xe_bo_put(bo);
		return ERR_PTR(err);
	}

	return bo;

    94         }
    95 
    96         return bo;
    97 }

regards,
dan carpenter

Re: [bug report] drm/xe/psmi: Add debugfs interface for PSMI

[Lucas De Marchi]

On Tue, Aug 26, 2025 at 10:51:02AM +0300, Dan Carpenter wrote:
>Hello Lucas De Marchi,
>
>Commit aaa0c1f50a3d ("drm/xe/psmi: Add debugfs interface for PSMI")
>from Aug 21, 2025 (linux-next), leads to the following Smatch static
>checker warning:
>
>	drivers/gpu/drm/xe/xe_psmi.c:93 psmi_alloc_object()
>	error: 'bo' dereferencing possible ERR_PTR()
>
>drivers/gpu/drm/xe/xe_psmi.c
>    68 static struct xe_bo *psmi_alloc_object(struct xe_device *xe,
>    69                                        unsigned int id, size_t bo_size)
>    70 {
>    71         struct xe_bo *bo = NULL;
>    72         struct xe_tile *tile;
>    73         int err;
>    74
>    75         if (!id || !bo_size)
>    76                 return NULL;
>
>I really encourage everyone to document why functions return both error
>pointers and NULL.  Here "bo_size" can never actually be zero so that's
>an impossible path.  Presumably id can be zero, but what's the point of
>storing a NULL in xe->psmi.capture_obj[0]?  It feels like it complicates
>things...

neither id nor bo_size can be 0 at this point. We could simply remove
the check or replace by

	xe_assert(xe, id);
	xe_assert(xe, bo_size);

id == 0 means system memory and that's blocked by

static int psmi_debugfs_capture_region_mask_set(void *data, u64 region_mask)
{
	...
	/* SMEM is not supported (see comments at top of file) */
	if (region_mask & 0x1)
         	return -EOPNOTSUPP;
	...
}

>
>    77
>    78         tile = &xe->tiles[id - 1];
>    79
>    80         /* VRAM: Allocate GEM object for the capture buffer */
>    81         bo = xe_bo_create_locked(xe, tile, NULL, bo_size,
>    82                                  ttm_bo_type_kernel,
>    83                                  XE_BO_FLAG_VRAM_IF_DGFX(tile) |
>    84                                  XE_BO_FLAG_PINNED |
>    85                                  XE_BO_FLAG_PINNED_LATE_RESTORE |
>    86                                  XE_BO_FLAG_NEEDS_CPU_ACCESS);
>    87
>    88         if (!IS_ERR(bo)) {
>                   ^^^^^^^^^^
>Better to flip these around.  Always do error handling, never success
>handling.
>
>    89                 /* Buffer written by HW, ensure stays resident */
>    90                 err = xe_bo_pin(bo);
>    91                 if (err)
>    92                         bo = ERR_PTR(err);
>                               ^^^^^^^^^^^^^^^^^
>bo set to error pointer.
>
>--> 93                 xe_bo_unlock(bo);
>                                    ^^
>Dead.

I think this is now fixed by commit 1f1541720f65 ("drm/xe: Rework
instances of variants of xe_bo_create_locked()").

sorry for the delay replying to this

Lucas De Marchi

>
>Don't we need to call xe_bo_put() or something to free the bo before
>returning?  Something like this?
>
>	if (IS_ERR(bo))
>		return ERR_CAST(bo);
>
>	/* Buffer written by HW, ensure stays resident */
>	err = xe_bo_pin(bo);
>	xe_bo_unlock(bo);
>	if (err) {
>		xe_bo_put(bo);
>		return ERR_PTR(err);
>	}
>
>	return bo;
>
>    94         }
>    95
>    96         return bo;
>    97 }
>
>regards,
>dan carpenter

Re: [bug report] drm/xe/psmi: Add debugfs interface for PSMI

[Dan Carpenter]

On Mon, Sep 22, 2025 at 03:52:40PM -0500, Lucas De Marchi wrote:
> On Tue, Aug 26, 2025 at 10:51:02AM +0300, Dan Carpenter wrote:
> > Hello Lucas De Marchi,
> > 
> > Commit aaa0c1f50a3d ("drm/xe/psmi: Add debugfs interface for PSMI")
> > from Aug 21, 2025 (linux-next), leads to the following Smatch static
> > checker warning:
> > 
> > 	drivers/gpu/drm/xe/xe_psmi.c:93 psmi_alloc_object()
> > 	error: 'bo' dereferencing possible ERR_PTR()
> > 
> > drivers/gpu/drm/xe/xe_psmi.c
> >    68 static struct xe_bo *psmi_alloc_object(struct xe_device *xe,
> >    69                                        unsigned int id, size_t bo_size)
> >    70 {
> >    71         struct xe_bo *bo = NULL;
> >    72         struct xe_tile *tile;
> >    73         int err;
> >    74
> >    75         if (!id || !bo_size)
> >    76                 return NULL;
> > 
> > I really encourage everyone to document why functions return both error
> > pointers and NULL.  Here "bo_size" can never actually be zero so that's
> > an impossible path.  Presumably id can be zero, but what's the point of
> > storing a NULL in xe->psmi.capture_obj[0]?  It feels like it complicates
> > things...
> 
> neither id nor bo_size can be 0 at this point. We could simply remove
> the check or replace by
> 
> 	xe_assert(xe, id);
> 	xe_assert(xe, bo_size);

That works, or you could return an error pointer.  That would also be fine.

regards,
dan carpenter