On 2023/11/16 11:50, Ilkka Koskinen wrote:
>
> Hi Shuai,
>
> I have a few comments below
>
>
...
>
>> +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.
>
>> +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;
>> +}
>
> ...
>
>> +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);
>
>> + 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
