On 2023/11/16 11:50, Ilkka Koskinen wrote:

Hi Shuai,

I have a few comments below

...

quoted

+static void dwc_pcie_pmu_time_based_event_enable(struct dwc_pcie_pmu *pcie_pmu,
+                      bool enable)
+{
+    struct pci_dev *pdev = pcie_pmu->pdev;
+    u16 ras_des_offset = pcie_pmu->ras_des_offset;
+
+    if (enable)
+        pci_clear_and_set_dword(pdev,
+            ras_des_offset + DWC_PCIE_TIME_BASED_ANAL_CTL,
+            DWC_PCIE_TIME_BASED_TIMER_START, 0x1);
+    else
+        pci_clear_and_set_dword(pdev,
+            ras_des_offset + DWC_PCIE_TIME_BASED_ANAL_CTL,
+            DWC_PCIE_TIME_BASED_TIMER_START, 0x0);

It's a matter of taste, but you could simply do:

    pci_clear_and_set_dword(pdev,
                 ras_des_offset + DWC_PCIE_TIME_BASED_ANAL_CTL,
                 DWC_PCIE_TIME_BASED_TIMER_START, enable);


However, I'm fine with either way.

Good suggestion, will fix it.

quoted

+static u64 dwc_pcie_pmu_read_lane_event_counter(struct perf_event *event)
+{
+    struct dwc_pcie_pmu *pcie_pmu = to_dwc_pcie_pmu(event->pmu);
+    struct pci_dev *pdev = pcie_pmu->pdev;
+    u16 ras_des_offset = pcie_pmu->ras_des_offset;
+    u32 val;
+
+    pci_read_config_dword(pdev, ras_des_offset + DWC_PCIE_EVENT_CNT_DATA, &val);
+
+    return val;
+}

...

quoted

+static int dwc_pcie_register_dev(struct pci_dev *pdev)
+{
+    struct platform_device *plat_dev;
+    struct dwc_pcie_dev_info *dev_info;
+    int ret;
+    u32 bdf;
+
+    bdf = PCI_DEVID(pdev->bus->number, pdev->devfn);
+    plat_dev = platform_device_register_data(NULL, "dwc_pcie_pmu", bdf,
+                         pdev, sizeof(*pdev));
+    ret = PTR_ERR_OR_ZERO(plat_dev);
+    if (ret)
+             return ret;

platform_device_register_data() doesn't return a null pointer and you don't really need 'ret'. You could do something like instead:

   if (IS_ERR(plat_dev))
          return PTR_ERR(plat_dev);

quoted

+    dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL);
+    if (!dev_info)
+        return -ENOMEM;
+
+    /* Cache platform device to handle pci device hotplug */
+    dev_info->plat_dev = plat_dev;
+    dev_info->pdev = pdev;
+    list_add(&dev_info->dev_node, &dwc_pcie_dev_info_head);
+
+    return 0;
+}
+
+static int dwc_pcie_pmu_notifier(struct notifier_block *nb,
+                     unsigned long action, void *data)
+{
+    struct device *dev = data;
+    struct pci_dev *pdev = to_pci_dev(dev);
+    struct dwc_pcie_dev_info *dev_info;
+
+    switch (action) {
+    case BUS_NOTIFY_ADD_DEVICE:
+        if (!dwc_pcie_match_des_cap(pdev))
+            return NOTIFY_DONE;
+        if (dwc_pcie_register_dev(pdev))
+            return NOTIFY_BAD;
+        break;
+    case BUS_NOTIFY_DEL_DEVICE:
+        dev_info = dwc_pcie_find_dev_info(pdev);
+        if (!dev_info)
+            return NOTIFY_DONE;
+        dwc_pcie_unregister_dev(dev_info);
+        break;
+    }
+
+    return NOTIFY_OK;
+}
+
+static struct notifier_block dwc_pcie_pmu_nb = {
+    .notifier_call = dwc_pcie_pmu_notifier,
+};
+
+static int dwc_pcie_pmu_probe(struct platform_device *plat_dev)
+{
+    struct pci_dev *pdev = plat_dev->dev.platform_data;
+    struct dwc_pcie_pmu *pcie_pmu;
+    char *name;
+    u32 bdf, val;
+    u16 vsec;
+    int ret;
+
+    vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_ALIBABA,
+                    DWC_PCIE_VSEC_RAS_DES_ID);

You nicely changed to use vendor list in this version but here the driver still tries to find Alibaba specific capability.

Sorry, I missed here.

I guess, you could search again using the vendor list. The other option would be to make dwc_pcie_match_des_cap() to return the vendor id, pass it to dwc_pcie_register_dev(), which would add it to device's platform data with
the pointer to the pci device.

The dwc_pcie_pmu_probe() is called by device which has DWC_PCIE_VSEC_RAS_DES_ID cap.
So I guess I can use pdev->vendor directly here, e.g?

	pci_find_vsec_capability(pdev, pdev->vendor, DWC_PCIE_VSEC_RAS_DES_ID);

Best Regards,
Shuai

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel