linux-kernel - Re: [PATCH v8 3/6] perf/marvell: Refactor to add version

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240924172332.00003dbf@Huawei.com>
Date: Tue, 24 Sep 2024 17:23:32 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Gowthami Thiagarajan <gthiagarajan@...vell.com>
CC: <will@...nel.org>, <mark.rutland@....com>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<gcherian@...vell.com>, <bbhushan2@...vell.com>, <sgoutham@...vell.com>
Subject: Re: [PATCH v8 3/6] perf/marvell: Refactor to add version - no
 functional change

On Thu, 19 Sep 2024 13:17:14 +0530
Gowthami Thiagarajan <gthiagarajan@...vell.com> wrote:

> This change is aimed at improving the maintainability of the code and
> laying the groundwork for versioning within the driver.
> 
> No functional changes are introduced in this commit; the driver's
> behavior and performance remain unchanged.
> 
> Signed-off-by: Gowthami Thiagarajan <gthiagarajan@...vell.com>

Versioning like this tends to scale badly as more versions turn up.
Can you just use more data in your pdata instead?
A template of the struct pmu that you copy and a callback for necessary
init that is version specific.

> ---
>  drivers/perf/marvell_cn10k_ddr_pmu.c | 63 ++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c b/drivers/perf/marvell_cn10k_ddr_pmu.c
> index 648ad3a740bf..65422fd5ddd2 100644
> --- a/drivers/perf/marvell_cn10k_ddr_pmu.c
> +++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
> @@ -124,10 +124,19 @@
>  #define CN10K_DDRC_PERF_CNT_VALUE_WR_OP		0x80D0
>  #define CN10K_DDRC_PERF_CNT_VALUE_RD_OP		0x80D8
>  
> +enum mrvl_ddr_pmu_version {
> +	DDR_PMU_V1 = 1,
> +};
> +
> +struct ddr_pmu_data {
> +	int id;
I don't see the point in this. Use the pdata structure
for this purpose and push anything else necessary into there.
Don't have an ID. That scales very badly as more device
types turn up over time.

> +};
> +
>  struct cn10k_ddr_pmu {
>  	struct pmu pmu;
>  	void __iomem *base;
>  	const struct ddr_pmu_platform_data *p_data;
> +	int version;
>  	unsigned int cpu;
>  	struct	device *dev;
>  	int active_events;
> @@ -738,12 +747,19 @@ static const struct ddr_pmu_platform_data cn10k_ddr_pmu_pdata = {
>  	.ops = &ddr_pmu_ops,
>  };
>  
> +#if defined(CONFIG_ACPI) || defined(CONFIG_OF)
> +static const struct ddr_pmu_data ddr_pmu_data = {
> +	.id   = DDR_PMU_V1,
> +};
> +#endif
> +
>  static int cn10k_ddr_perf_probe(struct platform_device *pdev)
>  {
>  	const struct ddr_pmu_data *dev_data;
>  	struct cn10k_ddr_pmu *ddr_pmu;
>  	struct resource *res;
>  	void __iomem *base;
> +	int version;
>  	char *name;
>  	int ret;
>  
> @@ -760,31 +776,36 @@ static int cn10k_ddr_perf_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	version = dev_data->id;
> +	ddr_pmu->version = version;
> +
>  	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
>  
>  	ddr_pmu->base = base;
>  
> -	ddr_pmu->p_data = &cn10k_ddr_pmu_pdata;
> -	/* Setup the PMU counter to work in manual mode */
> -	writeq_relaxed(OP_MODE_CTRL_VAL_MANUAL, ddr_pmu->base +
> -		       ddr_pmu->p_data->ddrc_perf_cnt_op_mode_ctrl);
> -
> -	ddr_pmu->pmu = (struct pmu) {
> -		.module	      = THIS_MODULE,
> -		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
> -		.task_ctx_nr = perf_invalid_context,
> -		.attr_groups = cn10k_attr_groups,
> -		.event_init  = cn10k_ddr_perf_event_init,
> -		.add	     = cn10k_ddr_perf_event_add,
> -		.del	     = cn10k_ddr_perf_event_del,
> -		.start	     = cn10k_ddr_perf_event_start,
> -		.stop	     = cn10k_ddr_perf_event_stop,
> -		.read	     = cn10k_ddr_perf_event_update,
> -		.pmu_enable  = cn10k_ddr_perf_pmu_enable,
> -		.pmu_disable = cn10k_ddr_perf_pmu_disable,
> -	};
> +	if (version == DDR_PMU_V1) {
> +		ddr_pmu->p_data = &cn10k_ddr_pmu_pdata;
> +		/* Setup the PMU counter to work in manual mode */
> +		writeq_relaxed(OP_MODE_CTRL_VAL_MANUAL, ddr_pmu->base +
> +			       ddr_pmu->p_data->ddrc_perf_cnt_op_mode_ctrl);
> +
> +		ddr_pmu->pmu = (struct pmu) {
> +			.module	      = THIS_MODULE,
> +			.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
> +			.task_ctx_nr = perf_invalid_context,
> +			.attr_groups = cn10k_attr_groups,
> +			.event_init  = cn10k_ddr_perf_event_init,
> +			.add	     = cn10k_ddr_perf_event_add,
> +			.del	     = cn10k_ddr_perf_event_del,
> +			.start	     = cn10k_ddr_perf_event_start,
> +			.stop	     = cn10k_ddr_perf_event_stop,
> +			.read	     = cn10k_ddr_perf_event_update,
> +			.pmu_enable  = cn10k_ddr_perf_pmu_enable,
> +			.pmu_disable = cn10k_ddr_perf_pmu_disable,
> +		};
> +	}
>  
>  	/* Choose this cpu to collect perf data */
>  	ddr_pmu->cpu = raw_smp_processor_id();
> @@ -827,7 +848,7 @@ static void cn10k_ddr_perf_remove(struct platform_device *pdev)
>  
>  #ifdef CONFIG_OF
>  static const struct of_device_id cn10k_ddr_pmu_of_match[] = {
> -	{ .compatible = "marvell,cn10k-ddr-pmu", },
> +	{ .compatible = "marvell,cn10k-ddr-pmu", .data = &ddr_pmu_data},
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, cn10k_ddr_pmu_of_match);
> @@ -835,7 +856,7 @@ MODULE_DEVICE_TABLE(of, cn10k_ddr_pmu_of_match);
>  
>  #ifdef CONFIG_ACPI
>  static const struct acpi_device_id cn10k_ddr_pmu_acpi_match[] = {
> -	{"MRVL000A", 0},
> +	{"MRVL000A", (kernel_ulong_t)&ddr_pmu_data},

Use the pdata itself for this, not a structure just used to get an ID
to then look it up in code.

>  	{},
>  };
>  MODULE_DEVICE_TABLE(acpi, cn10k_ddr_pmu_acpi_match);