On 4/18/2023 2:48 PM, Nick Desaulniers wrote:
On Tue, Apr 18, 2023 at 1:46 PM Jeffrey Hugo <quic_jhugo@xxxxxxxxxxx> wrote:
On 4/18/2023 1:20 PM, Tom Rix wrote:
clang static analysis reports
drivers/accel/qaic/qaic_data.c:610:2: warning: Undefined or garbage
value returned to caller [core.uninitialized.UndefReturn]
return ret;
^~~~~~~~~~
The ret variable is only set some of the time but is always returned.
So initialize ret to 0.
This does not appear to be entirely accurate to me.
Do you know what analysis Clang is doing? Is it limited to the function
itself?
remap_pfn_range, which initializes ret, will always run at-least once.
What happens if the loop body is never executed, say if `bo->sgt->sgl` is NULL?
qaic_gem_object_mmap() doesn't get called unless a valid GEM object is
provided by userspace. For userspace to get a valid GEM object, it has
to go through qaic_create_bo_ioctl(). qaic_create_bo_ioctl() will not
return a valid GEM object unless sgt is allocated and initialized.
The loop body will execute at-least once. The if body will execute
at-least once. remap_pfn_range() will run at-least once. Therefore,
ret is always initialized.
This is how the code is intended to operate, and how it appears to be
implemented from what I see. Unless I'm missing something.
I can see how Clang might not be able to infer these things, but I don't
believe the code is broken. I feel that the commit text is unclear and
suggests that the code is infact, broken.
Userspace should not call mmap() in a critical path thus I don't see a
true performance concern here. So while my understanding of the coding
style is that redundant initialization of variables are to be avoided, I
think we can say that this is not redundant because it silences a
warning (because Clang is limited).
Feels more accurate to say that Clang cannot detect that ret will be
initialized, and we want to quiet the warning from the false positive.
Fixes: ff13be830333 ("accel/qaic: Add datapath")
Signed-off-by: Tom Rix <trix@xxxxxxxxxx>
---
drivers/accel/qaic/qaic_data.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
index c0a574cd1b35..b46a16fb3080 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -591,7 +591,7 @@ static int qaic_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struc
struct qaic_bo *bo = to_qaic_bo(obj);
unsigned long offset = 0;
struct scatterlist *sg;
- int ret;
+ int ret = 0;
if (obj->import_attach)
return -EINVAL;
