Re: [PATCH v14 2/3] perf: Add Raspberry Pi BCM2835 AXI PMU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf/rpi_axi_pmu: Endianness bug in VPU mailbox IPC payloads
--

commit 5a995c8809331bd8433a8a216b6077dd563f9f07
Author: Ian Rogers <irogers@google.com>

perf: Add Raspberry Pi BCM2835 AXI PMU driver

Adds a PMU driver for the Raspberry Pi AXI performance monitors, providing
access to the System and VideoCore performance counters natively through
the perf subsystem on Broadcom BCM2835 and BCM2711 platforms.

> diff --git a/drivers/perf/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/perf/rpi_axi_pmu.c

[ ... ]

> +static void set_monitor_control(struct rpi_axi_pmu *pmu, enum monitor mon, u32 set)
> +{
> +	if (pmu->monitor[mon].use_mailbox_interface) {
> +		u32 tmp[3] = {pmu->monitor[mon].mailbox + GEN_CTRL, 1, set};

[Severity: Medium]
Will this payload work correctly on big-endian architectures? 

The VPU mailbox communication appears to use native u32 arrays for its 
payloads here without explicit cpu_to_le32() conversions. Does this cause
the little-endian VideoCore VPU to read swapped bytes when the driver is
compiled for big-endian systems?

> +		int err;

[ ... ]

> +static void set_bus_watcher_control(struct rpi_axi_pmu *pmu, enum monitor mon, int idx, u32 set)
> +{
> +	int watcher = watcher_offset(idx);
> +
> +	if (pmu->monitor[mon].use_mailbox_interface) {
> +		u32 tmp[3] = {pmu->monitor[mon].mailbox + watcher, 1, set};

[Severity: Medium]
Similarly, does this watcher configuration payload require endianness
conversions to prevent hardware communication failures on big-endian
systems?

> +		int err;

[ ... ]

> +static u32 rpi_axi_pmu_read_counter(struct rpi_axi_pmu *pmu, enum monitor mon, int idx,
> +				    enum counter counter)
> +{
> +	int watcher = watcher_offset(idx);
> +	int offset = counter_offset(counter);
> +	u32 ret;
> +	/* Use READ_ONCE to prevent KCSAN data race warnings during lockless IPC reads */
> +	if (!READ_ONCE(pmu->monitor[mon].hw_events.enabled[idx]))
> +		return 0;
> +	if (pmu->monitor[mon].use_mailbox_interface) {
> +		u32 tmp[3] = {
> +			pmu->monitor[mon].mailbox + watcher + offset,
> +			1, -1
> +		};
> +		int err;
> +
> +		might_sleep();
> +		lockdep_assert_held(&pmu->vpu_mutex);
> +		if (WARN_ON_ONCE(in_interrupt() || irqs_disabled()))
> +			return -1;
> +		err = rpi_firmware_property(pmu->firmware,
> +					    RPI_FIRMWARE_GET_PERIPH_REG,
> +					    tmp, sizeof(tmp));
> +		if (err < 0 || tmp[1] != 1) {
> +			dev_err_ratelimited(&pmu->pdev->dev, "Failed to read bus watcher\n");
> +			/* Return U32_MAX on IPC failure. */
> +			return U32_MAX;
> +		}
> +		ret = tmp[2];

[Severity: Medium]
When the read response is received, is a le32_to_cpu() conversion needed on
tmp[2] before it is consumed?

Using proper endianness conversions would align with other Broadcom
firmware drivers like clk-raspberrypi to correctly interface with the
hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002175738.3242646-1-irogers@google.com?part=2