Re: [PATCH v2 2/6] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device

[Yicong Yang]

On 2021/11/27 1:10, Mathieu Poirier wrote:
> 
> [...]
> 
>>
>>>>> +
>>>>> +#define HISI_PTT_TRACE_DMA_IRQ			0
>>>>> +#define HISI_PTT_TRACE_BUFLETS_CNT		4
>>>>> +#define HISI_PTT_TRACE_BUFLET_SIZE		SZ_4M
>>>>> +#define HISI_PTT_TRACE_BUFFER_SIZE		(HISI_PTT_TRACE_BUFLET_SIZE * \
>>>>> +						 HISI_PTT_TRACE_BUFLETS_CNT)
>>>>> +#define HISI_PTT_FILTER_UPDATE_FIFO_SIZE	16
>>>>> +
>>>>> +/* Delay time for filter updating work */
>>>>> +#define HISI_PTT_WORK_DELAY_MS		100UL
>>>>> +/* Wait time for DMA hardware to reset */
>>>>> +#define HISI_PTT_RESET_WAIT_MS		1000UL
>>>>> +/* Poll timeout and interval for waiting hardware work to finish */
>>>>> +#define HISI_PTT_WAIT_TIMEOUT_US	1000000UL
>>>>> +#define HISI_PTT_WAIT_POLL_INTERVAL_US	100UL
>>>>> +
>>>>> +#define HISI_PCIE_CORE_PORT_ID(devfn)	(PCI_FUNC(devfn) << 1)
>>>>> +
>>>>> +enum hisi_ptt_trace_status {
>>>>> +	HISI_PTT_TRACE_STATUS_OFF = 0,
>>>>> +	HISI_PTT_TRACE_STATUS_ON,
>>>>> +};
>>>>> +
>>>>> +struct hisi_ptt_dma_buflet {
>>>>
>>>> I'm not sure what a "buflet" is...  Probably best to just call this
>>>> hisi_ptt_dma_buffer" if it pertains to a buffer.  On that note it is hard to
>>>> know due to the lack of proper structure documentation.  Please have a look at
>>>> how "coresight_device" is documented.  The same applies to the rest of the
>>>> structures declared below.
>>>>
>>>
>>> It's mentioned in section 5 of hisi_ptt.rst as "Driver will allocate each DMA buffer
>>> (we call it buflet) of 4MiB ...".
>>
>> I just noticed the documentation in patch 5 - I will read it before continuing
>> with this set.
>>
> 
> As promised I read the documentation and it didn't do much to alliviate my
> concern about the terminology.  If a "buflet" is a DMA buffer than simply call
> it a DMA buffer, regardless of the size.  
> 

ok. If it's hard for the readers to understand, I'll use DMA buffer instead.

> The size of this patchset makes it hard enough to review as it is... On top
> of things reviewers have to remember that PTT means "PCIe tune and trace
> device", tune means "PCIe link's events" and trace means "TLP headers".
> 

ok. I used to make the complete function into one patch, will see how to split
it to make review easier.

Trace and tune denote the two function of the device. Trace denote the function
of tracing the TLP headers and tune denotes the function of read and modify
the PCIe link's events. It's unavoidable of knowing the acronyms for the user
of the device. :)

> But we also have to keep in mind that a buflet is really just a DMA buffer...
> 
> I will not continue reviewing this set.  Please fix the terminology and spin
> another revision.  Doing so I suggest you trim down the submission to the
> absolute minimum feature set this driver should provide - enhancements can be
> made later when a foundation has been laid out.
> 
> Robin has pointed out what seems to be serious problems with the
> implemenation.  That will also have to be sorted out for the next iteration.
> 
ok. Will discuss with robin with his concerns and with the comments fixed in
the next version.

Thanks,
Yicong

> Regards,
> Mathieu
> 
> 
>>>
>>> The total DMA buffer of PTT device is divided into 4 parts, and each part is called
>>> a 'buflet', which means a small buffer (there exists other words with similiar
>>> format: droplet, wavelet, ...).
>>>
>>> The device is designed like this to make sure we won't lose any data when we have to change
>>> the buffer address, the device can continue to write to the next buflet and don't
>>> neet to pause.
>>>
>>>>> +	struct list_head list;
>>>>> +	unsigned int size;
>>>>> +	dma_addr_t dma;
>>>>> +
>>>>> +	/*
>>>>> +	 * The address of the buflet holding the trace data.
>>>>> +	 * See Documentation/trace/hisi-ptt.rst for the details
>>>>
>>>> As of this writing, hisi-ptt.rst doesn't exist.
>>>>
>>>
>>> The comment intends to direct the user to the documentation if the user
>>> want to know details about the data format. The data format is decribed
>>> in the section 4 of the doc.
>>>
>>>>> +	 * of the data format.
>>>>> +	 */
>>>>> +	void *addr;
>>>>> +	int index;
>>>>> +};
>>>>> +
>>>>> +struct hisi_ptt_trace_ctrl {
>>>>> +	enum hisi_ptt_trace_status status;
>>>>> +	struct perf_output_handle handle;
>>>>> +	struct list_head trace_buf;
>>>>> +	/*
>>>>> +	 * The index of the buffer which trace data
>>>>> +	 * currently is writing to.
>>>>> +	 */
>>>>> +	u32 buf_index;
>>>>> +
>>>>> +	int default_cpu;
>>>>> +	u32 buflet_size;
>>>>> +	bool is_port;
>>>>> +	u32 format:1;		/* Format of the traced TLP headers */
>>>>> +	u32 type:4;		/* Type of the TLP headers to trace */
>>>>> +	u32 direction:2;	/* Direction of the TLP headers to trace */
>>>>> +	u32 filter:16;		/* Root port or Requester to filter the TLP headers */
>>>>> +
>>>>> +	phys_addr_t rmr_addr;
>>>>> +	size_t rmr_length;
>>>>> +	bool has_rmr;
>>>>> +};
>>>>> +
>>>>> +struct hisi_ptt_filter_desc {
>>>>> +	struct list_head list;
>>>>> +	struct pci_dev *pdev;
>>>>> +	u16 val;
>>>>> +};
>>>>> +
>>>>> +struct hisi_ptt_pmu_buf {
>>>>> +	size_t length;
>>>>> +	int nr_pages;
>>>>> +	void *base;
>>>>> +	long pos;
>>>>> +};
>>>>> +
>>>>> +/* Structure containing the information for filter updating */
>>>>> +struct hisi_ptt_filter_update_info {
>>>>> +	struct pci_dev *pdev;
>>>>> +	u32 port_devid;
>>>>> +	bool is_port;
>>>>> +	bool is_add;
>>>>> +	u16 val;
>>>>> +};
>>>>> +
>>>>> +struct hisi_ptt {
>>>>> +	struct hisi_ptt_trace_ctrl trace_ctrl;
>>>>> +	struct notifier_block hisi_ptt_nb;
>>>>> +	struct pmu hisi_ptt_pmu;
>>>>> +	void __iomem *iobase;
>>>>> +	struct pci_dev *pdev;
>>>>> +	/*
>>>>> +	 * Use the mutex to protect the filter list and
>>>>> +	 * serialize the perf process.
>>>>> +	 */
>>>>> +	struct mutex mutex;
>>>>> +	const char *name;
>>>>> +	u16 core_id;
>>>>> +	u16 sicl_id;
>>>>> +	/* PCI device range managed by the PTT device */
>>>>> +	u32 upper;
>>>>> +	u32 lower;
>>>>> +	u8 busnr;
>>>>> +
>>>>> +	/*
>>>>> +	 * The trace TLP headers can either be filtered by certain
>>>>> +	 * root port, or by the requester ID. Organize the filters
>>>>> +	 * by @port_filters and @req_filters here. The mask of all
>>>>> +	 * the valid ports is also cached for doing sanity check
>>>>> +	 * of user input.
>>>>> +	 */
>>>>> +	struct list_head port_filters;
>>>>> +	struct list_head req_filters;
>>>>> +	u16 port_mask;
>>>>> +
>>>>> +	/*
>>>>> +	 * We use a delayed work here to avoid indefinitely waiting for
>>>>> +	 * the hisi_ptt->mutex which protecting the filter list. The
>>>>> +	 * work will be delayed only if the mutex can not be held,
>>>>> +	 * otherwise no delay will be applied.
>>>>> +	 */
>>>>> +	struct delayed_work work;
>>>>> +	spinlock_t filter_update_lock;
>>>>> +	DECLARE_KFIFO(filter_update_kfifo, struct hisi_ptt_filter_update_info,
>>>>> +		      HISI_PTT_FILTER_UPDATE_FIFO_SIZE);
>>>>> +};
>>>>
>>>> It might be a good idea to spinoff a hisi_ptt.h for the above declarations.
>>>>
>>>
>>> I'm not sure whether it is common or permitted to provide a header which is only used by
>>> this driver with a single file.
>>
>> I've seen it done many times.
>>
>>>
>>>>> +
>>>>> +static inline struct hisi_ptt *to_hisi_ptt(struct pmu *pmu)
>>>>> +{
>>>>> +	return container_of(pmu, struct hisi_ptt, hisi_ptt_pmu);
>>>>> +}
>>>>
>>>> I'm not sure there is any value to this inline function.  I suggest to simply
>>>> call container_of() whenever it is needed.
>>>>
>>>
>>> ok.
>>>
>>>>> +
>>>>> +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 = &hisi_ptt->trace_ctrl;
>>>>> +	struct device *dev = &hisi_ptt->pdev->dev;
>>>>> +	struct hisi_ptt_dma_buflet *buflet, *tbuflet;
>>>>> +
>>>>> +	list_for_each_entry_safe(buflet, tbuflet, &ctrl->trace_buf, list) {
>>>>> +		list_del(&buflet->list);
>>>>> +
>>>>> +		if (ctrl->has_rmr)
>>>>> +			memunmap(buflet->addr);
>>>>> +		else
>>>>> +			dma_free_coherent(dev, buflet->size, buflet->addr,
>>>>> +					  buflet->dma);
>>>>> +
>>>>> +		kfree(buflet);
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> +static int hisi_ptt_alloc_trace_buf(struct hisi_ptt *hisi_ptt)
>>>>> +{
>>>>> +	struct hisi_ptt_trace_ctrl *ctrl = &hisi_ptt->trace_ctrl;
>>>>> +	struct device *dev = &hisi_ptt->pdev->dev;
>>>>> +	struct hisi_ptt_dma_buflet *buflet;
>>>>> +	int i, ret;
>>>>> +
>>>>> +	hisi_ptt->trace_ctrl.buf_index = 0;
>>>>> +
>>>>> +	/* Make sure the trace buffer is empty before allocating */
>>>>> +	if (!list_empty(&ctrl->trace_buf)) {
>>>>> +		list_for_each_entry(buflet, &ctrl->trace_buf, list)
>>>>> +			memset(buflet->addr, 0, buflet->size);
>>>>> +		return 0;
>>>>> +	}
>>>>> +
>>>>> +	for (i = 0; i < HISI_PTT_TRACE_BUFLETS_CNT; ++i) {
>>>>> +		buflet = kzalloc(sizeof(*buflet), GFP_KERNEL);
>>>>> +		if (!buflet) {
>>>>> +			ret = -ENOMEM;
>>>>> +			goto err;
>>>>> +		}
>>>>> +
>>>>> +		if (ctrl->has_rmr) {
>>>>> +			phys_addr_t base = ctrl->rmr_addr + i * ctrl->buflet_size;
>>>>> +
>>>>> +			buflet->dma = base;
>>>>> +			buflet->addr = memremap(base, ctrl->buflet_size, MEMREMAP_WB);
>>>>> +		} else {
>>>>> +			buflet->addr = dma_alloc_coherent(dev, ctrl->buflet_size,
>>>>> +							  &buflet->dma, GFP_KERNEL);
>>>>> +		}
>>>>> +
>>>>> +		if (!buflet->addr) {
>>>>> +			kfree(buflet);
>>>>> +			ret = -ENOMEM;
>>>>> +			goto err;
>>>>> +		}
>>>>> +
>>>>> +		memset(buflet->addr, 0, buflet->size);
>>>>> +
>>>>> +		buflet->index = i;
>>>>> +		buflet->size = ctrl->buflet_size;
>>>>> +		list_add_tail(&buflet->list, &ctrl->trace_buf);
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +err:
>>>>> +	hisi_ptt_free_trace_buf(hisi_ptt);
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static void hisi_ptt_trace_end(struct hisi_ptt *hisi_ptt)
>>>>> +{
>>>>> +	writel(0, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
>>>>> +	hisi_ptt->trace_ctrl.status = HISI_PTT_TRACE_STATUS_OFF;
>>>>> +}
>>>>> +
>>>>> +static int hisi_ptt_trace_start(struct hisi_ptt *hisi_ptt)
>>>>> +{
>>>>> +	struct hisi_ptt_trace_ctrl *ctrl = &hisi_ptt->trace_ctrl;
>>>>> +	struct hisi_ptt_dma_buflet *cur;
>>>>> +	u32 val;
>>>>> +
>>>>> +	/* Check device idle before start trace */
>>>>> +	if (hisi_ptt_wait_trace_hw_idle(hisi_ptt)) {
>>>>> +		pci_err(hisi_ptt->pdev, "Failed to start trace, the device is still busy.\n");
>>>>> +		return -EBUSY;
>>>>> +	}
>>>>> +
>>>>> +	/* Reset the DMA before start tracing */
>>>>> +	val = readl(hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
>>>>> +	val |= HISI_PTT_TRACE_CTRL_RST;
>>>>> +	writel(val, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
>>>>> +
>>>>> +	/*
>>>>> +	 * We'll be in the perf context where preemption is disabled,
>>>>> +	 * so use busy loop here.
>>>>> +	 */
>>>>> +	mdelay(HISI_PTT_RESET_WAIT_MS);
>>>>> +
>>>>> +	val = readl(hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
>>>>> +	val &= ~HISI_PTT_TRACE_CTRL_RST;
>>>>> +	writel(val, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
>>>>> +
>>>>> +	/* clear the interrupt status */
>>>>> +	writel(HISI_PTT_TRACE_INT_STAT_MASK, hisi_ptt->iobase + HISI_PTT_TRACE_INT_STAT);
>>>>> +	writel(0, hisi_ptt->iobase + HISI_PTT_TRACE_INT_MASK);
>>>>> +
>>>>> +	list_for_each_entry(cur, &ctrl->trace_buf, list) {
>>>>> +		writel(lower_32_bits(cur->dma),
>>>>> +		       hisi_ptt->iobase + HISI_PTT_TRACE_ADDR_BASE_LO_0 +
>>>>> +		       cur->index * HISI_PTT_TRACE_ADDR_STRIDE);
>>>>> +		writel(upper_32_bits(cur->dma),
>>>>> +		       hisi_ptt->iobase + HISI_PTT_TRACE_ADDR_BASE_HI_0 +
>>>>> +		       cur->index * HISI_PTT_TRACE_ADDR_STRIDE);
>>>>> +	}
>>>>> +	writel(ctrl->buflet_size, hisi_ptt->iobase + HISI_PTT_TRACE_ADDR_SIZE);
>>>>> +
>>>>> +	/* set the trace control register */
>>>>> +	val = FIELD_PREP(HISI_PTT_TRACE_CTRL_TYPE_SEL, ctrl->type);
>>>>> +	val |= FIELD_PREP(HISI_PTT_TRACE_CTRL_RXTX_SEL, ctrl->direction);
>>>>> +	val |= FIELD_PREP(HISI_PTT_TRACE_CTRL_DATA_FORMAT, ctrl->format);
>>>>> +	val |= FIELD_PREP(HISI_PTT_TRACE_CTRL_TARGET_SEL, hisi_ptt->trace_ctrl.filter);
>>>>> +	if (!hisi_ptt->trace_ctrl.is_port)
>>>>> +		val |= HISI_PTT_TRACE_CTRL_FILTER_MODE;
>>>>> +
>>>>> +	val |= HISI_PTT_TRACE_CTRL_EN;
>>>>> +	writel(val, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
>>>>> +
>>>>> +	ctrl->status = HISI_PTT_TRACE_STATUS_ON;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int hisi_ptt_update_aux(struct hisi_ptt *hisi_ptt, int index, bool stop)
>>>>> +{
>>>>> +	struct perf_output_handle *handle = &hisi_ptt->trace_ctrl.handle;
>>>>> +	struct perf_event *event = handle->event;
>>>>> +	struct hisi_ptt_dma_buflet *cur;
>>>>> +	struct hisi_ptt_pmu_buf *buf;
>>>>> +
>>>>> +	buf = perf_get_aux(handle);
>>>>> +	if (!buf || !handle->size)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	list_for_each_entry(cur, &hisi_ptt->trace_ctrl.trace_buf, list)
>>>>> +		if (cur->index == index)
>>>>> +			break;
>>>>> +
>>>>> +	memcpy(buf->base + buf->pos, cur->addr, cur->size);
>>>>> +	memset(cur->addr, 0, cur->size);
>>>>> +	buf->pos += cur->size;
>>>>> +
>>>>> +	if (stop) {
>>>>> +		perf_aux_output_end(handle, buf->pos);
>>>>> +	} else if (buf->length - buf->pos < cur->size) {
>>>>> +		perf_aux_output_skip(handle, buf->length - buf->pos);
>>>>> +		perf_aux_output_end(handle, buf->pos);
>>>>> +
>>>>> +		buf = perf_aux_output_begin(handle, event);
>>>>> +		if (!buf)
>>>>> +			return -EINVAL;
>>>>> +
>>>>> +		buf->pos = handle->head % buf->length;
>>>>> +		if (buf->length - buf->pos < cur->size) {
>>>>> +			perf_aux_output_end(handle, 0);
>>>>> +			return -EINVAL;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static irqreturn_t hisi_ptt_isr(int irq, void *context)
>>>>> +{
>>>>> +	struct hisi_ptt *hisi_ptt = context;
>>>>> +	u32 status, buf_idx;
>>>>> +
>>>>> +	status = readl(hisi_ptt->iobase + HISI_PTT_TRACE_INT_STAT);
>>>>> +	buf_idx = ffs(status) - 1;
>>>>> +
>>>>> +	/* Clear the interrupt status of buflet @buf_idx */
>>>>> +	writel(status, hisi_ptt->iobase + HISI_PTT_TRACE_INT_STAT);
>>>>> +
>>>>> +	/*
>>>>> +	 * Update the AUX buffer and cache the current buflet index,
>>>>> +	 * as we need to know this and save the data when the trace
>>>>> +	 * is ended out of the interrupt handler. End the trace
>>>>> +	 * if the updating fails.
>>>>> +	 */
>>>>> +	if (hisi_ptt_update_aux(hisi_ptt, buf_idx, false))
>>>>> +		hisi_ptt_trace_end(hisi_ptt);
>>>>> +	else
>>>>> +		hisi_ptt->trace_ctrl.buf_index = (buf_idx + 1) % HISI_PTT_TRACE_BUFLETS_CNT;
>>>>> +
>>>>> +	return IRQ_HANDLED;
>>>>> +}
>>>>> +
>>>>> +static irqreturn_t hisi_ptt_irq(int irq, void *context)
>>>>> +{
>>>>> +	struct hisi_ptt *hisi_ptt = context;
>>>>> +	u32 status;
>>>>> +
>>>>> +	status = readl(hisi_ptt->iobase + HISI_PTT_TRACE_INT_STAT);
>>>>> +	if (!(status & HISI_PTT_TRACE_INT_STAT_MASK))
>>>>> +		return IRQ_NONE;
>>>>> +
>>>>> +	return IRQ_WAKE_THREAD;
>>>>> +}
>>>>> +
>>>>> +static void hisi_ptt_irq_free_vectors(void *pdev)
>>>>> +{
>>>>> +	pci_free_irq_vectors(pdev);
>>>>> +}
>>>>> +
>>>>> +static int hisi_ptt_register_irq(struct hisi_ptt *hisi_ptt)
>>>>> +{
>>>>> +	struct pci_dev *pdev = hisi_ptt->pdev;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
>>>>> +	if (ret < 0) {
>>>>> +		pci_err(pdev, "failed to allocate irq vector, ret = %d.\n", ret);
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	ret = devm_add_action_or_reset(&pdev->dev, hisi_ptt_irq_free_vectors, pdev);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>> +
>>>>> +	ret = devm_request_threaded_irq(&pdev->dev,
>>>>> +					pci_irq_vector(pdev, HISI_PTT_TRACE_DMA_IRQ),
>>>>> +					hisi_ptt_irq, hisi_ptt_isr, 0,
>>>>> +					"hisi-ptt", hisi_ptt);
>>>>> +	if (ret) {
>>>>> +		pci_err(pdev, "failed to request irq %d, ret = %d.\n",
>>>>> +			pci_irq_vector(pdev, HISI_PTT_TRACE_DMA_IRQ), ret);
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +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 list_head *target_list;
>>>>> +	struct hisi_ptt *hisi_ptt;
>>>>> +
>>>>> +	hisi_ptt = container_of(delayed_work, struct hisi_ptt, work);
>>>>> +
>>>>> +	if (!mutex_trylock(&hisi_ptt->mutex)) {
>>>>> +		schedule_delayed_work(&hisi_ptt->work, HISI_PTT_WORK_DELAY_MS);
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	while (kfifo_get(&hisi_ptt->filter_update_kfifo, &info)) {
>>>>> +		target_list = info.is_port ? &hisi_ptt->port_filters :
>>>>> +			      &hisi_ptt->req_filters;
>>>>> +
>>>>> +		if (info.is_add) {
>>>>> +			filter = kzalloc(sizeof(*filter), GFP_KERNEL);
>>>>> +			if (!filter)
>>>>> +				continue;
>>>>> +
>>>>> +			filter->pdev = info.pdev;
>>>>> +			filter->val = info.val;
>>>>> +
>>>>> +			list_add_tail(&filter->list, target_list);
>>>>> +		} else {
>>>>> +			list_for_each_entry(filter, target_list, list)
>>>>> +				if (filter->val == info.val) {
>>>>> +					list_del(&filter->list);
>>>>> +					kfree(filter);
>>>>> +					break;
>>>>> +				}
>>>>> +		}
>>>>> +
>>>>> +		/* Update the available port mask */
>>>>> +		if (!info.is_port)
>>>>> +			continue;
>>>>> +
>>>>> +		if (info.is_add)
>>>>> +			hisi_ptt->port_mask |= info.val;
>>>>> +		else
>>>>> +			hisi_ptt->port_mask &= ~info.val;
>>>>> +	}
>>>>> +
>>>>> +	mutex_unlock(&hisi_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);
>>>>> +
>>>>> +	if (!root_port)
>>>>> +		return;
>>>>> +
>>>>> +	info->port_devid = PCI_DEVID(root_port->bus->number, root_port->devfn);
>>>>> +	if (info->port_devid < hisi_ptt->lower ||
>>>>> +	    info->port_devid > hisi_ptt->upper)
>>>>> +		return;
>>>>> +
>>>>> +	info->is_port = pci_pcie_type(info->pdev) == PCI_EXP_TYPE_ROOT_PORT;
>>>>> +	info->val = hisi_ptt_get_filter_val(info->pdev);
>>>>> +
>>>>> +	if (kfifo_in_spinlocked(&hisi_ptt->filter_update_kfifo, info, 1,
>>>>> +				&hisi_ptt->filter_update_lock))
>>>>> +		schedule_delayed_work(&hisi_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;
>>>>> +	case BUS_NOTIFY_DEL_DEVICE:
>>>>> +		info.is_add = false;
>>>>> +		break;
>>>>> +	default:
>>>>> +		return 0;
>>>>> +	}
>>>>> +
>>>>> +	hisi_ptt_update_fifo_in(hisi_ptt, &info);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int hisi_ptt_init_filters(struct pci_dev *pdev, void *data)
>>>>> +{
>>>>> +	struct hisi_ptt_filter_update_info info = {
>>>>> +		.pdev = pdev,
>>>>> +		.is_add = true,
>>>>> +	};
>>>>> +	struct hisi_ptt *hisi_ptt = data;
>>>>> +
>>>>> +	hisi_ptt_update_fifo_in(hisi_ptt, &info);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static void hisi_ptt_release_filters(struct hisi_ptt *hisi_ptt)
>>>>> +{
>>>>> +	struct hisi_ptt_filter_desc *filter, *tfilter;
>>>>> +
>>>>> +	list_for_each_entry_safe(filter, tfilter, &hisi_ptt->req_filters, list) {
>>>>> +		list_del(&filter->list);
>>>>> +		kfree(filter);
>>>>> +	}
>>>>> +
>>>>> +	list_for_each_entry_safe(filter, tfilter, &hisi_ptt->port_filters, list) {
>>>>> +		list_del(&filter->list);
>>>>> +		kfree(filter);
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> +static void hisi_ptt_init_ctrls(struct hisi_ptt *hisi_ptt)
>>>>> +{
>>>>> +	struct pci_dev *pdev = hisi_ptt->pdev;
>>>>> +	struct pci_bus *bus;
>>>>> +	u32 reg;
>>>>> +
>>>>> +	INIT_DELAYED_WORK(&hisi_ptt->work, hisi_ptt_update_filters);
>>>>> +	spin_lock_init(&hisi_ptt->filter_update_lock);
>>>>> +	INIT_KFIFO(hisi_ptt->filter_update_kfifo);
>>>>> +	INIT_LIST_HEAD(&hisi_ptt->port_filters);
>>>>> +	INIT_LIST_HEAD(&hisi_ptt->req_filters);
>>>>> +
>>>>> +	/*
>>>>> +	 * 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 = reg >> 16;
>>>>> +	hisi_ptt->lower = reg & 0xffff;
>>>>> +	hisi_ptt->busnr = PCI_BUS_NUM(hisi_ptt->upper);
>>>>> +
>>>>> +	reg = readl(hisi_ptt->iobase + HISI_PTT_LOCATION);
>>>>> +	hisi_ptt->core_id = FIELD_GET(HISI_PTT_CORE_ID, reg);
>>>>> +	hisi_ptt->sicl_id = FIELD_GET(HISI_PTT_SICL_ID, reg);
>>>>> +
>>>>> +	bus = pci_find_bus(pci_domain_nr(pdev->bus), hisi_ptt->busnr);
>>>>> +	if (bus)
>>>>> +		pci_walk_bus(bus, hisi_ptt_init_filters, hisi_ptt);
>>>>
>>>> What happens if bus is NULL?  Should this be reported to the caller and cause
>>>> hisi_ptt_probe() to fail?  Please add a comment that describes the scenario.
>>>>
>>>
>>> It's ok if the list is NULL in the probe process,
>>> 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.
>>>
>>
>> Please add the above as a comment in the code.
>>
>>
>>>>> +
>>>>> +	/* Initialize trace controls */
>>>>> +	INIT_LIST_HEAD(&hisi_ptt->trace_ctrl.trace_buf);
>>>>> +	hisi_ptt->trace_ctrl.buflet_size = HISI_PTT_TRACE_BUFLET_SIZE;
>>>>> +	hisi_ptt->trace_ctrl.default_cpu = cpumask_first(cpumask_of_node(dev_to_node(&pdev->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(&hisi_ptt->port_filters))
>>>>> +		return sysfs_emit(buf, "#### No available filter ####\n");
>>>>> +
>>>>> +	mutex_lock(&hisi_ptt->mutex);
>>>>> +	pos += sysfs_emit_at(buf, pos, "#### Root Ports ####\n");
>>>>> +	list_for_each_entry(filter, &hisi_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, &hisi_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(&hisi_ptt->mutex);
>>>>> +	return pos;
>>>>> +}
>>>>> +static DEVICE_ATTR_ADMIN_RO(available_filters);
>>>>> +
>>>>> +PMU_FORMAT_ATTR(filter,		"config:0-19");
>>>>> +PMU_FORMAT_ATTR(direction,	"config:20-23");
>>>>> +PMU_FORMAT_ATTR(type,		"config:24-31");
>>>>> +PMU_FORMAT_ATTR(format,		"config:32-35");
>>>>> +
>>>>> +static struct attribute *hisi_ptt_pmu_format_attrs[] = {
>>>>> +	&format_attr_filter.attr,
>>>>> +	&format_attr_direction.attr,
>>>>> +	&format_attr_type.attr,
>>>>> +	&format_attr_format.attr,
>>>>> +	NULL
>>>>> +};
>>>>> +
>>>>> +static struct attribute_group hisi_ptt_pmu_format_group = {
>>>>> +	.name = "format",
>>>>> +	.attrs = hisi_ptt_pmu_format_attrs,
>>>>> +};
>>>>> +
>>>>> +static struct attribute *hisi_ptt_pmu_filter_attrs[] = {
>>>>> +	&dev_attr_available_filters.attr,
>>>>> +	NULL
>>>>> +};
>>>>> +
>>>>> +static struct attribute_group hisi_ptt_pmu_filter_group = {
>>>>> +	.attrs = hisi_ptt_pmu_filter_attrs,
>>>>> +};
>>>>> +
>>>>> +static const struct attribute_group *hisi_ptt_pmu_groups[] = {
>>>>> +	&hisi_ptt_pmu_format_group,
>>>>> +	&hisi_ptt_pmu_filter_group,
>>>>> +	NULL
>>>>> +};
>>>>> +
>>>>> +/*
>>>>> + * The supported value of the direction parameter. See hisi_ptt.rst
>>>>> + * documentation for more details.
>>>>> + */
>>>>> +static u32 hisi_ptt_trace_available_direction[] = {
>>>>> +	0,
>>>>> +	1,
>>>>> +	2,
>>>>> +	3,
>>>>> +};
>>>>> +
>>>>> +/* Different types can be set simultaneously */
>>>>> +static u32 hisi_ptt_trace_available_type[] = {
>>>>> +	1,	/* posted_request */
>>>>> +	2,	/* non-posted_request */
>>>>> +	4,	/* completion */
>>>>> +};
>>>>> +
>>>>> +static u32 hisi_ptt_trace_availble_format[] = {
>>>>> +	0,	/* 4DW */
>>>>> +	1,	/* 8DW */
>>>>> +};
>>>>> +
>>>>> +/*
>>>>> + * Check whether the config is valid or not. Some configs are multi-selectable
>>>>> + * and can be set simultaneously, while some are single selectable (onehot).
>>>>> + * Use this function to check the non-onehot configs while
>>>>> + * hisi_ptt_trace_valid_config_onehot() for the onehot ones.
>>>>> + */
>>>>> +static int hisi_ptt_trace_valid_config(u32 val, u32 *available_list, u32 list_size)
>>>>> +{
>>>>> +	int i;
>>>>> +
>>>>> +	/*
>>>>> +	 * The non-onehot configs cannot be 0. Walk the available
>>>>> +	 * list and clear the valid bits of the config. If there
>>>>> +	 * is any resident bit after the walk then the config is
>>>>> +	 * invalid.
>>>>> +	 */
>>>>> +	for (i = 0; i < list_size; i++)
>>>>> +		val &= ~available_list[i];
>>>>> +
>>>>> +	return val ? -EINVAL : 0;
>>>>> +}
>>>>> +
>>>>> +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;
>>>>> +			break;
>>>>> +		}
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +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 &&
>>>>> +	    bitmap_subset(&val, &port_mask, BITS_PER_LONG)) {
>>>>> +		ret = 0;
>>>>> +	} else {
>>>>> +		list_for_each_entry(filter, &hisi_ptt->req_filters, list)
>>>>> +			if (filter->val == val) {
>>>>> +				ret = 0;
>>>>> +				break;
>>>>> +			}
>>>>> +	}
>>>>> +
>>>>> +	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 = &hisi_ptt->trace_ctrl;
>>>>> +	int ret;
>>>>> +	u32 val;
>>>>> +
>>>>> +	if (event->attr.type != hisi_ptt->hisi_ptt_pmu.type)
>>>>> +		return -ENOENT;
>>>>> +
>>>>> +	mutex_lock(&hisi_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);
>>>>> +
>>>>> +	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(&hisi_ptt->mutex);
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static void *hisi_ptt_pmu_setup_aux(struct perf_event *event, void **pages,
>>>>> +				    int nr_pages, bool overwrite)
>>>>> +{
>>>>> +	struct hisi_ptt_pmu_buf *buf;
>>>>> +	struct page **pagelist;
>>>>> +	int i;
>>>>> +
>>>>> +	if (overwrite) {
>>>>> +		dev_warn(event->pmu->dev, "Overwrite mode is not supported\n");
>>>>> +		return NULL;
>>>>> +	}
>>>>> +
>>>>> +	/* If the pages size less than buflets, we cannot start trace */
>>>>> +	if (nr_pages < HISI_PTT_TRACE_BUFFER_SIZE / PAGE_SIZE)
>>>>> +		return NULL;
>>>>> +
>>>>> +	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>>>>> +	if (!buf)
>>>>> +		return NULL;
>>>>> +
>>>>> +	pagelist = kcalloc(nr_pages, sizeof(*pagelist), GFP_KERNEL);
>>>>> +	if (!pagelist) {
>>>>> +		kfree(buf);
>>>>> +		return NULL;
>>>>> +	}
>>>>> +
>>>>> +	for (i = 0; i < nr_pages; i++)
>>>>> +		pagelist[i] = virt_to_page(pages[i]);
>>>>> +
>>>>> +	buf->base = vmap(pagelist, nr_pages, VM_MAP, PAGE_KERNEL);
>>>>> +	if (!buf->base) {
>>>>> +		kfree(pagelist);
>>>>> +		kfree(buf);
>>>>> +		return NULL;
>>>>> +	}
>>>>> +
>>>>> +	buf->nr_pages = nr_pages;
>>>>> +	buf->length = nr_pages * PAGE_SIZE;
>>>>> +	buf->pos = 0;
>>>>> +
>>>>> +	kfree(pagelist);
>>>>> +	return buf;
>>>>> +}
>>>>> +
>>>>> +static void hisi_ptt_pmu_free_aux(void *aux)
>>>>> +{
>>>>> +	struct hisi_ptt_pmu_buf *buf = aux;
>>>>> +
>>>>> +	vunmap(buf->base);
>>>>> +	kfree(buf);
>>>>> +}
>>>>> +
>>>>> +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 = &hisi_ptt->trace_ctrl.handle;
>>>>> +	struct hw_perf_event *hwc = &event->hw;
>>>>> +	struct hisi_ptt_pmu_buf *buf;
>>>>> +	int cpu = event->cpu;
>>>>> +	int ret;
>>>>> +
>>>>> +	hwc->state = 0;
>>>>> +	mutex_lock(&hisi_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 (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 stop;
>>>>> +	}
>>>>> +
>>>>> +	buf = perf_aux_output_begin(handle, event);
>>>>> +	if (!buf) {
>>>>> +		pci_dbg(hisi_ptt->pdev, "aux output begin failed\n");
>>>>> +		goto stop;
>>>>> +	}
>>>>> +
>>>>> +	buf->pos = handle->head % buf->length;
>>>>> +
>>>>> +	ret = hisi_ptt_trace_start(hisi_ptt);
>>>>> +	if (ret) {
>>>>> +		pci_dbg(hisi_ptt->pdev, "trace start failed, ret = %d\n", ret);
>>>>> +		perf_aux_output_end(handle, 0);
>>>>> +		goto stop;
>>>>> +	}
>>>>> +
>>>>> +	mutex_unlock(&hisi_ptt->mutex);
>>>>> +	return;
>>>>> +stop:
>>>>> +	event->hw.state |= PERF_HES_STOPPED;
>>>>> +	mutex_unlock(&hisi_ptt->mutex);
>>>>> +}
>>>>> +
>>>>> +static void hisi_ptt_pmu_stop(struct perf_event *event, int flags)
>>>>> +{
>>>>> +	struct hisi_ptt *hisi_ptt = to_hisi_ptt(event->pmu);
>>>>> +	struct hw_perf_event *hwc = &event->hw;
>>>>> +
>>>>> +	if (hwc->state & PERF_HES_STOPPED)
>>>>> +		return;
>>>>> +
>>>>> +	mutex_lock(&hisi_ptt->mutex);
>>>>> +	if (hisi_ptt->trace_ctrl.status == HISI_PTT_TRACE_STATUS_ON) {
>>>>> +		hisi_ptt_trace_end(hisi_ptt);
>>>>> +		WARN(hisi_ptt_wait_trace_hw_idle(hisi_ptt), "Device is still busy");
>>>>> +		hisi_ptt_update_aux(hisi_ptt, hisi_ptt->trace_ctrl.buf_index, true);
>>>>> +	}
>>>>> +	mutex_unlock(&hisi_ptt->mutex);
>>>>> +
>>>>> +	hwc->state |= PERF_HES_STOPPED;
>>>>> +	perf_event_update_userpage(event);
>>>>> +	hwc->state |= PERF_HES_UPTODATE;
>>>>> +}
>>>>> +
>>>>> +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 = &event->hw;
>>>>> +	int cpu = event->cpu;
>>>>> +
>>>>> +	if (cpu == -1 && smp_processor_id() != hisi_ptt->trace_ctrl.default_cpu)
>>>>> +		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;
>>>>> +}
>>>>> +
>>>>> +static void hisi_ptt_pmu_del(struct perf_event *event, int flags)
>>>>> +{
>>>>> +	hisi_ptt_pmu_stop(event, PERF_EF_UPDATE);
>>>>> +}
>>>>> +
>>>>> +static void hisi_ptt_unregister_pmu(void *priv)
>>>>> +{
>>>>> +	perf_pmu_unregister(priv);
>>>>> +}
>>>>> +
>>>>> +static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt)
>>>>> +{
>>>>> +	char *pmu_name;
>>>>> +	int ret;
>>>>> +
>>>>> +	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,
>>>>> +	};
>>>>> +
>>>>> +	pmu_name = devm_kasprintf(&hisi_ptt->pdev->dev, GFP_KERNEL, "hisi_ptt%u_%u",
>>>>> +				  hisi_ptt->sicl_id, hisi_ptt->core_id);
>>>>> +	if (!pmu_name)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	ret = perf_pmu_register(&hisi_ptt->hisi_ptt_pmu, pmu_name, -1);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	return devm_add_action_or_reset(&hisi_ptt->pdev->dev,
>>>>> +					hisi_ptt_unregister_pmu,
>>>>> +					&hisi_ptt->hisi_ptt_pmu);
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Get RMR address if provided by the firmware.
>>>>> + * Return 0 if the IOMMU doesn't present or the policy of the
>>>>> + * IOMMU domain is passthrough or we get a usable RMR region.
>>>>> + * Otherwise a negative value is returned.
>>>>> + */
>>>>> +static int hisi_ptt_get_rmr(struct hisi_ptt *hisi_ptt)
>>>>> +{
>>>>> +	struct pci_dev *pdev = hisi_ptt->pdev;
>>>>> +	struct iommu_domain *iommu_domain;
>>>>> +	struct iommu_resv_region *region;
>>>>> +	LIST_HEAD(list);
>>>>> +
>>>>> +	/*
>>>>> +	 * Use direct DMA if IOMMU does not present or the policy of the
>>>>> +	 * IOMMU domain is passthrough.
>>>>> +	 */
>>>>> +	iommu_domain = iommu_get_domain_for_dev(&pdev->dev);
>>>>> +	if (!iommu_domain || iommu_domain->type == IOMMU_DOMAIN_IDENTITY)
>>>>> +		return 0;
>>>>> +
>>>>> +	iommu_get_resv_regions(&pdev->dev, &list);
>>>>> +	list_for_each_entry(region, &list, list)
>>>>> +		if (region->type == IOMMU_RESV_DIRECT &&
>>>>> +		    region->length >= HISI_PTT_TRACE_BUFFER_SIZE) {
>>>>> +			hisi_ptt->trace_ctrl.has_rmr = true;
>>>>> +			hisi_ptt->trace_ctrl.rmr_addr = region->start;
>>>>> +			hisi_ptt->trace_ctrl.rmr_length = region->length;
>>>>> +			break;
>>>>> +		}
>>>>> +
>>>>> +	iommu_put_resv_regions(&pdev->dev, &list);
>>>>> +	return hisi_ptt->trace_ctrl.has_rmr ? 0 : -ENOMEM;
>>>>> +}
>>>>> +
>>>>> +static int hisi_ptt_probe(struct pci_dev *pdev,
>>>>> +			  const struct pci_device_id *id)
>>>>> +{
>>>>> +	struct hisi_ptt *hisi_ptt;
>>>>> +	int ret;
>>>>> +
>>>>> +	hisi_ptt = devm_kzalloc(&pdev->dev, sizeof(*hisi_ptt), GFP_KERNEL);
>>>>> +	if (!hisi_ptt)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	mutex_init(&hisi_ptt->mutex);
>>>>> +	hisi_ptt->pdev = pdev;
>>>>> +
>>>>> +	/*
>>>>> +	 * Lifetime of pci_dev is longer than hisi_ptt,
>>>>> +	 * so directly reference to the pci name string.
>>>>> +	 */
>>>>> +	hisi_ptt->name = pci_name(hisi_ptt->pdev);
>>>>> +	pci_set_drvdata(pdev, hisi_ptt);
>>>>> +
>>>>> +	ret = pcim_enable_device(pdev);
>>>>> +	if (ret) {
>>>>> +		pci_err(pdev, "failed to enable device, ret = %d.\n", ret);
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	ret = pcim_iomap_regions(pdev, BIT(2), hisi_ptt->name);
>>>>> +	if (ret) {
>>>>> +		pci_err(pdev, "failed to remap io memory, ret = %d.\n", ret);
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	hisi_ptt->iobase = pcim_iomap_table(pdev)[2];
>>>>> +
>>>>> +	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
>>>>> +	if (ret) {
>>>>> +		pci_err(pdev, "failed to set 64 bit dma mask, ret = %d.\n", ret);
>>>>> +		return ret;
>>>>> +	}
>>>>> +	pci_set_master(pdev);
>>>>> +
>>>>> +	ret = hisi_ptt_register_irq(hisi_ptt);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	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);
>>>>> +		return ret;
>>>>> +	}
>>>>
>>>> If this fails the pci_free_irq_vectors() is not called.
>>>>
>>>
>>> It'll be called as we've registered a devm callback when
>>> allocating the irq vector. See hisi_ptt_register_irq().
>>
>> Ah yes - thanks for pointing that out.
>>
>>>
>>>> I am out of time for today - I will continue tomorrow.
>>>>
>>>
>>> Sure. Thanks!
>>> Yicong
>>>
>>>> Thanks,
>>>> Mathieu
>>>>
>>>>> +
>>>>> +	ret = hisi_ptt_get_rmr(hisi_ptt);
>>>>> +	if (ret) {
>>>>> +		pci_err(pdev, "failed to get RMR region, ret = %d", ret);
>>>>> +		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(&pci_bus_type, &hisi_ptt->hisi_ptt_nb);
>>>>> +	if (ret)
>>>>> +		pci_warn(pdev, "failed to register filter update notifier, ret = %d", ret);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +void hisi_ptt_remove(struct pci_dev *pdev)
>>>>> +{
>>>>> +	struct hisi_ptt *hisi_ptt = pci_get_drvdata(pdev);
>>>>> +
>>>>> +	bus_unregister_notifier(&pci_bus_type, &hisi_ptt->hisi_ptt_nb);
>>>>> +
>>>>> +	/* Cancel any work that has been queued */
>>>>> +	cancel_delayed_work_sync(&hisi_ptt->work);
>>>>> +
>>>>> +	if (hisi_ptt->trace_ctrl.status == HISI_PTT_TRACE_STATUS_ON)
>>>>> +		hisi_ptt_trace_end(hisi_ptt);
>>>>> +
>>>>> +	hisi_ptt_free_trace_buf(hisi_ptt);
>>>>> +	hisi_ptt_release_filters(hisi_ptt);
>>>>> +}
>>>>> +
>>>>> +static const struct pci_device_id hisi_ptt_id_tbl[] = {
>>>>> +	{ PCI_DEVICE(PCI_VENDOR_ID_HUAWEI, 0xa12e) },
>>>>> +	{ }
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(pci, hisi_ptt_id_tbl);
>>>>> +
>>>>> +static struct pci_driver hisi_ptt_driver = {
>>>>> +	.name = "hisi_ptt",
>>>>> +	.id_table = hisi_ptt_id_tbl,
>>>>> +	.probe = hisi_ptt_probe,
>>>>> +	.remove = hisi_ptt_remove,
>>>>> +};
>>>>> +module_pci_driver(hisi_ptt_driver);
>>>>> +
>>>>> +MODULE_LICENSE("GPL v2");
>>>>> +MODULE_AUTHOR("Yicong Yang <yangyicong@xxxxxxxxxxxxx>");
>>>>> +MODULE_DESCRIPTION("Driver for HiSilicon PCIe tune and trace device");
>>>>> -- 
>>>>> 2.33.0
>>>>>
>>>> .
>>>>
> .
>