Re: [PATCH v4 2/5] coresight: tnoc: add AG tnoc standalone compatible to the platform driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unconditional freeing of a negative `atid` in `trace_noc_remove` triggers a WARN.
--

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

coresight: tnoc: add AG tnoc standalone compatible to the platform driver

This commit allows the Aggregator TNOC to probe as a platform device by
adding the "qcom,coresight-agtnoc" compatible and updating driver
callbacks to use generic names. It also restricts the ATID-unsupported
handling to the Interconnect TNOC ("qcom,coresight-itnoc") rather than
all non-AMBA devices.

> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
> index 9e8de4323d285..4ccd5f38b92a3 100644
> --- a/drivers/hwtracing/coresight/coresight-tnoc.c
> +++ b/drivers/hwtracing/coresight/coresight-tnoc.c
> @@ -130,7 +130,7 @@ static int trace_noc_init_default_data(struct trace_noc_drvdata *drvdata)
>  {
>  	int atid;
>  
> -	if (!dev_is_amba(drvdata->dev)) {
> +	if (of_device_is_compatible(drvdata->dev->of_node, "qcom,coresight-itnoc")) {
>  		drvdata->atid = -EOPNOTSUPP;
>  		return 0;
>  	}

[Severity: Medium]
If a malformed device tree uses compatible = "qcom,coresight-itnoc",
"arm,primecell"; the device will bind to the AMBA driver, and this new
check will assign drvdata->atid = -EOPNOTSUPP.

When the device unbinds, trace_noc_remove() will execute:

drivers/hwtracing/coresight/coresight-tnoc.c:trace_noc_remove() {
	...
	coresight_unregister(drvdata->csdev);
	coresight_trace_id_put_system_id(drvdata->atid);
}

Since trace_noc_remove() unconditionally passes drvdata->atid to
coresight_trace_id_put_system_id(), will this trigger a kernel WARN in
coresight_trace_id_free() due to the negative ID?

[ ... ]

> @@ -295,16 +295,18 @@ static int itnoc_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static void itnoc_remove(struct platform_device *pdev)
> +static void tnoc_platform_remove(struct platform_device *pdev)
>  {
>  	struct trace_noc_drvdata *drvdata = platform_get_drvdata(pdev);
>  
>  	coresight_unregister(drvdata->csdev);
>  	pm_runtime_disable(&pdev->dev);
> +	if (drvdata->atid > 0)
> +		coresight_trace_id_put_system_id(drvdata->atid);
>  }

[Severity: Medium]
Would it make sense to update trace_noc_remove() to include the same
if (drvdata->atid > 0) defensive check that was correctly added here to
tnoc_platform_remove()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-fix-tracenoc-probe-issue-v4-0-aefab449a470@oss.qualcomm.com?part=2