Re: [PATCH v2 2/3] perf/dwc_pcie: Support narrowed time-based counter for long time monitoring

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing PM runtime reference during asynchronous hrtimer lifecycle
--

commit 2e4285bc1c7667cda860c906d0df29f1548de698
Author: Yufan Dou <douyufan@picoheart.com>

perf/dwc_pcie: Support narrowed time-based counter for long time monitoring

This commit adds an optional hrtimer that fires every 2 seconds to read,
update, and reset the time-based counter. This works around hardware
implementations that do not implement the full 64-bit width of the counter
and limits short time monitoring usage.

> diff --git a/drivers/perf/dwc_pcie_pmu.c b/drivers/perf/dwc_pcie_pmu.c
> index 5385401fa9cf6..7ec8302d4090b 100644
> --- a/drivers/perf/dwc_pcie_pmu.c
> +++ b/drivers/perf/dwc_pcie_pmu.c

[ ... ]

> @@ -429,6 +453,26 @@ static int dwc_pcie_pmu_validate_group(struct perf_event *event)
>  	return 0;
>  }
>  
> +static enum hrtimer_restart dwc_pcie_pmu_hrtimer_callback(struct hrtimer *hrtimer)
> +{
> +	struct dwc_pcie_pmu *pcie_pmu = container_of(hrtimer, struct dwc_pcie_pmu, hrtimer);
> +	struct perf_event *event = pcie_pmu->time_based_event;
> +	struct hw_perf_event *hwc;
> +
> +	if (!event)
> +		return HRTIMER_NORESTART;
> +
> +	hwc = &event->hw;
> +	if (hwc->state & PERF_HES_STOPPED)
> +		return HRTIMER_NORESTART;
> +
> +	dwc_pcie_pmu_event_update(event);
> +	dwc_pcie_pmu_reset_time_based_counter(event);
> +	hrtimer_forward_now(hrtimer, ns_to_ktime(DWC_PCIE_PMU_TIMER_PERIOD_NS));

[Severity: High]
If the PCIe Root Port enters runtime suspend (such as D3hot or D3cold) while
this timer is active, will the hardware accesses in
dwc_pcie_pmu_event_update() cause problems?

Accessing the configuration space of a suspended device could return
corrupted data or trigger system errors like synchronous external aborts.

> +
> +	return HRTIMER_RESTART;
> +}

[ ... ]

> @@ -478,10 +522,15 @@ static void dwc_pcie_pmu_event_start(struct perf_event *event, int flags)
>  	hwc->state = 0;
>  	local64_set(&hwc->prev_count, 0);
>  
> -	if (type == DWC_PCIE_LANE_EVENT)
> +	if (type == DWC_PCIE_LANE_EVENT) {
>  		dwc_pcie_pmu_lane_event_enable(pcie_pmu, event, true);
> -	else if (type == DWC_PCIE_TIME_BASE_EVENT)
> +	} else if (type == DWC_PCIE_TIME_BASE_EVENT) {
>  		dwc_pcie_pmu_time_based_event_enable(pcie_pmu, true);
> +		if (pcie_pmu->timer_enable)
> +			hrtimer_start(&pcie_pmu->hrtimer,
> +				      ns_to_ktime(DWC_PCIE_PMU_TIMER_PERIOD_NS),
> +				      HRTIMER_MODE_REL_PINNED_HARD);

[Severity: High]
Should we be taking a PM runtime reference on the PCIe Root Port device when
starting this background hrtimer?

Since the hrtimer fires asynchronously every 2 seconds, ensuring the device
remains awake while the event is active would prevent the hardware accesses
in the timer callback from hitting a powered-down device.

> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629092717.74946-1-yang.yicong@picoheart.com?part=2