Re: [PATCH 3/3] MIPS: perf: Add SB1 and SB1A performance counter support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] MIPS: perf: double hw_perf_event_destroy() call when
  validate_group() fails
- [Medium] MIPS: perf: IP7 left unmasked after irq_work_run() disables
  last SB1 event
- [Medium] MIPS: perf: mipspmu_event_init() returns -EOPNOTSUPP before
  checking attr.type
- [Low] docs: mips: sb1-pmu: heading underline too short for section
  title
--

commit 4595e65c6c56d05599f7d396b72896c6da74a174
Author: Florian Fainelli <f.fainelli@gmail.com>

MIPS: perf: Add SB1 and SB1A performance counter support

This commit adds MIPS PMU support for the four CPU-local performance
counters on SB1 and SB1A processors, along with sysfs event aliases and
documentation.

> diff --git a/Documentation/arch/mips/sb1-pmu.rst b/Documentation/arch/mips/sb1-pmu.rst
> new file mode 100644
> index 0000000000000..d589a6262c65f
> --- /dev/null
> +++ b/Documentation/arch/mips/sb1-pmu.rst
> @@ -0,0 +1,162 @@

[ ... ]

> +Counter selection and mode filtering
> +-----------------------------------

[Severity: Low]
The hyphen underline here is 35 characters for a 36-character heading,
which triggers a Sphinx "Title underline too short" warning during
htmldocs builds.

> diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c
> index 196a070349b09..765cdb6f8cae3 100644
> --- a/arch/mips/kernel/perf_event_mipsxx.c
> +++ b/arch/mips/kernel/perf_event_mipsxx.c

[ ... ]

> @@ -564,6 +627,14 @@ static void mipspmu_enable(struct pmu *pmu)
>  	write_unlock(&pmuint_rwlock);
>  #endif
>  	resume_local_counters();
> +	if (is_sb1_pmu() &&
> +	    !bitmap_empty(this_cpu_ptr(&cpu_hw_events)->used_mask,
> +			  mipspmu.num_counters) &&
> +	    !irq_percpu_is_enabled(mipspmu.irq)) {
> +		/* Count/Compare shares IP7, but SiByte uses a separate timer. */
> +		write_c0_compare(read_c0_count() - 1);
> +		enable_percpu_irq(mipspmu.irq, IRQ_TYPE_NONE);
> +	}
>  }
>  
>  /*
> @@ -579,6 +650,8 @@ static void mipspmu_enable(struct pmu *pmu)
>   */
>  static void mipspmu_disable(struct pmu *pmu)
>  {
> +	if (is_sb1_pmu() && irq_percpu_is_enabled(mipspmu.irq))
> +		disable_percpu_irq(mipspmu.irq);
>  	pause_local_counters();
>  #ifdef CONFIG_MIPS_PERF_SHARED_TC_COUNTERS
>  	write_lock(&pmuint_rwlock);

[Severity: Medium]
Can this leave IP7 unmasked in hardware when the last event on a CPU is
disabled from inside the PMU interrupt handler?

handle_percpu_devid_irq()
  chip->irq_ack()                    /* mask_mips_irq() masks IP7 */
  mipsxx_pmu_handle_irq()
    mipsxx_pmu_handle_shared_irq()
      handle_associated_event()
        perf_event_overflow()        /* queues pending_disable_irq */
      irq_work_run()
        perf_pending_disable()
          ...
            mipspmu_disable()
              disable_percpu_irq()   /* clears percpu_enabled, masks IP7 */
            mipspmu_del()            /* clears last bit in used_mask */
            mipspmu_enable()         /* used_mask empty, skips enable */
  chip->irq_eoi()
    unmask_mips_irq()                <-- unmasks IP7 in CP0 Status

When a sampling event with a refresh limit (PERF_EVENT_IOC_REFRESH)
overflows and auto-disables via irq_work_run(), disable_percpu_irq()
clears the CPU in desc->percpu_enabled, but handle_percpu_devid_irq()
then runs chip->irq_eoi() (unmask_mips_irq()), which sets STATUSF_IP7
again.  That leaves IP7 unmasked with no events scheduled while
irq_percpu_is_enabled() returns false, so spurious interrupts fire
whenever CP0 Count wraps past Compare and sb1_pmu_cpu_offline() will
also skip masking IP7.

[ ... ]

> @@ -657,6 +737,10 @@ static int mipspmu_event_init(struct perf_event *event)
>  	if (has_branch_stack(event))
>  		return -EOPNOTSUPP;
>  
> +	if (is_sb1_pmu() && (event->attr.precise_ip || event->attr.config1 ||
> +			     event->attr.config2))
> +		return -EOPNOTSUPP;
> +
>  	switch (event->attr.type) {
>  	case PERF_TYPE_RAW:
>  	case PERF_TYPE_HARDWARE:

[Severity: Medium]
Should the precise_ip, config1 and config2 check come after the
event->attr.type switch?

Checking those fields first makes mipspmu_event_init() return
-EOPNOTSUPP instead of -ENOENT for events that do not target this PMU
(for example, PERF_TYPE_BREAKPOINT where bp_addr and bp_len union with
config1 and config2, or any PMU registered after mipspmu), causing
perf_init_event() to abort its PMU list walk early.

> @@ -683,7 +767,10 @@ static int mipspmu_event_init(struct perf_event *event)
>  	if (err)
>  		return err;
>  
> -	return __hw_perf_event_init(event);
> +	err = __hw_perf_event_init(event);
> +	if (err && is_sb1_pmu() && !event->destroy)
> +		hw_perf_event_destroy(event);
> +	return err;
>  }

[Severity: High]
Does a failure in validate_group() cause hw_perf_event_destroy() to run
twice for the same event?

arch/mips/kernel/perf_event_mipsxx.c:__hw_perf_event_init() {
    ...
	if (event->group_leader != event)
		err = validate_group(event);

	event->destroy = hw_perf_event_destroy;

	if (err)
		event->destroy(event);

	return err;
}

When validate_group() fails (such as when a fourth non-cycle event is
added to a group), __hw_perf_event_init() calls event->destroy(event)
and returns err with event->destroy still set to hw_perf_event_destroy.
Because perf_try_init_event() skips clearing event->destroy on
event_init() failure, perf_event_alloc()'s __free(__free_event) cleanup
calls event->destroy(event) a second time.  That decrements
active_events twice for one increment, dropping active_events to zero
while the group's earlier events are still open and resetting hardware
counters across all CPUs via on_each_cpu(reset_counters, ...).

Also, gating the !event->destroy cleanup on is_sb1_pmu() leaves
active_events leaked on non-SB1 MIPS PMUs when __hw_perf_event_init()
fails before setting event->destroy.  If __hw_perf_event_init() only
sets event->destroy after validate_group() succeeds (or clears it after
calling it), mipspmu_event_init() can handle the error cleanup in one
place.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112190916.3725141-1-f.fainelli@gmail.com?part=3