Re: [PATCH] accel/qaic: initialize ret variable to 0

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;







[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux