Message-ID: <fb795e51-ce01-e976-ac09-d3f384307623@huawei.com>
Date:   Mon, 12 Apr 2021 18:21:24 +0100
From:   John Garry <john.garry@...wei.com>
To:     "liuqi (BA)" <liuqi115@...wei.com>,
        "will@...nel.org" <will@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>
CC:     "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Zhangshaokun <zhangshaokun@...ilicon.com>
Subject: Re: [PATCH v2 1/2] drivers/perf: hisi: Add driver for HiSilicon PCIe
 PMU

On 12/04/2021 14:34, liuqi (BA) wrote:
> 
> Hi John,
> 
> Thanks for reviewing this.
> On 2021/4/9 18:22, John Garry wrote:
>> On 09/04/2021 10:05, Qi Liu wrote:
>>> PCIe PMU Root Complex Integrated End Point(RCiEP) device is supported
>>> to sample bandwidth, latency, buffer occupation etc.
>>>
>>> Each PMU RCiEP device monitors multiple Root Ports, and each RCiEP is
>>> registered as a PMU in /sys/bus/event_source/devices, so users can
>>> select target PMU, and use filter to do further sets.

side note: it would be good to mention what baseline the series is based 
on in the cover letter

>>>
>>> Filtering options contains:
>>> event        - select the event.
>>> subevent     - select the subevent.
>>> port         - select target Root Ports. Information of Root Ports
>>>                  are shown under sysfs.
>>> bdf          - select requester_id of target EP device.
>>> trig_len     - set trigger condition for starting event statistics.
>>> trigger_mode - set trigger mode. 0 means starting to statistic when
>>>                  bigger than trigger condition, and 1 means smaller.
>>> thr_len      - set threshold for statistics.
>>> thr_mode     - set threshold mode. 0 means count when bigger than
>>>                  threshold, and 1 means smaller.
>>>
>>> Signed-off-by: Qi Liu <liuqi115@...wei.com>
>>> ---
>>>    MAINTAINERS                                |    6 +
>>>    drivers/perf/Kconfig                       |    2 +
>>>    drivers/perf/Makefile                      |    1 +
>>>    drivers/perf/pci/Kconfig                   |   16 +
>>>    drivers/perf/pci/Makefile                  |    2 +
>>>    drivers/perf/pci/hisilicon/Makefile        |    5 +
>>>    drivers/perf/pci/hisilicon/hisi_pcie_pmu.c | 1011
>>> ++++++++++++++++++++++++++++
>>>    include/linux/cpuhotplug.h                 |    1 +
>>>    8 files changed, 1044 insertions(+)
>>>    create mode 100644 drivers/perf/pci/Kconfig
>>>    create mode 100644 drivers/perf/pci/Makefile
>>>    create mode 100644 drivers/perf/pci/hisilicon/Makefile
>>>    create mode 100644 drivers/perf/pci/hisilicon/hisi_pcie_pmu.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 3353de0..46c7861 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -8023,6 +8023,12 @@ W:    http://www.hisilicon.com
>>>    F:    Documentation/admin-guide/perf/hisi-pmu.rst
>>>    F:    drivers/perf/hisilicon
>>> +HISILICON PCIE PMU DRIVER
>>> +M:    Qi Liu <liuqi115@...wei.com>
>>> +S:    Maintained
>>> +F:    Documentation/admin-guide/perf/hisi-pcie-pmu.rst
>>
>> nit: this does not exist yet...
>>
> thanks, I'll move this add-maintainer-part to the second patch.

that's why I advocate the documentation first :)

>>> +F:    drivers/perf/pci/hisilicon/hisi_pcie_pmu.c
>>> +
>>>    HISILICON QM AND ZIP Controller DRIVER
>>>    M:    Zhou Wang <wangzhou1@...ilicon.com>
>>>    L:    linux-crypto@...r.kernel.org
>>> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
>>> index 3075cf1..99d4760 100644
>>> --- a/drivers/perf/Kconfig
>>> +++ b/drivers/perf/Kconfig
>>> @@ -139,4 +139,6 @@ config ARM_DMC620_PMU
>>>    source "drivers/perf/hisilicon/Kconfig"
>>> +source "drivers/perf/pci/Kconfig"
>>> +
>>>    endmenu
>>> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
>>> index 5260b11..1208c08 100644
>>> --- a/drivers/perf/Makefile
>>> +++ b/drivers/perf/Makefile
>>> @@ -14,3 +14,4 @@ obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
>>>    obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
>>>    obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
>>>    obj-$(CONFIG_ARM_DMC620_PMU) += arm_dmc620_pmu.o
>>> +obj-y += pci/
>>> diff --git a/drivers/perf/pci/Kconfig b/drivers/perf/pci/Kconfig
>>> new file mode 100644
>>> index 0000000..a119486
>>> --- /dev/null
>>> +++ b/drivers/perf/pci/Kconfig
>>> @@ -0,0 +1,16 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +#
>>> +# PCIe Performance Monitor Drivers
>>> +#
>>> +menu "PCIe Performance Monitor"
>>> +
>>> +config HISI_PCIE_PMU
>>> +    tristate "HiSilicon PCIE PERF PMU"
>>> +    depends on ARM64 && PCI && HISI_PMU
>>
>> What from HISI_PMU is needed? I couldn't find anything here
> The event_sysfs_show() and format_sysfs_show() function of
> hisi_uncore_pmu.h can be reused in hisi_pcie_pmu.c, So I add path in
> Makefile and include "hisi_uncore_pmu.h".

Right, but it would be nice to be able to build this under COMPILE_TEST. 
CONFIG_HISI_PMU cannot be built under COMPILE_TEST, so nice to not 
depend on it.

So you could put hisi_event_sysfs_show() as a static inline in 
hisi_uncore_pmu.h, so the dependency can be removed

Having said that, there is nothing really hisi specific in those 
functions like hisi_event_sysfs_show().

Can't we just create generic functions here?

hisi_event_sysfs_show() == cci400_pmu_cycle_event_show() == 
xgene_pmu_event_show()

> 
>>
>>> +    help
>>> +      Provide support for HiSilicon PCIe performance monitoring unit
>>> (PMU)
>>> +      RCiEP devices.
>>> +      Adds the PCIe PMU into perf events system for monitoring latency,
>>> +      bandwidth etc.
>>> +





>>> +#define HISI_PCIE_CHECK_EVENTS(name, list)             \
>>
>> "check" in a function name is too vague, as it does not imply any
>> side-effect from calling this function.
>>
>> And I think "build" or similar would be good to be included in the macro
>> name.
>>
>>> +    static inline bool is_##name##_event(u32 idx)                  \
>>
>> why inline?
>>
>>> +
>>> {                                                                   \
>>> +        if ((idx >= list[0] && idx <= list[1]) || idx == list[2])  \
>>> +            return true;                                        \
>>> +        return false;                                               \
>>
>> isn't this just:
>> {
>>       return (idx >= list[0] && idx <= list[1]) || idx == list[2])
>> }
> 
> thanks, will address in next version.
>>
>>> +    }
>>> +
>>> +/*
>>> + * The first element of event list is minimum index of TL-layer events
>>> + * and the second element is maximum index. The third element is index
>>> + * of a DL-layer event.
>>> + */
>>> +static const u32 bw_events_list[] = {0x04, 0x08, 0x84};
>>> +static const u32 latency_events_list[] = {0x10, 0x15, 0x85};
>>> +static const u32 bus_util_events_list[] = {0x20, 0x24, 0x09};
>>> +static const u32 buf_util_events_list[] = {0x28, 0x2a, 0x33};
>>> +
>>> +HISI_PCIE_CHECK_EVENTS(bw, bw_events_list);
>>> +HISI_PCIE_CHECK_EVENTS(latency, latency_events_list);
>>> +HISI_PCIE_CHECK_EVENTS(bus_util, bus_util_events_list);
>>> +HISI_PCIE_CHECK_EVENTS(buf_util, buf_util_events_list);
>>
>> Surely the macro can be fixed such that you need to have:
>> HISI_PCIE_CHECK_EVENTS(buf_util);
>>
> will address in next version.
>>
>>> +
>>> +static ssize_t hisi_pcie_cpumask_show(struct device *dev,
>>> +                      struct device_attribute *attr, char *buf)
>>> +{
>>> +    struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(dev_get_drvdata(dev));
>>> +
>>> +    return cpumap_print_to_pagebuf(true, buf, &pcie_pmu->cpumask);
>>> +}
>>> +
>>> +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);
>>> +}
>>> +
>>> +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));
>>> +}
>>> +
>>> +static void hisi_pcie_parse_data(struct hisi_pcie_pmu *pcie_pmu, u32
>>> reg_off,
>>
>> hisi_pcie_parse_data() name is way too vague. I mean, parse what data?
>>
>> is there is some standard format register we use for various data types,
>> then make that clear please
> Seems that we only have lower_32_bits() but do not have lower_16_bits().
> perhaps we could change the function name to hisi_pcie_parse_reg_value()?

ok, so even that name is better.

> 
>>
>>> +                   u16 *arg0, u16 *arg1)
>>> +{
>>> +    u32 val = readl(pcie_pmu->base + reg_off);
>>> +
>>> +    *arg0 = val & 0xffff;
>>> +    *arg1 = (val & 0xffff0000) >> 16;
>>> +}
>>> +
>>> +static u32 hisi_pcie_pmu_get_offset(u32 offset, u32 idx)
>>> +{
>>> +    return offset + HISI_PCIE_REG_STEP * idx;
>>> +}
>>> +
>>> +static u32 hisi_pcie_pmu_readl(struct hisi_pcie_pmu *pcie_pmu, u32
>>> reg_offset,
>>> +                   u32 idx)
>>> +{
>>> +    u32 offset = hisi_pcie_pmu_get_offset(reg_offset, idx);
>>> +
>>> +    return readl(pcie_pmu->base + offset);
>>> +}
>>> +
>>> +static void hisi_pcie_pmu_writel(struct hisi_pcie_pmu *pcie_pmu, u32
>>> reg_offset,
>>> +                 u32 idx, u32 val)
>>> +{
>>> +    u32 offset = hisi_pcie_pmu_get_offset(reg_offset, idx);
>>> +
>>> +    writel(val, pcie_pmu->base + offset);
>>> +}
>>> +
>>> +static u64 hisi_pcie_pmu_readq(struct hisi_pcie_pmu *pcie_pmu, u32
>>> reg_offset,
>>> +                   u32 idx)
>>> +{
>>> +    u32 offset = hisi_pcie_pmu_get_offset(reg_offset, idx);
>>> +
>>> +    return readq(pcie_pmu->base + offset);
>>> +}
>>> +
>>> +static void hisi_pcie_pmu_writeq(struct hisi_pcie_pmu *pcie_pmu, u32
>>> reg_offset,
>>> +                 u32 idx, u64 val)
>>> +{
>>> +    u32 offset = hisi_pcie_pmu_get_offset(reg_offset, idx);
>>> +
>>> +    writeq(val, pcie_pmu->base + offset);
>>> +}
>>> +
>>> +static u64 hisi_pcie_pmu_get_event(struct perf_event *event)
>>
>> that function name is too vague
>>
>>> +{
>>> +    return FIELD_PREP(HISI_PCIE_EVENT_M, hisi_pcie_get_event(event)) |
>>> +           FIELD_PREP(HISI_PCIE_SUBEVENT_M,
>>> hisi_pcie_get_subevent(event));
>>> +}
>>> +
>>> +static u64 hisi_pcie_pmu_get_target(struct perf_event *event)
>>
>> same here
>>
>>> +{
>>> +    u64 port, val;
>>> +
>>> +    port = hisi_pcie_get_port(event);
>>> +    if (port) {
>>> +        val = FIELD_PREP(HISI_PCIE_TARGET_M, port);
>>> +    } else {
>>> +        val = FIELD_PREP(HISI_PCIE_TARGET_M, hisi_pcie_get_bdf(event));
>>> +        val |= HISI_PCIE_TARGET_EN;
>>> +    }
>>> +
>>> +    return val;
>>> +}
>>> +
>>> +static u64 hisi_pcie_pmu_get_trigger(struct perf_event *event)
>>
>> and here
>>
>> what you really seem to be doing is getting the formatted fields for the
>> register
>>
>>
>> Having all these call-once functions seem to make life a bit more
>> difficult...
>>
> Do you mean config all the filters in one function?, will fix it, thanks.

Yes, I would suggest one function

> 
>>> +{
>>> +    u64 trig_len, val;
>>> +
>>> +    trig_len = hisi_pcie_get_trig_len(event);
>>> +    if (!trig_len)
>>> +        return 0;
>>> +
>>> +    val = FIELD_PREP(HISI_PCIE_TRIG_M, trig_len);
>>> +    val |= FIELD_PREP(HISI_PCIE_TRIG_MODE_M,
>>> +              hisi_pcie_get_trig_mode(event));
>>> +    val |= HISI_PCIE_TRIG_EN;
>>> +
>>> +    return val;
>>> +}
>>> +
>>> +static u64 hisi_pcie_pmu_get_thr(struct perf_event *event)
>>> +{
>>> +    u64 thr_len, val;
>>> +
>>> +    thr_len = hisi_pcie_get_thr_len(event);
>>> +    if (!thr_len)
>>> +        return 0;
>>> +
>>> +    val = FIELD_PREP(HISI_PCIE_THR_M, thr_len);
>>> +    val |= FIELD_PREP(HISI_PCIE_THR_MODE_M,
>>> hisi_pcie_get_thr_mode(event));
>>> +    val |= HISI_PCIE_THR_EN;
>>> +
>>> +    return val;
>>> +}
>>> +
>>> +static void hisi_pcie_pmu_config_filter(struct perf_event *event)
>>> +{
>>> +    struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
>>> +    struct hw_perf_event *hwc = &event->hw;
>>> +    u32 idx = hwc->idx;
>>> +    u64 val;
>>> +
>>> +    val = HISI_PCIE_DEFAULT_SET;
>>> +    val |= hisi_pcie_pmu_get_event(event);
>>> +    val |= hisi_pcie_pmu_get_target(event);
>>> +    val |= hisi_pcie_pmu_get_trigger(event);
>>> +    val |= hisi_pcie_pmu_get_thr(event);
>>> +    hisi_pcie_pmu_writeq(pcie_pmu, HISI_PCIE_EVENT_CTRL, idx, val);
>>> +}
>>> +
>>> +static void hisi_pcie_pmu_clear_filter(struct perf_event *event)
>>> +{
>>> +    struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
>>> +    struct hw_perf_event *hwc = &event->hw;
>>> +
>>> +    hisi_pcie_pmu_writeq(pcie_pmu, HISI_PCIE_EVENT_CTRL, hwc->idx,
>>> +                 HISI_PCIE_DEFAULT_SET);
>>> +}
>>> +
>>> +static bool hisi_pcie_pmu_valid_port(struct hisi_pcie_pmu *pcie_pmu,
>>> u16 bdf)
>>> +{
>>> +    return bdf <= pcie_pmu->bdf_max && bdf >= pcie_pmu->bdf_min;
>>
>> nit: it is standard to have:
>>
>>       if (x >= min && x <= max)
>>
>> i.e. have min on the left
>>
>> 2nd nit: it is passed bdf, yet "port" is in the name
>>
> will fix the function, bdf means the BDF of Root Port, I'll use rp_bdf
> in bext version to describe it.

ok

>>
>>> +}
>>> +
>>> +static bool hisi_pcie_pmu_valid_requester_id(struct hisi_pcie_pmu
>>> *pcie_pmu,
>>> +                        u32 bdf)
>>> +{
>>> +    struct pci_dev *root_port, *pdev;
>>> +
>>> +    pdev =
>>> pci_get_domain_bus_and_slot(pci_domain_nr(pcie_pmu->pdev->bus),
>>> +                       PCI_BUS_NUM(bdf),
>>> +                       GET_PCI_DEVFN(bdf));
>>> +    if (!pdev)
>>> +        return false;
>>> +
>>> +    root_port = pcie_find_root_port(pdev);
>>> +    if (!root_port)
>>> +        return false;
>>> +
>>> +    pci_dev_put(pdev);
>>> +    return hisi_pcie_pmu_valid_port(pcie_pmu, pci_dev_id(root_port));
>>> +}
>>> +
>>> +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);
>>
>> if this is just going to be fed back to userspace, I don't see why we
>> need a kernel log
>>
>> and the only caller also triggers an error message, which I doubt is needed
>>
> Print out the HISI_PCIE_EVENT_MAX and HISI_PCIE_SUBEVENT_MAX here may be
> more convenient for users to get the right value.
> If you this is redundant I'll remove it. :)

Don't we already tell this to userspace?

>>> +        return false;
>>> +    }
>>> +
>>> +    if (hisi_pcie_get_thr_len(event) > HISI_PCIE_THR_MAX_VAL)
>>> +        return false;
>>> +
>>> +    if (hisi_pcie_get_trig_len(event) > HISI_PCIE_TRIG_MAX_VAL)
>>> +        return false;
>>> +
>>> +    if (requester_id) {
>>> +        if (!hisi_pcie_pmu_valid_requester_id(pcie_pmu, requester_id)) {
>>> +            pci_err(pcie_pmu->pdev, "Invalid requester id.\n");
>>
>> see previous comments
>>
>>> +            return false;
>>> +        }
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>> +static bool hisi_pcie_pmu_validate_event_group(struct perf_event *event)
>>> +{
>>> +    struct perf_event *sibling, *leader = event->group_leader;
>>> +    int counters = 1;
>>> +
>>> +    if (!is_software_event(leader)) {
>>> +        if (leader->pmu != event->pmu)
>>> +            return false;
>>> +
>>> +        if (leader != event)
>>> +            counters++;
>>> +    }
>>> +
>>> +    for_each_sibling_event(sibling, event->group_leader) {
>>> +        if (is_software_event(sibling))
>>> +            continue;
>>> +
>>> +        if (sibling->pmu != event->pmu)
>>> +            return false;
>>> +
>>> +        counters++;
>>> +    }
>>> +
>>> +    return counters <= HISI_PCIE_MAX_COUNTERS;
>>> +}
>>> +
>>> +static int hisi_pcie_pmu_event_init(struct perf_event *event)
>>> +{
>>> +    struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
>>> +
>>> +    event->cpu = cpumask_first(&pcie_pmu->cpumask);
>>> +
>>> +    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;
>>> +
>>> +    /* Per-task mode is not supported. */
>>> +    if (event->cpu < 0)
>>
>> cpumask_first() gives an unsigned int - this can never happen

please fix this!

>>
>>> +        return -EINVAL;
>>> +
>>> +    if (!hisi_pcie_pmu_valid_filter(event, pcie_pmu)) {
>>> +        pci_err(pcie_pmu->pdev, "Invalid filter!\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (!hisi_pcie_pmu_validate_event_group(event))
>>> +        return -EINVAL;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static u64 hisi_pcie_pmu_process_data(struct perf_event *event, u64 val,



>>> +
>>> +static void hisi_pcie_pmu_disable_int(struct hisi_pcie_pmu *pcie_pmu,
>>> +                      struct hw_perf_event *hwc)
>>> +{
>>> +    u32 idx = hwc->idx;
>>> +
>>> +    hisi_pcie_pmu_writel(pcie_pmu, HISI_PCIE_INT_MASK, idx, 1);
>>> +}
>>> +
>>> +static void hisi_pcie_pmu_reset_counter(struct hisi_pcie_pmu *pcie_pmu,
>>> +                    struct hw_perf_event *hwc)
>>> +{
>>> +    u32 idx = hwc->idx;
>>> +
>>> +    hisi_pcie_pmu_writeq(pcie_pmu, HISI_PCIE_EVENT_CTRL, idx,
>>> +                 HISI_PCIE_RESET_CNT);
>>> +    hisi_pcie_pmu_writeq(pcie_pmu, HISI_PCIE_EVENT_CTRL, idx,
>>> +                 HISI_PCIE_DEFAULT_SET);
>>> +}
>>> +
>>> +static void hisi_pcie_pmu_start(struct perf_event *event, int flags)
>>> +{
>>> +    struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
>>> +    struct hw_perf_event *hwc = &event->hw;
>>> +    struct hw_perf_event_extra *hwc_ext;
>>> +    u64 prev_cnt, prev_cnt_ext;
>>> +
>>> +    hwc_ext = &hwc->extra_reg;
>>> +    if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
>>> +        return;
>>> +
>>> +    WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
>>> +    hwc->state = 0;
>>> +
>>> +    hisi_pcie_pmu_config_filter(event);
>>> +    hisi_pcie_pmu_enable_counter(pcie_pmu, hwc);
>>> +    hisi_pcie_pmu_enable_int(pcie_pmu, hwc);
>>> +    hisi_pcie_pmu_set_period(event);
>>> +
>>> +    if (flags & PERF_EF_RELOAD) {
>>> +        prev_cnt = local64_read(&hwc->prev_count);
>>> +        prev_cnt_ext = hwc_ext->config;
>>> +        hisi_pcie_pmu_write_counter(event, prev_cnt, prev_cnt_ext);
>>> +    }
>>> +
>>> +    perf_event_update_userpage(event);
>>> +}
>>> +
>>> +static void hisi_pcie_pmu_stop(struct perf_event *event, int flags)
>>> +{
>>> +    struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
>>> +    struct hw_perf_event *hwc = &event->hw;
>>> +
>>> +    hisi_pcie_pmu_read(event);
>>
>> why do this - it just reads a register? and so why not check the return
>> value?
>>
> hisi_pcie_pmu_read() is a void function, perhaps I should use
> hisi_pcie_pmu_update_counter().

I am not sure what your intention is with calling hisi_pcie_pmu_read(), 
so cannot advise

>>> +    hisi_pcie_pmu_disable_int(pcie_pmu, hwc);
>>> +    hisi_pcie_pmu_disable_counter(pcie_pmu, hwc);
>>> +    hisi_pcie_pmu_clear_filter(event);
>>> +    WARN_ON_ONCE(hwc->state & PERF_HES_STOPPED);
>>> +    hwc->state |= PERF_HES_STOPPED;
>>> +
>>> +    if (hwc->state & PERF_HES_UPTODATE)
>>> +        return;
>>> +
>>> +    hwc->state |= PERF_HES_UPTODATE;
>>> +}
>>> +
>>> +static int hisi_pcie_pmu_add(struct perf_event *event, int flags)
>>> +{
>>> +    struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
>>> +    struct hw_perf_event *hwc = &event->hw;
>>> +    int idx;
>>> +
>>> +    hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
>>> +    idx = hisi_pcie_pmu_get_event_idx(pcie_pmu);
>>> +    if (idx < 0)
>>> +        return idx;
>>> +
>>> +    hwc->idx = idx;
>>> +    pcie_pmu->hw_events[idx] = event;
>>> +
>>> +    /* Reset PMC to avoid interference caused by previous statistic. */



>>> +
>>> +static void hisi_pcie_pmu_irq_unregister(struct pci_dev *pdev,
>>> +                     struct hisi_pcie_pmu *pcie_pmu)
>>> +{
>>> +    free_irq(pcie_pmu->irq, pcie_pmu);
>>> +    pci_free_irq_vectors(pdev);
>>> +}
>>> +
>>> +static int hisi_pcie_pmu_online_cpu(unsigned int cpu, struct
>>> hlist_node *node)
>>> +{
>>> +    struct hisi_pcie_pmu *pcie_pmu = hlist_entry_safe(node,
>>> +                     struct hisi_pcie_pmu, node);
>>> +
>>> +    if (cpumask_empty(&pcie_pmu->cpumask)) {
>>> +        cpumask_set_cpu(cpu, &pcie_pmu->cpumask);
>>> +        WARN_ON(irq_set_affinity_hint(pcie_pmu->irq, cpumask_of(cpu)));
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +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;
>>> +
>>> +    if (!cpumask_test_and_clear_cpu(cpu, &pcie_pmu->cpumask))
>>
>> I do wonder why we even need maintain pcie_pmu->cpumask
>>
>> Can't we just use cpu_online_mask as appropiate instead?

?

>>
>>> +        return 0;
>>> +
>>> +    /* Choose a new CPU from all online cpus. */
>>> +    target = cpumask_any_but(cpu_online_mask, cpu);
>>> +    if (target >= nr_cpu_ids)
>>> +        return 0;
>>> +
>>> +    perf_pmu_migrate_context(&pcie_pmu->pmu, cpu, target);
>>> +    WARN_ON(irq_set_affinity_hint(pcie_pmu->irq, cpumask_of(target)));
>>> +
>>> +    return 0;
>>> +}
>>> +

Powered by blists - more mailing lists