RE: [PATCH v4 2/2] perf: Fujitsu: Add the Uncore PCI PMU driver

[Yoshihiro Furudera (Fujitsu)]

Hi, Jonathan
Thanks for you review/comments.

> On Thu, 16 Jan 2025 04:59:11 +0000
> Yoshihiro Furudera <fj5100bi@xxxxxxxxxxx> wrote:
> 
> > This adds a new dynamic PMU to the Perf Events framework to program
> > and control the Uncore PCI PMUs in Fujitsu chips.
> >
> > This driver was created with reference to drivers/perf/qcom_l3_pmu.c.
> >
> > This driver exports formatting and event information to sysfs so it
> > can be used by the perf user space tools with the syntaxes:
> >
> > perf stat -e pci_iod0_pci0/ea-pci/ ls
> > perf stat -e pci_iod0_pci0/event=0x80/ ls
> >
> > FUJITSU-MONAKA Specification URL:
> > https://github.com/fujitsu/FUJITSU-MONAKA
> >
> > Signed-off-by: Yoshihiro Furudera <fj5100bi@xxxxxxxxxxx>
> Hi,
> 
> As you can probably guess, similar comments in here.
> Assuming those little things tidied up feel free to add
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> to your v5.

I've got it.

> 
> > diff --git a/drivers/perf/fujitsu_pci_pmu.c
> > b/drivers/perf/fujitsu_pci_pmu.c new file mode 100644 index
> > 000000000000..2ce2ca19b5ea
> > --- /dev/null
> > +++ b/drivers/perf/fujitsu_pci_pmu.c
> > @@ -0,0 +1,553 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Driver for the Uncore PCI PMUs in Fujitsu chips.
> > + *
> > + * See Documentation/admin-guide/perf/fujitsu_pci_pmu.rst for more details.
> > + *
> > + * This driver is based on drivers/perf/qcom_l3_pmu.c
> > + * Copyright (c) 2015-2017, The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2024 Fujitsu. All rights reserved.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/bitops.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> 
> mod_devicetable.h probably should be here.

After checking, I found that
linux/mod_devicetable.h is included in linux/acpi.h.

> 
> > +#include <linux/perf_event.h>
> > +#include <linux/platform_device.h>
> > +
> > +/* Number of counters on each PMU */
> > +#define PCI_NUM_COUNTERS  8
> > +/* Mask for the event type field within perf_event_attr.config and EVTYPE reg
> */
> > +#define PCI_EVTYPE_MASK   0xFF
> > +
> > +/* Perfmon registers */
> > +#define PCI_PM_EVCNTR(__cntr) (0x000 + __cntr * 8) #define
> > +PCI_PM_CNTCTL(__cntr) (0x100 + __cntr * 8) #define
> > +PCI_PM_EVTYPE(__cntr) (0x200 + __cntr * 8)
> (__cntr)
> 
> > +/*
> > + * We must NOT create groups containing events from multiple hardware
> > +PMUs,
> > + * although mixing different software and hardware PMUs is allowed.
> > + */
> > +static bool fujitsu_pci__validate_event_group(struct perf_event
> > +*event) {
> > +	struct perf_event *leader = event->group_leader;
> > +	struct perf_event *sibling;
> > +	int counters = 0;
> > +
> > +	if (leader->pmu != event->pmu && !is_software_event(leader))
> > +		return false;
> > +
> > +	/* The sum of the counters used by the event and its leader event */
> > +	counters = 2;
> > +
> > +	for_each_sibling_event(sibling, leader) {
> > +		if (is_software_event(sibling))
> > +			continue;
> > +		if (sibling->pmu != event->pmu)
> > +			return false;
> > +		counters += 1;
> counters++;

I'll change "counters += 1" to "counters++"

> 
> > +	}
> > +
> > +	/*
> > +	 * If the group requires more counters than the HW has, it
> > +	 * cannot ever be scheduled.
> > +	 */
> > +	return counters <= PCI_NUM_COUNTERS; }
> 
> > +
> > +static const struct attribute_group fujitsu_pci_pmu_events_group = {
> > +	.name = "events",
> > +	.attrs = fujitsu_pci_pmu_events
> As below
> 
> > +};
> > +
> > +static ssize_t cpumask_show(struct device *dev,
> > +			    struct device_attribute *attr, char *buf) {
> > +	struct pci_pmu *pcipmu = to_pci_pmu(dev_get_drvdata(dev));
> > +
> > +	return cpumap_print_to_pagebuf(true, buf, &pcipmu->cpumask); }
> > +
> > +static DEVICE_ATTR_RO(cpumask);
> > +
> > +static struct attribute *fujitsu_pci_pmu_cpumask_attrs[] = {
> > +	&dev_attr_cpumask.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group fujitsu_pci_pmu_cpumask_attr_group = {
> > +	.attrs = fujitsu_pci_pmu_cpumask_attrs
> 
> Another trailing comma missing here.

I'll add trailing comma.
I'll do the same for the other Non-NULL parts too.

> 
> > +static int fujitsu_pci_pmu_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct acpi_device *acpi_dev;
> > +	struct pci_pmu *pcipmu;
> > +	struct resource *memrc;
> > +	char *name;
> > +	int ret;
> > +	u64 uid;
> > +
> > +	acpi_dev = ACPI_COMPANION(dev);
> > +	if (!acpi_dev)
> > +		return -ENODEV;
> > +
> > +	ret = acpi_dev_uid_to_integer(acpi_dev, &uid);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "unable to read ACPI uid\n");
> > +
> > +	pcipmu = devm_kzalloc(dev, sizeof(*pcipmu), GFP_KERNEL);
> > +	if (!pcipmu)
> > +		return -ENOMEM;
> > +
> > +	name = devm_kasprintf(dev, GFP_KERNEL, "pci_iod%llu_pci%llu",
> > +			  (uid >> 4) & 0xF, uid & 0xF);
> > +	if (!name)
> > +		return -ENOMEM;
> > +
> > +	pcipmu->pmu = (struct pmu) {
> > +		.parent		= dev,
> > +		.task_ctx_nr	= perf_invalid_context,
> > +
> > +		.pmu_enable	= fujitsu_pci__pmu_enable,
> > +		.pmu_disable	= fujitsu_pci__pmu_disable,
> > +		.event_init	= fujitsu_pci__event_init,
> > +		.add		= fujitsu_pci__event_add,
> > +		.del		= fujitsu_pci__event_del,
> > +		.start		= fujitsu_pci__event_start,
> > +		.stop		= fujitsu_pci__event_stop,
> > +		.read		= fujitsu_pci__event_read,
> > +
> > +		.attr_groups	= fujitsu_pci_pmu_attr_grps,
> > +		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE
> 
> As in previous.  Add trailing commas to all structure field fills that aren't
> terminating NULL type entries.
> 
> > +	};
> > +
> > +	pcipmu->regs = devm_platform_get_and_ioremap_resource(pdev, 0,
> &memrc);
> > +	if (IS_ERR(pcipmu->regs))
> > +		return PTR_ERR(pcipmu->regs);
> > +
> > +	fujitsu_pci__init(pcipmu);
> > +
> > +	ret = platform_get_irq(pdev, 0);
> > +	if (ret <= 0)
> > +		return ret;
> > +
> > +	ret = devm_request_irq(dev, ret, fujitsu_pci__handle_irq, 0,
> > +			       name, pcipmu);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Request for IRQ failed for slice
> @%pa\n",
> > +						&memrc->start);
> > +
> > +	/* Add this instance to the list used by the offline callback */
> > +	ret = cpuhp_state_add_instance(pci_pmu_cpuhp_state,
> &pcipmu->node);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Error registering hotplug");
> > +
> > +	ret = perf_pmu_register(&pcipmu->pmu, name, -1);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Failed to register PCI PMU\n");
> > +
> > +	dev_dbg(dev, "Registered %s, type: %d\n", name, pcipmu->pmu.type);
> > +
> > +	return 0;
> > +}

Best Regards,
Yoshihiro Furudera