Re: [PATCH v6 2/5] coresight: tnoc: Bind Aggregator TNOC on the platform bus

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Missing check for a valid trace ID before freeing it in the AMBA removal path, triggering a kernel WARNING.

Pre-existing issues:
- [High] `trace_noc_id()` returns a negative error instead of `0` when an ATID is unsupported, silently breaking trace path activation.
--

--- Patch [2]: [PATCH v6 2/5] coresight: tnoc: Bind Aggregator TNOC on the platform bus ---
commit 969e610c6fc6cafd77fce4593f5182410a622645
Author: Jie Gan <jie.gan@oss.qualcomm.com>

coresight: tnoc: Bind Aggregator TNOC on the platform bus

This commit moves the Aggregator TNOC to the platform bus since its AMBA
Component ID reads as 0x00000000, preventing normal AMBA matching. It also
renames the driver and callbacks to generic names and updates the ATID
allocation check to match the qcom,coresight-itnoc compatible string
instead of checking if the device is on the AMBA bus.

> diff --git a/drivers/hwtracing/coresight/coresight-tnoc.c b/drivers/hwtracing/coresight/coresight-tnoc.c
> index 9e8de4323d285..737cc802aefe6 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;

[Severity: High]
This is a pre-existing issue, but does assigning -EOPNOTSUPP here completely
break trace path activation for topologies relying on a downstream Aggregator
TNOC?

When queried, trace_noc_id() returns this negative error code instead of 0.
Because coresight_path_assign_trace_id() treats any non-zero return as an ID,
it evaluates IS_VALID_CS_TRACE_ID() on this negative value, which fails and
immediately aborts path initialization with -EINVAL. 

Should this driver store and return 0 instead when an ATID is unsupported, 
so that path iteration can successfully continue downstream?

>  		return 0;
>  	}

[ ... ]

> @@ -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]
This platform teardown path correctly verifies the trace ID is valid before
freeing it. Does the AMBA teardown path in trace_noc_remove() need the same
check added?

Because the ATID assignment condition was changed to rely on device tree
compatibles rather than bus types, a malformed device tree could bind an AMBA
device using the qcom,coresight-itnoc compatible string. This would result in
the ATID being set to -EOPNOTSUPP. 

During driver unbind, trace_noc_remove() would then pass this negative ID
directly into coresight_trace_id_put_system_id(), which would trigger a kernel
WARNING.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-fix-tracenoc-probe-issue-v6-0-41eb36fef8d9@oss.qualcomm.com?part=2