Re: [PATCH 2/3] perf: Add Broadcom BCM5301X AXI Trace Module counters

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 10/26/26 14:24, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] perf: bcm5301x_axi_event_init() calls for_each_sibling_event() with NULL ctx
> - [Low] docs: perf: title underline too short in bcm5301x-axi-pmu.rst
> --
> 
> commit 98ec05a97143b2d8c2fd04e15999d3fc2b116134
> Author: Florian Fainelli <florian.fainelli@broadcom.com>
> 
> perf: Add Broadcom BCM5301X AXI Trace Module counters
> 
> This commit adds a PMU driver and documentation for the Broadcom BCM5301X
> AXI Trace Module counters, exposing 16 unfiltered fixed counters through
> system-wide perf events with hrtimer-based 32-bit delta accounting.
> 
>> diff --git a/Documentation/admin-guide/perf/bcm5301x-axi-pmu.rst b/Documentation/admin-guide/perf/bcm5301x-axi-pmu.rst
>> new file mode 100644
>> index 0000000000000..720f042dfbd0b
>> --- /dev/null
>> +++ b/Documentation/admin-guide/perf/bcm5301x-axi-pmu.rst
>> @@ -0,0 +1,92 @@
>> +.. SPDX-License-Identifier: GPL-2.0-only
>> +
>> +Broadcom BCM5301X AXI Trace Module PMU
>> +====================================
> 
> [Severity: Low]
> In Documentation/admin-guide/perf/bcm5301x-axi-pmu.rst, the title underline is
> 36 characters while the title is 38 characters. Should the underline be
> extended by two characters so make htmldocs does not emit a title underline
> too short warning?

Yes, will fix that one.

> 
> [ ... ]
> 
>> diff --git a/drivers/perf/bcm5301x_axi_pmu.c b/drivers/perf/bcm5301x_axi_pmu.c
>> new file mode 100644
>> index 0000000000000..15da137a40859
>> --- /dev/null
>> +++ b/drivers/perf/bcm5301x_axi_pmu.c
> 
> [ ... ]
> 
>> +static int bcm5301x_axi_event_init(struct perf_event *event)
>> +{
>> +	struct bcm5301x_axi_pmu *axi = to_axi_pmu(event->pmu);
>> +	struct perf_event *sibling, *leader = event->group_leader;
>> +	unsigned long used = 0;
>> +
>> +	if (event->attr.type != event->pmu->type)
>> +		return -ENOENT;
>> +	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK ||
>> +	    event->cpu < 0)
>> +		return -EOPNOTSUPP;
>> +	if (event->attr.config >= ATM_NR_COUNTERS || event->attr.config1 ||
>> +	    event->attr.config2)
>> +		return -EINVAL;
>> +
>> +	/* A group cannot allocate the same fixed counter twice. */
>> +	if (!is_software_event(leader)) {
>> +		if (leader->pmu != event->pmu)
>> +			return -EINVAL;
>> +		__set_bit(leader->attr.config, &used);
>> +	}
>> +	for_each_sibling_event(sibling, leader) {
>> +		if (is_software_event(sibling))
>> +			continue;
>> +		if (sibling->pmu != event->pmu ||
>> +		    test_and_set_bit(sibling->attr.config, &used))
>> +			return -EINVAL;
>> +	}
>> +	if (event != leader && test_bit(event->attr.config, &used))
>> +		return -EINVAL;
> 
> [Severity: High]
> When userspace opens a standalone event or a group leader (where
> event == leader), can calling for_each_sibling_event(sibling, leader) in
> bcm5301x_axi_event_init() in drivers/perf/bcm5301x_axi_pmu.c trigger a
> lockdep warning or dereference a near-NULL pointer?
> 
> In the perf_event_alloc() -> perf_init_event() -> perf_try_init_event() path
> in kernel/events/core.c, leader->ctx->mutex is only locked when
> event->group_leader != event:
> 
> 	if (event->group_leader != event && pmu->task_ctx_nr != perf_sw_context) {
> 		ctx = perf_event_ctx_lock_nested(event->group_leader,
> 						 SINGLE_DEPTH_NESTING);
> 		BUG_ON(!ctx);
> 	}
> 
> 	event->pmu = pmu;
> 	ret = pmu->event_init(event);
> 
> When event == leader, leader->ctx is still NULL (it is only assigned later in
> perf_install_in_context()). Calling for_each_sibling_event(sibling, leader) in
> include/linux/perf_event.h expands to lockdep_assert_event_ctx(leader):
> 
> #define lockdep_assert_event_ctx(event)			\
> 	WARN_ON_ONCE(__lockdep_enabled &&			\
> 		     (this_cpu_read(hardirqs_enabled) &&	\
> 		      lockdep_is_held(&(event)->ctx->mutex) != LOCK_STATE_HELD))
> 
> With CONFIG_PROVE_LOCKING enabled, this passes &(leader)->ctx->mutex (an
> offset from NULL) to lockdep_is_held(), which triggers the WARN_ON_ONCE
> assertion and can fault in match_held_lock() if any held lock in
> current->held_locks has hlock->references != 0.
> 
> Should the group validation block in bcm5301x_axi_event_init() be guarded by
> if (event != leader)?

Thanks, this is also valid. Group validation is now guarded by event != 
leader, preserving validation when adding siblings while avoiding the 
NULL context for new leaders.
-- 
Florian