Re: [PATCH 2/4] perf/arm_cspmu: Improve APMT-based PMU naming

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The computed PMU identifier `id` is ignored during string formatting, resulting in incorrect PMU names. Furthermore, `apmt_node->inst_primary` (u64) is truncated when assigned to `id` (u32).
--

--- Patch [2]: [PATCH 2/4] perf/arm_cspmu: Improve APMT-based PMU naming ---
commit a5bff38f664770528aefdc9e26aafbe6dc541317
Author: Robin Murphy <robin.murphy@arm.com>

perf/arm_cspmu: Improve APMT-based PMU naming

This commit attempts to improve APMT-based PMU naming on ACPI systems. It
modifies arm_cspmu_get_name() to substitute arbitrary numbers with more
meaningful identifiers based on APMT definitions by using a switch statement
over the PMU type.

> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index e6292021f653e..b6565eda8e521 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -250,38 +250,44 @@ static const char *arm_cspmu_get_name(const struct arm_cspmu *cspmu)
[ ... ]
> +	case ACPI_APMT_NODE_TYPE_MC:
> +		id = apmt_node->id;
> +		break;
> +	case ACPI_APMT_NODE_TYPE_SMMU:
> +	case ACPI_APMT_NODE_TYPE_PCIE_ROOT:
> +		id = apmt_node->inst_primary;

[Severity: High]
Does this assignment truncate the 64-bit inst_primary value?

Looking at struct acpi_apmt_node, inst_primary is a u64, but the id variable
is defined as a u32. This might cause sysfs name collisions if the upper 32
bits are non-zero.

> +		break;
> +	case ACPI_APMT_NODE_TYPE_CACHE:
> +		id = apmt_node->inst_secondary;
> +		break;
> +	};
>  
> -	return name;
> +	return devm_kasprintf(dev, GFP_KERNEL, "%s_%s_%u", PMUNAME,
> +			      arm_cspmu_type_str[pmu_type],
> +			      apmt_node->id);

[Severity: High]
Is it intentional to use apmt_node->id here instead of the local variable id
we just computed in the switch statement above?

Since the computed id is ignored, multiple PMUs of the same type sharing
the same apmt_node->id might result in identical names being generated.
Will this cause sysfs name collisions and driver registration failures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783605641.git.robin.murphy@arm.com?part=2