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: Tue, 05 May 2026 08:22:43 +1000
Message-ID: <review-patch2-20260504130603.1474043-3-sumit.garg@kernel.org>
In-Reply-To: <20260504130603.1474043-3-sumit.garg@kernel.org>
References: <20260504130603.1474043-1-sumit.garg@kernel.org> <20260504130603.1474043-3-sumit.garg@kernel.org>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This introduces the core abstraction: `struct qcom_pas_ops` with function pointers for all PAS operations, a global `ops_ptr` with `smp_load_acquire`/`smp_store_release` synchronization, and a public API surface in `include/linux/firmware/qcom/qcom_pas.h`.

**Concern: race in `qcom_pas_ops_register()`:**

```c
void qcom_pas_ops_register(struct qcom_pas_ops *ops)
{
	if (!qcom_pas_is_available())
		smp_store_release(&ops_ptr, ops);
	else
		pr_err("qcom_pas: ops already registered by %s\n",
		       ops_ptr->drv_name);
}
```

This is not atomic -- two concurrent calls (e.g., SCM and TEE probing at the same time on different CPUs) could both see `!qcom_pas_is_available()` and both store. The second silently overwrites the first with no error. A `cmpxchg` would be safer:

```c
if (cmpxchg(&ops_ptr, NULL, ops) != NULL)
    pr_err(...);
```

In practice this race may be unlikely since probe order and module loading serialize things, but the current code invites a subtle ordering bug.

**Minor:** The `QCOM_PAS` Kconfig entry is `tristate` but has no `depends on` or `default` line and no user-visible prompt. This is fine as it's selected by other configs, but worth noting it can only be enabled via `select`.

---

---
Generated by Claude Code Patch Reviewer