On Tue, 23 May 2023 17:32:27 +0800
Yicong Yang <yangyicong@xxxxxxxxxx> wrote:
> From: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
>
> The PTT can only filter the traced TLP headers by the Root Ports or the
> Requester ID of the Endpoint, which are located on the same core of the
PCI core (could be confused with CPU core)
> PTT device. The filter value used is derived from the BDF number of the
> supported Root Port or the Endpoint. It's not friendly enough for the
> users since it requires the user to be familiar enough with the platform
> and calculate the filter value manually.
Could we in theory push this problem to userspace? If so perhaps
call out advantages / disadvantages of doing so?
>
> This patch export the available filters through sysfs. Each available
> filters is presented as an individual file with the name of the BDF
> number of the related PCIe device. The files are created under
> $(PTT PMU dir)/available_root_port_filters and
> $(PTT PMU dir)/available_requester_filters respectively. The filter
> value can be known by reading the related file.
>
> Then the users can easily know the available filters for trace and get
> the filter values without calculating.
>
> Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
A few minor editorial type comments inline
Jonathan
> ---
> .../ABI/testing/sysfs-devices-hisi_ptt | 50 +++++
> Documentation/trace/hisi-ptt.rst | 6 +
> drivers/hwtracing/ptt/hisi_ptt.c | 206 ++++++++++++++++++
> drivers/hwtracing/ptt/hisi_ptt.h | 14 ++
> 4 files changed, 276 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-hisi_ptt b/Documentation/ABI/testing/sysfs-devices-hisi_ptt
> index 82de6d710266..190ed504346b 100644
> --- a/Documentation/ABI/testing/sysfs-devices-hisi_ptt
> +++ b/Documentation/ABI/testing/sysfs-devices-hisi_ptt
> @@ -59,3 +59,53 @@ Description: (RW) Control the allocated buffer watermark of outbound packets.
> The available tune data is [0, 1, 2]. Writing a negative value
> will return an error, and out of range values will be converted
> to 2. The value indicates a probable level of the event.
> +
> +What: /sys/devices/hisi_ptt<sicl_id>_<core_id>/root_port_filters
> +Date: May 2023
> +KernelVersion: 6.5
> +Contact: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> +Description: This directory contains the files providing the PCIe Root Port filters
> + information used for PTT trace. Each file is named after the supported
> + Root Port device name <domain>:<bus>:<device>.<function>.
> +
> + See the description of the "filter" in Documentation/trace/hisi-ptt.rst
> + for more information.
> +
> +What: /sys/devices/hisi_ptt<sicl_id>_<core_id>/root_port_filters/multiselect
> +Date: May 2023
> +KernelVersion: 6.5
> +Contact: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> +Description: (Read) Indicates whether this kind of filter can be multiselected
> + or not. 1 for multiselectable, 0 for not.
multiselect term isn't totally clear - so I'd say exactly what it means here.
Indicates if this kind of filter can be selected at the same time as
others filters, or must be used on it's own.
> +
> +What: /sys/devices/hisi_ptt<sicl_id>_<core_id>/root_port_filters/<bdf>
> +Date: May 2023
> +KernelVersion: 6.5
> +Contact: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> +Description: (Read) Indicates the filter value of this Root Port filter, which
> + can be used to control the TLP headers to trace by the PTT trace.
> +
> +What: /sys/devices/hisi_ptt<sicl_id>_<core_id>/requester_filters
> +Date: May 2023
> +KernelVersion: 6.5
> +Contact: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> +Description: This directory contains the files providing the PCIe Requester filters
> + information used for PTT trace. Each file is named after the supported
> + Endpoint device name <domain>:<bus>:<device>.<function>.
> +
> + See the description of the "filter" in Documentation/trace/hisi-ptt.rst
> + for more information.
> +
> +What: /sys/devices/hisi_ptt<sicl_id>_<core_id>/requester_filters/multiselect
> +Date: May 2023
> +KernelVersion: 6.5
> +Contact: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> +Description: (Read) Indicates whether this kind of filter can be multiselected
> + or not. 1 for multiselectable, 0 for not.
As above - explain multiselect using a different set of terms.
> +
> +What: /sys/devices/hisi_ptt<sicl_id>_<core_id>/requester_filters/<bdf>
> +Date: May 2023
> +KernelVersion: 6.5
> +Contact: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> +Description: (Read) Indicates the filter value of this Requester filter, which
> + can be used to control the TLP headers to trace by the PTT trace.
> diff --git a/Documentation/trace/hisi-ptt.rst b/Documentation/trace/hisi-ptt.rst
> index 3641aca4287a..b8c7d71aee32 100644
> --- a/Documentation/trace/hisi-ptt.rst
> +++ b/Documentation/trace/hisi-ptt.rst
> @@ -148,6 +148,12 @@ For example, if the desired filter is Endpoint function 0000:01:00.1 the filter
> value will be 0x00101. If the desired filter is Root Port 0000:00:10.0 then
> then filter value is calculated as 0x80001.
>
> +The driver also presents every supported Root Port and Requester filter through
> +sysfs. Each filter will be an individual file with name of its related PCIe
> +device name (domain:bus:device.function). The files of Root Port filters are
> +under $(PTT PMU dir)/root_port_filters and files of Requester filters
> +are under $(PTT PMU dir)/requester_filters.
> +
> Note that multiple Root Ports can be specified at one time, but only one
> Endpoint function can be specified in one trace. Specifying both Root Port
> and function at the same time is not supported. Driver maintains a list of
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
> index 9b4acbc434b0..65f4288a1915 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.c
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -404,6 +404,142 @@ hisi_ptt_alloc_add_filter(struct hisi_ptt *hisi_ptt, u16 devid, bool is_port)
> return filter;
> }
>
> +static ssize_t hisi_ptt_filter_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct hisi_ptt_filter_desc *filter;
> + unsigned long filter_val;
> +
> + filter = container_of(attr, struct hisi_ptt_filter_desc, attr);
> + filter_val = hisi_ptt_get_filter_val(filter->devid, filter->is_port) |
> + (filter->is_port ? HISI_PTT_PMU_FILTER_IS_PORT : 0);
> +
> + return sysfs_emit(buf, "0x%05lx\n", filter_val);
> +}
> +
> +static int hisi_ptt_create_rp_filter_attr(struct hisi_ptt *hisi_ptt,
> + struct hisi_ptt_filter_desc *filter)
> +{
> + struct kobject *kobj = &hisi_ptt->hisi_ptt_pmu.dev->kobj;
> +
> + filter->attr.attr.name = filter->name;
> + filter->attr.attr.mode = 0400; /* DEVICE_ATTR_ADMIN_RO */
> + filter->attr.show = hisi_ptt_filter_show;
> +
> + return sysfs_add_file_to_group(kobj, &filter->attr.attr,
> + HISI_PTT_RP_FILTERS_GRP_NAME);
> +}
> +
> +static void hisi_ptt_remove_rp_filter_attr(struct hisi_ptt *hisi_ptt,
> + struct hisi_ptt_filter_desc *filter)
> +{
> + struct kobject *kobj = &hisi_ptt->hisi_ptt_pmu.dev->kobj;
> +
> + sysfs_remove_file_from_group(kobj, &filter->attr.attr,
> + HISI_PTT_RP_FILTERS_GRP_NAME);
> +}
> +
> +static int hisi_ptt_create_req_filter_attr(struct hisi_ptt *hisi_ptt,
> + struct hisi_ptt_filter_desc *filter)
> +{
> + struct kobject *kobj = &hisi_ptt->hisi_ptt_pmu.dev->kobj;
> +
> + filter->attr.attr.name = filter->name;
> + filter->attr.attr.mode = 0400; /* DEVICE_ATTR_ADMIN_RO */
> + filter->attr.show = hisi_ptt_filter_show;
> +
> + return sysfs_add_file_to_group(kobj, &filter->attr.attr,
> + HISI_PTT_REQ_FILTERS_GRP_NAME);
> +}
> +
> +static void hisi_ptt_remove_req_filter_attr(struct hisi_ptt *hisi_ptt,
> + struct hisi_ptt_filter_desc *filter)
> +{
> + struct kobject *kobj = &hisi_ptt->hisi_ptt_pmu.dev->kobj;
> +
> + sysfs_remove_file_from_group(kobj, &filter->attr.attr,
> + HISI_PTT_REQ_FILTERS_GRP_NAME);
> +}
> +
> +static int hisi_ptt_create_filter_attr(struct hisi_ptt *hisi_ptt,
> + struct hisi_ptt_filter_desc *filter)
> +{
> + int ret;
> +
> + if (filter->is_port)
> + ret = hisi_ptt_create_rp_filter_attr(hisi_ptt, filter);
> + else
> + ret = hisi_ptt_create_req_filter_attr(hisi_ptt, filter);
> +
> + if (ret)
> + pci_err(hisi_ptt->pdev, "failed to create sysfs attribute for filter %s\n",
> + filter->name);
> +
> + return ret;
> +}
> +
> +static void hisi_ptt_remove_filter_attr(struct hisi_ptt *hisi_ptt,
> + struct hisi_ptt_filter_desc *filter)
> +{
> + if (filter->is_port)
> + hisi_ptt_remove_rp_filter_attr(hisi_ptt, filter);
> + else
> + hisi_ptt_remove_req_filter_attr(hisi_ptt, filter);
> +}
> +
> +static void hisi_ptt_remove_all_filter_attributes(void *data)
> +{
> + struct hisi_ptt_filter_desc *filter;
> + struct hisi_ptt *hisi_ptt = data;
> +
> + mutex_lock(&hisi_ptt->filter_lock);
> +
> + list_for_each_entry(filter, &hisi_ptt->req_filters, list)
> + hisi_ptt_remove_filter_attr(hisi_ptt, filter);
> +
> + list_for_each_entry(filter, &hisi_ptt->port_filters, list)
> + hisi_ptt_remove_filter_attr(hisi_ptt, filter);
> +
> + hisi_ptt->sysfs_inited = false;
> + mutex_unlock(&hisi_ptt->filter_lock);
> +}
> +
> +static int hisi_ptt_init_filter_attributes(struct hisi_ptt *hisi_ptt)
> +{
> + struct hisi_ptt_filter_desc *filter;
> + int ret;
> +
> + mutex_lock(&hisi_ptt->filter_lock);
> +
> + /*
> + * Register the reset callback in the first stage. In reset we traverse
> + * the filters list to remove the sysfs attributes so itself alone can
"itself alone" is confusing. Maybe "the callback"
> + * be called safely even without below filter attributes creation work.
> + */
> + ret = devm_add_action(&hisi_ptt->pdev->dev,
> + hisi_ptt_remove_all_filter_attributes,
> + hisi_ptt);
> + if (ret)
> + goto out;
> +
> + list_for_each_entry(filter, &hisi_ptt->port_filters, list) {
> + ret = hisi_ptt_create_filter_attr(hisi_ptt, filter);
> + if (ret)
> + goto out;
> + }
> +
> + list_for_each_entry(filter, &hisi_ptt->req_filters, list) {
> + ret = hisi_ptt_create_filter_attr(hisi_ptt, filter);
> + if (ret)
> + goto out;
> + }
> +
> + hisi_ptt->sysfs_inited = true;
> +out:
> + mutex_unlock(&hisi_ptt->filter_lock);
> + return ret;
> +}
> +
...
