* [bug report] drm/xe/psmi: Add debugfs interface for PSMI
@ 2025-08-26 7:51 Dan Carpenter
2025-09-22 20:52 ` Lucas De Marchi
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2025-08-26 7:51 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: intel-xe
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] drm/xe/psmi: Add debugfs interface for PSMI
2025-08-26 7:51 [bug report] drm/xe/psmi: Add debugfs interface for PSMI Dan Carpenter
@ 2025-09-22 20:52 ` Lucas De Marchi
2025-09-23 5:39 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Lucas De Marchi @ 2025-09-22 20:52 UTC (permalink / raw)
To: Dan Carpenter; +Cc: intel-xe
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] drm/xe/psmi: Add debugfs interface for PSMI
2025-09-22 20:52 ` Lucas De Marchi
@ 2025-09-23 5:39 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2025-09-23 5:39 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: intel-xe
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-09-23 5:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-26 7:51 [bug report] drm/xe/psmi: Add debugfs interface for PSMI Dan Carpenter
2025-09-22 20:52 ` Lucas De Marchi
2025-09-23 5:39 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox