Re: [PATCH v5 3/3] soc: qcom: rpmh-rsc: Output debug information from RSC

[Konrad Dybcio]

On 10/5/26 11:11 AM, Maulik Shah wrote:
> From: "Raju P.L.S.S.S.N" <rplsssn@xxxxxxxxxxxxxx>
> 
> When an RPMh transfer times out there is no visibility into which
> TCS was stuck, what commands it was carrying, or whether the
> completion IRQ was pending at the GIC. Add rpmh_rsc_debug() to
> capture this state at timeout:
> 
> - Iterates all in-use TCSes and dumps per-TCS control register
>   state (controller status, AMC mode, IRQ status) and per-command
>   register state (address, data, message ID, completion status).
>   The accelerator type (ARC/VRM/BCM) is decoded from the address
>   using cmd_db_hw_type_str().

I believe all of these are valid concerns from an LLM (4 maybe
less verbatim but the point stands):

1. The diagnosis logic is wrong in several cases.
else if ((busy && !gic_irq_sts) || !aoss_irq_sts)  -> "AOSS did not respond"
else if (gic_irq_sts)                              -> "IRQ pending at GIC but not handled"
- If the IRQ is pending at the GIC but aoss_irq_sts is false, it blames AOSS instead of reporting the pending IRQ.
- If busy == 0, for example because the handler reclaimed the TCS just after the timeout, aoss_irq_sts stays false. It then blames AOSS with nothing to base that on.
- If the RSC IRQ status is DONE but the GIC shows NOT PENDING, no message is printed.
- A clearer order is:
- GIC pending: handler delayed or Linux lockup.
- Otherwise, RSC IRQ status not set: AOSS didn't respond.
- Otherwise: IRQ asserted in the RSC but not seen at the GIC.

 2. Unlocked access to other TCSes' requests.
- rpmh_rsc_debug() walks tcs_in_use and get_req_from_tcs() with no drv->lock.
- req->state and req->wait_for_compl are then dereferenced.
- The request for another TCS lives on some other caller's stack, such as DEFINE_RPMH_MSG_ONSTACK. If tcs_tx_done() completes it while we dump, that is a possible use-after-return.
- Taking the lock with spin_trylock_irqsave() would avoid blocking in a lockup scenario. Alternatively, snapshot only the registers and don't dereference req.

 3. The cover letter example contradicts itself.
- It shows irq-sts:WAITING, a command with +resp-received, "Completion is done", and then "Accelerator(s) at AOSS did not respond".
- "Completion is done" at timeout means we raced with completion. A command that already received a response contradicts the AOSS message.
- This looks like the real consequence of issue 1.

 4. Uninitialized gic_irq_sts. It's only read when ret == 0, so it's safe today. Initialize it anyway to avoid -Wmaybe-uninitialized noise.

 5. Early return when irq_get_irq_data() returns NULL. The completion state is then never printed, even though it's useful on its own.

 6. TCS dump when req == NULL. print_tcs_info() jumps to print_tcs_data and prints CMD= lines with no TCS= header, so you can't tell which TCS they belong to. Print a header in that case too.

 8. Include order. <linux/irq.h> is inserted between interrupt.h and io.h. It should come after iopoll.h.

 9. drv->irq assignment. It's set after devm_request_irq(). That works because clients are populated later, but setting it before requesting the IRQ is more robust.

Konrad