Re: [PATCH v3 2/4] hwtracing: hisi_ptt: Add support for dynamically updating the filter list

[Yicong Yang <yangyicong@huawei.com>]

On 2023/6/6 17:56, Jonathan Cameron wrote:
> On Tue, 23 May 2023 17:32:26 +0800
> Yicong Yang <yangyicong@huawei.com> wrote:
> 
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> 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 <yangyicong@hisilicon.com>
> Hi Yangyicong,
> 
> A few comments inline

Thanks for the comments! Will fix them up in next version.

> 
> Jonathan
> 
>> ---
>>  Documentation/trace/hisi-ptt.rst |   6 +-
>>  drivers/hwtracing/ptt/hisi_ptt.c | 165 +++++++++++++++++++++++++++++--
>>  drivers/hwtracing/ptt/hisi_ptt.h |  39 ++++++++
>>  3 files changed, 201 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/trace/hisi-ptt.rst b/Documentation/trace/hisi-ptt.rst
>> index 4f87d8e21065..3641aca4287a 100644
>> --- a/Documentation/trace/hisi-ptt.rst
>> +++ b/Documentation/trace/hisi-ptt.rst
>> @@ -153,9 +153,9 @@ 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
>>  available filters and will check the invalid inputs.
>>  
>> -Currently the available filters are detected in driver's probe. If the supported
>> -devices are removed/added after probe, you may need to reload the driver to update
>> -the filters.
>> +The available filters will be dynamically updates, which means you will always
> 
> updated, 
> 
>> +get correct filter information when hotplug events happen, or when you manually
>> +remove/rescan the devices.
>>  
>>  2. Type
>>  -------
>> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
>> index 548cfef51ace..9b4acbc434b0 100644
>> --- a/drivers/hwtracing/ptt/hisi_ptt.c
>> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
>> @@ -357,24 +357,41 @@ static int hisi_ptt_register_irq(struct hisi_ptt *hisi_ptt)
> 
> ...
> 
>>  static struct hisi_ptt_filter_desc *
>> -hisi_ptt_alloc_add_filter(struct hisi_ptt *hisi_ptt, struct pci_dev *pdev)
>> +hisi_ptt_alloc_add_filter(struct hisi_ptt *hisi_ptt, u16 devid, bool is_port)
>>  {
>>  	struct hisi_ptt_filter_desc *filter;
>> +	u8 devfn = devid & 0xff;
>> +	char *filter_name;
>> +
>> +	filter_name = kasprintf(GFP_KERNEL, "%04x:%02x:%02x.%d", pci_domain_nr(hisi_ptt->pdev->bus),
>> +				 PCI_BUS_NUM(devid), PCI_SLOT(devfn), PCI_FUNC(devfn));
>> +	if (!filter_name) {
>> +		pci_err(hisi_ptt->pdev, "failed to allocate name for filter %s\n",
>> +			filter_name);
>> +		kfree(filter);
> 
> Don't think you've allocated filter yet.
> 
>> +		return NULL;
>> +	}
>>  
>>  	filter = kzalloc(sizeof(*filter), GFP_KERNEL);
>>  	if (!filter) {
>>  		pci_err(hisi_ptt->pdev, "failed to add filter for %s\n",
>> -			pci_name(pdev));
>> +			filter_name);
>>  		return NULL;
>>  	}
>>  
>> -	filter->devid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>> -	filter->is_port = pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT;
>> +	filter->name = filter_name;
>> +	filter->is_port = is_port;
>> +	filter->devid = devid;
>> +
>>  	if (filter->is_port) {
>>  		list_add_tail(&filter->list, &hisi_ptt->port_filters);
>>  
>> @@ -387,6 +404,102 @@ hisi_ptt_alloc_add_filter(struct hisi_ptt *hisi_ptt, struct pci_dev *pdev)
>>  	return filter;
>>  }
> 
> 
> 
> 
> ...
> 
>> @@ -633,14 +752,19 @@ static int hisi_ptt_trace_valid_filter(struct hisi_ptt *hisi_ptt, u64 config)
>>  	 * For Requester ID filters, walk the available filter list to see
>>  	 * whether we have one matched.
>>  	 */
>> +	mutex_lock(&hisi_ptt->filter_lock);
>>  	if (!hisi_ptt->trace_ctrl.is_port) {
>>  		list_for_each_entry(filter, &hisi_ptt->req_filters, list) {
>> -			if (val == hisi_ptt_get_filter_val(filter->devid, filter->is_port))
>> +			if (val == hisi_ptt_get_filter_val(filter->devid, filter->is_port)) {
>> +				mutex_unlock(&hisi_ptt->filter_lock);
> 
> Perhaps a single exit location makes more sense given we need to unlock in
> all paths?  goto unlock; etc
> 
>>  				return 0;
>> +			}
>>  		}
>>  	} else if (bitmap_subset(&val, &port_mask, BITS_PER_LONG)) {
>> +		mutex_unlock(&hisi_ptt->filter_lock);
>>  		return 0;
>>  	}
>> +	mutex_unlock(&hisi_ptt->filter_lock);
>>  
>>  	return -EINVAL;
>>  }
>> @@ -916,6 +1040,31 @@ static int hisi_ptt_register_pmu(struct hisi_ptt *hisi_ptt)
>>  					&hisi_ptt->hisi_ptt_pmu);
>>  }
> 
> 
> 
>> diff --git a/drivers/hwtracing/ptt/hisi_ptt.h b/drivers/hwtracing/ptt/hisi_ptt.h
>> index 5beb1648c93a..d080b306af77 100644
>> --- a/drivers/hwtracing/ptt/hisi_ptt.h
>> +++ b/drivers/hwtracing/ptt/hisi_ptt.h
> 
> ...
> 
>> @@ -161,6 +182,7 @@ struct hisi_ptt_pmu_buf {
>>   * struct hisi_ptt - Per PTT device data
>>   * @trace_ctrl:   the control information of PTT trace
>>   * @hotplug_node: node for register cpu hotplug event
>> + * @hisi_ptt_nb:  dynamic filter update notifier
> 
> Wrong location. Order should match the fields in the structure.
> So this belongs just above hotplug_node
> 
> 
>>   * @hisi_ptt_pmu: the pum device of trace
>>   * @iobase:       base IO address of the device
>>   * @pdev:         pci_dev of this PTT device
>> @@ -170,10 +192,15 @@ struct hisi_ptt_pmu_buf {
>>   * @lower_bdf:    the lower BDF range of the PCI devices managed by this PTT device
>>   * @port_filters: the filter list of root ports
>>   * @req_filters:  the filter list of requester ID
>> + * @filter_lock:  lock to protect the filters
>>   * @port_mask:    port mask of the managed root ports
>> + * @work:         delayed work for filter updating
>> + * @filter_update_lock: spinlock to protect the filter update fifo
>> + * @filter_update_fifo: fifo of the filters waiting to update the filter list
>>   */
>>  struct hisi_ptt {
>>  	struct hisi_ptt_trace_ctrl trace_ctrl;
>> +	struct notifier_block hisi_ptt_nb;
>>  	struct hlist_node hotplug_node;
>>  	struct pmu hisi_ptt_pmu;
>>  	void __iomem *iobase;
>> @@ -192,7 +219,19 @@ struct hisi_ptt {
>>  	 */
>>  	struct list_head port_filters;
>>  	struct list_head req_filters;
>> +	struct mutex filter_lock;
>>  	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);
>>  };
>>  
>>  #define to_hisi_ptt(pmu) container_of(pmu, struct hisi_ptt, hisi_ptt_pmu)
> 
> .
>