| Message ID | 20230606074938.97724-1-xueshuai@linux.alibaba.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drivers/perf: add Synopsys DesignWare PCIe PMU driver support | expand |
On 2023/6/6 15:49, Shuai Xue wrote: > changes since v5: > - Rewrite the commit log to follow policy in pci_ids.h (Bjorn Helgaas) > - return error code when __dwc_pcie_pmu_probe failed (Baolin Wang) > - call 'cpuhp_remove_multi_state()' when exiting the driver. (Baolin Wang) > - pick up Review-by tag from Baolin for Patch 1 and 3 > > changes since v4: > > 1. addressing commens from Bjorn Helgaas: > - reorder the includes by alpha > - change all macros with upper-case hex > - change ras_des type into u16 > - remove unnecessary outer "()" > - minor format changes > > 2. Address commensts from Jonathan Cameron: > - rewrite doc and add a example to show how to use lane event > > 3. fix compile error reported by: kernel test robot > - remove COMPILE_TEST and add depend on PCI in kconfig > - add Reported-by: kernel test robot <lkp@intel.com> > > Changes since v3: > > 1. addressing comments from Robin Murphy: > - add a prepare patch to define pci id in linux/pci_ids.h > - remove unnecessary 64BIT dependency > - fix DWC_PCIE_PER_EVENT_OFF/ON macro > - remove dwc_pcie_pmu struct and move all its fileds into dwc_pcie_rp_info > - remove unnecessary format field show > - use sysfs_emit() instead of all the assorted sprintf() and snprintf() calls. > - remove unnecessary spaces and remove unnecessary cast to follow event show convention > - remove pcie_pmu_event_attr_is_visible > - fix a refcout leak on error branch when walk pci device in for_each_pci_dev > - remove bdf field from dwc_pcie_rp_info and calculate it at runtime > - finish all the checks before allocating rp_info to avoid hanging wasted memory > - remove some unused fields > - warp out control register configuration from sub function to .add() > - make function return type with a proper signature > - fix lane event count enable by clear DWC_PCIE_CNT_ENABLE field first > - pass rp_info directly to the read_*_counter helpers and in start, stop and add callbacks > - move event type validtion into .event_init() > - use is_sampling_event() to be consistent with everything else of pmu drivers > - remove unnecessary dev_err message in .event_init() > - return EINVAL instead EOPNOTSUPP for not a valid event > - finish all the checks before start modifying the event > - fix sibling event check by comparing event->pmu with sibling->pmu > - probe PMU for each rootport independently > - use .update() as .read() directly > - remove dynamically generating symbolic name of lane event > - redefine static symbolic name of lane event and leave lane filed to user > - add CPU hotplug support > > 2. addressing comments from Baolin: > - add a mask to avoid possible overflow > > Changes since v2 addressing comments from Baolin: > - remove redundant macro definitions > - use dev_err to print error message > - change pmu_is_register to boolean > - use PLATFORM_DEVID_NONE macro > - fix module author format > > Changes since v1: > > 1. address comments from Jonathan: > - drop marco for PMU name and VSEC version > - simplify code with PCI standard marco > - simplify code with FIELD_PREP()/FIELD_GET() to replace shift marco > - name register filed with single _ instead double > - wrap dwc_pcie_pmu_{write}_dword out and drop meaningless snaity check > - check vendor id while matching vesc with pci_find_vsec_capability() > - remove RP_NUM_MAX and use a list to organize PMU devices for rootports > - replace DWC_PCIE_CREATE_BDF with standard PCI_DEVID > - comments on riping register together > > 2. address comments from Bjorn: > - rename DWC_PCIE_VSEC_ID to DWC_PCIE_VSEC_RAS_DES_ID > - rename cap_pos to ras_des > - simplify declare of device_attribute with DEVICE_ATTR_RO > - simplify code with PCI standard macro and API like pcie_get_width_cap() > - fix some code style problem and typo > - drop meaningless snaity check of container_of > > 3. address comments from Yicong: > - use sysfs_emit() to replace sprintf() > - simplify iteration of pci device with for_each_pci_dev > - pick preferred CPUs on a near die and add comments > - unregister PMU drivers only for failed ones > - log on behalf PMU device and give more hint > - fix some code style problem > > (Thanks for all comments and they are very valuable to me) > > This patchset adds the PCIe Performance Monitoring Unit (PMU) driver support > for T-Head Yitian 710 SoC chip. Yitian 710 is based on the Synopsys PCI Express > Core controller IP which provides statistics feature. > > Shuai Xue (4): > docs: perf: Add description for Synopsys DesignWare PCIe PMU driver > PCI: Add Alibaba Vendor ID to linux/pci_ids.h > drivers/perf: add DesignWare PCIe PMU driver > MAINTAINERS: add maintainers for DesignWare PCIe PMU driver > > .../admin-guide/perf/dwc_pcie_pmu.rst | 97 +++ > Documentation/admin-guide/perf/index.rst | 1 + > MAINTAINERS | 6 + > drivers/infiniband/hw/erdma/erdma_hw.h | 2 - > drivers/perf/Kconfig | 7 + > drivers/perf/Makefile | 1 + > drivers/perf/dwc_pcie_pmu.c | 706 ++++++++++++++++++ > include/linux/pci_ids.h | 2 + > 8 files changed, 820 insertions(+), 2 deletions(-) > create mode 100644 Documentation/admin-guide/perf/dwc_pcie_pmu.rst > create mode 100644 drivers/perf/dwc_pcie_pmu.c > Hi, all, Gently ping. Any comments are welcomed. Thank you. Best Regards, Shuai
On 2023/6/16 16:39, Shuai Xue wrote: > > > On 2023/6/6 15:49, Shuai Xue wrote: >> changes since v5: >> - Rewrite the commit log to follow policy in pci_ids.h (Bjorn Helgaas) >> - return error code when __dwc_pcie_pmu_probe failed (Baolin Wang) >> - call 'cpuhp_remove_multi_state()' when exiting the driver. (Baolin Wang) >> - pick up Review-by tag from Baolin for Patch 1 and 3 >> >> changes since v4: >> >> 1. addressing commens from Bjorn Helgaas: >> - reorder the includes by alpha >> - change all macros with upper-case hex >> - change ras_des type into u16 >> - remove unnecessary outer "()" >> - minor format changes >> >> 2. Address commensts from Jonathan Cameron: >> - rewrite doc and add a example to show how to use lane event >> >> 3. fix compile error reported by: kernel test robot >> - remove COMPILE_TEST and add depend on PCI in kconfig >> - add Reported-by: kernel test robot <lkp@intel.com> >> >> Changes since v3: >> >> 1. addressing comments from Robin Murphy: >> - add a prepare patch to define pci id in linux/pci_ids.h >> - remove unnecessary 64BIT dependency >> - fix DWC_PCIE_PER_EVENT_OFF/ON macro >> - remove dwc_pcie_pmu struct and move all its fileds into dwc_pcie_rp_info >> - remove unnecessary format field show >> - use sysfs_emit() instead of all the assorted sprintf() and snprintf() calls. >> - remove unnecessary spaces and remove unnecessary cast to follow event show convention >> - remove pcie_pmu_event_attr_is_visible >> - fix a refcout leak on error branch when walk pci device in for_each_pci_dev >> - remove bdf field from dwc_pcie_rp_info and calculate it at runtime >> - finish all the checks before allocating rp_info to avoid hanging wasted memory >> - remove some unused fields >> - warp out control register configuration from sub function to .add() >> - make function return type with a proper signature >> - fix lane event count enable by clear DWC_PCIE_CNT_ENABLE field first >> - pass rp_info directly to the read_*_counter helpers and in start, stop and add callbacks >> - move event type validtion into .event_init() >> - use is_sampling_event() to be consistent with everything else of pmu drivers >> - remove unnecessary dev_err message in .event_init() >> - return EINVAL instead EOPNOTSUPP for not a valid event >> - finish all the checks before start modifying the event >> - fix sibling event check by comparing event->pmu with sibling->pmu >> - probe PMU for each rootport independently >> - use .update() as .read() directly >> - remove dynamically generating symbolic name of lane event >> - redefine static symbolic name of lane event and leave lane filed to user >> - add CPU hotplug support >> >> 2. addressing comments from Baolin: >> - add a mask to avoid possible overflow >> >> Changes since v2 addressing comments from Baolin: >> - remove redundant macro definitions >> - use dev_err to print error message >> - change pmu_is_register to boolean >> - use PLATFORM_DEVID_NONE macro >> - fix module author format >> >> Changes since v1: >> >> 1. address comments from Jonathan: >> - drop marco for PMU name and VSEC version >> - simplify code with PCI standard marco >> - simplify code with FIELD_PREP()/FIELD_GET() to replace shift marco >> - name register filed with single _ instead double >> - wrap dwc_pcie_pmu_{write}_dword out and drop meaningless snaity check >> - check vendor id while matching vesc with pci_find_vsec_capability() >> - remove RP_NUM_MAX and use a list to organize PMU devices for rootports >> - replace DWC_PCIE_CREATE_BDF with standard PCI_DEVID >> - comments on riping register together >> >> 2. address comments from Bjorn: >> - rename DWC_PCIE_VSEC_ID to DWC_PCIE_VSEC_RAS_DES_ID >> - rename cap_pos to ras_des >> - simplify declare of device_attribute with DEVICE_ATTR_RO >> - simplify code with PCI standard macro and API like pcie_get_width_cap() >> - fix some code style problem and typo >> - drop meaningless snaity check of container_of >> >> 3. address comments from Yicong: >> - use sysfs_emit() to replace sprintf() >> - simplify iteration of pci device with for_each_pci_dev >> - pick preferred CPUs on a near die and add comments >> - unregister PMU drivers only for failed ones >> - log on behalf PMU device and give more hint >> - fix some code style problem >> >> (Thanks for all comments and they are very valuable to me) >> >> This patchset adds the PCIe Performance Monitoring Unit (PMU) driver support >> for T-Head Yitian 710 SoC chip. Yitian 710 is based on the Synopsys PCI Express >> Core controller IP which provides statistics feature. >> >> Shuai Xue (4): >> docs: perf: Add description for Synopsys DesignWare PCIe PMU driver >> PCI: Add Alibaba Vendor ID to linux/pci_ids.h >> drivers/perf: add DesignWare PCIe PMU driver >> MAINTAINERS: add maintainers for DesignWare PCIe PMU driver >> >> .../admin-guide/perf/dwc_pcie_pmu.rst | 97 +++ >> Documentation/admin-guide/perf/index.rst | 1 + >> MAINTAINERS | 6 + >> drivers/infiniband/hw/erdma/erdma_hw.h | 2 - >> drivers/perf/Kconfig | 7 + >> drivers/perf/Makefile | 1 + >> drivers/perf/dwc_pcie_pmu.c | 706 ++++++++++++++++++ >> include/linux/pci_ids.h | 2 + >> 8 files changed, 820 insertions(+), 2 deletions(-) >> create mode 100644 Documentation/admin-guide/perf/dwc_pcie_pmu.rst >> create mode 100644 drivers/perf/dwc_pcie_pmu.c >> > > Hi, all, > > Gently ping. Any comments are welcomed. Hi, all, Gentle ping. > > Thank you. > > > Best Regards, > Shuai > >
On 2023/7/10 20:04, Shuai Xue wrote: > > > On 2023/6/16 16:39, Shuai Xue wrote: >> >> >> On 2023/6/6 15:49, Shuai Xue wrote: >>> changes since v5: >>> - Rewrite the commit log to follow policy in pci_ids.h (Bjorn Helgaas) >>> - return error code when __dwc_pcie_pmu_probe failed (Baolin Wang) >>> - call 'cpuhp_remove_multi_state()' when exiting the driver. (Baolin Wang) >>> - pick up Review-by tag from Baolin for Patch 1 and 3 >>> >>> changes since v4: >>> >>> 1. addressing commens from Bjorn Helgaas: >>> - reorder the includes by alpha >>> - change all macros with upper-case hex >>> - change ras_des type into u16 >>> - remove unnecessary outer "()" >>> - minor format changes >>> >>> 2. Address commensts from Jonathan Cameron: >>> - rewrite doc and add a example to show how to use lane event >>> >>> 3. fix compile error reported by: kernel test robot >>> - remove COMPILE_TEST and add depend on PCI in kconfig >>> - add Reported-by: kernel test robot <lkp@intel.com> >>> >>> Changes since v3: >>> >>> 1. addressing comments from Robin Murphy: >>> - add a prepare patch to define pci id in linux/pci_ids.h >>> - remove unnecessary 64BIT dependency >>> - fix DWC_PCIE_PER_EVENT_OFF/ON macro >>> - remove dwc_pcie_pmu struct and move all its fileds into dwc_pcie_rp_info >>> - remove unnecessary format field show >>> - use sysfs_emit() instead of all the assorted sprintf() and snprintf() calls. >>> - remove unnecessary spaces and remove unnecessary cast to follow event show convention >>> - remove pcie_pmu_event_attr_is_visible >>> - fix a refcout leak on error branch when walk pci device in for_each_pci_dev >>> - remove bdf field from dwc_pcie_rp_info and calculate it at runtime >>> - finish all the checks before allocating rp_info to avoid hanging wasted memory >>> - remove some unused fields >>> - warp out control register configuration from sub function to .add() >>> - make function return type with a proper signature >>> - fix lane event count enable by clear DWC_PCIE_CNT_ENABLE field first >>> - pass rp_info directly to the read_*_counter helpers and in start, stop and add callbacks >>> - move event type validtion into .event_init() >>> - use is_sampling_event() to be consistent with everything else of pmu drivers >>> - remove unnecessary dev_err message in .event_init() >>> - return EINVAL instead EOPNOTSUPP for not a valid event >>> - finish all the checks before start modifying the event >>> - fix sibling event check by comparing event->pmu with sibling->pmu >>> - probe PMU for each rootport independently >>> - use .update() as .read() directly >>> - remove dynamically generating symbolic name of lane event >>> - redefine static symbolic name of lane event and leave lane filed to user >>> - add CPU hotplug support >>> >>> 2. addressing comments from Baolin: >>> - add a mask to avoid possible overflow >>> >>> Changes since v2 addressing comments from Baolin: >>> - remove redundant macro definitions >>> - use dev_err to print error message >>> - change pmu_is_register to boolean >>> - use PLATFORM_DEVID_NONE macro >>> - fix module author format >>> >>> Changes since v1: >>> >>> 1. address comments from Jonathan: >>> - drop marco for PMU name and VSEC version >>> - simplify code with PCI standard marco >>> - simplify code with FIELD_PREP()/FIELD_GET() to replace shift marco >>> - name register filed with single _ instead double >>> - wrap dwc_pcie_pmu_{write}_dword out and drop meaningless snaity check >>> - check vendor id while matching vesc with pci_find_vsec_capability() >>> - remove RP_NUM_MAX and use a list to organize PMU devices for rootports >>> - replace DWC_PCIE_CREATE_BDF with standard PCI_DEVID >>> - comments on riping register together >>> >>> 2. address comments from Bjorn: >>> - rename DWC_PCIE_VSEC_ID to DWC_PCIE_VSEC_RAS_DES_ID >>> - rename cap_pos to ras_des >>> - simplify declare of device_attribute with DEVICE_ATTR_RO >>> - simplify code with PCI standard macro and API like pcie_get_width_cap() >>> - fix some code style problem and typo >>> - drop meaningless snaity check of container_of >>> >>> 3. address comments from Yicong: >>> - use sysfs_emit() to replace sprintf() >>> - simplify iteration of pci device with for_each_pci_dev >>> - pick preferred CPUs on a near die and add comments >>> - unregister PMU drivers only for failed ones >>> - log on behalf PMU device and give more hint >>> - fix some code style problem >>> >>> (Thanks for all comments and they are very valuable to me) >>> >>> This patchset adds the PCIe Performance Monitoring Unit (PMU) driver support >>> for T-Head Yitian 710 SoC chip. Yitian 710 is based on the Synopsys PCI Express >>> Core controller IP which provides statistics feature. >>> >>> Shuai Xue (4): >>> docs: perf: Add description for Synopsys DesignWare PCIe PMU driver >>> PCI: Add Alibaba Vendor ID to linux/pci_ids.h >>> drivers/perf: add DesignWare PCIe PMU driver >>> MAINTAINERS: add maintainers for DesignWare PCIe PMU driver >>> >>> .../admin-guide/perf/dwc_pcie_pmu.rst | 97 +++ >>> Documentation/admin-guide/perf/index.rst | 1 + >>> MAINTAINERS | 6 + >>> drivers/infiniband/hw/erdma/erdma_hw.h | 2 - >>> drivers/perf/Kconfig | 7 + >>> drivers/perf/Makefile | 1 + >>> drivers/perf/dwc_pcie_pmu.c | 706 ++++++++++++++++++ >>> include/linux/pci_ids.h | 2 + >>> 8 files changed, 820 insertions(+), 2 deletions(-) >>> create mode 100644 Documentation/admin-guide/perf/dwc_pcie_pmu.rst >>> create mode 100644 drivers/perf/dwc_pcie_pmu.c >>> >> >> Hi, all, >> >> Gently ping. Any comments are welcomed. > > > Hi, all, > > Gentle ping. > Hi, all Gentle reminder, thank you. >> >> Thank you. >> >> >> Best Regards, >> Shuai >> >>
On Mon, 24 Jul 2023 10:34:08 +0800 Shuai Xue <xueshuai@linux.alibaba.com> wrote: > On 2023/7/10 20:04, Shuai Xue wrote: > > > > > > On 2023/6/16 16:39, Shuai Xue wrote: > >> > >> > >> On 2023/6/6 15:49, Shuai Xue wrote: > >>> changes since v5: > >>> - Rewrite the commit log to follow policy in pci_ids.h (Bjorn Helgaas) > >>> - return error code when __dwc_pcie_pmu_probe failed (Baolin Wang) > >>> - call 'cpuhp_remove_multi_state()' when exiting the driver. (Baolin Wang) > >>> - pick up Review-by tag from Baolin for Patch 1 and 3 > >>> > >>> changes since v4: > >>> > >>> 1. addressing commens from Bjorn Helgaas: > >>> - reorder the includes by alpha > >>> - change all macros with upper-case hex > >>> - change ras_des type into u16 > >>> - remove unnecessary outer "()" > >>> - minor format changes > >>> > >>> 2. Address commensts from Jonathan Cameron: > >>> - rewrite doc and add a example to show how to use lane event > >>> > >>> 3. fix compile error reported by: kernel test robot > >>> - remove COMPILE_TEST and add depend on PCI in kconfig > >>> - add Reported-by: kernel test robot <lkp@intel.com> > >>> > >>> Changes since v3: > >>> > >>> 1. addressing comments from Robin Murphy: > >>> - add a prepare patch to define pci id in linux/pci_ids.h > >>> - remove unnecessary 64BIT dependency > >>> - fix DWC_PCIE_PER_EVENT_OFF/ON macro > >>> - remove dwc_pcie_pmu struct and move all its fileds into dwc_pcie_rp_info > >>> - remove unnecessary format field show > >>> - use sysfs_emit() instead of all the assorted sprintf() and snprintf() calls. > >>> - remove unnecessary spaces and remove unnecessary cast to follow event show convention > >>> - remove pcie_pmu_event_attr_is_visible > >>> - fix a refcout leak on error branch when walk pci device in for_each_pci_dev > >>> - remove bdf field from dwc_pcie_rp_info and calculate it at runtime > >>> - finish all the checks before allocating rp_info to avoid hanging wasted memory > >>> - remove some unused fields > >>> - warp out control register configuration from sub function to .add() > >>> - make function return type with a proper signature > >>> - fix lane event count enable by clear DWC_PCIE_CNT_ENABLE field first > >>> - pass rp_info directly to the read_*_counter helpers and in start, stop and add callbacks > >>> - move event type validtion into .event_init() > >>> - use is_sampling_event() to be consistent with everything else of pmu drivers > >>> - remove unnecessary dev_err message in .event_init() > >>> - return EINVAL instead EOPNOTSUPP for not a valid event > >>> - finish all the checks before start modifying the event > >>> - fix sibling event check by comparing event->pmu with sibling->pmu > >>> - probe PMU for each rootport independently > >>> - use .update() as .read() directly > >>> - remove dynamically generating symbolic name of lane event > >>> - redefine static symbolic name of lane event and leave lane filed to user > >>> - add CPU hotplug support > >>> > >>> 2. addressing comments from Baolin: > >>> - add a mask to avoid possible overflow > >>> > >>> Changes since v2 addressing comments from Baolin: > >>> - remove redundant macro definitions > >>> - use dev_err to print error message > >>> - change pmu_is_register to boolean > >>> - use PLATFORM_DEVID_NONE macro > >>> - fix module author format > >>> > >>> Changes since v1: > >>> > >>> 1. address comments from Jonathan: > >>> - drop marco for PMU name and VSEC version > >>> - simplify code with PCI standard marco > >>> - simplify code with FIELD_PREP()/FIELD_GET() to replace shift marco > >>> - name register filed with single _ instead double > >>> - wrap dwc_pcie_pmu_{write}_dword out and drop meaningless snaity check > >>> - check vendor id while matching vesc with pci_find_vsec_capability() > >>> - remove RP_NUM_MAX and use a list to organize PMU devices for rootports > >>> - replace DWC_PCIE_CREATE_BDF with standard PCI_DEVID > >>> - comments on riping register together > >>> > >>> 2. address comments from Bjorn: > >>> - rename DWC_PCIE_VSEC_ID to DWC_PCIE_VSEC_RAS_DES_ID > >>> - rename cap_pos to ras_des > >>> - simplify declare of device_attribute with DEVICE_ATTR_RO > >>> - simplify code with PCI standard macro and API like pcie_get_width_cap() > >>> - fix some code style problem and typo > >>> - drop meaningless snaity check of container_of > >>> > >>> 3. address comments from Yicong: > >>> - use sysfs_emit() to replace sprintf() > >>> - simplify iteration of pci device with for_each_pci_dev > >>> - pick preferred CPUs on a near die and add comments > >>> - unregister PMU drivers only for failed ones > >>> - log on behalf PMU device and give more hint > >>> - fix some code style problem > >>> > >>> (Thanks for all comments and they are very valuable to me) > >>> > >>> This patchset adds the PCIe Performance Monitoring Unit (PMU) driver support > >>> for T-Head Yitian 710 SoC chip. Yitian 710 is based on the Synopsys PCI Express > >>> Core controller IP which provides statistics feature. > >>> > >>> Shuai Xue (4): > >>> docs: perf: Add description for Synopsys DesignWare PCIe PMU driver > >>> PCI: Add Alibaba Vendor ID to linux/pci_ids.h > >>> drivers/perf: add DesignWare PCIe PMU driver > >>> MAINTAINERS: add maintainers for DesignWare PCIe PMU driver > >>> > >>> .../admin-guide/perf/dwc_pcie_pmu.rst | 97 +++ > >>> Documentation/admin-guide/perf/index.rst | 1 + > >>> MAINTAINERS | 6 + > >>> drivers/infiniband/hw/erdma/erdma_hw.h | 2 - > >>> drivers/perf/Kconfig | 7 + > >>> drivers/perf/Makefile | 1 + > >>> drivers/perf/dwc_pcie_pmu.c | 706 ++++++++++++++++++ > >>> include/linux/pci_ids.h | 2 + > >>> 8 files changed, 820 insertions(+), 2 deletions(-) > >>> create mode 100644 Documentation/admin-guide/perf/dwc_pcie_pmu.rst > >>> create mode 100644 drivers/perf/dwc_pcie_pmu.c > >>> > >> > >> Hi, all, > >> > >> Gently ping. Any comments are welcomed. > > > > > > Hi, all, > > > > Gentle ping. > > > > Hi, all > > Gentle reminder, thank you. Hi Shuai, Really a question for Bjorn I think, but here is my 2 cents... The problem here is that we need to do that fundamental redesign of the way the PCI ports drivers work. I'm not sure there is a path to merging this until that is done. The bigger problem is that I'm not sure anyone is actively looking at that yet. I'd like to look at this (as I have the same problem for some other drivers), but it is behind various other things on my todo list. Bjorn might be persuaded on a temporary solution, but that would come with some maintenance problems, particularly when we try to do it 'right' in the future. Maybe adding another service driver would be a stop gap as long as we know we won't keep doing so for ever. Not sure. Jonathan > > >> > >> Thank you. > >> > >> > >> Best Regards, > >> Shuai > >> > >> >
On 2023/7/24 17:18, Jonathan Cameron wrote: > Really a question for Bjorn I think, but here is my 2 cents... > > The problem here is that we need to do that fundamental redesign of the > way the PCI ports drivers work. I'm not sure there is a path to merging > this until that is done. The bigger problem is that I'm not sure anyone > is actively looking at that yet. I'd like to look at this (as I have > the same problem for some other drivers), but it is behind various > other things on my todo list. > > Bjorn might be persuaded on a temporary solution, but that would come > with some maintenance problems, particularly when we try to do it > 'right' in the future. Maybe adding another service driver would be > a stop gap as long as we know we won't keep doing so for ever. Not sure. Thank you for your reply, and got your point, :) + Bjorn >>>> The approach used here is to separately walk the PCI topology and >>>> register the devices. It can 'maybe' get away with that because no >>>> interrupts and I assume resets have no nasty impacts on it because >>>> the device is fairly simple. In general that's not going to work. >>>> CXL does a similar trick (which I don't much like, but too late >>>> now), but we've also run into the problem of how to get interrupts >>>> if not the main driver. >>> >>> Yes, this is a real problem. I think the "walk all PCI devices >>> looking for one we like" approach is terrible because it breaks a lot >>> of driver model assumptions (no device ID to autoload module via udev, >>> hotplug doesn't work, etc), but we don't have a good alternative right >>> now. >>> >>> I think portdrv is slightly better because at least it claims the >>> device in the usual way and gives a way for service drivers to >>> register with it. But I don't really like that either because it >>> created a new weird /sys/bus/pci_express hierarchy full of these >>> sub-devices that aren't really devices, and it doesn't solve the >>> module load and hotplug issues. >>> >>> I would like to have portdrv be completely built into the PCI core and >>> not claim Root Ports or Switch Ports. Then those devices would be >>> available via the usual driver model for driver loading and binding >>> and for hotplug. >> >> Let me see if I understand this correctly as I can think of a few options >> that perhaps are inline with what you are thinking. >> >> 1) All the portdrv stuff converted to normal PCI core helper functions >> that a driver bound to the struct pci_dev can use. >> 2) Driver core itself provides a bunch of extra devices alongside the >> struct pci_dev one to which additional drivers can bind? - so kind >> of portdrv handling, but squashed into the PCI device topology? >> 3) Have portdrv operated under the hood, so all the services etc that >> it provides don't require a driver to be bound at all. Then >> allow usual VID/DID based driver binding. >> >> If 1 - we are going to run into class device restrictions and that will >> just move where we have to handle the potential vendor specific parts. >> We probably don't want that to be a hydra with all the functionality >> and lookups etc driven from there, so do we end up with sub devices >> of that new PCI port driver with a discover method based on either >> vsec + VID or DVSEC with devices created under the main pci_dev. >> That would have to include nastiness around interrupt discovery for >> those sub devices. So ends up roughly like port_drv. >> >> I don't think 2 solves anything. >> >> For 3 - interrupts and ownership of facilities is going to be tricky >> as initially those need to be owned by the PCI core (no device driver bound) >> and then I guess handed off to the driver once it shows up? Maybe that >> driver should call a pci_claim_port() that gives it control of everything >> and pci_release_port() that hands it all back to the core. That seems >> racey. > > Yes, 3 is the option I want to explore. That's what we already do for > things like ASPM. Agreed, interrupts is a potential issue. I think > the architected parts of config space should be implicitly owned by > the PCI core, with interfaces à la pci_disable_link_state() if drivers > need them. > > Bjorn > https://lore.kernel.org/lkml/ZGUAWxoEngmqFcLJ@bhelgaas/ @Bjorn Is there a path to merging this patch set until your explore is done? And are you still actively looking at that yet? I am not quite familiar with PCI core, but I would like to help work on that. Thank you. Best Regards, Shuai
On Mon, Jul 24, 2023 at 10:18:07AM +0100, Jonathan Cameron wrote: > On Mon, 24 Jul 2023 10:34:08 +0800 > Shuai Xue <xueshuai@linux.alibaba.com> wrote: > > On 2023/7/10 20:04, Shuai Xue wrote: > > > On 2023/6/16 16:39, Shuai Xue wrote: > > >> On 2023/6/6 15:49, Shuai Xue wrote: > > >>> This patchset adds the PCIe Performance Monitoring Unit (PMU) driver support > > >>> for T-Head Yitian 710 SoC chip. Yitian 710 is based on the Synopsys PCI Express > > >>> Core controller IP which provides statistics feature. > ... > Really a question for Bjorn I think, but here is my 2 cents... > > The problem here is that we need to do that fundamental redesign of the > way the PCI ports drivers work. I'm not sure there is a path to merging > this until that is done. The bigger problem is that I'm not sure anyone > is actively looking at that yet. I'd like to look at this (as I have > the same problem for some other drivers), but it is behind various > other things on my todo list. > > Bjorn might be persuaded on a temporary solution, but that would come > with some maintenance problems, particularly when we try to do it > 'right' in the future. Maybe adding another service driver would be > a stop gap as long as we know we won't keep doing so for ever. Not sure. I think the question here is around the for_each_pci_dev() in __dwc_pcie_pmu_probe()? I don't *like* that because of the assumptions it breaks (autoload doesn't work, hotplug doesn't work), but: - There are several other drivers that also do this, - I don't have a better suggest for any of them, - It's not a drivers/pci thing, so not really up to me anyway, so I don't have any problem with this being merged as-is, as long as you can live with the limitations. I don't think this series does anything to work around those limitations, i.e., it doesn't make up fake device IDs for module loading or fake events for hotplug, so it seems like we could improve the implementation later if we ever have a way to do it. Bjorn
On 2023/7/26 04:59, Bjorn Helgaas wrote: > On Mon, Jul 24, 2023 at 10:18:07AM +0100, Jonathan Cameron wrote: >> On Mon, 24 Jul 2023 10:34:08 +0800 >> Shuai Xue <xueshuai@linux.alibaba.com> wrote: >>> On 2023/7/10 20:04, Shuai Xue wrote: >>>> On 2023/6/16 16:39, Shuai Xue wrote: >>>>> On 2023/6/6 15:49, Shuai Xue wrote: > >>>>>> This patchset adds the PCIe Performance Monitoring Unit (PMU) driver support >>>>>> for T-Head Yitian 710 SoC chip. Yitian 710 is based on the Synopsys PCI Express >>>>>> Core controller IP which provides statistics feature. > >> ... >> Really a question for Bjorn I think, but here is my 2 cents... >> >> The problem here is that we need to do that fundamental redesign of the >> way the PCI ports drivers work. I'm not sure there is a path to merging >> this until that is done. The bigger problem is that I'm not sure anyone >> is actively looking at that yet. I'd like to look at this (as I have >> the same problem for some other drivers), but it is behind various >> other things on my todo list. >> >> Bjorn might be persuaded on a temporary solution, but that would come >> with some maintenance problems, particularly when we try to do it >> 'right' in the future. Maybe adding another service driver would be >> a stop gap as long as we know we won't keep doing so for ever. Not sure. > > I think the question here is around the for_each_pci_dev() in > __dwc_pcie_pmu_probe()? I don't *like* that because of the > assumptions it breaks (autoload doesn't work, hotplug doesn't work), > but: > > - There are several other drivers that also do this, > - I don't have a better suggest for any of them, > - It's not a drivers/pci thing, so not really up to me anyway, > > so I don't have any problem with this being merged as-is, as long as > you can live with the limitations. > > I don't think this series does anything to work around those > limitations, i.e., it doesn't make up fake device IDs for module > loading or fake events for hotplug, so it seems like we could improve > the implementation later if we ever have a way to do it. > > Bjorn + Will Ok, thank you for confirmation, Bjorn. Then it comes to perf driver parts and it is really a question for @Will I think. What's your opinion about merging this patch set, @Will? Best Regards, Shuai
On Thu, Jul 27, 2023 at 11:45:22AM +0800, Shuai Xue wrote: > > > On 2023/7/26 04:59, Bjorn Helgaas wrote: > > On Mon, Jul 24, 2023 at 10:18:07AM +0100, Jonathan Cameron wrote: > >> On Mon, 24 Jul 2023 10:34:08 +0800 > >> Shuai Xue <xueshuai@linux.alibaba.com> wrote: > >>> On 2023/7/10 20:04, Shuai Xue wrote: > >>>> On 2023/6/16 16:39, Shuai Xue wrote: > >>>>> On 2023/6/6 15:49, Shuai Xue wrote: > > > >>>>>> This patchset adds the PCIe Performance Monitoring Unit (PMU) driver support > >>>>>> for T-Head Yitian 710 SoC chip. Yitian 710 is based on the Synopsys PCI Express > >>>>>> Core controller IP which provides statistics feature. > > > >> ... > >> Really a question for Bjorn I think, but here is my 2 cents... > >> > >> The problem here is that we need to do that fundamental redesign of the > >> way the PCI ports drivers work. I'm not sure there is a path to merging > >> this until that is done. The bigger problem is that I'm not sure anyone > >> is actively looking at that yet. I'd like to look at this (as I have > >> the same problem for some other drivers), but it is behind various > >> other things on my todo list. > >> > >> Bjorn might be persuaded on a temporary solution, but that would come > >> with some maintenance problems, particularly when we try to do it > >> 'right' in the future. Maybe adding another service driver would be > >> a stop gap as long as we know we won't keep doing so for ever. Not sure. > > > > I think the question here is around the for_each_pci_dev() in > > __dwc_pcie_pmu_probe()? I don't *like* that because of the > > assumptions it breaks (autoload doesn't work, hotplug doesn't work), > > but: > > > > - There are several other drivers that also do this, > > - I don't have a better suggest for any of them, > > - It's not a drivers/pci thing, so not really up to me anyway, > > > > so I don't have any problem with this being merged as-is, as long as > > you can live with the limitations. > > > > I don't think this series does anything to work around those > > limitations, i.e., it doesn't make up fake device IDs for module > > loading or fake events for hotplug, so it seems like we could improve > > the implementation later if we ever have a way to do it. > > > > Bjorn > > + Will > > Ok, thank you for confirmation, Bjorn. Then it comes to perf driver parts and > it is really a question for @Will I think. > > What's your opinion about merging this patch set, @Will? No fundamental objection from me, but I'll have a closer look when you post a version addressing the feedback from Jonathan and Yicong. Cheers, Will
On 2023/7/28 21:39, Will Deacon wrote: > On Thu, Jul 27, 2023 at 11:45:22AM +0800, Shuai Xue wrote: >> >> >> On 2023/7/26 04:59, Bjorn Helgaas wrote: >>> On Mon, Jul 24, 2023 at 10:18:07AM +0100, Jonathan Cameron wrote: >>>> On Mon, 24 Jul 2023 10:34:08 +0800 >>>> Shuai Xue <xueshuai@linux.alibaba.com> wrote: >>>>> On 2023/7/10 20:04, Shuai Xue wrote: >>>>>> On 2023/6/16 16:39, Shuai Xue wrote: >>>>>>> On 2023/6/6 15:49, Shuai Xue wrote: >>> >>>>>>>> This patchset adds the PCIe Performance Monitoring Unit (PMU) driver support >>>>>>>> for T-Head Yitian 710 SoC chip. Yitian 710 is based on the Synopsys PCI Express >>>>>>>> Core controller IP which provides statistics feature. >>> >>>> ... >>>> Really a question for Bjorn I think, but here is my 2 cents... >>>> >>>> The problem here is that we need to do that fundamental redesign of the >>>> way the PCI ports drivers work. I'm not sure there is a path to merging >>>> this until that is done. The bigger problem is that I'm not sure anyone >>>> is actively looking at that yet. I'd like to look at this (as I have >>>> the same problem for some other drivers), but it is behind various >>>> other things on my todo list. >>>> >>>> Bjorn might be persuaded on a temporary solution, but that would come >>>> with some maintenance problems, particularly when we try to do it >>>> 'right' in the future. Maybe adding another service driver would be >>>> a stop gap as long as we know we won't keep doing so for ever. Not sure. >>> >>> I think the question here is around the for_each_pci_dev() in >>> __dwc_pcie_pmu_probe()? I don't *like* that because of the >>> assumptions it breaks (autoload doesn't work, hotplug doesn't work), >>> but: >>> >>> - There are several other drivers that also do this, >>> - I don't have a better suggest for any of them, >>> - It's not a drivers/pci thing, so not really up to me anyway, >>> >>> so I don't have any problem with this being merged as-is, as long as >>> you can live with the limitations. >>> >>> I don't think this series does anything to work around those >>> limitations, i.e., it doesn't make up fake device IDs for module >>> loading or fake events for hotplug, so it seems like we could improve >>> the implementation later if we ever have a way to do it. >>> >>> Bjorn >> >> + Will >> >> Ok, thank you for confirmation, Bjorn. Then it comes to perf driver parts and >> it is really a question for @Will I think. >> >> What's your opinion about merging this patch set, @Will? > > No fundamental objection from me, but I'll have a closer look when you > post a version addressing the feedback from Jonathan and Yicong. Thanks for your input! I appreciate that you don't have any fundamental objections to merging the patch set. I'll definitely take into account the feedback from Jonathan and Yicong before posting a revised version. Best Regards, Cheers. Shuai
changes since v5: - Rewrite the commit log to follow policy in pci_ids.h (Bjorn Helgaas) - return error code when __dwc_pcie_pmu_probe failed (Baolin Wang) - call 'cpuhp_remove_multi_state()' when exiting the driver. (Baolin Wang) - pick up Review-by tag from Baolin for Patch 1 and 3 changes since v4: 1. addressing commens from Bjorn Helgaas: - reorder the includes by alpha - change all macros with upper-case hex - change ras_des type into u16 - remove unnecessary outer "()" - minor format changes 2. Address commensts from Jonathan Cameron: - rewrite doc and add a example to show how to use lane event 3. fix compile error reported by: kernel test robot - remove COMPILE_TEST and add depend on PCI in kconfig - add Reported-by: kernel test robot <lkp@intel.com> Changes since v3: 1. addressing comments from Robin Murphy: - add a prepare patch to define pci id in linux/pci_ids.h - remove unnecessary 64BIT dependency - fix DWC_PCIE_PER_EVENT_OFF/ON macro - remove dwc_pcie_pmu struct and move all its fileds into dwc_pcie_rp_info - remove unnecessary format field show - use sysfs_emit() instead of all the assorted sprintf() and snprintf() calls. - remove unnecessary spaces and remove unnecessary cast to follow event show convention - remove pcie_pmu_event_attr_is_visible - fix a refcout leak on error branch when walk pci device in for_each_pci_dev - remove bdf field from dwc_pcie_rp_info and calculate it at runtime - finish all the checks before allocating rp_info to avoid hanging wasted memory - remove some unused fields - warp out control register configuration from sub function to .add() - make function return type with a proper signature - fix lane event count enable by clear DWC_PCIE_CNT_ENABLE field first - pass rp_info directly to the read_*_counter helpers and in start, stop and add callbacks - move event type validtion into .event_init() - use is_sampling_event() to be consistent with everything else of pmu drivers - remove unnecessary dev_err message in .event_init() - return EINVAL instead EOPNOTSUPP for not a valid event - finish all the checks before start modifying the event - fix sibling event check by comparing event->pmu with sibling->pmu - probe PMU for each rootport independently - use .update() as .read() directly - remove dynamically generating symbolic name of lane event - redefine static symbolic name of lane event and leave lane filed to user - add CPU hotplug support 2. addressing comments from Baolin: - add a mask to avoid possible overflow Changes since v2 addressing comments from Baolin: - remove redundant macro definitions - use dev_err to print error message - change pmu_is_register to boolean - use PLATFORM_DEVID_NONE macro - fix module author format Changes since v1: 1. address comments from Jonathan: - drop marco for PMU name and VSEC version - simplify code with PCI standard marco - simplify code with FIELD_PREP()/FIELD_GET() to replace shift marco - name register filed with single _ instead double - wrap dwc_pcie_pmu_{write}_dword out and drop meaningless snaity check - check vendor id while matching vesc with pci_find_vsec_capability() - remove RP_NUM_MAX and use a list to organize PMU devices for rootports - replace DWC_PCIE_CREATE_BDF with standard PCI_DEVID - comments on riping register together 2. address comments from Bjorn: - rename DWC_PCIE_VSEC_ID to DWC_PCIE_VSEC_RAS_DES_ID - rename cap_pos to ras_des - simplify declare of device_attribute with DEVICE_ATTR_RO - simplify code with PCI standard macro and API like pcie_get_width_cap() - fix some code style problem and typo - drop meaningless snaity check of container_of 3. address comments from Yicong: - use sysfs_emit() to replace sprintf() - simplify iteration of pci device with for_each_pci_dev - pick preferred CPUs on a near die and add comments - unregister PMU drivers only for failed ones - log on behalf PMU device and give more hint - fix some code style problem (Thanks for all comments and they are very valuable to me) This patchset adds the PCIe Performance Monitoring Unit (PMU) driver support for T-Head Yitian 710 SoC chip. Yitian 710 is based on the Synopsys PCI Express Core controller IP which provides statistics feature. Shuai Xue (4): docs: perf: Add description for Synopsys DesignWare PCIe PMU driver PCI: Add Alibaba Vendor ID to linux/pci_ids.h drivers/perf: add DesignWare PCIe PMU driver MAINTAINERS: add maintainers for DesignWare PCIe PMU driver .../admin-guide/perf/dwc_pcie_pmu.rst | 97 +++ Documentation/admin-guide/perf/index.rst | 1 + MAINTAINERS | 6 + drivers/infiniband/hw/erdma/erdma_hw.h | 2 - drivers/perf/Kconfig | 7 + drivers/perf/Makefile | 1 + drivers/perf/dwc_pcie_pmu.c | 706 ++++++++++++++++++ include/linux/pci_ids.h | 2 + 8 files changed, 820 insertions(+), 2 deletions(-) create mode 100644 Documentation/admin-guide/perf/dwc_pcie_pmu.rst create mode 100644 drivers/perf/dwc_pcie_pmu.c