Re: [PATCH v5 2/2] perf: marvell: Add CN20K DDR PMU support

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [High] Missing Hardware Initialization for CN20K Programmable Counters.
- [High] Undefined Behavior via Unvalidated Event `config` on CN20K.
- [Medium] Event ID Conflicts and Incorrect Hardware Routing.
- [Medium] Broken Enable Logic and Stale Hardware State for `cfg1_base` Events.
--

commit 5466234dfcbce6dad8475395fbdf56b2dfd56308
Author: Geetha sowjanya <gakula@marvell.com>

    perf: marvell: Add CN20K DDR PMU support
    
    The CN20K DRAM Subsystem exposes eight programmable
    performance counters and two fixed counters for DDR
    read and write traffic.  Software selects events for
    the programmable counters from traffic at the DDR PHY
    interface, the CHI interconnect, or inside the DDR controller.

> diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c b/drivers/perf/marvell_cn10k_ddr_pmu.c
> index 72ac17efd846a..7e2e1823b009c 100644
> --- a/drivers/perf/marvell_cn10k_ddr_pmu.c
> +++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
[ ... ]
> @@ -61,6 +73,23 @@
>   * DO NOT change these event-id numbers, they are used to
>   * program event bitmap in h/w.
>   */
> +
> +/* CN20K specific events */
> +#define EVENT_PERF_OP_IS_RD16			61
> +#define EVENT_PERF_OP_IS_RD32			60
> +#define EVENT_PERF_OP_IS_WR16			59
> +#define EVENT_PERF_OP_IS_WR32			58
> +#define EVENT_OP_IS_ENTER_DSM			44
> +#define EVENT_OP_IS_RFM				43
> +
> +#define EVENT_CN20K_OP_IS_TCR_MRR			50
> +#define EVENT_CN20K_OP_IS_DQSOSC_MRR			49
> +#define EVENT_CN20K_OP_IS_DQSOSC_MPC			48
> +#define EVENT_CN20K_VISIBLE_WIN_LIMIT_REACHED_WR	47
> +#define EVENT_CN20K_VISIBLE_WIN_LIMIT_REACHED_RD	46
> +#define EVENT_CN20K_OP_IS_ZQLATCH			21
> +#define EVENT_CN20K_OP_IS_ZQSTART			22
> +

Could these new EVENT_CN20K_OP_IS_ZQ* IDs conflict with existing events?

For example, EVENT_PRECHARGE_FOR_OTHER is also 21, and EVENT_RDWR_TRANSITIONS
is 22. Since they share the same ID value, wouldn't standard events 21 and 22
incorrectly trigger the cfg1_base routing logic added in
cn10k_ddr_perf_event_add() for CN20K?

[ ... ]
> @@ -524,9 +637,9 @@ static void cn10k_ddr_perf_counter_enable(struct cn10k_ddr_pmu *pmu,
>  					  int counter, bool enable)
>  {
>  	const struct ddr_pmu_platform_data *p_data = pmu->p_data;
> +	unsigned int silicon_flags = pmu->p_data->silicon_flags;
>  	u64 ctrl_reg = pmu->p_data->cnt_op_mode_ctrl;
>  	const struct ddr_pmu_ops *ops = pmu->ops;
> -	bool is_ody = pmu->p_data->is_ody;
>  	u32 reg;
>  	u64 val;
>  
> @@ -546,7 +659,7 @@ static void cn10k_ddr_perf_counter_enable(struct cn10k_ddr_pmu *pmu,
>  
>  		writeq_relaxed(val, pmu->base + reg);
>  
> -		if (is_ody) {
> +		if (silicon_flags & IS_ODY) {
>  			if (enable) {
>  				/*
>  				 * Setup the PMU counter to work in

Are CN20K programmable counters ever actually enabled?

Unlike CN10K, CN20K does not register global .pmu_enable or .pmu_disable
callbacks during probe. And since this check is only for IS_ODY, the hardware
counters are never configured to work in manual mode or started via
cn10k_ddr_perf_counter_start(), which might leave them non-functional.

Additionally, just before this block, the function applies EVENT_ENABLE
exclusively to cfg_base:

    reg = DDRC_PERF_CFG(p_data->cfg_base, counter);
    val = readq_relaxed(pmu->base + reg);

    if (enable)
        val |= EVENT_ENABLE;

How are the ZQSTART and ZQLATCH events handled, since they are supposed to be
written to cfg1_base? It looks like EVENT_ENABLE would inadvertently be
applied to cfg_base instead.

[ ... ]
> @@ -642,10 +756,17 @@ static int cn10k_ddr_perf_event_add(struct perf_event *event, int flags)
>  	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
>  		/* Generic counters, configure event id */
>  		reg_offset = DDRC_PERF_CFG(p_data->cfg_base, counter);
> -		ret = ddr_perf_get_event_bitmap(config, &val, pmu);
> -		if (ret)
> -			return ret;
>  
> +		if (silicon_flags & IS_CN20K) {
> +			val =  (1ULL << (config - 1));
> +			if (config == EVENT_CN20K_OP_IS_ZQSTART ||
> +			    config == EVENT_CN20K_OP_IS_ZQLATCH)
> +				reg_offset = DDRC_PERF_CFG(p_data->cfg1_base, counter);
> +		} else {
> +			ret = ddr_perf_get_event_bitmap(config, &val, pmu);
> +			if (ret)
> +				return ret;
> +		}
>  		writeq_relaxed(val, pmu->base + reg_offset);
>  	} else {

Is it possible for the config variable (which comes from event->attr.config)
to cause undefined behavior here?

Since the CN20K path bypasses ddr_perf_get_event_bitmap(), it lacks
bounds checking on config. If a user provides config = 0, config - 1
could result in a negative shift. If config >= 65, the shift amount
would exceed the width of the 64-bit operand.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260413165621.10921-1-gakula@marvell.com?part=2