Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1433214 > unrolled thread
| Started by | Tai Nguyen <ttnguyen@apm.com> |
|---|---|
| First post | 2016-06-28 21:00 +0200 |
| Last post | 2016-07-06 23:50 +0200 |
| Articles | 5 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH v6 0/4] perf: Add APM X-Gene SoC Performance Monitoring Unit driver Tai Nguyen <ttnguyen@apm.com> - 2016-06-28 21:00 +0200
[PATCH v6 2/4] Documentation: Add documentation for APM X-Gene SoC PMU DTS binding Tai Nguyen <ttnguyen@apm.com> - 2016-06-28 21:00 +0200
[PATCH v6 4/4] arm64: dts: apm: Add APM X-Gene SoC PMU DTS entries Tai Nguyen <ttnguyen@apm.com> - 2016-06-28 21:00 +0200
Re: [PATCH v6 3/4] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver Mark Rutland <mark.rutland@arm.com> - 2016-07-06 19:50 +0200
Re: [PATCH v6 3/4] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver Tai Tri Nguyen <ttnguyen@apm.com> - 2016-07-06 23:50 +0200
| From | Tai Nguyen <ttnguyen@apm.com> |
|---|---|
| Date | 2016-06-28 21:00 +0200 |
| Subject | [PATCH v6 0/4] perf: Add APM X-Gene SoC Performance Monitoring Unit driver |
| Message-ID | <rP6oO-2h2-9@gated-at.bofh.it> |
In addition to the X-Gene ARM CPU performance monitoring unit (PMU), there
are PMU for the SoC system devices such as L3 cache(s), I/O bridge(s),
memory controller bridges and memory. These PMU devices are loosely
architected to follow the same model as the PMU for ARM cores.
Signed-off-by: Tai Nguyen <ttnguyen@apm.com>
---
v6:
* Add IRQF_NOBALANCING and IRQF_NO_THREAD flags to the PMU overflow interrupt
Exclude the interrupt from irq balancing and prevent the context from being
threaded
v5:
* Remove hw_perf_event::extra_reg field use
Change GET_CNTR to use hw_perf_event::idx
Change GET_AGENTID/GET_AGEN1ID to use hw_perf_event::config_base
* Use compound literal structure defines for format and event attribute groups
to statically define them at compile time
* Bitwise invert the meaning of agent mask in config1 field.
* Fix update pmu_counter_event pointer before starting event
* Add reset of pmu_dev->pmu_counter_event to NULL in xgene_perf_del
* Use exactly half of max period to fix the overflow counter issue and account
for the possiblity of extreme interrupt latency
* Use spin lock instead of interrupt masking in overflow interrupt handler
* Remove unnecessary update of hw_perf_event::period_left
v4:
* Alphabetically sorting header files
* Remove dynamic allocation for PMU format and event attribute groups
Create shared constant attribute groups per each class
* Remove perf_sample_data as this perf driver doesn't support sampling
* Consistently use the PCP_PMU_V{1,2} defines
* Set affinity to make sure the overflow interrupt is handled by the
same assigned CPU
v3:
* Remove index property use in PMU device sub nodes
v2:
* Use bitmask for event asignned counter mask pmu_dev->cntr_assign_mask
* Remove unnecessary spinlocks in perf add/del operations
* Remove unnecessary condition checks
* Enforce CPU assignment to one CPU for perf operarations
* Set the task_ctx_nr to perf_invalid_context for perf driver
* Remove irrelevant pt_rregs
* Change perf sysfs attributes to be fixed instead of dynamic
* Fix checking for an ACPI companion device instead of EFI enable
* Add documentation for config/config1 fields format and perf tool example
---
Tai Nguyen (4):
MAINTAINERS: Add entry for APM X-Gene SoC PMU driver
Documentation: Add documentation for APM X-Gene SoC PMU DTS binding
perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver
arm64: dts: apm: Add APM X-Gene SoC PMU DTS entries
.../devicetree/bindings/perf/apm-xgene-pmu.txt | 112 ++
Documentation/perf/xgene-pmu.txt | 48 +
MAINTAINERS | 8 +
arch/arm64/boot/dts/apm/apm-storm.dtsi | 58 +
drivers/perf/Kconfig | 7 +
drivers/perf/Makefile | 1 +
drivers/perf/xgene_pmu.c | 1360 ++++++++++++++++++++
7 files changed, 1594 insertions(+)
create mode 100644 Documentation/devicetree/bindings/perf/apm-xgene-pmu.txt
create mode 100644 Documentation/perf/xgene-pmu.txt
create mode 100644 drivers/perf/xgene_pmu.c
--
1.9.1
[toc] | [next] | [standalone]
| From | Tai Nguyen <ttnguyen@apm.com> |
|---|---|
| Date | 2016-06-28 21:00 +0200 |
| Subject | [PATCH v6 2/4] Documentation: Add documentation for APM X-Gene SoC PMU DTS binding |
| Message-ID | <rP6oO-2h2-21@gated-at.bofh.it> |
| In reply to | #1433214 |
Signed-off-by: Tai Nguyen <ttnguyen@apm.com>
Acked-by: Rob Herring <robh@kernel.org>
---
.../devicetree/bindings/perf/apm-xgene-pmu.txt | 112 +++++++++++++++++++++
1 file changed, 112 insertions(+)
create mode 100644 Documentation/devicetree/bindings/perf/apm-xgene-pmu.txt
diff --git a/Documentation/devicetree/bindings/perf/apm-xgene-pmu.txt b/Documentation/devicetree/bindings/perf/apm-xgene-pmu.txt
new file mode 100644
index 0000000..afb11cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/perf/apm-xgene-pmu.txt
@@ -0,0 +1,112 @@
+* APM X-Gene SoC PMU bindings
+
+This is APM X-Gene SoC PMU (Performance Monitoring Unit) module.
+The following PMU devices are supported:
+
+ L3C - L3 cache controller
+ IOB - IO bridge
+ MCB - Memory controller bridge
+ MC - Memory controller
+
+The following section describes the SoC PMU DT node binding.
+
+Required properties:
+- compatible : Shall be "apm,xgene-pmu" for revision 1 or
+ "apm,xgene-pmu-v2" for revision 2.
+- regmap-csw : Regmap of the CPU switch fabric (CSW) resource.
+- regmap-mcba : Regmap of the MCB-A (memory bridge) resource.
+- regmap-mcbb : Regmap of the MCB-B (memory bridge) resource.
+- reg : First resource shall be the CPU bus PMU resource.
+- interrupts : Interrupt-specifier for PMU IRQ.
+
+Required properties for L3C subnode:
+- compatible : Shall be "apm,xgene-pmu-l3c".
+- reg : First resource shall be the L3C PMU resource.
+
+Required properties for IOB subnode:
+- compatible : Shall be "apm,xgene-pmu-iob".
+- reg : First resource shall be the IOB PMU resource.
+
+Required properties for MCB subnode:
+- compatible : Shall be "apm,xgene-pmu-mcb".
+- reg : First resource shall be the MCB PMU resource.
+- enable-bit-index : The bit indicates if the according MCB is enabled.
+
+Required properties for MC subnode:
+- compatible : Shall be "apm,xgene-pmu-mc".
+- reg : First resource shall be the MC PMU resource.
+- enable-bit-index : The bit indicates if the according MC is enabled.
+
+Example:
+ csw: csw@7e200000 {
+ compatible = "apm,xgene-csw", "syscon";
+ reg = <0x0 0x7e200000 0x0 0x1000>;
+ };
+
+ mcba: mcba@7e700000 {
+ compatible = "apm,xgene-mcb", "syscon";
+ reg = <0x0 0x7e700000 0x0 0x1000>;
+ };
+
+ mcbb: mcbb@7e720000 {
+ compatible = "apm,xgene-mcb", "syscon";
+ reg = <0x0 0x7e720000 0x0 0x1000>;
+ };
+
+ pmu: pmu@78810000 {
+ compatible = "apm,xgene-pmu-v2";
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+ regmap-csw = <&csw>;
+ regmap-mcba = <&mcba>;
+ regmap-mcbb = <&mcbb>;
+ reg = <0x0 0x78810000 0x0 0x1000>;
+ interrupts = <0x0 0x22 0x4>;
+
+ pmul3c@7e610000 {
+ compatible = "apm,xgene-pmu-l3c";
+ reg = <0x0 0x7e610000 0x0 0x1000>;
+ };
+
+ pmuiob@7e940000 {
+ compatible = "apm,xgene-pmu-iob";
+ reg = <0x0 0x7e940000 0x0 0x1000>;
+ };
+
+ pmucmcb@7e710000 {
+ compatible = "apm,xgene-pmu-mcb";
+ reg = <0x0 0x7e710000 0x0 0x1000>;
+ enable-bit-index = <0>;
+ };
+
+ pmucmcb@7e730000 {
+ compatible = "apm,xgene-pmu-mcb";
+ reg = <0x0 0x7e730000 0x0 0x1000>;
+ enable-bit-index = <1>;
+ };
+
+ pmucmc@7e810000 {
+ compatible = "apm,xgene-pmu-mc";
+ reg = <0x0 0x7e810000 0x0 0x1000>;
+ enable-bit-index = <0>;
+ };
+
+ pmucmc@7e850000 {
+ compatible = "apm,xgene-pmu-mc";
+ reg = <0x0 0x7e850000 0x0 0x1000>;
+ enable-bit-index = <1>;
+ };
+
+ pmucmc@7e890000 {
+ compatible = "apm,xgene-pmu-mc";
+ reg = <0x0 0x7e890000 0x0 0x1000>;
+ enable-bit-index = <2>;
+ };
+
+ pmucmc@7e8d0000 {
+ compatible = "apm,xgene-pmu-mc";
+ reg = <0x0 0x7e8d0000 0x0 0x1000>;
+ enable-bit-index = <3>;
+ };
+ };
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Tai Nguyen <ttnguyen@apm.com> |
|---|---|
| Date | 2016-06-28 21:00 +0200 |
| Subject | [PATCH v6 4/4] arm64: dts: apm: Add APM X-Gene SoC PMU DTS entries |
| Message-ID | <rP6oO-2h2-23@gated-at.bofh.it> |
| In reply to | #1433214 |
Signed-off-by: Tai Nguyen <ttnguyen@apm.com>
---
arch/arm64/boot/dts/apm/apm-storm.dtsi | 58 ++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
index fe30f76..62806e0 100644
--- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
@@ -566,6 +566,64 @@
};
};
+ pmu: pmu@78810000 {
+ compatible = "apm,xgene-pmu-v2";
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+ regmap-csw = <&csw>;
+ regmap-mcba = <&mcba>;
+ regmap-mcbb = <&mcbb>;
+ reg = <0x0 0x78810000 0x0 0x1000>;
+ interrupts = <0x0 0x22 0x4>;
+
+ pmul3c@7e610000 {
+ compatible = "apm,xgene-pmu-l3c";
+ reg = <0x0 0x7e610000 0x0 0x1000>;
+ };
+
+ pmuiob@7e940000 {
+ compatible = "apm,xgene-pmu-iob";
+ reg = <0x0 0x7e940000 0x0 0x1000>;
+ };
+
+ pmucmcb@7e710000 {
+ compatible = "apm,xgene-pmu-mcb";
+ reg = <0x0 0x7e710000 0x0 0x1000>;
+ enable-bit-index = <0>;
+ };
+
+ pmucmcb@7e730000 {
+ compatible = "apm,xgene-pmu-mcb";
+ reg = <0x0 0x7e730000 0x0 0x1000>;
+ enable-bit-index = <1>;
+ };
+
+ pmucmc@7e810000 {
+ compatible = "apm,xgene-pmu-mc";
+ reg = <0x0 0x7e810000 0x0 0x1000>;
+ enable-bit-index = <0>;
+ };
+
+ pmucmc@7e850000 {
+ compatible = "apm,xgene-pmu-mc";
+ reg = <0x0 0x7e850000 0x0 0x1000>;
+ enable-bit-index = <1>;
+ };
+
+ pmucmc@7e890000 {
+ compatible = "apm,xgene-pmu-mc";
+ reg = <0x0 0x7e890000 0x0 0x1000>;
+ enable-bit-index = <2>;
+ };
+
+ pmucmc@7e8d0000 {
+ compatible = "apm,xgene-pmu-mc";
+ reg = <0x0 0x7e8d0000 0x0 0x1000>;
+ enable-bit-index = <3>;
+ };
+ };
+
pcie0: pcie@1f2b0000 {
status = "disabled";
device_type = "pci";
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Mark Rutland <mark.rutland@arm.com> |
|---|---|
| Date | 2016-07-06 19:50 +0200 |
| Subject | Re: [PATCH v6 3/4] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver |
| Message-ID | <rRZ7s-3Lq-7@gated-at.bofh.it> |
| In reply to | #1433214 |
Hi,
This looks mostly good now, though there are some remaining issues that
I've commented on below.
On Tue, Jun 28, 2016 at 11:50:44AM -0700, Tai Nguyen wrote:
> Signed-off-by: Tai Nguyen <ttnguyen@apm.com>
> ---
> Documentation/perf/xgene-pmu.txt | 48 ++
> drivers/perf/Kconfig | 7 +
> drivers/perf/Makefile | 1 +
> drivers/perf/xgene_pmu.c | 1360 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1416 insertions(+)
> create mode 100644 Documentation/perf/xgene-pmu.txt
> create mode 100644 drivers/perf/xgene_pmu.c
> +static const struct attribute *xgene_pmu_cpumask_attrs[] = {
> + &dev_attr_cpumask.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group pmu_cpumask_attr_group = {
> + .attrs = (struct attribute **) xgene_pmu_cpumask_attrs,
> +};
As mentioned previously, we shouldn't cast away constness.
Please remove the const from the definition of xgene_pmu_cpumask_attrs,
and remove the cast.
[...]
> +static int xgene_perf_event_init(struct perf_event *event)
> +{
> + struct xgene_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
> + struct hw_perf_event *hw = &event->hw;
> +
> + /* Test the event attr type check for PMU enumeration */
> + if (event->attr.type != event->pmu->type)
> + return -ENOENT;
> +
> + /*
> + * SOC PMU counters are shared across all cores.
> + * Therefore, it does not support per-process mode.
> + * Also, it does not support event sampling mode.
> + */
> + if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
> + return -EINVAL;
> +
> + /* SOC counters do not have usr/os/guest/host bits */
> + if (event->attr.exclude_user || event->attr.exclude_kernel ||
> + event->attr.exclude_host || event->attr.exclude_guest)
> + return -EINVAL;
> +
> + if (event->cpu < 0)
> + return -EINVAL;
> + /*
> + * Many perf core operations (eg. events rotation) operate on a
> + * single CPU context. This is obvious for CPU PMUs, where one
> + * expects the same sets of events being observed on all CPUs,
> + * but can lead to issues for off-core PMUs, where each
> + * event could be theoretically assigned to a different CPU. To
> + * mitigate this, we enforce CPU assignment to one, selected
> + * processor (the one described in the "cpumask" attribute).
> + */
> + event->cpu = cpumask_first(&pmu_dev->parent->cpu);
> +
> + hw->config = event->attr.config;
> + /*
> + * Each bit of the config1 field represents an agent from which the
> + * request of the event come. The event is counted only if it's caused
> + * by a request of an agent has the bit cleared.
> + * By default, the event is counted for all agents.
> + */
> + hw->config_base = event->attr.config1;
> +
> + return 0;
> +}
You also need to validate the event group as a whole. See the end of
arm_ccn_pmu_event_init() in drivers/bus/arm-ccn.c for an example, where
we check the leader and sibling list.
> +static void xgene_perf_enable_event(struct perf_event *event)
> +{
> + struct xgene_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
> +
> + xgene_pmu_write_evttype(pmu_dev, GET_CNTR(event), GET_EVENTID(event));
> + xgene_pmu_write_agentmsk(pmu_dev, ~((u32)GET_AGENTID(event)));
> + if (pmu_dev->inf->type == PMU_TYPE_IOB)
> + xgene_pmu_write_agent1msk(pmu_dev, ~((u32)GET_AGENT1ID(event)));
> +
> + xgene_pmu_start_counters(pmu_dev);
This call to xgene_pmu_start_counters should be moved out into
pmu::pmu_enable(), which the core code will call for you. Similarly, you
should implement pmu::pmu_disable() using xgene_pmu_stop_counters().
That avoids poking the HW repeatedly to enable/disable all counters,
ensures that event groups count for the same time, etc.
In your pmu::pmu_enable() implementation you should probably check
whether you have any events (e.g. by looking at the counter bitmap). If
there are no events, you can avoid pointlessly enabling the PMU HW.
> + xgene_pmu_enable_counter(pmu_dev, GET_CNTR(event));
> + xgene_pmu_enable_counter_int(pmu_dev, GET_CNTR(event));
> +}
[...]
> +static irqreturn_t xgene_pmu_isr(int irq, void *dev_id)
> +{
> + struct xgene_pmu_dev_ctx *ctx, *temp_ctx;
> + struct xgene_pmu *xgene_pmu = dev_id;
> + unsigned long flags;
> + u32 val;
> +
> + raw_spin_lock_irqsave(&xgene_pmu->lock, flags);
> +
> + /* Get Interrupt PMU source */
> + val = readl(xgene_pmu->pcppmu_csr + PCPPMU_INTSTATUS_REG);
> + if (val & PCPPMU_INT_MCU) {
> + list_for_each_entry_safe(ctx, temp_ctx,
> + &xgene_pmu->mcpmus, next) {
> + _xgene_pmu_isr(irq, ctx->pmu_dev);
> + }
> + }
No handlers modify the list, so there's no need to use
list_for_each_entry_safe, and you don't need the tmp_ctx variable.
> + if (val & PCPPMU_INT_MCB) {
> + list_for_each_entry_safe(ctx, temp_ctx,
> + &xgene_pmu->mcbpmus, next) {
> + _xgene_pmu_isr(irq, ctx->pmu_dev);
> + }
> + }
> + if (val & PCPPMU_INT_L3C) {
> + list_for_each_entry_safe(ctx, temp_ctx,
> + &xgene_pmu->l3cpmus, next) {
> + _xgene_pmu_isr(irq, ctx->pmu_dev);
> + }
> + }
> + if (val & PCPPMU_INT_IOB) {
> + list_for_each_entry_safe(ctx, temp_ctx,
> + &xgene_pmu->iobpmus, next) {
> + _xgene_pmu_isr(irq, ctx->pmu_dev);
> + }
> + }
> +
> + raw_spin_unlock_irqrestore(&xgene_pmu->lock, flags);
> +
> + return IRQ_HANDLED;
> +}
[...]
> +static struct
> +xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
> + struct acpi_device *adev, u32 type)
> +{
> + struct device *dev = xgene_pmu->dev;
> + struct list_head resource_list;
> + struct xgene_pmu_dev_ctx *ctx;
> + const union acpi_object *obj;
> + struct hw_pmu_info *inf;
> + void __iomem *dev_csr;
> + struct resource res;
> + int enable_bit;
> + int rc;
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return NULL;
> +
> + INIT_LIST_HEAD(&resource_list);
> + rc = acpi_dev_get_resources(adev, &resource_list,
> + acpi_pmu_dev_add_resource, &res);
> + acpi_dev_free_resource_list(&resource_list);
> + if (rc < 0 || IS_ERR(&res)) {
> + dev_err(dev, "PMU type %d: No resource address found\n", type);
> + goto err;
> + }
> +
> + dev_csr = devm_ioremap_resource(dev, &res);
> + if (IS_ERR(dev_csr)) {
> + dev_err(dev, "PMU type %d: Fail to map resource\n", type);
> + rc = PTR_ERR(dev_csr);
> + goto err;
> + }
> +
> + /* A PMU device node without enable-bit-index is always enabled */
> + rc = acpi_dev_get_property(adev, "enable-bit-index",
> + ACPI_TYPE_INTEGER, &obj);
> + if (rc < 0)
> + enable_bit = 0;
> + else
> + enable_bit = (int) obj->integer.value;
> +
> + ctx->name = xgene_pmu_dev_name(type, enable_bit);
> + inf = &ctx->inf;
> + inf->type = type;
> + inf->csr = dev_csr;
> + inf->enable_mask = 1 << enable_bit;
> +
> + return ctx;
> +err:
> + devm_kfree(dev, ctx);
> + return NULL;
> +}
> +
> +static acpi_status acpi_pmu_dev_add(acpi_handle handle, u32 level,
> + void *data, void **return_value)
> +{
> + struct xgene_pmu *xgene_pmu = data;
> + struct xgene_pmu_dev_ctx *ctx;
> + struct acpi_device *adev;
> +
> + if (acpi_bus_get_device(handle, &adev))
> + return AE_OK;
> + if (acpi_bus_get_status(adev) || !adev->status.present)
> + return AE_OK;
> +
> + if (!strcmp(acpi_device_hid(adev), "APMC0D5D"))
> + ctx = acpi_get_pmu_hw_inf(xgene_pmu, adev, PMU_TYPE_L3C);
> + else if (!strcmp(acpi_device_hid(adev), "APMC0D5E"))
> + ctx = acpi_get_pmu_hw_inf(xgene_pmu, adev, PMU_TYPE_IOB);
> + else if (!strcmp(acpi_device_hid(adev), "APMC0D5F"))
> + ctx = acpi_get_pmu_hw_inf(xgene_pmu, adev, PMU_TYPE_MCB);
> + else if (!strcmp(acpi_device_hid(adev), "APMC0D60"))
> + ctx = acpi_get_pmu_hw_inf(xgene_pmu, adev, PMU_TYPE_MC);
> + else
> + ctx = NULL;
> +
> + if (!ctx)
> + return AE_OK;
> +
> + if (xgene_pmu_dev_add(xgene_pmu, ctx))
> + return AE_OK;
Given that xgene_pmu_dev_add() failed, don't we leak resources allocated
in acpi_get_pmu_hw_inf(), e.g. ctx?
I don't think returning AE_OK is correct here.
Why do we not fail to probe if this occurs?
> +
> + switch (ctx->inf.type) {
> + case PMU_TYPE_L3C:
> + list_add(&ctx->next, &xgene_pmu->l3cpmus);
> + break;
> + case PMU_TYPE_IOB:
> + list_add(&ctx->next, &xgene_pmu->iobpmus);
> + break;
> + case PMU_TYPE_MCB:
> + list_add(&ctx->next, &xgene_pmu->mcbpmus);
> + break;
> + case PMU_TYPE_MC:
> + list_add(&ctx->next, &xgene_pmu->mcpmus);
> + break;
> + }
> + return AE_OK;
> +}
> +
> +static int acpi_pmu_probe_pmu_dev(struct xgene_pmu *xgene_pmu,
> + struct platform_device *pdev)
> +{
> + struct device *dev = xgene_pmu->dev;
> + acpi_handle handle;
> + acpi_status status;
> +
> + handle = ACPI_HANDLE(dev);
> + if (!handle)
> + return -EINVAL;
> +
> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> + acpi_pmu_dev_add, NULL, xgene_pmu, NULL);
> + if (ACPI_FAILURE(status))
> + dev_err(dev, "failed to probe PMU devices\n");
> + return 0;
> +}
Surely we should pass on an error?
> +static struct
> +xgene_pmu_dev_ctx *fdt_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
> + struct device_node *np, u32 type)
> +{
> + struct device *dev = xgene_pmu->dev;
> + struct xgene_pmu_dev_ctx *ctx;
> + struct hw_pmu_info *inf;
> + void __iomem *dev_csr;
> + struct resource res;
> + int enable_bit;
> + int rc;
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return NULL;
> + rc = of_address_to_resource(np, 0, &res);
> + if (rc < 0) {
> + dev_err(dev, "PMU type %d: No resource address found\n", type);
> + goto err;
> + }
> + dev_csr = devm_ioremap_resource(dev, &res);
> + if (IS_ERR(dev_csr)) {
> + dev_err(dev, "PMU type %d: Fail to map resource\n", type);
> + rc = PTR_ERR(dev_csr);
> + goto err;
> + }
> +
> + /* A PMU device node without enable-bit-index is always enabled */
> + if (of_property_read_u32(np, "enable-bit-index", &enable_bit))
> + enable_bit = 0;
> +
> + ctx->name = xgene_pmu_dev_name(type, enable_bit);
> + inf = &ctx->inf;
> + inf->type = type;
> + inf->csr = dev_csr;
> + inf->enable_mask = 1 << enable_bit;
> +
> + return ctx;
> +err:
> + devm_kfree(dev, ctx);
> + return NULL;
> +}
> +
> +static int fdt_pmu_probe_pmu_dev(struct xgene_pmu *xgene_pmu,
> + struct platform_device *pdev)
> +{
> + struct xgene_pmu_dev_ctx *ctx;
> + struct device_node *np;
> +
> + for_each_child_of_node(pdev->dev.of_node, np) {
> + if (!of_device_is_available(np))
> + continue;
> +
> + if (of_device_is_compatible(np, "apm,xgene-pmu-l3c"))
> + ctx = fdt_get_pmu_hw_inf(xgene_pmu, np, PMU_TYPE_L3C);
> + else if (of_device_is_compatible(np, "apm,xgene-pmu-iob"))
> + ctx = fdt_get_pmu_hw_inf(xgene_pmu, np, PMU_TYPE_IOB);
> + else if (of_device_is_compatible(np, "apm,xgene-pmu-mcb"))
> + ctx = fdt_get_pmu_hw_inf(xgene_pmu, np, PMU_TYPE_MCB);
> + else if (of_device_is_compatible(np, "apm,xgene-pmu-mc"))
> + ctx = fdt_get_pmu_hw_inf(xgene_pmu, np, PMU_TYPE_MC);
> + else
> + ctx = NULL;
> +
> + if (!ctx)
> + continue;
> +
> + if (xgene_pmu_dev_add(xgene_pmu, ctx))
> + continue;
As with acpi_pmu_dev_add, doesn't this leak the contexts allocated in
fdt_get_pmu_hw_inf?
Why do we not fail to probe if this occurs?
> +
> + switch (ctx->inf.type) {
> + case PMU_TYPE_L3C:
> + list_add(&ctx->next, &xgene_pmu->l3cpmus);
> + break;
> + case PMU_TYPE_IOB:
> + list_add(&ctx->next, &xgene_pmu->iobpmus);
> + break;
> + case PMU_TYPE_MCB:
> + list_add(&ctx->next, &xgene_pmu->mcbpmus);
> + break;
> + case PMU_TYPE_MC:
> + list_add(&ctx->next, &xgene_pmu->mcpmus);
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int xgene_pmu_probe_pmu_dev(struct xgene_pmu *xgene_pmu,
> + struct platform_device *pdev)
> +{
> + if (has_acpi_companion(&pdev->dev))
> + return acpi_pmu_probe_pmu_dev(xgene_pmu, pdev);
> + return fdt_pmu_probe_pmu_dev(xgene_pmu, pdev);
> +}
> +
[...]
> +static const struct xgene_pmu_data xgene_pmu_data = {
> + .id = PCP_PMU_V1,
> + .data = 0,
> +};
> +
> +static const struct xgene_pmu_data xgene_pmu_v2_data = {
> + .id = PCP_PMU_V2,
> + .data = 0,
> +};
The data field on either of these is never used. I think you can remove
it.
> +static int xgene_pmu_probe(struct platform_device *pdev)
> +{
> + const struct xgene_pmu_data *dev_data;
> + const struct of_device_id *of_id;
> + struct xgene_pmu *xgene_pmu;
> + struct resource *res;
> + int irq, rc;
> + int version;
> +
> + xgene_pmu = devm_kzalloc(&pdev->dev, sizeof(*xgene_pmu), GFP_KERNEL);
> + if (!xgene_pmu)
> + return -ENOMEM;
> + xgene_pmu->dev = &pdev->dev;
> + platform_set_drvdata(pdev, xgene_pmu);
> +
> + version = -EINVAL;
> + of_id = of_match_device(xgene_pmu_of_match, &pdev->dev);
> + if (of_id) {
> + dev_data = (const struct xgene_pmu_data *) of_id->data;
> + version = dev_data->id;
> + }
> +
> +#ifdef CONFIG_ACPI
> + if (ACPI_COMPANION(&pdev->dev)) {
> + const struct acpi_device_id *acpi_id;
> +
> + acpi_id = acpi_match_device(xgene_pmu_acpi_match, &pdev->dev);
> + if (acpi_id)
> + version = (int) acpi_id->driver_data;
> + }
> +#endif
> + if (version < 0)
> + return -ENODEV;
> +
> + INIT_LIST_HEAD(&xgene_pmu->l3cpmus);
> + INIT_LIST_HEAD(&xgene_pmu->iobpmus);
> + INIT_LIST_HEAD(&xgene_pmu->mcbpmus);
> + INIT_LIST_HEAD(&xgene_pmu->mcpmus);
> +
> + xgene_pmu->version = version;
> + dev_info(&pdev->dev, "X-Gene PMU version %d\n", xgene_pmu->version);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + xgene_pmu->pcppmu_csr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(xgene_pmu->pcppmu_csr)) {
> + dev_err(&pdev->dev, "ioremap failed for PCP PMU resource\n");
> + rc = PTR_ERR(xgene_pmu->pcppmu_csr);
> + goto err;
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "No IRQ resource\n");
> + rc = -EINVAL;
> + goto err;
> + }
> + rc = devm_request_irq(&pdev->dev, irq, xgene_pmu_isr,
> + IRQF_NOBALANCING | IRQF_NO_THREAD,
> + dev_name(&pdev->dev), xgene_pmu);
> + if (rc) {
> + dev_err(&pdev->dev, "Could not request IRQ %d\n", irq);
> + goto err;
> + }
> +
> + raw_spin_lock_init(&xgene_pmu->lock);
> +
> + /* Check for active MCBs and MCUs */
> + rc = xgene_pmu_probe_active_mcb_mcu(xgene_pmu, pdev);
> + if (rc) {
> + dev_warn(&pdev->dev, "Unknown MCB/MCU active status\n");
> + xgene_pmu->mcb_active_mask = 0x1;
> + xgene_pmu->mc_active_mask = 0x1;
> + }
> +
> + /* Pick one core to use for cpumask attributes */
> + cpumask_set_cpu(smp_processor_id(), &xgene_pmu->cpu);
> +
> + /* Make sure that the overflow interrupt is handled by this CPU */
> + rc = irq_set_affinity(irq, &xgene_pmu->cpu);
> + if (rc) {
> + dev_err(&pdev->dev, "Failed to set interrupt affinity!\n");
> + goto err;
> + }
> +
> + /* Enable interrupt */
> + xgene_pmu_unmask_int(xgene_pmu);
It's probably better to do this after probing the sub-devices.
> +
> + /* Walk through the tree for all PMU perf devices */
> + rc = xgene_pmu_probe_pmu_dev(xgene_pmu, pdev);
> + if (rc) {
> + dev_err(&pdev->dev, "No PMU perf devices found!\n");
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + if (xgene_pmu->pcppmu_csr)
> + devm_iounmap(&pdev->dev, xgene_pmu->pcppmu_csr);
> + devm_kfree(&pdev->dev, xgene_pmu);
> +
> + return rc;
> +}
Thanks,
Mark.
[toc] | [prev] | [next] | [standalone]
| From | Tai Tri Nguyen <ttnguyen@apm.com> |
|---|---|
| Date | 2016-07-06 23:50 +0200 |
| Subject | Re: [PATCH v6 3/4] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver |
| Message-ID | <rS2RI-6bB-17@gated-at.bofh.it> |
| In reply to | #1437871 |
Hi Mark,
Thanks.
On Wed, Jul 6, 2016 at 10:49 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> This looks mostly good now, though there are some remaining issues that
> I've commented on below.
>
> On Tue, Jun 28, 2016 at 11:50:44AM -0700, Tai Nguyen wrote:
>> Signed-off-by: Tai Nguyen <ttnguyen@apm.com>
>> ---
>> Documentation/perf/xgene-pmu.txt | 48 ++
>> drivers/perf/Kconfig | 7 +
>> drivers/perf/Makefile | 1 +
>> drivers/perf/xgene_pmu.c | 1360 ++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 1416 insertions(+)
>> create mode 100644 Documentation/perf/xgene-pmu.txt
>> create mode 100644 drivers/perf/xgene_pmu.c
>
>> +static const struct attribute *xgene_pmu_cpumask_attrs[] = {
>> + &dev_attr_cpumask.attr,
>> + NULL,
>> +};
>> +
>> +static const struct attribute_group pmu_cpumask_attr_group = {
>> + .attrs = (struct attribute **) xgene_pmu_cpumask_attrs,
>> +};
>
> As mentioned previously, we shouldn't cast away constness.
>
> Please remove the const from the definition of xgene_pmu_cpumask_attrs,
> and remove the cast.
>
Yes, my bad.
I changed the others but missed it for cpumark attr group.
[...]
>> +
>> + hw->config = event->attr.config;
>> + /*
>> + * Each bit of the config1 field represents an agent from which the
>> + * request of the event come. The event is counted only if it's caused
>> + * by a request of an agent has the bit cleared.
>> + * By default, the event is counted for all agents.
>> + */
>> + hw->config_base = event->attr.config1;
>> +
>> + return 0;
>> +}
>
> You also need to validate the event group as a whole. See the end of
> arm_ccn_pmu_event_init() in drivers/bus/arm-ccn.c for an example, where
> we check the leader and sibling list.
Okay thanks, I'll refer to the arm-ccn.c
>
>> +static void xgene_perf_enable_event(struct perf_event *event)
>> +{
>> + struct xgene_pmu_dev *pmu_dev = to_pmu_dev(event->pmu);
>> +
>> + xgene_pmu_write_evttype(pmu_dev, GET_CNTR(event), GET_EVENTID(event));
>> + xgene_pmu_write_agentmsk(pmu_dev, ~((u32)GET_AGENTID(event)));
>> + if (pmu_dev->inf->type == PMU_TYPE_IOB)
>> + xgene_pmu_write_agent1msk(pmu_dev, ~((u32)GET_AGENT1ID(event)));
>> +
>> + xgene_pmu_start_counters(pmu_dev);
>
> This call to xgene_pmu_start_counters should be moved out into
> pmu::pmu_enable(), which the core code will call for you. Similarly, you
> should implement pmu::pmu_disable() using xgene_pmu_stop_counters().
>
> That avoids poking the HW repeatedly to enable/disable all counters,
> ensures that event groups count for the same time, etc.
>
> In your pmu::pmu_enable() implementation you should probably check
> whether you have any events (e.g. by looking at the counter bitmap). If
> there are no events, you can avoid pointlessly enabling the PMU HW.
Okay, I'll implement pmu::pmu_enable() and pmu::pmu_disable() as per
your suggestion.
>
>> + xgene_pmu_enable_counter(pmu_dev, GET_CNTR(event));
>> + xgene_pmu_enable_counter_int(pmu_dev, GET_CNTR(event));
>> +}
>
> [...]
>
>> +static irqreturn_t xgene_pmu_isr(int irq, void *dev_id)
>> +{
>> + struct xgene_pmu_dev_ctx *ctx, *temp_ctx;
>> + struct xgene_pmu *xgene_pmu = dev_id;
>> + unsigned long flags;
>> + u32 val;
>> +
>> + raw_spin_lock_irqsave(&xgene_pmu->lock, flags);
>> +
>> + /* Get Interrupt PMU source */
>> + val = readl(xgene_pmu->pcppmu_csr + PCPPMU_INTSTATUS_REG);
>> + if (val & PCPPMU_INT_MCU) {
>> + list_for_each_entry_safe(ctx, temp_ctx,
>> + &xgene_pmu->mcpmus, next) {
>> + _xgene_pmu_isr(irq, ctx->pmu_dev);
>> + }
>> + }
>
> No handlers modify the list, so there's no need to use
> list_for_each_entry_safe, and you don't need the tmp_ctx variable.
Right, I will use list_for_each_entry instead.
>
>> + if (val & PCPPMU_INT_MCB) {
>> + list_for_each_entry_safe(ctx, temp_ctx,
>> + &xgene_pmu->mcbpmus, next) {
>> + _xgene_pmu_isr(irq, ctx->pmu_dev);
>> + }
>> + }
>> + if (val & PCPPMU_INT_L3C) {
>> + list_for_each_entry_safe(ctx, temp_ctx,
>> + &xgene_pmu->l3cpmus, next) {
>> + _xgene_pmu_isr(irq, ctx->pmu_dev);
>> + }
>> + }
>> + if (val & PCPPMU_INT_IOB) {
>> + list_for_each_entry_safe(ctx, temp_ctx,
>> + &xgene_pmu->iobpmus, next) {
>> + _xgene_pmu_isr(irq, ctx->pmu_dev);
>> + }
>> + }
>> +
>> + raw_spin_unlock_irqrestore(&xgene_pmu->lock, flags);
>> +
>> + return IRQ_HANDLED;
>> +}
[...]
>> +
>> +static acpi_status acpi_pmu_dev_add(acpi_handle handle, u32 level,
>> + void *data, void **return_value)
>> +{
>> + struct xgene_pmu *xgene_pmu = data;
>> + struct xgene_pmu_dev_ctx *ctx;
>> + struct acpi_device *adev;
>> +
>> + if (acpi_bus_get_device(handle, &adev))
>> + return AE_OK;
>> + if (acpi_bus_get_status(adev) || !adev->status.present)
>> + return AE_OK;
>> +
>> + if (!strcmp(acpi_device_hid(adev), "APMC0D5D"))
>> + ctx = acpi_get_pmu_hw_inf(xgene_pmu, adev, PMU_TYPE_L3C);
>> + else if (!strcmp(acpi_device_hid(adev), "APMC0D5E"))
>> + ctx = acpi_get_pmu_hw_inf(xgene_pmu, adev, PMU_TYPE_IOB);
>> + else if (!strcmp(acpi_device_hid(adev), "APMC0D5F"))
>> + ctx = acpi_get_pmu_hw_inf(xgene_pmu, adev, PMU_TYPE_MCB);
>> + else if (!strcmp(acpi_device_hid(adev), "APMC0D60"))
>> + ctx = acpi_get_pmu_hw_inf(xgene_pmu, adev, PMU_TYPE_MC);
>> + else
>> + ctx = NULL;
>> +
>> + if (!ctx)
>> + return AE_OK;
>> +
>> + if (xgene_pmu_dev_add(xgene_pmu, ctx))
>> + return AE_OK;
>
> Given that xgene_pmu_dev_add() failed, don't we leak resources allocated
> in acpi_get_pmu_hw_inf(), e.g. ctx?
>
> I don't think returning AE_OK is correct here.
>
> Why do we not fail to probe if this occurs?
I'll fix the ctx resource leakage if xgene_pmu_dev_add() fails.
However, I still need to return AE_OK for skipping the device.
Otherwise the whole acpi_walk_namespace() returns error and terminates
the search.
The problem is that MC PMUs mayn't be enabled if the DIMM doesn't exist.
Other common ACPI failures are still properly returned here.
[...]
>> +static int acpi_pmu_probe_pmu_dev(struct xgene_pmu *xgene_pmu,
>> + struct platform_device *pdev)
>> +{
>> + struct device *dev = xgene_pmu->dev;
>> + acpi_handle handle;
>> + acpi_status status;
>> +
>> + handle = ACPI_HANDLE(dev);
>> + if (!handle)
>> + return -EINVAL;
>> +
>> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
>> + acpi_pmu_dev_add, NULL, xgene_pmu, NULL);
>> + if (ACPI_FAILURE(status))
>> + dev_err(dev, "failed to probe PMU devices\n");
>> + return 0;
>> +}
>
> Surely we should pass on an error?
I'll fix it.
[...]
>> +static int fdt_pmu_probe_pmu_dev(struct xgene_pmu *xgene_pmu,
>> + struct platform_device *pdev)
>> +{
>> + struct xgene_pmu_dev_ctx *ctx;
>> + struct device_node *np;
>> +
>> + for_each_child_of_node(pdev->dev.of_node, np) {
>> + if (!of_device_is_available(np))
>> + continue;
>> +
>> + if (of_device_is_compatible(np, "apm,xgene-pmu-l3c"))
>> + ctx = fdt_get_pmu_hw_inf(xgene_pmu, np, PMU_TYPE_L3C);
>> + else if (of_device_is_compatible(np, "apm,xgene-pmu-iob"))
>> + ctx = fdt_get_pmu_hw_inf(xgene_pmu, np, PMU_TYPE_IOB);
>> + else if (of_device_is_compatible(np, "apm,xgene-pmu-mcb"))
>> + ctx = fdt_get_pmu_hw_inf(xgene_pmu, np, PMU_TYPE_MCB);
>> + else if (of_device_is_compatible(np, "apm,xgene-pmu-mc"))
>> + ctx = fdt_get_pmu_hw_inf(xgene_pmu, np, PMU_TYPE_MC);
>> + else
>> + ctx = NULL;
>> +
>> + if (!ctx)
>> + continue;
>> +
>> + if (xgene_pmu_dev_add(xgene_pmu, ctx))
>> + continue;
>
> As with acpi_pmu_dev_add, doesn't this leak the contexts allocated in
> fdt_get_pmu_hw_inf?
>
> Why do we not fail to probe if this occurs?
>
I'll fix the resource leakage in case of failure.
[...]
>
>> +static const struct xgene_pmu_data xgene_pmu_data = {
>> + .id = PCP_PMU_V1,
>> + .data = 0,
>> +};
>> +
>> +static const struct xgene_pmu_data xgene_pmu_v2_data = {
>> + .id = PCP_PMU_V2,
>> + .data = 0,
>> +};
>
> The data field on either of these is never used. I think you can remove
> it.
Sure, will remove it.
>
>> +static int xgene_pmu_probe(struct platform_device *pdev)
>> +{
>> + const struct xgene_pmu_data *dev_data;
>> + const struct of_device_id *of_id;
>> + struct xgene_pmu *xgene_pmu;
>> + struct resource *res;
>> + int irq, rc;
>> + int version;
>> +
>> + xgene_pmu = devm_kzalloc(&pdev->dev, sizeof(*xgene_pmu), GFP_KERNEL);
>> + if (!xgene_pmu)
>> + return -ENOMEM;
>> + xgene_pmu->dev = &pdev->dev;
>> + platform_set_drvdata(pdev, xgene_pmu);
>> +
>> + version = -EINVAL;
>> + of_id = of_match_device(xgene_pmu_of_match, &pdev->dev);
>> + if (of_id) {
>> + dev_data = (const struct xgene_pmu_data *) of_id->data;
>> + version = dev_data->id;
>> + }
>> +
>> +#ifdef CONFIG_ACPI
>> + if (ACPI_COMPANION(&pdev->dev)) {
>> + const struct acpi_device_id *acpi_id;
>> +
>> + acpi_id = acpi_match_device(xgene_pmu_acpi_match, &pdev->dev);
>> + if (acpi_id)
>> + version = (int) acpi_id->driver_data;
>> + }
>> +#endif
>> + if (version < 0)
>> + return -ENODEV;
>> +
>> + INIT_LIST_HEAD(&xgene_pmu->l3cpmus);
>> + INIT_LIST_HEAD(&xgene_pmu->iobpmus);
>> + INIT_LIST_HEAD(&xgene_pmu->mcbpmus);
>> + INIT_LIST_HEAD(&xgene_pmu->mcpmus);
>> +
>> + xgene_pmu->version = version;
>> + dev_info(&pdev->dev, "X-Gene PMU version %d\n", xgene_pmu->version);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + xgene_pmu->pcppmu_csr = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(xgene_pmu->pcppmu_csr)) {
>> + dev_err(&pdev->dev, "ioremap failed for PCP PMU resource\n");
>> + rc = PTR_ERR(xgene_pmu->pcppmu_csr);
>> + goto err;
>> + }
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq < 0) {
>> + dev_err(&pdev->dev, "No IRQ resource\n");
>> + rc = -EINVAL;
>> + goto err;
>> + }
>> + rc = devm_request_irq(&pdev->dev, irq, xgene_pmu_isr,
>> + IRQF_NOBALANCING | IRQF_NO_THREAD,
>> + dev_name(&pdev->dev), xgene_pmu);
>> + if (rc) {
>> + dev_err(&pdev->dev, "Could not request IRQ %d\n", irq);
>> + goto err;
>> + }
>> +
>> + raw_spin_lock_init(&xgene_pmu->lock);
>> +
>> + /* Check for active MCBs and MCUs */
>> + rc = xgene_pmu_probe_active_mcb_mcu(xgene_pmu, pdev);
>> + if (rc) {
>> + dev_warn(&pdev->dev, "Unknown MCB/MCU active status\n");
>> + xgene_pmu->mcb_active_mask = 0x1;
>> + xgene_pmu->mc_active_mask = 0x1;
>> + }
>> +
>> + /* Pick one core to use for cpumask attributes */
>> + cpumask_set_cpu(smp_processor_id(), &xgene_pmu->cpu);
>> +
>> + /* Make sure that the overflow interrupt is handled by this CPU */
>> + rc = irq_set_affinity(irq, &xgene_pmu->cpu);
>> + if (rc) {
>> + dev_err(&pdev->dev, "Failed to set interrupt affinity!\n");
>> + goto err;
>> + }
>> +
>> + /* Enable interrupt */
>> + xgene_pmu_unmask_int(xgene_pmu);
>
> It's probably better to do this after probing the sub-devices.
>
Okay, I'll move it after the sub-devices probing below.
>> +
>> + /* Walk through the tree for all PMU perf devices */
>> + rc = xgene_pmu_probe_pmu_dev(xgene_pmu, pdev);
>> + if (rc) {
>> + dev_err(&pdev->dev, "No PMU perf devices found!\n");
>> + goto err;
>> + }
>> +
>> + return 0;
Thanks,
--
Tai
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web