Re: [PATCH v4 4/8] hisi_ptt: Add support for dynamically updating the filter list

2022-02-21 Thread Jonathan Cameron via iommu
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


[PATCH v4 4/8] hisi_ptt: Add support for dynamically updating the filter list

2022-02-21 Thread Yicong Yang via iommu
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 
---
 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;
 }
 
-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;
+   }
 
-   filter->pdev = pdev;
-   list_add_tail(>list, target_list);
+   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;
+   }
+   }
 
-   /* 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);
+   /* Update the available port mask */
+   if (!is_port)
+   continue;
+
+   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;
+   case BUS_NOTIFY_DEL_DEVICE:
+   info.is_add = false;
+   break;
+   default:
+   return 0;
+   }
+
+   hisi_ptt_update_fifo_in(hisi_ptt, );
+
+   return 0;
+}
+
+static int