[bug report] habanalabs: replace GFP_ATOMIC with GFP_KERNEL

[bug report] habanalabs: replace GFP_ATOMIC with GFP_KERNEL

[Dan Carpenter]

Hello Ofir Bitton,

The patch d5eb8373b2ce: "habanalabs: replace GFP_ATOMIC with
GFP_KERNEL" from Feb 14, 2021, leads to the following
Smatch static checker warning:

	drivers/misc/habanalabs/common/command_buffer.c:318 hl_cb_create()
	warn: sleeping in atomic context

drivers/misc/habanalabs/common/command_buffer.c
    311 		if (rc)
    312 			goto release_cb;
    313 	}
    314 
    315 	spin_lock(&mgr->cb_lock);
                          ^^^^^^^^^^^^^
    316 	rc = idr_alloc(&mgr->cb_handles, cb, 1, 0, GFP_ATOMIC);
    317 	if (rc < 0)
--> 318 		rc = idr_alloc(&mgr->cb_handles, cb, 1, 0, GFP_KERNEL);
                                                                   ^^^^^^^^^^
This falls back to GFP_KERNEL if GFP_ATOMIC fails.  I don't understand
that.  If it's at all a performance improvement to call idr_alloc() with
ATOMIC first, then the correct thing to do is to fix idr_alloc() instead
of working around it in the caller.

    319 	spin_unlock(&mgr->cb_lock);
    320 

The other warning is also idr_alloc() but holding a different spin lock.

	drivers/misc/habanalabs/common/memory.c:126 alloc_device_memory()
	warn: sleeping in atomic context

   124  
   125          spin_lock(&vm->idr_lock);
                ^^^^^^^^^^^^^^^^^^^^^^^^
   126          handle = idr_alloc(&vm->phys_pg_pack_handles, phys_pg_pack, 1, 0,
   127                                  GFP_KERNEL);
                                        ^^^^^^^^^^
   128          spin_unlock(&vm->idr_lock);
   129  
   130          if (handle < 0) {
   131                  dev_err(hdev->dev, "Failed to get handle for page\n");
   132                  rc = -EFAULT;
   133                  goto idr_err;
   134          }

regards,
dan carpenter

RE: [bug report] habanalabs: replace GFP_ATOMIC with GFP_KERNEL

[Ofir Bitton]

On Fri, Aug 13, 2021 at 3:38 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> [???? ???? ???? ???????? ????? ?????? ?- dan.carpenter@oracle.com. ??? ???? ???? ???? ?- http://aka.ms/LearnAboutSenderIdentification.]
> 
> Hello Ofir Bitton,
> 
> The patch d5eb8373b2ce: "habanalabs: replace GFP_ATOMIC with GFP_KERNEL" from Feb 14, 2021, leads to the following Smatch static checker warning:
> 
>         drivers/misc/habanalabs/common/command_buffer.c:318 hl_cb_create()
>         warn: sleeping in atomic context
> 
> drivers/misc/habanalabs/common/command_buffer.c
>     311                 if (rc)
>     312                         goto release_cb;
>     313         }
>     314
>     315         spin_lock(&mgr->cb_lock);
>                           ^^^^^^^^^^^^^
>     316         rc = idr_alloc(&mgr->cb_handles, cb, 1, 0, GFP_ATOMIC);
>     317         if (rc < 0)
> --> 318                 rc = idr_alloc(&mgr->cb_handles, cb, 1, 0, GFP_KERNEL);
>                                                                    ^^^^^^^^^^ This falls back to GFP_KERNEL if GFP_ATOMIC fails.  I don't understand that.  If it's at all a performance improvement to call idr_alloc() with ATOMIC first, then the correct thing to do is to fix idr_alloc() instead of working around it in the caller.
> 
>     319         spin_unlock(&mgr->cb_lock);
>     320
> 
> The other warning is also idr_alloc() but holding a different spin lock.
> 
>         drivers/misc/habanalabs/common/memory.c:126 alloc_device_memory()
>         warn: sleeping in atomic context
> 
>    124
>    125          spin_lock(&vm->idr_lock);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^
>    126          handle = idr_alloc(&vm->phys_pg_pack_handles, phys_pg_pack, 1, 0,
>    127                                  GFP_KERNEL);
>                                         ^^^^^^^^^^
>    128          spin_unlock(&vm->idr_lock);
>    129
>    130          if (handle < 0) {
>    131                  dev_err(hdev->dev, "Failed to get handle for page\n");
>    132                  rc = -EFAULT;
>    133                  goto idr_err;
>    134          }
>
> regards,
> dan carpenter

Thanks for noticing I will handle it.
Ofir