+Herbert Xu, Deva
Hello Dan,
On 07/01/20 12:53 pm, Dan Carpenter wrote:
Hello Rijo Thomas,
The patch 757cc3e9ff1d: "tee: add AMD-TEE driver" from Dec 27, 2019, leads to the following static checker warning:
drivers/tee/amdtee/core.c:447 amdtee_driver_init() warn: 'drv_data' isn't an ERR_PTR
drivers/tee/amdtee/core.c 435 static int __init amdtee_driver_init(void) 436 { 437 struct amdtee *amdtee = NULL; ^^^^^^^^^^^^^ 438 struct tee_device *teedev; 439 struct tee_shm_pool *pool = ERR_PTR(-EINVAL); ^^^^^^^^^^^^^^^^^^^^^^^^
These look like they are left overs from one error label style error handling. We're trying to enable the warning about unused assignments so it would be good to get rid of these.
Okay, I will remove unused assignments in the driver.
440 int rc; 441 442 rc = psp_check_tee_status(); 443 if (rc) 444 goto err_fail; ^^^^^^^^^^^^^ Ideally psp_check_tee_status() would print its own error warning and we could return directly here.
Sure, I can add a warning here and return directly.
445 446 drv_data = kzalloc(sizeof(*drv_data), GFP_KERNEL); 447 if (IS_ERR(drv_data)) ^^^^^^^^^^^^^^^^ This should check for NULL instead of error pointer.
Okay I shall fix this.
448 return -ENOMEM; 449 450 amdtee = kzalloc(sizeof(*amdtee), GFP_KERNEL); 451 if (IS_ERR(amdtee)) { ^^^^^^^^^^^^^^ Same.
Okay.
452 rc = -ENOMEM; 453 goto err_kfree_drv_data; 454 } 455 456 pool = amdtee_config_shm(); 457 if (IS_ERR(pool)) { 458 pr_err("shared pool configuration error\n"); 459 rc = PTR_ERR(pool); 460 goto err_kfree_amdtee; 461 } 462 463 teedev = tee_device_alloc(&amdtee_desc, NULL, pool, amdtee); 464 if (IS_ERR(teedev)) { 465 rc = PTR_ERR(teedev); 466 goto err;
Better to create a "err_free_pool" label. The tee_device_unregister() is a no-op on this path so that's fine, but it's easier to read if we have the err_free_pool label.
Okay, I will add err_free_pool label just after tee_device_unregister.
467 } 468 amdtee->teedev = teedev; 469 470 rc = tee_device_register(amdtee->teedev); 471 if (rc) 472 goto err;
"goto err;" is very vague. We are unregistering a device which was never registered which feels wrong but actually works fine...
I shall rename err to err_device_unregister.
473 474 amdtee->pool = pool; 475 476 drv_data->amdtee = amdtee; 477 478 pr_info("amd-tee driver initialization successful\n"); 479 return 0; 480 481 err: 482 tee_device_unregister(amdtee->teedev); 483 if (pool) ^^^^^^^^^ Always true.
Agree, I shall remove this check.
484 tee_shm_pool_free(pool); 485 486 err_kfree_amdtee: 487 kfree(amdtee); 488 489 err_kfree_drv_data: 490 kfree(drv_data); 491 drv_data = NULL; 492 493 err_fail: 494 pr_err("amd-tee driver initialization failed\n"); 495 return rc; 496 }
regards, dan carpenter
I shall update the driver with these fixes in cryptodev-2.6 tree.
Thanks, Rijo