On 2023/6/6 17:56, Jonathan Cameron wrote:
> On Tue, 23 May 2023 17:32:26 +0800
> Yicong Yang <yangyicong@xxxxxxxxxx> wrote:
>
>> From: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxx>
> 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)
>
> .
>
