Re: [PATCH 2/5] accel/qaic: Add <linux/device-id/mhi.h> to qaic_drv.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing teardown of registered drivers (Sahara, RAS, Timesync) when module initialization fails in qaic_init().
- [High] The return value of init_srcu_struct() is ignored, which can cause a kernel panic if the per-CPU memory allocation fails.
--

--- Patch [2]: [PATCH 2/5] accel/qaic: Add <linux/device-id/mhi.h> to qaic_drv.c ---
commit 659a1f299787dffbb6e5f80693c0d8c629fc6ab4
Author: Youssef Samir <youssef.abdulrahman@oss.qualcomm.com>

accel/qaic: Add <linux/device-id/mhi.h> to qaic_drv.c
    
qaic_drv.c uses mhi_device_id implicitly. Include <linux/device-id/mhi.h>.

> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index 0acf6cdd44f81..c1bb6db78d129 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -4,6 +4,7 @@
>  /* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
>  
>  #include <linux/delay.h>
> +#include <linux/device-id/mhi.h>

[Severity: High]
This is a pre-existing issue, but does qaic_init() leave dangling references if
module initialization fails?

If qaic_ssr_register() fails, the code jumps to the free_bootlog label:

drivers/accel/qaic/qaic_drv.c:qaic_init() {
	...
free_bootlog:
	qaic_bootlog_unregister();
free_mhi:
	mhi_driver_unregister(&qaic_mhi_driver);
free_pci:
	pci_unregister_driver(&qaic_pci_driver);
	return ret;
}

This error path unregisters the bootlog, MHI driver, and PCI driver, but skips
unregistering the sahara, ras, and timesync components that were initialized
earlier.

Could the MHI core eventually access this freed memory, leading to a
use-after-free panic?

[Severity: High]
This is a pre-existing issue, but can the ignored return value of
init_srcu_struct() in qaic_open() lead to a panic?

drivers/accel/qaic/qaic_drv.c:qaic_open() {
	...
	init_srcu_struct(&usr->qddev_lock);
	...
}

If it fails to allocate required internal structures (returning e.g. -ENOMEM),
the srcu_sup pointer in usr->qddev_lock is left as NULL. If the subsequent
mutex_lock_interruptible() fails, the code jumps to cleanup_usr and calls:

drivers/accel/qaic/qaic_drv.c:qaic_open() {
	...
	cleanup_srcu_struct(&usr->qddev_lock);
	...
}

Will cleanup_srcu_struct() blindly dereference the NULL srcu_sup pointer,
causing a crash?

>  #include <linux/dma-mapping.h>
>  #include <linux/idr.h>
>  #include <linux/interrupt.h>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727173326.2219191-1-youssef.abdulrahman@oss.qualcomm.com?part=2