| 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
| ||
|
Message-ID: <3597d068-2c44-9450-4a0c-4704f3639a37@linaro.org>
Date: Fri, 5 Aug 2022 10:38:04 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Jiucheng Xu <jiucheng.xu@...ogic.com>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-amlogic@...ts.infradead.org, devicetree@...r.kernel.org
Cc: Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Will Deacon <will@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Neil Armstrong <narmstrong@...libre.com>,
Kevin Hilman <khilman@...libre.com>,
Jerome Brunet <jbrunet@...libre.com>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
Chris Healy <cphealy@...il.com>,
kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v4 1/4] perf/amlogic: Add support for Amlogic meson G12
SoC DDR PMU driver
On 05/08/2022 09:14, Jiucheng Xu wrote:
> This patch adds support Amlogic meson G12 series SoC
> DDR bandwidth PMU driver framework and interfaces.
>
> The PMU not only can monitor the total DDR bandwidth,
> but also the bandwidth which is from individual IP module.
>
> Example usage:
>
> $ perf stat -a -e aml_ddr_bw/total_rw_bytes/ -I 1000 sleep 10
>
> - or -
Thank you for your patch. There is something to discuss/improve.
> +
> +static void fmt_attr_fill(struct attribute **fmt_attr)
> +{
> + int i;
> +
> + for (i = 0; fmt_attr[i]; i++)
> + ddr_perf_format_attrs[i] = fmt_attr[i];
> +
> + ddr_perf_format_attrs[i] = NULL;
> +}
> +
> +static int ddr_pmu_parse_dt(struct platform_device *pdev, struct dmc_hw_info *info)
> +{
> + /*struct pinctrl *p;*/
No dead code in kernel.
> + void __iomem *base;
> + int i, ret = -EINVAL;
> +
> + for (i = 0; i < info->dmc_nr; i++) {
Your binding does not allow to have more than 1. Remove dead code.
> + /* resource 0 for ddr register base */
> + base = devm_platform_ioremap_resource(pdev, i);
> + if (IS_ERR(base)) {
> + dev_err(&pdev->dev, "couldn't ioremap ddr reg %d\n", i);
Error message is not needed.
> + return PTR_ERR(base);
> + }
> + info->ddr_reg[i] = base;
> + }
> +
> + /* resource i for pll register base */
> + base = devm_platform_ioremap_resource(pdev, i);
> + if (IS_ERR(base)) {
> + dev_err(&pdev->dev, "couldn't ioremap for pll reg\n");
Error message is not needed.
> + return PTR_ERR(base);
> + }
> + info->pll_reg = base;
> +
> + ret = platform_get_irq(pdev, 0);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "couldn't get irq\n");
Error message is not needed.
> + return ret;
> + }
> + info->irq_num = ret;
> +
> + ret = devm_request_irq(&pdev->dev, info->irq_num, dmc_irq_handler,
> + IRQF_NOBALANCING, dev_name(&pdev->dev),
> + (void *)info);
> + if (ret < 0)
> + dev_err(&pdev->dev, "ddr request irq failed\n");
> +
> + return ret;
> +}
> +
> +int meson_ddr_pmu_create(struct platform_device *pdev, struct ddr_pmu *pmu)
> +{
> + int ret;
> + char *name;
> + struct pmu tmp_pmu = {
> + .module = THIS_MODULE,
> + .capabilities = PERF_PMU_CAP_NO_EXCLUDE,
> + .task_ctx_nr = perf_invalid_context,
> + .attr_groups = attr_groups,
> + .event_init = meson_ddr_perf_event_init,
> + .add = meson_ddr_perf_event_add,
> + .del = meson_ddr_perf_event_del,
> + .start = meson_ddr_perf_event_start,
> + .stop = meson_ddr_perf_event_stop,
> + .read = meson_ddr_perf_event_update,
> + };
> +
> + pmu->pmu = tmp_pmu;
> +
> + ret = ddr_pmu_parse_dt(pdev, &pmu->info);
> + if (ret < 0)
> + return ret;
> +
> + fmt_attr_fill(pmu->info.fmt_attr);
> +
> + pmu->cpu = raw_smp_processor_id();
smp_processor_id()
(or why do you need to use raw?)
> +
> + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, DDR_PERF_DEV_NAME);
> + if (!name) {
> + dev_err(&pdev->dev, "couldn't allocat name\n");
No error messages for allocation. Run basic tools on your code -
coccinelle, checkaptch, smatch and sparse.
> + return -ENOMEM;
> + }
> +
> + ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, name, NULL,
> + ddr_perf_offline_cpu);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "cpuhp_setup_state_multi failed\n");
> + return ret;
> + }
> + pmu->cpuhp_state = ret;
> +
> + /* Register the pmu instance for cpu hotplug */
> + ret = cpuhp_state_add_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> + if (ret) {
> + dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
> + goto cpuhp_instance_err;
> + }
> +
> + fill_event_attr(pmu);
> +
> + ret = perf_pmu_register(&pmu->pmu, name, -1);
> + if (ret) {
> + dev_err(&pdev->dev, "perf pmu register failed\n");
> + goto pmu_register_err;
> + }
> +
> + pmu->name = name;
> + pmu->dev = &pdev->dev;
> + pmu->pmu_enabled = false;
> +
> + platform_set_drvdata(pdev, pmu);
> +
> + return 0;
> +
> +pmu_register_err:
> + cpuhp_state_remove_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> +cpuhp_instance_err:
> + cpuhp_remove_state(pmu->cpuhp_state);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(meson_ddr_pmu_create);
Why is this exported?
Also misses kerneldoc.
> +
> +int meson_ddr_pmu_remove(struct platform_device *pdev)
> +{
> + struct ddr_pmu *pmu = platform_get_drvdata(pdev);
> +
> + perf_pmu_unregister(&pmu->pmu);
> + cpuhp_state_remove_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> + cpuhp_remove_state(pmu->cpuhp_state);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(meson_ddr_pmu_remove);
Why is this exported?
Also misses kerneldoc.
> diff --git a/drivers/perf/amlogic/meson_g12_ddr_pmu.c b/drivers/perf/amlogic/meson_g12_ddr_pmu.c
> new file mode 100644
> index 000000000000..ba1908733bee
> --- /dev/null
> +++ b/drivers/perf/amlogic/meson_g12_ddr_pmu.c
> @@ -0,0 +1,388 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Amlogic, Inc. All rights reserved.
> + */
(...)
> +static int dmc_g12_irq_handler(struct dmc_hw_info *info,
> + struct dmc_counter *counter)
> +{
> + unsigned int val;
> + int ret = -EINVAL;
> +
> + val = readl(info->ddr_reg[0] + DMC_MON_G12_CTRL0);
> + if (val & DMC_QOS_IRQ) {
> + dmc_g12_get_counters(info, counter);
> + /* clear irq flags */
> + writel(val, info->ddr_reg[0] + DMC_MON_G12_CTRL0);
> + ret = 0;
> + }
> + return ret;
> +}
> +
> +static struct dmc_pmu_hw_ops g12_ops = {
static const?
> + .enable = dmc_g12_counter_enable,
> + .disable = dmc_g12_counter_disable,
> + .irq_handler = dmc_g12_irq_handler,
> + .get_counters = dmc_g12_get_counters,
> + .config_axi_id = dmc_g12_config_axi_id,
> +};
> +
> +static int __init g12_ddr_pmu_probe(struct platform_device *pdev)
> +{
> + struct ddr_pmu *pmu;
> +
> + if (of_device_is_compatible(pdev->dev.of_node,
> + "amlogic,g12a-ddr-pmu")) {
> + format_attr_nna.attr.mode = 0;
> + format_attr_gdc.attr.mode = 0;
> + format_attr_arm1.attr.mode = 0;
> + format_attr_mipi_isp.attr.mode = 0;
No. That's not correct patter. You must use variant specific driver data.
> + } else if (of_device_is_compatible(pdev->dev.of_node,
> + "amlogic,sm1-ddr-pmu")) {
> + format_attr_gdc.attr.mode = 0;
> + format_attr_arm1.attr.mode = 0;
> + format_attr_mipi_isp.attr.mode = 0;
No. That's not correct patter. You must use variant specific driver data.
> + }
> +
> + pmu = devm_kzalloc(&pdev->dev, sizeof(struct ddr_pmu), GFP_KERNEL);
> + if (!pmu)
> + return -ENOMEM;
> +
> + /*
> + * G12 series Soc have single dmc controller and
> + * 4x ddr bandwidth monitor channels
> + */
> + pmu->info.dmc_nr = 1;
> + pmu->info.chann_nr = 4;
> + pmu->info.ops = &g12_ops;
> + pmu->info.fmt_attr = g12_pmu_format_attrs;
> +
> + return meson_ddr_pmu_create(pdev, pmu);
> +}
> +
> +static int __exit g12_ddr_pmu_remove(struct platform_device *pdev)
> +{
> + meson_ddr_pmu_remove(pdev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id meson_ddr_pmu_dt_match[] = {
> + {
> + .compatible = "amlogic,g12-ddr-pmu",
Undocumented compatible. Did you run checkpatch and fix all the errors?
> + },
> + {
> + .compatible = "amlogic,g12a-ddr-pmu",
> + },
> + {
> + .compatible = "amlogic,g12b-ddr-pmu",
> + },
> + {
> + .compatible = "amlogic,sm1-ddr-pmu",
Why four different entries for the same devices without driver data?
This is confusing.
> + },
> + {}
> +};
> +
> +static struct platform_driver g12_ddr_pmu_driver = {
> + .driver = {
> + .name = "amlogic,ddr-pmu",
> + .of_match_table = meson_ddr_pmu_dt_match,
> + },
> + .remove = __exit_p(g12_ddr_pmu_remove),
You made the driver non-hotpluggable - why?
In the same time it is still unbindable, whis is a bit confusing. If you
can unbind it, you should be able to hot-unplug it.
> +};
> +
> +module_platform_driver_probe(g12_ddr_pmu_driver, g12_ddr_pmu_probe);
> +MODULE_AUTHOR("Jiucheng Xu");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Amlogic G12 series SoC DDR PMU");
> diff --git a/include/soc/amlogic/meson_ddr_pmu.h b/include/soc/amlogic/meson_ddr_pmu.h
> new file mode 100644
> index 000000000000..882efe3c2f58
> --- /dev/null
> +++ b/include/soc/amlogic/meson_ddr_pmu.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2022 Amlogic, Inc. All rights reserved.
> + */
> +
> +#ifndef __MESON_DDR_PMU_H__
> +#define __MESON_DDR_PMU_H__
> +
> +#define MAX_CHANNEL_NUM 8
> +
> +enum {
> + ALL_CHAN_COUNTER_ID,
> + CHAN1_COUNTER_ID,
> + CHAN2_COUNTER_ID,
> + CHAN3_COUNTER_ID,
> + CHAN4_COUNTER_ID,
> + CHAN5_COUNTER_ID,
> + CHAN6_COUNTER_ID,
> + CHAN7_COUNTER_ID,
> + CHAN8_COUNTER_ID,
> + COUNTER_MAX_ID,
> +};
> +
> +struct dmc_hw_info;
> +
> +struct dmc_counter {
> + u64 all_cnt; /* The count of all requests come in/out ddr controller */
> + union {
> + u64 all_req;
> + struct {
> + u64 all_idle_cnt;
> + u64 all_16bit_cnt;
> + };
> + };
> + u64 channel_cnt[MAX_CHANNEL_NUM]; /* To save a DMC bandwidth-monitor channel counter */
> +};
> +
> +struct dmc_pmu_hw_ops {
> + void (*enable)(struct dmc_hw_info *info);
> + void (*disable)(struct dmc_hw_info *info);
> + /* Bind an axi line to a bandwidth-monitor channel */
> + void (*config_axi_id)(struct dmc_hw_info *info, int axi_id, int chann);
> + int (*irq_handler)(struct dmc_hw_info *info,
> + struct dmc_counter *counter);
> + void (*get_counters)(struct dmc_hw_info *info,
> + struct dmc_counter *counter);
> +};
> +
> +struct dmc_hw_info {
> + struct dmc_pmu_hw_ops *ops;
> + void __iomem *ddr_reg[4];
> + unsigned long timer_value; /* Timer value in TIMER register */
> + void __iomem *pll_reg;
> + int irq_num; /* irq vector number */
> + int dmc_nr; /* The number of dmc controller */
> + int chann_nr; /* The number of dmc bandwidth monitor channels */
> + int id; /* The number of supported channels */
> + struct attribute **fmt_attr;
> +};
> +
> +struct ddr_pmu {
> + struct pmu pmu;
> + struct dmc_hw_info info;
> + struct dmc_counter counters; /* save counters from hw */
> + bool pmu_enabled;
> + struct device *dev;
> + char *name;
> + struct hlist_node node;
> + enum cpuhp_state cpuhp_state;
> + int cpu; /* for cpu hotplug */
> +};
Linux-wide headers should not include your private data structures.
Entier header looks unused - should be made private.
Best regards,
Krzysztof
Powered by blists - more mailing lists