Message-ID: <20210611233355.GA183580@rocinante>
Date:   Sat, 12 Jun 2021 01:33:55 +0200
From:   Krzysztof Wilczyński <kw@...ux.com>
To:     Qi Liu <liuqi115@...wei.com>
Cc:     will@...nel.org, mark.rutland@....com, bhelgaas@...gle.com,
        linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linuxarm@...wei.com,
        zhangshaokun@...ilicon.com
Subject: Re: [PATCH v6 2/2] drivers/perf: hisi: Add driver for HiSilicon PCIe
 PMU

Hi Qi,

Thank you for sending the patch over!

[...]
> +/*
> + * This driver adds support for PCIe PMU RCiEP device. Related
> + * perf events are bandwidth, bandwidth utilization, latency
> + * etc.
> + *
> + * Copyright (C) 2021 HiSilicon Limited
> + * Author: Qi Liu<liuqi115@...wei.com>
> + */

A small nitpick: missing space between your name and the e-mail address.

[...]
> +static ssize_t hisi_pcie_event_sysfs_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct dev_ext_attribute *eattr;
> +
> +	eattr = container_of(attr, struct dev_ext_attribute, attr);
> +
> +	return sysfs_emit(buf, "config=0x%lx\n", (unsigned long)eattr->var);
> +}

I am not that familiar with the perf drivers, thus I might be completely
wrong here, but usually for sysfs objects a single value is preferred,
so that this "config=" technically would not be needed, unless this is
somewhat essential to the consumers of this attribute to know what the
value is?  What do you think?

[...]
> +static ssize_t hisi_pcie_identifier_show(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(dev_get_drvdata(dev));
> +
> +	return sysfs_emit(buf, "0x%x\n", pcie_pmu->identifier);
> +}

What about using the "%#x" formatting flag?  It would automatically
added the "0x" prefix, etc.

> +static ssize_t hisi_pcie_bus_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(dev_get_drvdata(dev));
> +
> +	return sysfs_emit(buf, "0x%02x\n", PCI_BUS_NUM(pcie_pmu->bdf_min));
> +}

Same as above, what about "%#02x"?

[...]
> +static bool hisi_pcie_pmu_valid_filter(struct perf_event *event,
> +				       struct hisi_pcie_pmu *pcie_pmu)
> +{
> +	u32 subev_idx = hisi_pcie_get_subevent(event);
> +	u32 event_idx = hisi_pcie_get_event(event);
> +	u32 requester_id = hisi_pcie_get_bdf(event);
> +
> +	if (subev_idx > HISI_PCIE_SUBEVENT_MAX ||
> +	    event_idx > HISI_PCIE_EVENT_MAX) {
> +		pci_err(pcie_pmu->pdev,
> +			"Max event index and max subevent index is: %d, %d.\n",
> +			HISI_PCIE_EVENT_MAX, HISI_PCIE_SUBEVENT_MAX);
> +		return false;
> +	}

Was this error message above intended to be a debug message?  It's a bit
opaque in terms what the error actually is here.  We might need to clear
it up a little.

[...]
> +static bool hisi_pcie_pmu_validate_event_group(struct perf_event *event)
> +{
> +	struct perf_event *sibling, *leader = event->group_leader;
> +	int counters = 1;

How big this counter could become?

Would it ever be greater than HISI_PCIE_MAX_COUNTERS?  I am asking, as
if it would be ever greater, then perhaps unsigned int would be better
to use, and if not, then perhaps something smaller than int?  What do
you think, does this even make sense to change?

[...]
> +static int hisi_pcie_pmu_event_init(struct perf_event *event)
> +{
> +	struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
> +
> +	event->cpu = pcie_pmu->on_cpu;
> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;
> +
> +	/* Sampling is not supported. */
> +	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
> +		return -EOPNOTSUPP;
> +
> +	if (!hisi_pcie_pmu_valid_filter(event, pcie_pmu)) {
> +		pci_err(pcie_pmu->pdev, "Invalid filter!\n");
> +		return -EINVAL;
> +	}

[...]
> +/*
> + * The bandwidth, latency, bus utilization and buffer occupancy features are
> + * calculated from data in HISI_PCIE_CNT and extended data in HISI_PCIE_EXT_CNT.
> + * Other features are obtained only by HISI_PCIE_CNT.
> + * So data and data_ext are processed in this function to get performanace
> + * value like, bandwidth, latency, etc.
> + */

A small typo in the world "performance" above.

[...]
> +static u64 hisi_pcie_pmu_get_performance(struct perf_event *event, u64 data,
> +					 u64 data_ext)
> +{
> +#define CONVERT_DW_TO_BYTE(x)	(sizeof(u32) * (x))

I would move this macro at the top alongside other constants and macros,
as here it makes the code harder to read.  What do you think?

[...]
> +static int hisi_pcie_pmu_irq_register(struct pci_dev *pdev,
> +				      struct hisi_pcie_pmu *pcie_pmu)
> +{
> +	int irq, ret;
> +
> +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> +	if (ret < 0) {
> +		pci_err(pdev, "Failed to enable MSI vectors, ret = %d!\n", ret);
> +		return ret;
> +	}

This is a nitpick, so feel free to ignore it, but what do you think of
changing this (and also other messages alike) message to be, for
example:

  pci_err(pdev, "Failed to enable MSI vectors: %d\n", ret);

Why?  I personally don't find displaying a return code/value followed by
a punctuation easy to read, especially when looking through a lot of
lines and other messages in the kernel ring buffer.

[...]
> +
> +	irq = pci_irq_vector(pdev, 0);
> +	ret = request_irq(irq, hisi_pcie_pmu_irq,
> +			  IRQF_NOBALANCING | IRQF_NO_THREAD, "hisi_pcie_pmu",
> +			  pcie_pmu);
> +	if (ret) {
> +		pci_err(pdev, "Failed to register irq, ret = %d!\n", ret);
> +		pci_free_irq_vectors(pdev);
> +		return ret;
> +	}

It would be "IRQ" in the error message above.

[...]
> +static int hisi_pcie_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> +	struct hisi_pcie_pmu *pcie_pmu = hlist_entry_safe(node,
> +					 struct hisi_pcie_pmu, node);
> +	unsigned int target;
> +
> +	/* Nothing to do if this CPU doesn't own the PMU */
> +	if (pcie_pmu->on_cpu != cpu)
> +		return 0;
> +
> +	/* Choose a new CPU from all online cpus. */
> +	target = cpumask_first(cpu_online_mask);
> +	if (target >= nr_cpu_ids) {
> +		pci_err(pcie_pmu->pdev, "There is no cpu to set!\n");
> +		return 0;
> +	}

To be consistent, it would be "CPUs" and "CPU" in the above.

[...]
> +static struct device_attribute hisi_pcie_pmu_bus_attr =
> +	__ATTR(bus, 0444, hisi_pcie_bus_show, NULL);
[...]
> +static struct device_attribute hisi_pcie_pmu_cpumask_attr =
> +	__ATTR(cpumask, 0444, hisi_pcie_cpumask_show, NULL);
[...]
> +static struct device_attribute hisi_pcie_pmu_identifier_attr =
> +	__ATTR(identifier, 0444, hisi_pcie_identifier_show, NULL);

Would it be at possible for any of the above __ATTR() macros to be
replaced with the DEVICE_ATTR_RO() macro?  Or perhaps with __ATTR_RO()
if the other one would be a good fit?

[...]
> +static int hisi_pcie_init_dev(struct pci_dev *pdev)
> +{
> +	int ret;
> +
> +	ret = pci_enable_device(pdev);
> +	if (ret) {
> +		pci_err(pdev, "Failed to enable pci device, ret = %d.\n", ret);
> +		return ret;
> +	}
> +
> +	ret = pci_request_mem_regions(pdev, "hisi_pcie_pmu");
> +	if (ret < 0) {
> +		pci_err(pdev, "Failed to request pci mem regions, ret = %d.\n",
> +			ret);
> +		pci_disable_device(pdev);
> +		return ret;
> +	}

It would be "PCI" in both error messages above.

[...]
> +static int __init hisi_pcie_module_init(void)
> +{
> +	int ret;
> +
> +	ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_PCIE_PMU_ONLINE,
> +				      "AP_PERF_ARM_HISI_PCIE_PMU_ONLINE",
> +				      hisi_pcie_pmu_online_cpu,
> +				      hisi_pcie_pmu_offline_cpu);
> +	if (ret) {
> +		pr_err("Failed to setup PCIE PMU hotplug, ret = %d.\n", ret);
> +		return ret;
> +	}

It would be "PCIe" in the error message above.

	Krzysztof

Powered by blists - more mailing lists