Re: [PATCH v7 06/20] perf: arm_pmuv3: Add method to partition the PMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Missing newlines in printk messages.
- [Low] Kernel doc for armv8pmu_partition() directly contradicts the implementation.
--

commit 6d1ad2c30d1fee65c9f04a9893ee57e6ba41fe28
Author: Colton Lewis <coltonlewis@google.com>

perf: arm_pmuv3: Add method to partition the PMU

This commit creates a module parameter reserved_host_counters to reserve
a number of PMU counters for the host. It also adds the function
armv8pmu_partition() to check the validity of the reservation and record
that a partition has happened.

> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> index 7ff3139dda893..6e447227d801f 100644
> --- a/drivers/perf/arm_pmuv3.c
> +++ b/drivers/perf/arm_pmuv3.c

[ ... ]

> +/**
> + * armv8pmu_partition() - Partition the PMU
> + * @pmu: Pointer to pmu being partitioned
> + * @host_counters: Number of host counters to reserve
> + *
> + * Partition the given PMU by taking a number of host counters to
> + * reserve and, if it is a valid reservation, recording the
> + * corresponding HPMN value in the max_guest_counters field of the PMU and
> + * clearing the guest-reserved counters from the counter mask.
> + *
> + * Return: 0 on success, -ERROR otherwise
> + */

Is this kernel doc description accurate for the current implementation?

The documentation states this function is responsible for clearing the
guest-reserved counters from the counter mask, but the implementation below
leaves the counter mask unmodified.

> +static int armv8pmu_partition(struct arm_pmu *pmu, int host_counters)
> +{
> +	u8 nr_counters;
> +	u8 hpmn;
> +
> +	if (!armv8pmu_reservation_is_valid(host_counters)) {
> +		pr_err("PMU partition reservation of %d host counters is not valid", host_counters);

Does this message need a trailing newline? 

In the kernel, pr_err() does not automatically append a newline, so
subsequent kernel log messages might be appended to the same line.

> +		return -EINVAL;
> +	}
> +
> +	nr_counters = armv8pmu_pmcr_n_read();
> +	hpmn = nr_counters - host_counters;
> +
> +	pmu->max_guest_counters = hpmn;
> +	armv8pmu_is_partitioned = true;
> +
> +	pr_info("Partitioned PMU with %d host counters -> %u guest counters", host_counters, hpmn);

Similarly, does this pr_info() call also need a \n to prevent garbling the
dmesg output?

> +
> +	return 0;
> +}
> +
>  static void __armv8pmu_probe_pmu(void *info)
>  {

[ ... ]

>  	/* Add the CPU instructions counter */
>  	if (pmuv3_has_icntr())
> -		set_bit(ARMV8_PMU_INSTR_IDX, cpu_pmu->cntr_mask);
> +		set_bit(ARMV8_PMU_INSTR_IDX, cpu_pmu->hw_cntr_mask);
> +
> +	bitmap_copy(cpu_pmu->cntr_mask, cpu_pmu->hw_cntr_mask, ARMPMU_MAX_HWEVENTS);
> +
> +	if (reserved_host_counters >= 0) {
> +		if (has_host_pmu_partition_support())
> +			armv8pmu_partition(cpu_pmu, reserved_host_counters);
> +		else
> +			pr_err("PMU partition is not supported");

Might this pr_err() also be missing a trailing newline?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260504211813.1804997-1-coltonlewis@google.com?part=6