From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: firmware: qcom: Add a generic PAS service
Date: Fri, 13 Mar 2026 14:26:31 +1000
Message-ID: <review-patch2-20260312062756.694390-3-sumit.garg@kernel.org>
In-Reply-To: <20260312062756.694390-3-sumit.garg@kernel.org>
References: <20260312062756.694390-1-sumit.garg@kernel.org> <20260312062756.694390-3-sumit.garg@kernel.org>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the core abstraction layer. Several concerns:

**Global singleton with weak synchronization.** `ops_ptr` is a bare global =
pointer (`qcom_pas.c:13`):
```c
struct qcom_pas_ops *ops_ptr;
```
While `smp_load_acquire`/`smp_store_release` pairs are used, there's no pro=
tection against concurrent use during unregister. If `qcom_pas_ops_unregist=
er()` is called while a client is mid-call through `ops_ptr->init_image(...=
)`, this is a use-after-free. There's no refcounting or RCU protection.

**No `-EPROBE_DEFER` return from non-`is_available` functions.** Functions =
like `qcom_pas_init_image()` return `-ENODEV` when `ops_ptr` is NULL, but t=
he client likely already checked `qcom_pas_is_available()`. Still, a TOCTOU=
 gap exists between checking availability and calling the function.

**`qcom_pas_context` exposed in public header.** The `qcom_pas_context` str=
uct at `include/linux/firmware/qcom/qcom_pas.h:12-21` exposes internal fiel=
ds like `ptr`, `phys`, `use_tzmem` that are backend-specific. This should i=
deally be opaque to consumers.

**`get_rsc_table` ops return type is `void *` but the public API returns `s=
truct resource_table *`.** In `qcom_pas.h:36`:
```c
void *(*get_rsc_table)(struct device *dev, ...);
```
But `qcom_pas_get_rsc_table()` in `qcom_pas.c:149` returns `struct resource=
_table *`. This implicit cast works but is sloppy =E2=80=94 the ops callbac=
k should return `struct resource_table *` for type safety.

**`pas_id` parameter inconsistency.** Some functions take `pas_id` as a par=
ameter (e.g., `qcom_pas_init_image`, `qcom_pas_shutdown`) while others get =
it from the context (e.g., `qcom_pas_get_rsc_table`, `qcom_pas_prepare_and_=
auth_reset`). This is inherited from the old API but worth noting as confus=
ing.

---
Generated by Claude Code Patch Reviewer