Re: [PATCH] arm64: dts: qcom: kaanapali: fix traceNoC probe issue

[sashiko-bot@kernel.org]

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

New issues:
- [High] Switching traceNoC to the platform itnoc driver breaks trace enablement for connected sources.

Pre-existing issues:
- [High] Probe error path unconditionally invokes PM runtime suspend, causing a NULL pointer dereference.
- [Medium] Driver removal and unbind paths cause a clock enable count underflow warning.
--

commit 152b245041f20740bb96bedaa8fe0632753867d1
Author: Jie Gan <jie.gan@oss.qualcomm.com>

arm64: dts: qcom: kaanapali: fix traceNoC probe issue

This commit fixes probing of the traceNoC device by switching from the AMBA
bus to the platform itnoc driver.

> diff --git a/arch/arm64/boot/dts/qcom/kaanapali.dtsi b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> index 7aa9653bd456e..84561b5faf811 100644
> --- a/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
> @@ -5003,12 +5003,12 @@ tpdm_pcie_rscc_out: endpoint {
>  			};
>  		};
>  
> -		tn@111b8000 {
> -			compatible = "qcom,coresight-tnoc", "arm,primecell";
> +		itnoc@111b8000 {
> +			compatible = "qcom,coresight-itnoc";

[Severity: High]
This is a pre-existing issue, but does switching to the platform driver expose
a NULL pointer dereference if probing fails?

If _tnoc_probe() fails in itnoc_probe(), the code unconditionally calls
pm_runtime_put() before drvdata is fully initialized. This triggers:

drivers/hwtracing/coresight/coresight-tnoc.c:itnoc_runtime_suspend() {
    struct trace_noc_drvdata *drvdata = dev_get_drvdata(dev);

    clk_disable_unprepare(drvdata->pclk);
}

Could this dereference a NULL drvdata during the error path?

[Severity: Medium]
This isn't a bug introduced by this patch, but does using the itnoc driver
lead to a clock enable count underflow on unbind?

The clock is fetched with managed enablement in _tnoc_probe():

    ret = coresight_get_enable_clocks(dev, &drvdata->pclk, NULL);

This registers a devres cleanup action. However, the driver also manages the
clock manually:

drivers/hwtracing/coresight/coresight-tnoc.c:itnoc_runtime_suspend() {
    clk_disable_unprepare(drvdata->pclk);
}

If the driver is unbound while in a runtime-suspended state, will the devres
framework attempt to disable the already-disabled clock?

[Severity: High]
Will changing the compatible string without "arm,primecell" break trace
enablement for connected sources like TPDM?

When probed as a platform device, the driver initializes atid to unsupported:

drivers/hwtracing/coresight/coresight-tnoc.c:trace_noc_init_default_data() {
    if (!dev_is_amba(drvdata->dev)) {
        drvdata->atid = -EOPNOTSUPP;
        return 0;
    }
}

During trace enablement, coresight_path_assign_trace_id() could fail because
-EOPNOTSUPP is evaluated as an invalid trace ID:

drivers/hwtracing/coresight/coresight-core.c:coresight_path_assign_trace_id() {
    if (!IS_VALID_CS_TRACE_ID(trace_id))
        return -EINVAL;
}

Does this immediately abort trace enablement for any source relying on this
traceNoC?

>  			reg = <0x0 0x111b8000 0x0 0x4200>;
>  
>  			clocks = <&aoss_qmp>;
> -			clock-names = "apb_pclk";
> +			clock-names = "apb";

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-fix-tracenoc-probe-issue-v1-1-bcc785198fc5@oss.qualcomm.com?part=1