Date:   Wed, 16 Jun 2021 09:09:40 +0800
From:   "liuqi (BA)" <liuqi115@...wei.com>
To:     Krzysztof Wilczyński <kw@...ux.com>,
        Linuxarm <linuxarm@...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>, <zhangshaokun@...ilicon.com>
Subject: Re: [PATCH v6 2/2] drivers/perf: hisi: Add driver for HiSilicon PCIe
 PMU


Hi Krzysztof,
On 2021/6/12 7:33, Krzysztof Wilczyński wrote:
> 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.
> 
thanks, will fix this.
> [...]
>> +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?
"config=" is a supported for userspace tool, it is a kind of alias, so 
cannot be remover here, thanks.
> 
> [...]
>> +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.
> 
thanks, will fix this.
>> +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"?
> 
thanks, will fix this.
> [...]
>> +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.
> 
thanks, will change this message to pci_dbg next time.
> [...]
>> +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?
> 
I think this "counter" is used to caculate how many events have been set 
in cmdline, so it will always bigger than zero. So int and u32 seems 
same here.Thanks,

> [...]
>> +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.
> 
thanks, will fix this.
> [...]
>> +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.
> 

got it, will fix this next time.
> [...]
>> +
>> +	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.
> 
ok, will change this, thanks.
> [...]
>> +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?
> 
yes， DEVICE_ATTR_RO() macro could be used here, thanks.
> [...]
>> +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.
> 
will fix it.

> [...]
>> +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.
> 
will fix it.
Thanks,
Qi
> 	Krzysztof
> .
> 

Powered by blists - more mailing lists