On Mon, 21 Feb 2022 16:43:03 +0800
Yicong Yang <yangyicong@xxxxxxxxxxxxx> 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 <yangyicong@xxxxxxxxxxxxx>
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(&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);
>
> @@ -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(&pci_bus_type, &hisi_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(&pci_bus_type, &hisi_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(&hisi_ptt->work);
> +
> if (hisi_ptt->trace_ctrl.status == HISI_PTT_TRACE_STATUS_ON)
> hisi_ptt_trace_end(hisi_ptt);
>
...
