Re: [PATCH v8 6/8] perf tool: Add support for parsing HiSilicon PCIe Trace packet
On Mon, 16 May 2022 20:52:21 +0800
Yicong Yang wrote:
> From: Qi Liu
>
> Add support for using 'perf report --dump-raw-trace' to parse PTT packet.
>
> Example usage:
>
> Output will contain raw PTT data and its textual representation, such
> as:
>
> 0 0 0x5810 [0x30]: PERF_RECORD_AUXTRACE size: 0x40 offset: 0
> ref: 0xa5d50c725 idx: 0 tid: -1 cpu: 0
> .
> . ... HISI PTT data: size 4194304 bytes
> . : 00 00 00 00 Prefix
> . 0004: 08 20 00 60 Header DW0
> . 0008: ff 02 00 01 Header DW1
> . 000c: 20 08 00 00 Header DW2
> . 0010: 10 e7 44 ab Header DW3
> . 0014: 2a a8 1e 01 Time
> . 0020: 00 00 00 00 Prefix
> . 0024: 01 00 00 60 Header DW0
> . 0028: 0f 1e 00 01 Header DW1
> . 002c: 04 00 00 00 Header DW2
> . 0030: 40 00 81 02 Header DW3
> . 0034: ee 02 00 00 Time
>
>
> Signed-off-by: Qi Liu
> Signed-off-by: Yicong Yang
>From point of view of a reviewer who doesn't know this code well, this
all looks sensible. One trivial comment inline.
Thanks,
Jonathan
> diff --git a/tools/perf/util/hisi-ptt.c b/tools/perf/util/hisi-ptt.c
> new file mode 100644
> index ..2afc1a663c2a
> --- /dev/null
> +
> +static void hisi_ptt_free(struct perf_session *session)
> +{
> + struct hisi_ptt *ptt = container_of(session->auxtrace, struct hisi_ptt,
> + auxtrace);
> +
> + session->auxtrace = NULL;
> + free(ptt);
> +}
> +
> +static bool hisi_ptt_evsel_is_auxtrace(struct perf_session *session,
> +struct evsel *evsel)
> +{
> + struct hisi_ptt *ptt = container_of(session->auxtrace, struct hisi_ptt,
> auxtrace);
Check for consistent wrapping of lines like this. This doesn't match the one
just above.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 5/8] perf tool: Add support for HiSilicon PCIe Tune and Trace device driver
On Mon, 16 May 2022 20:52:20 +0800
Yicong Yang wrote:
> From: Qi Liu
>
> HiSilicon PCIe tune and trace device (PTT) could dynamically tune
> the PCIe link's events, and trace the TLP headers).
>
> This patch add support for PTT device in perf tool, so users could
> use 'perf record' to get TLP headers trace data.
>
> Signed-off-by: Qi Liu
> Signed-off-by: Yicong Yang
One query inline.
> diff --git a/tools/perf/arch/arm/util/auxtrace.c
> b/tools/perf/arch/arm/util/auxtrace.c
> index 384c7cfda0fd..297fffedf45e 100644
> --- a/tools/perf/arch/arm/util/auxtrace.c
> +++ b/tools/perf/arch/arm/util/auxtrace.c
...
> static struct perf_pmu *find_pmu_for_event(struct perf_pmu **pmus,
> int pmu_nr, struct evsel *evsel)
> {
> @@ -71,17 +120,21 @@ struct auxtrace_record
> {
> struct perf_pmu *cs_etm_pmu = NULL;
> struct perf_pmu **arm_spe_pmus = NULL;
> + struct perf_pmu **hisi_ptt_pmus = NULL;
> struct evsel *evsel;
> struct perf_pmu *found_etm = NULL;
> struct perf_pmu *found_spe = NULL;
> + struct perf_pmu *found_ptt = NULL;
> int auxtrace_event_cnt = 0;
> int nr_spes = 0;
> + int nr_ptts = 0;
>
> if (!evlist)
> return NULL;
>
> cs_etm_pmu = perf_pmu__find(CORESIGHT_ETM_PMU_NAME);
> arm_spe_pmus = find_all_arm_spe_pmus(_spes, err);
> + hisi_ptt_pmus = find_all_hisi_ptt_pmus(_ptts, err);
>
> evlist__for_each_entry(evlist, evsel) {
> if (cs_etm_pmu && !found_etm)
> @@ -89,9 +142,13 @@ struct auxtrace_record
>
> if (arm_spe_pmus && !found_spe)
> found_spe = find_pmu_for_event(arm_spe_pmus, nr_spes,
> evsel);
> +
> + if (arm_spe_pmus && !found_spe)
if (hisi_ptt_pmus && !found_ptt) ?
Otherwise, I'm not sure what the purpose of the checking against spe is.
> + found_ptt = find_pmu_for_event(hisi_ptt_pmus, nr_ptts,
> evsel);
> }
>
> free(arm_spe_pmus);
> + free(hisi_ptt_pmus);
>
> if (found_etm)
> auxtrace_event_cnt++;
> @@ -99,6 +156,9 @@ struct auxtrace_record
> if (found_spe)
> auxtrace_event_cnt++;
>
> + if (found_ptt)
> + auxtrace_event_cnt++;
> +
> if (auxtrace_event_cnt > 1) {
> pr_err("Concurrent AUX trace operation not currently
> supported\n");
> *err = -EOPNOTSUPP;
> @@ -111,6 +171,9 @@ struct auxtrace_record
> #if defined(__aarch64__)
> if (found_spe)
> return arm_spe_recording_init(err, found_spe);
> +
> + if (found_ptt)
> + return hisi_ptt_recording_init(err, found_ptt);
> #endif
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 4/8] perf arm: Refactor event list iteration in auxtrace_record__init()
On Mon, 16 May 2022 20:52:19 +0800
Yicong Yang wrote:
> From: Qi Liu
>
> Use find_pmu_for_event() to simplify logic in auxtrace_record__init().
Possibly reword as
"Add find_pmu_for_event() and use to simplify logic in
auxtrace_record_init(). find_pmu_for_event() will be
reused in subsequent patches."
>
> Signed-off-by: Qi Liu
> Signed-off-by: Yicong Yang
FWIW as this isn't an area I know much about. It seems
like a good cleanup and functionally equivalent.
Reviewed-by: Jonathan Cameron
> ---
> tools/perf/arch/arm/util/auxtrace.c | 53 ++---
> 1 file changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/tools/perf/arch/arm/util/auxtrace.c
> b/tools/perf/arch/arm/util/auxtrace.c
> index 5fc6a2a3dbc5..384c7cfda0fd 100644
> --- a/tools/perf/arch/arm/util/auxtrace.c
> +++ b/tools/perf/arch/arm/util/auxtrace.c
> @@ -50,16 +50,32 @@ static struct perf_pmu **find_all_arm_spe_pmus(int
> *nr_spes, int *err)
> return arm_spe_pmus;
> }
>
> +static struct perf_pmu *find_pmu_for_event(struct perf_pmu **pmus,
> +int pmu_nr, struct evsel *evsel)
> +{
> + int i;
> +
> + if (!pmus)
> + return NULL;
> +
> + for (i = 0; i < pmu_nr; i++) {
> + if (evsel->core.attr.type == pmus[i]->type)
> + return pmus[i];
> + }
> +
> + return NULL;
> +}
> +
> struct auxtrace_record
> *auxtrace_record__init(struct evlist *evlist, int *err)
> {
> - struct perf_pmu *cs_etm_pmu;
> + struct perf_pmu *cs_etm_pmu = NULL;
> + struct perf_pmu **arm_spe_pmus = NULL;
> struct evsel *evsel;
> - bool found_etm = false;
> + struct perf_pmu *found_etm = NULL;
> struct perf_pmu *found_spe = NULL;
> - struct perf_pmu **arm_spe_pmus = NULL;
> + int auxtrace_event_cnt = 0;
> int nr_spes = 0;
> - int i = 0;
>
> if (!evlist)
> return NULL;
> @@ -68,24 +84,23 @@ struct auxtrace_record
> arm_spe_pmus = find_all_arm_spe_pmus(_spes, err);
>
> evlist__for_each_entry(evlist, evsel) {
> - if (cs_etm_pmu &&
> - evsel->core.attr.type == cs_etm_pmu->type)
> - found_etm = true;
> -
> - if (!nr_spes || found_spe)
> - continue;
> -
> - for (i = 0; i < nr_spes; i++) {
> - if (evsel->core.attr.type == arm_spe_pmus[i]->type) {
> - found_spe = arm_spe_pmus[i];
> - break;
> - }
> - }
> + if (cs_etm_pmu && !found_etm)
> + found_etm = find_pmu_for_event(_etm_pmu, 1, evsel);
> +
> + if (arm_spe_pmus && !found_spe)
> + found_spe = find_pmu_for_event(arm_spe_pmus, nr_spes,
> evsel);
> }
> +
> free(arm_spe_pmus);
>
> - if (found_etm && found_spe) {
> - pr_err("Concurrent ARM Coresight ETM and SPE operation not
> currently supported\n");
> + if (found_etm)
> + auxtrace_event_cnt++;
> +
> + if (found_spe)
> + auxtrace_event_cnt++;
> +
> + if (auxtrace_event_cnt > 1) {
> + pr_err("Concurrent AUX trace operation not currently
> supported\n");
> *err = -EOPNOTSUPP;
> return NULL;
> }
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 2/8] hwtracing: hisi_ptt: Add trace function support for HiSilicon PCIe Tune and Trace device
On Mon, 16 May 2022 20:52:17 +0800
Yicong Yang wrote:
> HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex integrated
> Endpoint(RCiEP) device, providing the capability to dynamically monitor and
> tune the PCIe traffic and trace the TLP headers.
>
> Add the driver for the device to enable the trace function. Register PMU
> device of PTT trace, then users can use trace through perf command. The
> driver makes use of perf AUX trace function and support the following
> events to configure the trace:
>
> - filter: select Root port or Endpoint to trace
> - type: select the type of traced TLP headers
> - direction: select the direction of traced TLP headers
> - format: select the data format of the traced TLP headers
>
> This patch initially add a basic driver of PTT trace.
>
> Signed-off-by: Yicong Yang
Hi Yicong,
It's been a while since I looked at this driver, so I'll admit
I can't remember if any of the things I've raised below were
previously discussed.
All minor stuff (biggest is question of failing cleanly in unlikely
case of failing the allocation in the filter addition vs carrying
on anyway), so feel free to add
Reviewed-by: Jonathan Cameron
> diff --git a/drivers/hwtracing/ptt/Makefile b/drivers/hwtracing/ptt/Makefile
> new file mode 100644
> index ..908c09a98161
> --- /dev/null
> +++ b/drivers/hwtracing/ptt/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_HISI_PTT) += hisi_ptt.o
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c
> b/drivers/hwtracing/ptt/hisi_ptt.c
> new file mode 100644
> index ..ef25ce98f664
> --- /dev/null
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
...
> +
> +static int hisi_ptt_init_filters(struct pci_dev *pdev, void *data)
> +{
> + struct hisi_ptt_filter_desc *filter;
> + struct hisi_ptt *hisi_ptt = data;
> +
> + filter = kzalloc(sizeof(*filter), GFP_KERNEL);
> + if (!filter) {
> + pci_err(hisi_ptt->pdev, "failed to add filter %s\n",
> pci_name(pdev));
If this fails we carry on anyway (no error checking on the bus_walk).
I think we should error out in that case (would need to use a flag placed
somewhere in hisi_ptt to tell we had an error).
That would complicate the unwind though.
Easiest way to do that unwind is probably to register a separate
devm_add_action_or_reset() callback for each filter.
If you prefer to carry on even with this allocation error, then maybe add a
comment
here somewhere to make it clear that will happen.
> + return -ENOMEM;
> + }
> +
> + filter->devid = PCI_DEVID(pdev->bus->number, pdev->devfn);
> +
> + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) {
> + filter->is_port = true;
> + list_add_tail(>list, _ptt->port_filters);
> +
> + /* Update the available port mask */
> + hisi_ptt->port_mask |= hisi_ptt_get_filter_val(filter->devid,
> true);
> + } else {
> + list_add_tail(>list, _ptt->req_filters);
> + }
> +
> + return 0;
> +}
> +
> +static void hisi_ptt_release_filters(void *data)
> +{
> + struct hisi_ptt_filter_desc *filter, *tmp;
> + struct hisi_ptt *hisi_ptt = data;
> +
> + list_for_each_entry_safe(filter, tmp, _ptt->req_filters, list) {
> + list_del(>list);
> + kfree(filter);
I think with separate release per entry above, this bit become simpler as
we walk all the elements in the devm_ callback list rather than two lists here.
> + }
> +
> + list_for_each_entry_safe(filter, tmp, _ptt->port_filters, list) {
> + list_del(>list);
> + kfree(filter);
> + }
> +}
> +
...
> +
> +static int hisi_ptt_init_ctrls(struct hisi_ptt *hisi_ptt)
> +{
> + struct pci_dev *pdev = hisi_ptt->pdev;
> + struct pci_bus *bus;
> + int ret;
> + u32 reg;
> +
> + INIT_LIST_HEAD(_ptt->port_filters);
> + INIT_LIST_HEAD(_ptt->req_filters);
> +
> + ret = hisi_ptt_config_trace_buf(hisi_ptt);
> + if (ret)
> + return ret;
> +
> + /*
> + * The device range register provides the information about the
> + * root ports which the RCiEP can control and trace. The RCiEP
> + * and the root ports it support are on the same PCIe core, with
> + * same domain number but maybe different bus number. The device
> + * range register will tell us which root ports we can support,
> + * Bit[31:16] indicates the upper BDF numbers of the root port,
> + * while Bit[15:0] indicates the lower.
> + */
> + reg = readl(hisi_ptt->iobase + HISI_PTT_DEVICE_RANGE);
> + hisi_ptt->upper_bdf = FIELD_GET(HISI_PTT_DEVICE_RANGE_UPPER, reg);
> + hisi_ptt->lower_bdf = FIELD_GET(HISI_PTT_DEVICE_RANGE_LOWER, reg);
> +
> + bus = pci_find_bus(pci_domain_nr(pdev->bus),
> PCI_BUS_NUM(hisi_ptt->upper_bdf));
> + if (bus)
> + pci_walk_bus(bus, hisi_ptt_init_filters, hisi_ptt);
> +
> + ret = devm_add_action_or_reset(>dev,
Re: [PATCH v5 3/8] hisi_ptt: Register PMU device for PTT trace
On Tue, 8 Mar 2022 19:13:08 +0800
Yicong Yang wrote:
> On 2022/3/8 18:21, Jonathan Cameron wrote:
> > On Tue, 8 Mar 2022 16:49:25 +0800
> > Yicong Yang wrote:
> >
> >> Register PMU device of PTT trace, then users can use trace through perf
> >> command. The driver makes use of perf AUX trace and support following
> >> events to configure the trace:
> >>
> >> - filter: select Root port or Endpoint to trace
> >> - type: select the type of traced TLP headers
> >> - direction: select the direction of traced TLP headers
> >> - format: select the data format of the traced TLP headers
> >>
> >> This patch adds the PMU driver part of PTT trace. The perf command support
> >> of PTT trace is added in the following patch.
> >>
> >> Signed-off-by: Yicong Yang
> >
> > It seems to me that you ended up doing both suggestions for
> > how to clean up the remove order when it was meant to be
> > a question of picking one or the other.
> >
> > Otherwise this looks good to me - so with that tidied up
> >
>
> Hi Jonathan,
>
> Thanks for the comments. I'd like to illustrate the reason why I decide to
> manually unregister the PMU device.
>
> The DMA buffers are devm allocated when necessary. They're only allocated
> when user is going to use the PTT in the first time after the driver's probe,
> so when driver removal the buffers are released prior to the PMU device's
> unregistration. I think there's a race condition.
>
> IIUC, The PMU device(as the user interface) should be unregistered first then
> we're safe to free the DMA buffers. But unregister the PMU device by devm
> cannot keep that order.
Ok. Please add a comment in the remove() giving this reasoning.
Jonathan
>
> Thanks,
> Yicong
>
> > Reviewed-by: Jonathan Cameron
> >
> >> ---
> >
> >> +
> >> +static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt)
> >> +{
> >> + u16 core_id, sicl_id;
> >> + char *pmu_name;
> >> + u32 reg;
> >> +
> >> + hisi_ptt->hisi_ptt_pmu = (struct pmu) {
> >> + .module = THIS_MODULE,
> >> + .capabilities = PERF_PMU_CAP_EXCLUSIVE | PERF_PMU_CAP_ITRACE,
> >> + .task_ctx_nr= perf_sw_context,
> >> + .attr_groups= hisi_ptt_pmu_groups,
> >> + .event_init = hisi_ptt_pmu_event_init,
> >> + .setup_aux = hisi_ptt_pmu_setup_aux,
> >> + .free_aux = hisi_ptt_pmu_free_aux,
> >> + .start = hisi_ptt_pmu_start,
> >> + .stop = hisi_ptt_pmu_stop,
> >> + .add= hisi_ptt_pmu_add,
> >> + .del= hisi_ptt_pmu_del,
> >> + };
> >> +
> >> + reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION);
> >> + core_id = FIELD_GET(HISI_PTT_CORE_ID, reg);
> >> + sicl_id = FIELD_GET(HISI_PTT_SICL_ID, reg);
> >> +
> >> + pmu_name = devm_kasprintf(_ptt->pdev->dev, GFP_KERNEL,
> >> "hisi_ptt%u_%u",
> >> +sicl_id, core_id);
> >> + if (!pmu_name)
> >> + return -ENOMEM;
> >> +
> >> + return perf_pmu_register(_ptt->hisi_ptt_pmu, pmu_name, -1);
> >
> > As below, you can put back the devm cleanup that you had in v4 now you
> > have modified how the filter cleanup is done to also be devm managed.
> >
> >> +}
> >> +
> >> /*
> >> * The DMA of PTT trace can only use direct mapping, due to some
> >> * hardware restriction. Check whether there is an IOMMU or the
> >> @@ -303,15 +825,32 @@ static int hisi_ptt_probe(struct pci_dev *pdev,
> >>
> >>pci_set_master(pdev);
> >>
> >> + ret = hisi_ptt_register_irq(hisi_ptt);
> >> + if (ret)
> >> + return ret;
> >> +
> >>ret = hisi_ptt_init_ctrls(hisi_ptt);
> >>if (ret) {
> >>pci_err(pdev, "failed to init controls, ret = %d.\n", ret);
> >>return ret;
> >>}
> >>
> >> + ret = hisi_ptt_register_pmu(hisi_ptt);
> >> + if (ret) {
> >> + pci_err(pdev, "failed to register pmu device, ret = %d", ret);
> >> + return ret;
> >> + }
> >> +
> >>return 0;
> >> }
> >>
> >> +void hisi_ptt_remove(struct pci_dev *pdev)
> >> +{
> >> + struct hisi_ptt *hisi_ptt = pci_get_drvdata(pdev);
> >> +
> >> + perf_pmu_unregister(_ptt->hisi_ptt_pmu);
> >
> > Now you have the filter cleanup occurring using a devm_add_action_or_reset()
> > there is no need to have a manual cleanup of this - you can
> > use the approach of a devm_add_action_or_reset like you had in v4.
> >
> > As it is the last call in the probe() order it will be the first one
> > called in the device managed cleanup.
> >
> >> +}
> >> +
> >
> >
> > .
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 8/8] MAINTAINERS: Add maintainer for HiSilicon PTT driver
On Tue, 8 Mar 2022 16:49:30 +0800 Yicong Yang wrote: > Add maintainer for driver and documentation of HiSilicon PTT device. > > Signed-off-by: Yicong Yang FWIW Reviewed-by: Jonathan Cameron I've left the perf tool and iommu patches without tags from me as I don't have the background to do a thorough review. Thanks, Jonathan > --- > MAINTAINERS | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index ea3e6c914384..237c618a74d5 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8689,6 +8689,13 @@ F: Documentation/admin-guide/perf/hisi-pcie-pmu.rst > F: Documentation/admin-guide/perf/hisi-pmu.rst > F: drivers/perf/hisilicon > > +HISILICON PTT DRIVER > +M: Yicong Yang > +L: linux-ker...@vger.kernel.org > +S: Maintained > +F: Documentation/trace/hisi-ptt.rst > +F: drivers/hwtracing/ptt/ > + > HISILICON QM AND ZIP Controller DRIVER > M: Zhou Wang > L: linux-cry...@vger.kernel.org ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 4/8] hisi_ptt: Add support for dynamically updating the filter list
On Tue, 8 Mar 2022 16:49:26 +0800
Yicong Yang wrote:
> The PCIe devices supported by the PTT trace can be removed/rescanned by
> hotplug or through sysfs. Add support for dynamically updating the
> available filter list by registering a PCI bus notifier block. Then user
> can always get latest information about available tracing filters and
> driver can block the invalid filters of which related devices no longer
> exist in the system.
>
> Signed-off-by: Yicong Yang
You've made the change I requested in v4 so
Reviewed-by: Jonathan Cameron
> ---
> drivers/hwtracing/ptt/hisi_ptt.c | 157 ---
> drivers/hwtracing/ptt/hisi_ptt.h | 34 +++
> 2 files changed, 176 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c
> b/drivers/hwtracing/ptt/hisi_ptt.c
> index f06fbbb8a12a..953d36dfcc89 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.c
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -270,25 +270,118 @@ static int hisi_ptt_register_irq(struct hisi_ptt
> *hisi_ptt)
> return 0;
> }
>
> -static int hisi_ptt_init_filters(struct pci_dev *pdev, void *data)
> +static void hisi_ptt_update_filters(struct work_struct *work)
> {
> + struct delayed_work *delayed_work = to_delayed_work(work);
> + struct hisi_ptt_filter_update_info info;
> struct hisi_ptt_filter_desc *filter;
> - struct hisi_ptt *hisi_ptt = data;
> struct list_head *target_list;
> + struct hisi_ptt *hisi_ptt;
>
> - target_list = pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT ?
> - _ptt->port_filters : _ptt->req_filters;
> + hisi_ptt = container_of(delayed_work, struct hisi_ptt, work);
>
> - filter = kzalloc(sizeof(*filter), GFP_KERNEL);
> - if (!filter)
> - return -ENOMEM;
> + if (!mutex_trylock(_ptt->mutex)) {
> + schedule_delayed_work(_ptt->work, HISI_PTT_WORK_DELAY_MS);
> + return;
> + }
> +
> + while (kfifo_get(_ptt->filter_update_kfifo, )) {
> + bool is_port = pci_pcie_type(info.pdev) ==
> PCI_EXP_TYPE_ROOT_PORT;
> + u16 val = hisi_ptt_get_filter_val(info.pdev);
> +
> + target_list = is_port ? _ptt->port_filters :
> _ptt->req_filters;
> +
> + if (info.is_add) {
> + filter = kzalloc(sizeof(*filter), GFP_KERNEL);
> + if (!filter)
> + continue;
> +
> + filter->pdev = info.pdev;
> + list_add_tail(>list, target_list);
> + } else {
> + list_for_each_entry(filter, target_list, list)
> + if (hisi_ptt_get_filter_val(filter->pdev) ==
> val) {
> + list_del(>list);
> + kfree(filter);
> + break;
> + }
> + }
>
> - filter->pdev = pdev;
> - list_add_tail(>list, target_list);
> + /* Update the available port mask */
> + if (!is_port)
> + continue;
>
> - /* Update the available port mask */
> - if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
> - hisi_ptt->port_mask |= hisi_ptt_get_filter_val(pdev);
> + if (info.is_add)
> + hisi_ptt->port_mask |= val;
> + else
> + hisi_ptt->port_mask &= ~val;
> + }
> +
> + mutex_unlock(_ptt->mutex);
> +}
> +
> +static void hisi_ptt_update_fifo_in(struct hisi_ptt *hisi_ptt,
> + struct hisi_ptt_filter_update_info *info)
> +{
> + struct pci_dev *root_port = pcie_find_root_port(info->pdev);
> + u32 port_devid;
> +
> + if (!root_port)
> + return;
> +
> + port_devid = PCI_DEVID(root_port->bus->number, root_port->devfn);
> + if (port_devid < hisi_ptt->lower ||
> + port_devid > hisi_ptt->upper)
> + return;
> +
> + if (kfifo_in_spinlocked(_ptt->filter_update_kfifo, info, 1,
> + _ptt->filter_update_lock))
> + schedule_delayed_work(_ptt->work, 0);
> + else
> + pci_warn(hisi_ptt->pdev,
> + "filter update fifo overflow for target %s\n",
> + pci_name(info->pdev));
> +}
> +
> +/*
> + * A PCI bus notifier is used here for dynamically updating the filter
> + * list.
> + */
> +static int hisi_ptt_notifier_call(struct notifier_block *nb, unsigned long
> action,
> + void *data)
> +{
> + struct hisi_ptt *hisi_ptt = container_of(nb, struct hisi_ptt,
> hisi_ptt_nb);
> + struct hisi_ptt_filter_update_info info;
> + struct device *dev = data;
> + struct pci_dev *pdev = to_pci_dev(dev);
> +
> + info.pdev = pdev;
> +
> + switch (action) {
> + case BUS_NOTIFY_ADD_DEVICE:
> + info.is_add = true;
> + break;
>
Re: [PATCH v5 3/8] hisi_ptt: Register PMU device for PTT trace
On Tue, 8 Mar 2022 16:49:25 +0800
Yicong Yang wrote:
> Register PMU device of PTT trace, then users can use trace through perf
> command. The driver makes use of perf AUX trace and support following
> events to configure the trace:
>
> - filter: select Root port or Endpoint to trace
> - type: select the type of traced TLP headers
> - direction: select the direction of traced TLP headers
> - format: select the data format of the traced TLP headers
>
> This patch adds the PMU driver part of PTT trace. The perf command support
> of PTT trace is added in the following patch.
>
> Signed-off-by: Yicong Yang
It seems to me that you ended up doing both suggestions for
how to clean up the remove order when it was meant to be
a question of picking one or the other.
Otherwise this looks good to me - so with that tidied up
Reviewed-by: Jonathan Cameron
> ---
> +
> +static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt)
> +{
> + u16 core_id, sicl_id;
> + char *pmu_name;
> + u32 reg;
> +
> + hisi_ptt->hisi_ptt_pmu = (struct pmu) {
> + .module = THIS_MODULE,
> + .capabilities = PERF_PMU_CAP_EXCLUSIVE | PERF_PMU_CAP_ITRACE,
> + .task_ctx_nr= perf_sw_context,
> + .attr_groups= hisi_ptt_pmu_groups,
> + .event_init = hisi_ptt_pmu_event_init,
> + .setup_aux = hisi_ptt_pmu_setup_aux,
> + .free_aux = hisi_ptt_pmu_free_aux,
> + .start = hisi_ptt_pmu_start,
> + .stop = hisi_ptt_pmu_stop,
> + .add= hisi_ptt_pmu_add,
> + .del= hisi_ptt_pmu_del,
> + };
> +
> + reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION);
> + core_id = FIELD_GET(HISI_PTT_CORE_ID, reg);
> + sicl_id = FIELD_GET(HISI_PTT_SICL_ID, reg);
> +
> + pmu_name = devm_kasprintf(_ptt->pdev->dev, GFP_KERNEL,
> "hisi_ptt%u_%u",
> + sicl_id, core_id);
> + if (!pmu_name)
> + return -ENOMEM;
> +
> + return perf_pmu_register(_ptt->hisi_ptt_pmu, pmu_name, -1);
As below, you can put back the devm cleanup that you had in v4 now you
have modified how the filter cleanup is done to also be devm managed.
> +}
> +
> /*
> * The DMA of PTT trace can only use direct mapping, due to some
> * hardware restriction. Check whether there is an IOMMU or the
> @@ -303,15 +825,32 @@ static int hisi_ptt_probe(struct pci_dev *pdev,
>
> pci_set_master(pdev);
>
> + ret = hisi_ptt_register_irq(hisi_ptt);
> + if (ret)
> + return ret;
> +
> ret = hisi_ptt_init_ctrls(hisi_ptt);
> if (ret) {
> pci_err(pdev, "failed to init controls, ret = %d.\n", ret);
> return ret;
> }
>
> + ret = hisi_ptt_register_pmu(hisi_ptt);
> + if (ret) {
> + pci_err(pdev, "failed to register pmu device, ret = %d", ret);
> + return ret;
> + }
> +
> return 0;
> }
>
> +void hisi_ptt_remove(struct pci_dev *pdev)
> +{
> + struct hisi_ptt *hisi_ptt = pci_get_drvdata(pdev);
> +
> + perf_pmu_unregister(_ptt->hisi_ptt_pmu);
Now you have the filter cleanup occurring using a devm_add_action_or_reset()
there is no need to have a manual cleanup of this - you can
use the approach of a devm_add_action_or_reset like you had in v4.
As it is the last call in the probe() order it will be the first one
called in the device managed cleanup.
> +}
> +
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 2/8] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
On Mon, 21 Feb 2022 21:13:45 +0800
Yicong Yang wrote:
> Hi Jonathan,
>
> On 2022/2/21 19:18, Jonathan Cameron wrote:
> > On Mon, 21 Feb 2022 16:43:01 +0800
> > Yicong Yang wrote:
> >
> >> HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex
> >> integrated Endpoint(RCiEP) device, providing the capability
> >> to dynamically monitor and tune the PCIe traffic, and trace
> >> the TLP headers.
> >>
> >> Add the driver for the device to enable the trace function.
> >> This patch adds basic function of trace, including the device's
> >> probe and initialization, functions for trace buffer allocation
> >> and trace enable/disable, register an interrupt handler to
> >> simply response to the DMA events. The user interface of trace
> >> will be added in the following patch.
> >>
> >> Signed-off-by: Yicong Yang
> >
> > Hi Yicong,
> >
> > A few really minor things inline, particularly one place
> > where you can improve the error handling.
> > It's always fiddly to handle errors in a pci_walk_bus() but
> > in this case it's not too difficult as you just need to store
> > the retval somewhere in the private data then retrieve it
> > after the pci_walk_bus() call.
> >
>
> Thanks for the quick reply!
>
> The pci_walk_bus() in this patch will fail only if the memory allocation
> of filter struct fails. We won't allocate memory in the pci_bus_walk()
> after Patch 4 so it will never fail. Maybe I can add some comments
> mentioning this.
Great. Given that answers my only significant question.
Reviewed-by: Jonathan Cameron
>
> I also expressed this inline.
>
> > Thanks,
> >
> > Jonathan
> >
> >
> >
> >> ---
> >> drivers/Makefile | 1 +
> >> drivers/hwtracing/Kconfig| 2 +
> >> drivers/hwtracing/ptt/Kconfig| 11 +
> >> drivers/hwtracing/ptt/Makefile | 2 +
> >> drivers/hwtracing/ptt/hisi_ptt.c | 370 +++
> >> drivers/hwtracing/ptt/hisi_ptt.h | 149 +
> >> 6 files changed, 535 insertions(+)
> >> create mode 100644 drivers/hwtracing/ptt/Kconfig
> >> create mode 100644 drivers/hwtracing/ptt/Makefile
> >> create mode 100644 drivers/hwtracing/ptt/hisi_ptt.c
> >> create mode 100644 drivers/hwtracing/ptt/hisi_ptt.h
> >>
> >> diff --git a/drivers/Makefile b/drivers/Makefile
> >> index a110338c860c..ab3411e4eba5 100644
> >> --- a/drivers/Makefile
> >> +++ b/drivers/Makefile
> >> @@ -175,6 +175,7 @@ obj-$(CONFIG_USB4) += thunderbolt/
> >> obj-$(CONFIG_CORESIGHT) += hwtracing/coresight/
> >> obj-y += hwtracing/intel_th/
> >> obj-$(CONFIG_STM) += hwtracing/stm/
> >> +obj-$(CONFIG_HISI_PTT)+= hwtracing/ptt/
> >> obj-$(CONFIG_ANDROID) += android/
> >> obj-$(CONFIG_NVMEM) += nvmem/
> >> obj-$(CONFIG_FPGA)+= fpga/
> >> diff --git a/drivers/hwtracing/Kconfig b/drivers/hwtracing/Kconfig
> >> index 13085835a636..911ee977103c 100644
> >> --- a/drivers/hwtracing/Kconfig
> >> +++ b/drivers/hwtracing/Kconfig
> >> @@ -5,4 +5,6 @@ source "drivers/hwtracing/stm/Kconfig"
> >>
> >> source "drivers/hwtracing/intel_th/Kconfig"
> >>
> >> +source "drivers/hwtracing/ptt/Kconfig"
> >> +
> >> endmenu
> >> diff --git a/drivers/hwtracing/ptt/Kconfig b/drivers/hwtracing/ptt/Kconfig
> >> new file mode 100644
> >> index ..41fa83921a07
> >> --- /dev/null
> >> +++ b/drivers/hwtracing/ptt/Kconfig
> >> @@ -0,0 +1,11 @@
> >> +# SPDX-License-Identifier: GPL-2.0-only
> >> +config HISI_PTT
> >> + tristate "HiSilicon PCIe Tune and Trace Device"
> >> + depends on ARM64 && PCI && HAS_DMA && HAS_IOMEM
> >> + help
> >> +HiSilicon PCIe Tune and Trace Device exists as a PCIe RCiEP
> >> +device, and it provides support for PCIe traffic tuning and
> >> +tracing TLP headers to the memory.
> >> +
> >> +This driver can also be built as a module. If so, the module
> >> +will be called hisi_ptt.
> >> diff --git a/drivers/hwtracing/ptt/Makefile
> >> b/drivers/hwtracing/ptt/Makefile
> >> new file mode 100644
> >> index ..908c09a98161
> >> --- /dev/null
> >> +++ b/drivers/hwtracing/ptt/Makefile
> >> @@ -0,0 +1,2 @@
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +obj-$(CONFIG_HISI_PTT) += hisi_ptt.o
> >> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c
> >> b/drivers/hwtracing/ptt/hisi_ptt.c
> >> new file mode 100644
> >> index ..a5b4f09ccd1e
> >> --- /dev/null
> >> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> >> @@ -0,0 +1,370 @@
> >
> > ...
> >
> >> +static void hisi_ptt_free_trace_buf(struct hisi_ptt *hisi_ptt)
> >> +{
> >> + struct hisi_ptt_trace_ctrl *ctrl = _ptt->trace_ctrl;
> >> + struct device *dev = _ptt->pdev->dev;
> >> + int i;
> >> +
> >> + if (!ctrl->trace_buf)
> >> + return;
> >> +
> >> + for (i = 0; i < HISI_PTT_TRACE_BUF_CNT; i++)
> >> + if (ctrl->trace_buf[i].addr)
> >> + dma_free_coherent(dev, HISI_PTT_TRACE_BUF_SIZE,
> >> +
Re: [PATCH v4 7/8] docs: Add HiSilicon PTT device driver documentation
On Mon, 21 Feb 2022 16:43:06 +0800 Yicong Yang wrote: > Document the introduction and usage of HiSilicon PTT device driver. > > Signed-off-by: Yicong Yang Reviewed-by: Jonathan Cameron > --- > Documentation/trace/hisi-ptt.rst | 303 +++ > 1 file changed, 303 insertions(+) > create mode 100644 Documentation/trace/hisi-ptt.rst > > diff --git a/Documentation/trace/hisi-ptt.rst > b/Documentation/trace/hisi-ptt.rst > new file mode 100644 > index ..13677705ee1f > --- /dev/null > +++ b/Documentation/trace/hisi-ptt.rst > @@ -0,0 +1,303 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +== > +HiSilicon PCIe Tune and Trace device > +== > + > +Introduction > + > + > +HiSilicon PCIe tune and trace device (PTT) is a PCIe Root Complex > +integrated Endpoint (RCiEP) device, providing the capability > +to dynamically monitor and tune the PCIe link's events (tune), > +and trace the TLP headers (trace). The two functions are independent, > +but is recommended to use them together to analyze and enhance the > +PCIe link's performance. > + > +On Kunpeng 930 SoC, the PCIe Root Complex is composed of several > +PCIe cores. Each PCIe core includes several Root Ports and a PTT > +RCiEP, like below. The PTT device is capable of tuning and > +tracing the links of the PCIe core. > +:: > + +--Core 0---+ > + | | [ PTT ] | > + | | [Root Port]---[Endpoint] > + | | [Root Port]---[Endpoint] > + | | [Root Port]---[Endpoint] > +Root Complex |--Core 1---+ > + | | [ PTT ] | > + | | [Root Port]---[ Switch ]---[Endpoint] > + | | [Root Port]---[Endpoint] `-[Endpoint] > + | | [Root Port]---[Endpoint] > + +---+ > + > +The PTT device driver registers one PMU device for each PTT device. > +The name of each PTT device is composed of 'hisi_ptt' prefix with > +the id of the SICL and the Core where it locates. The Kunpeng 930 > +SoC encapsulates multiple CPU dies (SCCL, Super CPU Cluster) and > +IO dies (SICL, Super I/O Cluster), where there's one PCIe Root > +Complex for each SICL. > +:: > +/sys/devices/hisi_ptt_ > + > +Tune > + > + > +PTT tune is designed for monitoring and adjusting PCIe link parameters > (events). > +Currently we support events in 4 classes. The scope of the events > +covers the PCIe core to which the PTT device belongs. > + > +Each event is presented as a file under $(PTT PMU dir)/tune, and > +a simple open/read/write/close cycle will be used to tune the event. > +:: > +$ cd /sys/devices/hisi_ptt_/tune > +$ ls > +qos_tx_cplqos_tx_npqos_tx_p > +tx_path_rx_req_alloc_buf_level > +tx_path_tx_req_alloc_buf_level > +$ cat qos_tx_dp > +1 > +$ echo 2 > qos_tx_dp > +$ cat qos_tx_dp > +2 > + > +Current value (numerical value) of the event can be simply read > +from the file, and the desired value written to the file to tune. > + > +1. Tx path QoS control > + > + > +The following files are provided to tune the QoS of the tx path of > +the PCIe core. > + > +- qos_tx_cpl: weight of Tx completion TLPs > +- qos_tx_np: weight of Tx non-posted TLPs > +- qos_tx_p: weight of Tx posted TLPs > + > +The weight influences the proportion of certain packets on the PCIe link. > +For example, for the storage scenario, increase the proportion > +of the completion packets on the link to enhance the performance as > +more completions are consumed. > + > +The available tune data of these events is [0, 1, 2]. > +Writing a negative value will return an error, and out of range > +values will be converted to 2. Note that the event value just > +indicates a probable level, but is not precise. > + > +2. Tx path buffer control > +- > + > +Following files are provided to tune the buffer of tx path of the PCIe core. > + > +- tx_path_rx_req_alloc_buf_level: watermark of Rx requested > +- tx_path_tx_req_alloc_buf_level: watermark of Tx requested > + > +These events influence the watermark of the buffer allocated for each > +type. Rx means the inbound while Tx means outbound. The packets will > +be stored in the buffer first and then transmitted either when the > +watermark reached or when timed out. For a busy direction, you should > +increase the related buffer watermark to avoid frequently posting and > +thus enhance the performance. In most cases just keep the default value. > + > +The available tune data of above events is [0, 1, 2]. > +Writing a negative value will return an error, and out of range > +values will be converted to 2. Note that the event value just > +indicates a probable level, but is not precise. > + > +Trace > += > + > +PTT trace is designed for dumping the TLP headers
Re: [PATCH v4 4/8] hisi_ptt: Add support for dynamically updating the filter list
On Mon, 21 Feb 2022 16:43:03 +0800
Yicong Yang wrote:
> The PCIe devices supported by the PTT trace can be removed/rescanned
> by hotplug or through sysfs. Add support for dynamically updating
> the available filter list by registering a PCI bus notifier block.
> Then user can always get latest information about available tracing
> filters and driver can block the invalid filters of which related
> devices no longer exist in the system.
>
> Signed-off-by: Yicong Yang
One comment following on from ordering of mixed devm and manual cleanup
in earlier patches.
Otherwise looks fine to me.
> ---
> drivers/hwtracing/ptt/hisi_ptt.c | 138 ---
> drivers/hwtracing/ptt/hisi_ptt.h | 34
> 2 files changed, 160 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c
> b/drivers/hwtracing/ptt/hisi_ptt.c
> index c2b6f8aa9f1e..50193a331faa 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.c
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -269,25 +269,118 @@ static int hisi_ptt_register_irq(struct hisi_ptt
> *hisi_ptt)
> return 0;
> }
>
...
> @@ -313,6 +406,9 @@ static void hisi_ptt_init_ctrls(struct hisi_ptt *hisi_ptt)
> struct pci_bus *bus;
> u32 reg;
>
> + INIT_DELAYED_WORK(_ptt->work, hisi_ptt_update_filters);
> + spin_lock_init(_ptt->filter_update_lock);
> + INIT_KFIFO(hisi_ptt->filter_update_kfifo);
> INIT_LIST_HEAD(_ptt->port_filters);
> INIT_LIST_HEAD(_ptt->req_filters);
>
> @@ -329,6 +425,13 @@ static void hisi_ptt_init_ctrls(struct hisi_ptt
> *hisi_ptt)
> hisi_ptt->upper = FIELD_GET(HISI_PTT_DEVICE_RANGE_UPPER, reg);
> hisi_ptt->lower = FIELD_GET(HISI_PTT_DEVICE_RANGE_LOWER, reg);
>
> + /*
> + * No need to fail if the bus is NULL here as the device
> + * maybe hotplugged after the PTT driver probe, in which
> + * case we can detect the event and update the list as
> + * we register a bus notifier for dynamically updating
> + * the filter list.
> + */
> bus = pci_find_bus(pci_domain_nr(pdev->bus),
> PCI_BUS_NUM(hisi_ptt->upper));
> if (bus)
> pci_walk_bus(bus, hisi_ptt_init_filters, hisi_ptt);
> @@ -832,6 +935,12 @@ static int hisi_ptt_probe(struct pci_dev *pdev,
> return ret;
> }
>
> + /* Register the bus notifier for dynamically updating the filter list */
> + hisi_ptt->hisi_ptt_nb.notifier_call = hisi_ptt_notifier_call;
> + ret = bus_register_notifier(_bus_type, _ptt->hisi_ptt_nb);
> + if (ret)
> + pci_warn(pdev, "failed to register filter update notifier, ret
> = %d", ret);
> +
> return 0;
> }
>
> @@ -839,6 +948,11 @@ void hisi_ptt_remove(struct pci_dev *pdev)
> {
> struct hisi_ptt *hisi_ptt = pci_get_drvdata(pdev);
>
> + bus_unregister_notifier(_bus_type, _ptt->hisi_ptt_nb);
> +
wrt to earlier comment on ordering you'll also need to move these to
a devm_action() call to keep the ordering clean wrt to probe vs remove().
> + /* Cancel any work that has been queued */
> + cancel_delayed_work_sync(_ptt->work);
> +
> if (hisi_ptt->trace_ctrl.status == HISI_PTT_TRACE_STATUS_ON)
> hisi_ptt_trace_end(hisi_ptt);
>
...
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 3/8] hisi_ptt: Register PMU device for PTT trace
On Mon, 21 Feb 2022 16:43:02 +0800
Yicong Yang wrote:
> Register PMU device of PTT trace, then users can use
> trace through perf command. The driver makes use of perf
> AUX trace and support following events to configure the
> trace:
>
> - filter: select Root port or Endpoint to trace
> - type: select the type of traced TLP headers
> - direction: select the direction of traced TLP headers
> - format: select the data format of the traced TLP headers
>
> This patch adds the PMU driver part of PTT trace. The perf
> command support of PTT trace is added in the following
> patch.
>
> Signed-off-by: Yicong Yang
A few minor comments inline.
Thanks,
Jonathan
> +static int hisi_ptt_trace_init_filter(struct hisi_ptt *hisi_ptt, u64 config)
> +{
> + unsigned long val, port_mask = hisi_ptt->port_mask;
> + struct hisi_ptt_filter_desc *filter;
> + int ret = -EINVAL;
> +
> + hisi_ptt->trace_ctrl.is_port = FIELD_GET(HISI_PTT_PMU_FILTER_IS_PORT,
> config);
> + val = FIELD_GET(HISI_PTT_PMU_FILTER_VAL_MASK, config);
> +
> + /*
> + * Port filters are defined as bit mask. For port filters, check
> + * the bits in the @val are within the range of hisi_ptt->port_mask
> + * and whether it's empty or not, otherwise user has specified
> + * some unsupported root ports.
> + *
> + * For Requester ID filters, walk the available filter list to see
> + * whether we have one matched.
> + */
> + if (!hisi_ptt->trace_ctrl.is_port) {
> + list_for_each_entry(filter, _ptt->req_filters, list)
> + if (val == hisi_ptt_get_filter_val(filter->pdev)) {
> + ret = 0;
> + break;
> + }
> + } else if (bitmap_subset(, _mask, BITS_PER_LONG)) {
> + ret = 0;
> + }
> +
> + if (ret)
> + return ret;
> +
> + hisi_ptt->trace_ctrl.filter = val;
> + return 0;
> +}
> +
> +static int hisi_ptt_pmu_event_init(struct perf_event *event)
> +{
> + struct hisi_ptt *hisi_ptt = to_hisi_ptt(event->pmu);
> + struct hisi_ptt_trace_ctrl *ctrl = _ptt->trace_ctrl;
> + int ret;
> + u32 val;
> +
> + if (event->attr.type != hisi_ptt->hisi_ptt_pmu.type)
> + return -ENOENT;
> +
> + mutex_lock(_ptt->mutex);
> +
> + ret = hisi_ptt_trace_init_filter(hisi_ptt, event->attr.config);
> + if (ret < 0)
> + goto out;
> +
> + val = FIELD_GET(HISI_PTT_PMU_DIRECTION_MASK, event->attr.config);
> + ret = hisi_ptt_trace_valid_config_onehot(val,
> hisi_ptt_trace_available_direction,
> +
> ARRAY_SIZE(hisi_ptt_trace_available_direction));
> + if (ret < 0)
> + goto out;
> + ctrl->direction = val;
> +
> + val = FIELD_GET(HISI_PTT_PMU_TYPE_MASK, event->attr.config);
> +
For consistency, no blank line here.
> + ret = hisi_ptt_trace_valid_config(val, hisi_ptt_trace_available_type,
> +
> ARRAY_SIZE(hisi_ptt_trace_available_type));
> + if (ret < 0)
> + goto out;
> + ctrl->type = val;
> +
> + val = FIELD_GET(HISI_PTT_PMU_FORMAT_MASK, event->attr.config);
> + ret = hisi_ptt_trace_valid_config_onehot(val,
> hisi_ptt_trace_availble_format,
> +
> ARRAY_SIZE(hisi_ptt_trace_availble_format));
> + if (ret < 0)
> + goto out;
> + ctrl->format = val;
> +
> +out:
> + mutex_unlock(_ptt->mutex);
> + return ret;
> +}
...
> +
> +static void hisi_ptt_pmu_start(struct perf_event *event, int flags)
> +{
> + struct hisi_ptt *hisi_ptt = to_hisi_ptt(event->pmu);
> + struct perf_output_handle *handle = _ptt->trace_ctrl.handle;
> + struct hw_perf_event *hwc = >hw;
> + struct hisi_ptt_pmu_buf *buf;
> + int cpu = event->cpu;
> + int ret;
> +
> + hwc->state = 0;
> + mutex_lock(_ptt->mutex);
> + if (hisi_ptt->trace_ctrl.status == HISI_PTT_TRACE_STATUS_ON) {
> + pci_dbg(hisi_ptt->pdev, "trace has already started\n");
> + goto stop;
If it is already started setting the state to STOPPED without doing anything
to change the hardware state doesn't feel right.
I'm assuming we only get here as a result of a bug, so perhaps its fine
to do this.
> + }
> +
> + if (cpu == -1)
> + cpu = hisi_ptt->trace_ctrl.default_cpu;
> +
> + /*
> + * Handle the interrupt on the same cpu which starts the trace to avoid
> + * context mismatch. Otherwise we'll trigger the WARN from the perf
> + * core in event_function_local().
> + */
> + WARN_ON(irq_set_affinity(pci_irq_vector(hisi_ptt->pdev,
> HISI_PTT_TRACE_DMA_IRQ),
> + cpumask_of(cpu)));
> +
> + ret = hisi_ptt_alloc_trace_buf(hisi_ptt);
> + if (ret) {
> + pci_dbg(hisi_ptt->pdev, "alloc trace buf failed, ret = %d\n",
> ret);
> + goto
Re: [PATCH v4 2/8] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
On Mon, 21 Feb 2022 16:43:01 +0800
Yicong Yang wrote:
> HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex
> integrated Endpoint(RCiEP) device, providing the capability
> to dynamically monitor and tune the PCIe traffic, and trace
> the TLP headers.
>
> Add the driver for the device to enable the trace function.
> This patch adds basic function of trace, including the device's
> probe and initialization, functions for trace buffer allocation
> and trace enable/disable, register an interrupt handler to
> simply response to the DMA events. The user interface of trace
> will be added in the following patch.
>
> Signed-off-by: Yicong Yang
Hi Yicong,
A few really minor things inline, particularly one place
where you can improve the error handling.
It's always fiddly to handle errors in a pci_walk_bus() but
in this case it's not too difficult as you just need to store
the retval somewhere in the private data then retrieve it
after the pci_walk_bus() call.
Thanks,
Jonathan
> ---
> drivers/Makefile | 1 +
> drivers/hwtracing/Kconfig| 2 +
> drivers/hwtracing/ptt/Kconfig| 11 +
> drivers/hwtracing/ptt/Makefile | 2 +
> drivers/hwtracing/ptt/hisi_ptt.c | 370 +++
> drivers/hwtracing/ptt/hisi_ptt.h | 149 +
> 6 files changed, 535 insertions(+)
> create mode 100644 drivers/hwtracing/ptt/Kconfig
> create mode 100644 drivers/hwtracing/ptt/Makefile
> create mode 100644 drivers/hwtracing/ptt/hisi_ptt.c
> create mode 100644 drivers/hwtracing/ptt/hisi_ptt.h
>
> diff --git a/drivers/Makefile b/drivers/Makefile
> index a110338c860c..ab3411e4eba5 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -175,6 +175,7 @@ obj-$(CONFIG_USB4)+= thunderbolt/
> obj-$(CONFIG_CORESIGHT) += hwtracing/coresight/
> obj-y+= hwtracing/intel_th/
> obj-$(CONFIG_STM)+= hwtracing/stm/
> +obj-$(CONFIG_HISI_PTT) += hwtracing/ptt/
> obj-$(CONFIG_ANDROID)+= android/
> obj-$(CONFIG_NVMEM) += nvmem/
> obj-$(CONFIG_FPGA) += fpga/
> diff --git a/drivers/hwtracing/Kconfig b/drivers/hwtracing/Kconfig
> index 13085835a636..911ee977103c 100644
> --- a/drivers/hwtracing/Kconfig
> +++ b/drivers/hwtracing/Kconfig
> @@ -5,4 +5,6 @@ source "drivers/hwtracing/stm/Kconfig"
>
> source "drivers/hwtracing/intel_th/Kconfig"
>
> +source "drivers/hwtracing/ptt/Kconfig"
> +
> endmenu
> diff --git a/drivers/hwtracing/ptt/Kconfig b/drivers/hwtracing/ptt/Kconfig
> new file mode 100644
> index ..41fa83921a07
> --- /dev/null
> +++ b/drivers/hwtracing/ptt/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config HISI_PTT
> + tristate "HiSilicon PCIe Tune and Trace Device"
> + depends on ARM64 && PCI && HAS_DMA && HAS_IOMEM
> + help
> + HiSilicon PCIe Tune and Trace Device exists as a PCIe RCiEP
> + device, and it provides support for PCIe traffic tuning and
> + tracing TLP headers to the memory.
> +
> + This driver can also be built as a module. If so, the module
> + will be called hisi_ptt.
> diff --git a/drivers/hwtracing/ptt/Makefile b/drivers/hwtracing/ptt/Makefile
> new file mode 100644
> index ..908c09a98161
> --- /dev/null
> +++ b/drivers/hwtracing/ptt/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_HISI_PTT) += hisi_ptt.o
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c
> b/drivers/hwtracing/ptt/hisi_ptt.c
> new file mode 100644
> index ..a5b4f09ccd1e
> --- /dev/null
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -0,0 +1,370 @@
...
> +static void hisi_ptt_free_trace_buf(struct hisi_ptt *hisi_ptt)
> +{
> + struct hisi_ptt_trace_ctrl *ctrl = _ptt->trace_ctrl;
> + struct device *dev = _ptt->pdev->dev;
> + int i;
> +
> + if (!ctrl->trace_buf)
> + return;
> +
> + for (i = 0; i < HISI_PTT_TRACE_BUF_CNT; i++)
> + if (ctrl->trace_buf[i].addr)
> + dma_free_coherent(dev, HISI_PTT_TRACE_BUF_SIZE,
> + ctrl->trace_buf[i].addr,
> + ctrl->trace_buf[i].dma);
> +
> + kfree(ctrl->trace_buf);
> + ctrl->trace_buf = NULL;
> +}
> +
> +static int hisi_ptt_alloc_trace_buf(struct hisi_ptt *hisi_ptt)
> +{
> + struct hisi_ptt_trace_ctrl *ctrl = _ptt->trace_ctrl;
> + struct device *dev = _ptt->pdev->dev;
> + int i;
> +
> + hisi_ptt->trace_ctrl.buf_index = 0;
> +
> + /* If the trace buffer has already been allocated, zero it. */
> + if (ctrl->trace_buf) {
> + for (i = 0; i < HISI_PTT_TRACE_BUF_CNT; i++)
> + memset(ctrl->trace_buf[i].addr, 0,
> HISI_PTT_TRACE_BUF_SIZE);
> + return 0;
> + }
> +
> + ctrl->trace_buf = kcalloc(HISI_PTT_TRACE_BUF_CNT, sizeof(struct
> hisi_ptt_dma_buffer),
Slight
Re: [PATCH v3 6/8] docs: Add HiSilicon PTT device driver documentation
On Mon, 24 Jan 2022 21:11:16 +0800 Yicong Yang wrote: > Document the introduction and usage of HiSilicon PTT device driver. > > Signed-off-by: Yicong Yang Nice document. A few trivial typos inline. I would give a RB except I've suggested you change a part of the sysfs interface which will affect the relevant documentation. Thanks, Jonathan > --- > Documentation/trace/hisi-ptt.rst | 304 +++ > 1 file changed, 304 insertions(+) > create mode 100644 Documentation/trace/hisi-ptt.rst > > diff --git a/Documentation/trace/hisi-ptt.rst > b/Documentation/trace/hisi-ptt.rst > new file mode 100644 > index ..f3269b11a2f6 > --- /dev/null > +++ b/Documentation/trace/hisi-ptt.rst > @@ -0,0 +1,304 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +== > +HiSilicon PCIe Tune and Trace device > +== > + > +Introduction > + > + > +HiSilicon PCIe tune and trace device (PTT) is a PCIe Root Complex > +integrated Endpoint (RCiEP) device, providing the capability > +to dynamically monitor and tune the PCIe link's events (tune), > +and trace the TLP headers (trace). The two functions are independent, > +but is recommended to use them together to analyze and enhance the > +PCIe link's performance. > + > +On Kunpeng 930 SoC, the PCIe Root Complex is composed of several > +PCIe cores. Each PCIe core includes several Root Ports and a PTT > +RCiEP, like below. The PTT device is capable of tuning and > +tracing the link of the PCIe core. links > +:: > + +--Core 0---+ > + | | [ PTT ] | > + | | [Root Port]---[Endpoint] > + | | [Root Port]---[Endpoint] > + | | [Root Port]---[Endpoint] > +Root Complex |--Core 1---+ > + | | [ PTT ] | > + | | [Root Port]---[ Switch ]---[Endpoint] > + | | [Root Port]---[Endpoint] `-[Endpoint] > + | | [Root Port]---[Endpoint] > + +---+ > + > +The PTT device driver registers PMU device for each PTT device. registers one PMU device .. > +The name of each PTT device is composed of 'hisi_ptt' prefix with > +the id of the SICL and the Core where it locates. The Kunpeng 930 > +SoC encapsulates multiple CPU dies (SCCL, Super CPU Cluster) and > +IO dies (SICL, Super I/O Cluster), where there's one PCIe Root > +Complex for each SICL. > +:: > +/sys/devices/hisi_ptt_ > + > +Tune > + > + > +PTT tune is designed for monitoring and adjusting PCIe link parameters > (events). > +Currently we support events in 4 classes. The scope of the events > +covers the PCIe core to which the PTT device belongs. > + > +Each event is presented as a file under $(PTT PMU dir)/tune, and > +mostly a simple open/read/write/close cycle will be used to tune drop "mostly" as it doesn't add anything other than potential confusion. > +the event. > +:: > +$ cd /sys/devices/hisi_ptt_/tune > +$ ls > +qos_tx_cplqos_tx_npqos_tx_p > +tx_path_rx_req_alloc_buf_level > +tx_path_tx_req_alloc_buf_level > +$ cat qos_tx_dp > +1 > +$ echo 2 > qos_tx_dp > +$ cat qos_tx_dp > +2 > + > +Current value (numerical value) of the event can be simply read > +from the file, and the desired value written to the file to tune. > + > +1. Tx path QoS control > + > + > +The following files are provided to tune the QoS of the tx path of > +the PCIe core. > + > +- qos_tx_cpl: weight of Tx completion TLPs > +- qos_tx_np: weight of Tx non-posted TLPs > +- qos_tx_p: weight of Tx posted TLPs > + > +The weight influences the proportion of certain packets on the PCIe link. > +For example, for the storage scenario, increase the proportion > +of the completion packets on the link to enhance the performance as > +more completions are consumed. > + > +The available tune data of these events is [0, 1, 2]. > +Writing a negative value will return an error, and out of range > +values will be converted to 2. Note that the event value just > +indicates a probable level, but is not precise. > + > +2. Tx path buffer control > +- > + > +Following files are provided to tune the buffer of tx path of the PCIe core. > + > +- tx_path_rx_req_alloc_buf_level: watermark of Rx requested > +- tx_path_tx_req_alloc_buf_level: watermark of Tx requested > + > +These events influence the watermark of the buffer allocated for each > +type. Rx means the inbound while Tx means outbound. The packets will > +be stored in the buffer first and then posted either when the watermark Change "posted" to "transmitted" as posted has a special meaning in PCI and I don't think that is what you mean here... (I could be wrong!) > +reached or when timed out. For a busy direction, you should increase > +the related buffer watermark to avoid frequently
Re: [PATCH v3 5/8] perf tool: Add support for HiSilicon PCIe Tune and Trace device driver
On Mon, 24 Jan 2022 21:11:15 +0800
Yicong Yang wrote:
> From: Qi Liu
>
> 'perf record' and 'perf report --dump-raw-trace' supported in this
> patch.
>
> Example usage:
>
> Output will contain raw PTT data and its textual representation, such
> as:
>
> 0 0 0x5810 [0x30]: PERF_RECORD_AUXTRACE size: 0x40 offset: 0
> ref: 0xa5d50c725 idx: 0 tid: -1 cpu: 0
> .
> . ... HISI PTT data: size 4194304 bytes
> . : 00 00 00 00 Prefix
> . 0004: 08 20 00 60 Header DW0
> . 0008: ff 02 00 01 Header DW1
> . 000c: 20 08 00 00 Header DW2
> . 0010: 10 e7 44 ab Header DW3
> . 0014: 2a a8 1e 01 Time
> . 0020: 00 00 00 00 Prefix
> . 0024: 01 00 00 60 Header DW0
> . 0028: 0f 1e 00 01 Header DW1
> . 002c: 04 00 00 00 Header DW2
> . 0030: 40 00 81 02 Header DW3
> . 0034: ee 02 00 00 Time
>
>
> Signed-off-by: Qi Liu
> Signed-off-by: Yicong Yang
Hi. This is unfortunately out of my areas of expertise, so I just
took a quick glance and noticed one generic c thing that could be
tidied up.
> diff --git a/tools/perf/util/hisi_ptt.c b/tools/perf/util/hisi_ptt.c
> new file mode 100644
> index ..75fa89f3fae3
> --- /dev/null
> +++ b/tools/perf/util/hisi_ptt.c
...
> +
> +static void hisi_ptt_free_queue(void *priv)
> +{
> + struct hisi_ptt_queue *pttq = priv;
> +
> + if (!pttq)
> + return;
> +
> + free(pttq);
free() is safe against a null ptr, so you don't need the
if (!pttq) return;
See free(3) man page.
> +}
> +
> +static void hisi_ptt_free_events(struct perf_session *session)
> +{
> + struct hisi_ptt *ptt = container_of(session->auxtrace, struct hisi_ptt,
> + auxtrace);
> + struct auxtrace_queues *queues = >queues;
> + unsigned int i;
> +
> + for (i = 0; i < queues->nr_queues; i++) {
> + hisi_ptt_free_queue(queues->queue_array[i].priv);
> + queues->queue_array[i].priv = NULL;
> + }
> + auxtrace_queues__free(queues);
> +}
> +
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 4/8] hisi_ptt: Add tune function support for HiSilicon PCIe Tune and Trace device
On Mon, 24 Jan 2022 21:11:14 +0800
Yicong Yang wrote:
> Add tune function for the HiSilicon Tune and Trace device. The interface
> of tune is exposed through sysfs attributes of PTT PMU device.
>
> Signed-off-by: Yicong Yang
A few trivial things inline, but looks good in general to me.
With those tidied up
Reviewed-by: Jonathan Cameron
> ---
> drivers/hwtracing/ptt/hisi_ptt.c | 154 +++
> drivers/hwtracing/ptt/hisi_ptt.h | 19
> 2 files changed, 173 insertions(+)
>
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c
> b/drivers/hwtracing/ptt/hisi_ptt.c
> index 2994354e690b..b11e702eb506 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.c
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -21,6 +21,159 @@
>
> #include "hisi_ptt.h"
>
> +static int hisi_ptt_wait_tuning_finish(struct hisi_ptt *hisi_ptt)
> +{
> + u32 val;
> +
> + return readl_poll_timeout(hisi_ptt->iobase + HISI_PTT_TUNING_INT_STAT,
> + val, !(val & HISI_PTT_TUNING_INT_STAT_MASK),
> + HISI_PTT_WAIT_POLL_INTERVAL_US,
> + HISI_PTT_WAIT_TIMEOUT_US);
> +}
> +
> +static int hisi_ptt_tune_data_get(struct hisi_ptt *hisi_ptt,
> + u32 event, u16 *data)
> +{
> + u32 reg;
> +
> + reg = readl(hisi_ptt->iobase + HISI_PTT_TUNING_CTRL);
> + reg &= ~(HISI_PTT_TUNING_CTRL_CODE | HISI_PTT_TUNING_CTRL_SUB);
> + reg |= FIELD_PREP(HISI_PTT_TUNING_CTRL_CODE | HISI_PTT_TUNING_CTRL_SUB,
> + event);
> + writel(reg, hisi_ptt->iobase + HISI_PTT_TUNING_CTRL);
> +
> + /* Write all 1 to indicates it's the read process */
> + writel(~0UL, hisi_ptt->iobase + HISI_PTT_TUNING_DATA);
Just to check, this is includes the bits above the DATA_VAL_MASK?
Fine if so, just seems odd to define a field but then write
parts of the register that aren't part of that field.
> +
> + if (hisi_ptt_wait_tuning_finish(hisi_ptt))
> + return -ETIMEDOUT;
> +
> + reg = readl(hisi_ptt->iobase + HISI_PTT_TUNING_DATA);
> + reg &= HISI_PTT_TUNING_DATA_VAL_MASK;
> + *data = (u16)reg;
As below, prefer a FIELD_GET() for this.
> +
> + return 0;
> +}
> +
> +static int hisi_ptt_tune_data_set(struct hisi_ptt *hisi_ptt,
> + u32 event, u16 data)
> +{
> + u32 reg;
> +
> + reg = readl(hisi_ptt->iobase + HISI_PTT_TUNING_CTRL);
> + reg &= ~(HISI_PTT_TUNING_CTRL_CODE | HISI_PTT_TUNING_CTRL_SUB);
> + reg |= FIELD_PREP(HISI_PTT_TUNING_CTRL_CODE | HISI_PTT_TUNING_CTRL_SUB,
> + event);
> + writel(reg, hisi_ptt->iobase + HISI_PTT_TUNING_CTRL);
> +
> + reg = data;
Given you defined HISI_PTT_TUNING_DATA_VAL_MASK why not use it here
writel(FIELD_PREP(..), ...)?
> + writel(reg, hisi_ptt->iobase + HISI_PTT_TUNING_DATA);
> +
> + if (hisi_ptt_wait_tuning_finish(hisi_ptt))
> + return -ETIMEDOUT;
> +
> + return 0;
> +}
> +
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/8] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
On Mon, 24 Jan 2022 21:11:11 +0800
Yicong Yang wrote:
> HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex
> integrated Endpoint(RCiEP) device, providing the capability
> to dynamically monitor and tune the PCIe traffic, and trace
> the TLP headers.
>
> Add the driver for the device to enable the trace function.
> This patch adds basic function of trace, including the device's
> probe and initialization, functions for trace buffer allocation
> and trace enable/disable, register an interrupt handler to
> simply response to the DMA events. The user interface of trace
> will be added in the following patch.
>
> Signed-off-by: Yicong Yang
Hi Yicong,
I've not been following all the earlier discussion on this driver closely
so I may well raise something that has already been addressed. If so
just ignore the comment.
Thanks,
Jonathan
> ---
> drivers/Makefile | 1 +
> drivers/hwtracing/Kconfig| 2 +
> drivers/hwtracing/ptt/Kconfig| 11 +
> drivers/hwtracing/ptt/Makefile | 2 +
> drivers/hwtracing/ptt/hisi_ptt.c | 398 +++
> drivers/hwtracing/ptt/hisi_ptt.h | 159
> 6 files changed, 573 insertions(+)
> create mode 100644 drivers/hwtracing/ptt/Kconfig
> create mode 100644 drivers/hwtracing/ptt/Makefile
> create mode 100644 drivers/hwtracing/ptt/hisi_ptt.c
> create mode 100644 drivers/hwtracing/ptt/hisi_ptt.h
>
> diff --git a/drivers/Makefile b/drivers/Makefile
> index a110338c860c..ab3411e4eba5 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -175,6 +175,7 @@ obj-$(CONFIG_USB4)+= thunderbolt/
> obj-$(CONFIG_CORESIGHT) += hwtracing/coresight/
> obj-y+= hwtracing/intel_th/
> obj-$(CONFIG_STM)+= hwtracing/stm/
> +obj-$(CONFIG_HISI_PTT) += hwtracing/ptt/
> obj-$(CONFIG_ANDROID)+= android/
> obj-$(CONFIG_NVMEM) += nvmem/
> obj-$(CONFIG_FPGA) += fpga/
> diff --git a/drivers/hwtracing/Kconfig b/drivers/hwtracing/Kconfig
> index 13085835a636..911ee977103c 100644
> --- a/drivers/hwtracing/Kconfig
> +++ b/drivers/hwtracing/Kconfig
> @@ -5,4 +5,6 @@ source "drivers/hwtracing/stm/Kconfig"
>
> source "drivers/hwtracing/intel_th/Kconfig"
>
> +source "drivers/hwtracing/ptt/Kconfig"
> +
> endmenu
> diff --git a/drivers/hwtracing/ptt/Kconfig b/drivers/hwtracing/ptt/Kconfig
> new file mode 100644
> index ..4f4f2459ac47
> --- /dev/null
> +++ b/drivers/hwtracing/ptt/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config HISI_PTT
> + tristate "HiSilicon PCIe Tune and Trace Device"
> + depends on ARM64 && PCI && HAS_DMA && HAS_IOMEM
> + help
> + HiSilicon PCIe Tune and Trace Device exist as a PCIe RCiEP
> + device, provides support for PCIe traffic tuning and
> + tracing TLP headers to the memory.
> +
> + This driver can also be built as a module. If so, the module
> + will be called hisi_ptt.
> diff --git a/drivers/hwtracing/ptt/Makefile b/drivers/hwtracing/ptt/Makefile
> new file mode 100644
> index ..908c09a98161
> --- /dev/null
> +++ b/drivers/hwtracing/ptt/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_HISI_PTT) += hisi_ptt.o
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c
> b/drivers/hwtracing/ptt/hisi_ptt.c
> new file mode 100644
> index ..6d0a0ca5c0a9
> --- /dev/null
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -0,0 +1,398 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for HiSilicon PCIe tune and trace device
> + *
> + * Copyright (c) 2022 HiSilicon Technologies Co., Ltd.
> + * Author: Yicong Yang
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "hisi_ptt.h"
> +
> +static u16 hisi_ptt_get_filter_val(struct pci_dev *pdev)
> +{
> + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)
> + return BIT(HISI_PCIE_CORE_PORT_ID(PCI_SLOT(pdev->devfn)));
> +
> + return PCI_DEVID(pdev->bus->number, pdev->devfn);
> +}
> +
> +static int hisi_ptt_wait_trace_hw_idle(struct hisi_ptt *hisi_ptt)
> +{
> + u32 val;
> +
> + return readl_poll_timeout(hisi_ptt->iobase + HISI_PTT_TRACE_STS, val,
> + val & HISI_PTT_TRACE_IDLE,
> + HISI_PTT_WAIT_POLL_INTERVAL_US,
> + HISI_PTT_WAIT_TIMEOUT_US);
> +}
> +
> +static void hisi_ptt_free_trace_buf(struct hisi_ptt *hisi_ptt)
> +{
> + struct hisi_ptt_trace_ctrl *ctrl = _ptt->trace_ctrl;
> + struct device *dev = _ptt->pdev->dev;
> + struct hisi_ptt_dma_buffer *buffer, *tbuffer;
> +
> + list_for_each_entry_safe(buffer, tbuffer, >trace_buf, list) {
> + list_del(>list);
> + dma_free_coherent(dev, buffer->size, buffer->addr,
> +
Re: [PATCH v3 2/8] hisi_ptt: Register PMU device for PTT trace
On Mon, 24 Jan 2022 21:11:12 +0800
Yicong Yang wrote:
> Register PMU device of PTT trace, then users can use
> trace through perf command. The driver makes use of perf
> AUX trace and support following events to configure the
> trace:
>
> - filter: select Root port or Endpoint to trace
> - type: select the type of traced TLP headers
> - direction: select the direction of traced TLP headers
> - format: select the data format of the traced TLP headers
>
> This patch adds the PMU driver part of PTT trace. The perf
> command support of PTT trace is added in the following
> patch.
>
> Signed-off-by: Yicong Yang
> ---
> @@ -294,6 +346,405 @@ static void hisi_ptt_init_ctrls(struct hisi_ptt
> *hisi_ptt)
> hisi_ptt->trace_ctrl.default_cpu =
> cpumask_first(cpumask_of_node(dev_to_node(>dev)));
> }
>
> +#define HISI_PTT_PMU_FILTER_IS_PORT BIT(19)
> +#define HISI_PTT_PMU_FILTER_VAL_MASK GENMASK(15, 0)
> +#define HISI_PTT_PMU_DIRECTION_MASK GENMASK(23, 20)
> +#define HISI_PTT_PMU_TYPE_MASK GENMASK(31, 24)
> +#define HISI_PTT_PMU_FORMAT_MASK GENMASK(35, 32)
> +
> +static ssize_t available_filters_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct hisi_ptt *hisi_ptt = to_hisi_ptt(dev_get_drvdata(dev));
> + struct hisi_ptt_filter_desc *filter;
> + int pos = 0;
> +
> + if (list_empty(_ptt->port_filters))
> + return sysfs_emit(buf, " No available filter \n");
> +
This is a very unusual sysfs attribute.
They are supposed to be one "thing" per file, so I'd have expected this to
be at least two files
root_ports_available_filters
request_available_filters
and no available filter is indicated by these attribute returning an empty
string.
However you need to match convention for hwtracing drivers so if
this is common approach perhaps you could point me to a similar
example? My grep skills didn't find me one.
> + mutex_lock(_ptt->mutex);
> + pos += sysfs_emit_at(buf, pos, " Root Ports \n");
> + list_for_each_entry(filter, _ptt->port_filters, list)
> + pos += sysfs_emit_at(buf, pos, "%s 0x%05lx\n",
> + pci_name(filter->pdev),
> + hisi_ptt_get_filter_val(filter->pdev) |
> + HISI_PTT_PMU_FILTER_IS_PORT);
> +
> + pos += sysfs_emit_at(buf, pos, " Requesters \n");
> + list_for_each_entry(filter, _ptt->req_filters, list)
> + pos += sysfs_emit_at(buf, pos, "%s 0x%05x\n",
> + pci_name(filter->pdev),
> + hisi_ptt_get_filter_val(filter->pdev));
> +
> + mutex_unlock(_ptt->mutex);
> + return pos;
> +}
> +static DEVICE_ATTR_ADMIN_RO(available_filters);
> +
...
> +static int hisi_ptt_trace_valid_config_onehot(u32 val, u32 *available_list,
> u32 list_size)
> +{
> + int i, ret = -EINVAL;
> +
> + for (i = 0; i < list_size; i++)
> + if (val == available_list[i]) {
> + ret = 0;
return 0;
> + break;
> + }
> +
> + return ret;
return -EINVAL;
> +}
> +
> +
> +static void hisi_ptt_pmu_free_aux(void *aux)
> +{
> + struct hisi_ptt_pmu_buf *buf = aux;
> +
> + vunmap(buf->base);
> + kfree(buf);
> +}
> +
...
> +static int hisi_ptt_pmu_add(struct perf_event *event, int flags)
> +{
> + struct hisi_ptt *hisi_ptt = to_hisi_ptt(event->pmu);
> + struct hw_perf_event *hwc = >hw;
> + int cpu = event->cpu;
> +
> + if (cpu == -1 && smp_processor_id() != hisi_ptt->trace_ctrl.default_cpu)
This check is not entirely obvious to me. Perhaps a comment would help
readers understand why this condition is successful, but doesn't involve
actually starting the pmu?
> + return 0;
> +
> + hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
> +
> + if (flags & PERF_EF_START) {
> + hisi_ptt_pmu_start(event, PERF_EF_RELOAD);
> + if (hwc->state & PERF_HES_STOPPED)
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
...
> /*
> * The DMA of PTT trace can only use direct mapping, due to some
> * hardware restriction. Check whether there is an iommu or the
> @@ -359,6 +810,12 @@ static int hisi_ptt_probe(struct pci_dev *pdev,
>
> hisi_ptt_init_ctrls(hisi_ptt);
>
> + ret = hisi_ptt_register_pmu(hisi_ptt);
> + if (ret) {
> + pci_err(pdev, "failed to register pmu device, ret = %d", ret);
Given I think this exposes userspace interfaces, it should be the very
last thing done in probe(). Otherwise we have a race condition (at least in
theory) where someone starts using it before we then fail the iommu mapping
check.
> + return ret;
> + }
> +
> ret = hisi_ptt_check_iommu_mapping(hisi_ptt);
> if (ret) {
>
Re: [patch 08/37] genirq/msi: Provide msi_device_populate/destroy_sysfs()
On Sat, 27 Nov 2021 02:20:19 +0100 (CET)
Thomas Gleixner wrote:
> Add new allocation functions which can be activated by domain info
> flags. They store the groups pointer in struct msi_device_data.
>
> Signed-off-by: Thomas Gleixner
A few trivial comments...
> ---
> include/linux/msi.h | 12 +++-
> kernel/irq/msi.c| 42 --
> 2 files changed, 51 insertions(+), 3 deletions(-)
>
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -174,9 +174,11 @@ struct msi_desc {
> /**
> * msi_device_data - MSI per device data
> * @lock:Spinlock to protect register access
> + * @attrs: Pointer to the sysfs attribute group
> */
> struct msi_device_data {
> - raw_spinlock_t lock;
> + raw_spinlock_t lock;
Trivial: Move the alignment change back to patch 2.
> + const struct attribute_group**attrs;
> };
>
> int msi_setup_device_data(struct device *dev);
> @@ -242,10 +244,16 @@ void pci_msi_mask_irq(struct irq_data *d
> void pci_msi_unmask_irq(struct irq_data *data);
>
> #ifdef CONFIG_SYSFS
> +int msi_device_populate_sysfs(struct device *dev);
> +void msi_device_destroy_sysfs(struct device *dev);
> +
> const struct attribute_group **msi_populate_sysfs(struct device *dev);
> void msi_destroy_sysfs(struct device *dev,
> const struct attribute_group **msi_irq_groups);
> #else
> +static inline int msi_device_populate_sysfs(struct device *dev) { return 0; }
> +static inline void msi_device_destroy_sysfs(struct device *dev) { }
> +
> static inline const struct attribute_group **msi_populate_sysfs(struct
> device *dev)
> {
> return NULL;
> @@ -393,6 +401,8 @@ enum {
> MSI_FLAG_MUST_REACTIVATE= (1 << 5),
> /* Is level-triggered capable, using two messages */
> MSI_FLAG_LEVEL_CAPABLE = (1 << 6),
> + /* Populate sysfs on alloc() and destroy it on free() */
> + MSI_FLAG_DEV_SYSFS = (1 << 7),
> };
>
> int msi_domain_set_affinity(struct irq_data *data, const struct cpumask
> *mask,
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -214,6 +214,20 @@ const struct attribute_group **msi_popul
> }
>
> /**
> + * msi_device_populate_sysfs - Populate msi_irqs sysfs entries for a device
> + * @dev: The device(PCI, platform etc) which will get sysfs entries
Space after device
> + */
> +int msi_device_populate_sysfs(struct device *dev)
> +{
> + const struct attribute_group **group = msi_populate_sysfs(dev);
> +
> + if (IS_ERR(group))
> + return PTR_ERR(group);
> + dev->msi.data->attrs = group;
> + return 0;
> +}
> +
> +/**
> * msi_destroy_sysfs - Destroy msi_irqs sysfs entries for devices
> * @dev: The device(PCI, platform etc) who will remove sysfs
> entries
> * @msi_irq_groups: attribute_group for device msi_irqs entries
> @@ -239,6 +253,17 @@ void msi_destroy_sysfs(struct device *de
> kfree(msi_irq_groups);
> }
> }
> +
> +/**
> + * msi_device_destroy_sysfs - Destroy msi_irqs sysfs entries for a device
> + * @dev: The device(PCI, platform etc) for which to remove
> + * sysfs entries
> + */
> +void msi_device_destroy_sysfs(struct device *dev)
> +{
> + msi_destroy_sysfs(dev, dev->msi.data->attrs);
> + dev->msi.data->attrs = NULL;
> +}
> #endif
>
> #ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> @@ -686,8 +711,19 @@ int msi_domain_alloc_irqs(struct irq_dom
> {
> struct msi_domain_info *info = domain->host_data;
> struct msi_domain_ops *ops = info->ops;
> + int ret;
>
> - return ops->domain_alloc_irqs(domain, dev, nvec);
> + ret = ops->domain_alloc_irqs(domain, dev, nvec);
> + if (ret)
> + return ret;
> +
> + if (!(info->flags & MSI_FLAG_DEV_SYSFS))
> + return 0;
> +
> + ret = msi_device_populate_sysfs(dev);
> + if (ret)
> + msi_domain_free_irqs(domain, dev);
> + return ret;
> }
>
> void __msi_domain_free_irqs(struct irq_domain *domain, struct device *dev)
> @@ -726,7 +762,9 @@ void msi_domain_free_irqs(struct irq_dom
> struct msi_domain_info *info = domain->host_data;
> struct msi_domain_ops *ops = info->ops;
>
> - return ops->domain_free_irqs(domain, dev);
> + if (info->flags & MSI_FLAG_DEV_SYSFS)
> + msi_device_destroy_sysfs(dev);
> + ops->domain_free_irqs(domain, dev);
> }
>
> /**
>
> ___
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
