[Deepin-Kernel-SIG] [linux 6.6-y] [Upstream] [Alibaba] Support Root Port PMU for Yitian710 （回合upstream社区rootport pmu 驱动到6.6内核）#831
Merged
Conversation
ANBZ: #8565 commit cae4061 upstream. Alibaba's T-Head Yitan 710 SoC includes Synopsys' DesignWare Core PCIe controller which implements PMU for performance and functional debugging to facilitate system maintenance. Document it to provide guidance on how to use it. Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Yicong Yang <yangyicong@hisilicon.com> Tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> Link: https://lore.kernel.org/r/20231208025652.87192-2-xueshuai@linux.alibaba.com Signed-off-by: Will Deacon <will@kernel.org> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
ANBZ: #8565 commit ad6534c upstream. The Alibaba Vendor ID (0x1ded) is now used by Alibaba elasticRDMA ("erdma") and will be shared with the upcoming PCIe PMU ("dwc_pcie_pmu"). Move the Vendor ID to linux/pci_ids.h so that it can shared by several drivers later. Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> # pci_ids.h Tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> Link: https://lore.kernel.org/r/20231208025652.87192-3-xueshuai@linux.alibaba.com Signed-off-by: Will Deacon <will@kernel.org> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
ANBZ: #8565 commit ac16087 upstream. The clear and set pattern is commonly used for accessing PCI config, move the helper pci_clear_and_set_dword() from aspm.c into PCI header. In addition, rename to pci_clear_and_set_config_dword() to retain the "config" information and match the other accessors. No functional change intended. Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> Tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> Link: https://lore.kernel.org/r/20231208025652.87192-4-xueshuai@linux.alibaba.com Signed-off-by: Will Deacon <will@kernel.org> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
ANBZ: #8565 commit af9597a upstream. This commit adds the PCIe Performance Monitoring Unit (PMU) driver support for T-Head Yitian SoC chip. Yitian is based on the Synopsys PCI Express Core controller IP which provides statistics feature. The PMU is a PCIe configuration space register block provided by each PCIe Root Port in a Vendor-Specific Extended Capability named RAS D.E.S (Debug, Error injection, and Statistics). To facilitate collection of statistics the controller provides the following two features for each Root Port: - one 64-bit counter for Time Based Analysis (RX/TX data throughput and time spent in each low-power LTSSM state) and - one 32-bit counter for Event Counting (error and non-error events for a specified lane) Note: There is no interrupt for counter overflow. This driver adds PMU devices for each PCIe Root Port. And the PMU device is named based the BDF of Root Port. For example, 30:03.0 PCI bridge: Device 1ded:8000 (rev 01) the PMU device name for this Root Port is dwc_rootport_3018. Example usage of counting PCIe RX TLP data payload (Units of bytes):: $# perf stat -a -e dwc_rootport_3018/Rx_PCIe_TLP_Data_Payload/ average RX bandwidth can be calculated like this: PCIe TX Bandwidth = Rx_PCIe_TLP_Data_Payload / Measure_Time_Window Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Yicong Yang <yangyicong@hisilicon.com> Reviewed-and-tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> Link: https://lore.kernel.org/r/20231208025652.87192-5-xueshuai@linux.alibaba.com [will: Fix sparse error due to use of uninitialised 'vsec' symbol in dwc_pcie_match_des_cap()] Signed-off-by: Will Deacon <will@kernel.org> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
Reviewer's GuideThis PR upstreams Alibaba’s root‐port PMU support into Linux 6.6 by refactoring a PCI config helper, centralizing the Alibaba vendor ID, and adding a Synopsys DesignWare PCIe root–port PMU driver under perf. Sequence Diagram for DWC PCIe PMU RegistrationsequenceDiagram
participant MI as Module Init (dwc_pcie_pmu_init)
participant PCICore as PCI Core
participant PlatCore as Platform Core
participant CPUHP as CPU Hotplug Subsystem
participant PerfCore as Perf Subsystem
participant PMUDriver as dwc_pcie_pmu_driver
MI->>PCICore: for_each_pci_dev(pdev)
PCICore-->>MI: pdev
MI->>MI: dwc_pcie_match_des_cap(pdev)?
alt PCI device has DWC PMU capability
MI->>PlatCore: dwc_pcie_register_dev(pdev) (via platform_device_register_data)
PlatCore-->>MI: plat_dev created
MI->>MI: Cache dev_info (pdev, plat_dev)
end
MI->>CPUHP: cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, callbacks)
CPUHP-->>MI: dwc_pcie_pmu_hp_state (handler id)
MI->>PlatCore: platform_driver_register(&dwc_pcie_pmu_driver)
PlatCore->>PMUDriver: probe(plat_dev)
PMUDriver->>PMUDriver: Allocate & Initialize dwc_pcie_pmu struct
PMUDriver->>CPUHP: cpuhp_state_add_instance(dwc_pcie_pmu_hp_state, &pcie_pmu->cpuhp_node)
PMUDriver->>PerfCore: perf_pmu_register(&pcie_pmu->pmu)
PerfCore-->>PMUDriver: Registration status
PMUDriver-->>PlatCore: Probe result
MI->>PCICore: bus_register_notifier(&pci_bus_type, &dwc_pcie_pmu_nb)
PCICore-->>MI: Notifier registration status
Sequence Diagram for DWC PCIe PMU Event MonitoringsequenceDiagram
actor User as User (perf tool)
participant PerfCore as Perf Subsystem
participant PMUCallbacks as dwc_pcie_pmu Callbacks
participant HW as PCIe Hardware Registers
User->>PerfCore: Start Monitoring Event (e.g., perf stat -e dwc_pmu/event/)
PerfCore->>PMUCallbacks: pmu.add(event, flags) (dwc_pcie_pmu_event_add)
PMUCallbacks->>HW: Configure Event Registers (pci_write_config_dword)
HW-->>PMUCallbacks: Config OK
PMUCallbacks-->>PerfCore: Event Added
PerfCore->>PMUCallbacks: pmu.start(event, flags) (dwc_pcie_pmu_event_start)
PMUCallbacks->>HW: Enable Counter in Registers (pci_clear_and_set_config_dword)
HW-->>PMUCallbacks: Counter Enabled
PMUCallbacks-->>PerfCore: Event Started
loop Periodically or on demand by PerfCore
PerfCore->>PMUCallbacks: pmu.read(event) (dwc_pcie_pmu_event_update)
PMUCallbacks->>HW: Read Counter Value from Registers (pci_read_config_dword)
HW-->>PMUCallbacks: Counter Value
PMUCallbacks->>PMUCallbacks: Update event->count (local64_add)
PMUCallbacks-->>PerfCore: Value Updated in event structure
end
User->>PerfCore: Stop Monitoring
PerfCore->>PMUCallbacks: pmu.stop(event, flags) (dwc_pcie_pmu_event_stop)
PMUCallbacks->>HW: Disable Counter in Registers
HW-->>PMUCallbacks: Counter Disabled
PMUCallbacks->>PMUCallbacks: pmu.read(event) (final read)
PMUCallbacks-->>PerfCore: Event Stopped
PerfCore->>PMUCallbacks: pmu.del(event, flags) (dwc_pcie_pmu_event_del)
PMUCallbacks->>PMUCallbacks: Cleanup (pcie_pmu->event[type] = NULL)
PMUCallbacks-->>PerfCore: Event Deleted
Class Diagram for pci_clear_and_set_dword RefactoringclassDiagram
class PCI_Config_Access {
<<Module: drivers/pci/access.c>>
+pci_clear_and_set_config_dword(dev: const struct pci_dev*, pos: int, clear: u32, set: u32): void
}
class PCI_ASPM {
<<Module: drivers/pci/pcie/aspm.c>>
-pci_clear_and_set_dword(pdev: struct pci_dev*, pos: int, clear: u32, set: u32): void /* Removed */
+aspm_calc_l12_info(): void /* Updated to call new function */
+pcie_config_aspm_l1ss(): void /* Updated to call new function */
}
class PCI_Header {
<<Header: include/linux/pci.h>>
+pci_clear_and_set_config_dword(dev: const struct pci_dev*, pos: int, clear: u32, set: u32): void
}
PCI_ASPM ..> PCI_Config_Access : uses pci_clear_and_set_config_dword()
PCI_Config_Access ..> PCI_Header : implements declaration in
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the Alibaba Yitian710 root port PMU by introducing new PCIe PMU functionality and updating vendor definitions. Key changes include the addition of a new vendor ID for Alibaba devices, the introduction of a new helper API (pci_clear_and_set_config_dword) for PCI configuration modifications, and the corresponding updates to perf Makefile, Kconfig, and documentation to support the new DWC PCIe PMU.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| include/linux/pci_ids.h | Added a new PCI vendor definition for Alibaba. |
| include/linux/pci.h | Declared the new pci_clear_and_set_config_dword API. |
| drivers/perf/Makefile | Registered the new DWC PCIe PMU driver. |
| drivers/perf/Kconfig | Added a configuration option for the DWC PCIe PMU. |
| drivers/pci/pcie/aspm.c | Replaced local pci_clear_and_set_dword calls with pci_clear_and_set_config_dword. |
| drivers/pci/access.c | Implemented the new pci_clear_and_set_config_dword function. |
| drivers/infiniband/hw/erdma/erdma_hw.h | Removed redundant PCI_VENDOR_ID_ALIBABA definition. |
| Documentation/admin-guide/perf/* | Updated documentation to include details for the new DWC PCIe PMU. |
Comments suppressed due to low confidence (1)
include/linux/pci_ids.h:2615
- Ensure that the centralized definition of PCI_VENDOR_ID_ALIBABA in pci_ids.h covers all usage across the codebase, replacing any redundant definitions like the one removed from erdma_hw.h.
#define PCI_VENDOR_ID_ALIBABA 0x1ded
| { | ||
| u32 val; | ||
|
|
||
| pci_read_config_dword(dev, pos, &val); |
There was a problem hiding this comment.
[nitpick] Consider adding an inline comment for pci_clear_and_set_config_dword to clarify its intended usage and improve maintainability.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Avenger-285714 - I've reviewed your changes - here's some feedback:
- Split the core PCI config helper refactor and the new DWC PCIe PMU driver into separate patches so each change can be reviewed and backported independently.
- Add documentation for the new pci_clear_and_set_config_dword helper in the PCI core API docs to make its usage clear to other drivers.
- Populate the newly added Documentation/admin-guide/perf/dwc_pcie_pmu.rst with driver usage examples and event descriptions.
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return PTR_ERR(plat_dev); | ||
|
|
||
| dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL); | ||
| if (!dev_info) |
There was a problem hiding this comment.
issue (bug_risk): Resource leak on kzalloc failure
Call platform_device_unregister() before returning on kzalloc() failure to prevent a resource leak.
Comment on lines
+602
to
+611
| void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos, | ||
| u32 clear, u32 set) | ||
| { | ||
| u32 val; | ||
|
|
||
| pci_read_config_dword(dev, pos, &val); | ||
| val &= ~clear; | ||
| val |= set; | ||
| pci_write_config_dword(dev, pos, val); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Ignoring return value from pci_read_config_dword
Check the return value of pci_read_config_dword to ensure 'val' is valid before using it.
Suggested change
| void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos, | |
| u32 clear, u32 set) | |
| { | |
| u32 val; | |
| pci_read_config_dword(dev, pos, &val); | |
| val &= ~clear; | |
| val |= set; | |
| pci_write_config_dword(dev, pos, val); | |
| } | |
| void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos, | |
| u32 clear, u32 set) | |
| { | |
| u32 val; | |
| int ret; | |
| ret = pci_read_config_dword(dev, pos, &val); | |
| if (ret) | |
| return; | |
| val &= ~clear; | |
| val |= set; | |
| pci_write_config_dword(dev, pos, val); | |
| } |
|
|
||
| if (type == DWC_PCIE_LANE_EVENT) { | ||
| lane = DWC_PCIE_EVENT_LANE(event); | ||
| if (lane < 0 || lane >= pcie_pmu->nr_lanes) |
There was a problem hiding this comment.
nitpick: Redundant check: lane is always non-negative
Since 'lane' is unsigned, 'lane < 0' is unnecessary. Only check 'lane >= pcie_pmu->nr_lanes'.
Comment on lines
+769
to
+773
| platform_driver_register_err: | ||
| cpuhp_remove_multi_state(dwc_pcie_pmu_hp_state); | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Platform driver not unregistered on init failure
Call platform_driver_unregister() in the error path after a failed bus_register_notifier() to ensure proper cleanup.
Suggested change
| platform_driver_register_err: | |
| cpuhp_remove_multi_state(dwc_pcie_pmu_hp_state); | |
| return ret; | |
| } | |
| platform_driver_register_err: | |
| cpuhp_remove_multi_state(dwc_pcie_pmu_hp_state); | |
| platform_driver_unregister(&dwc_pcie_pmu_driver); | |
| return ret; | |
| } |
|
|
||
| 30:03.0 PCI bridge: Device 1ded:8000 (rev 01) | ||
|
|
||
| the PMU device name for this Root Port is dwc_rootport_3018. |
There was a problem hiding this comment.
issue: The derivation of the PMU device name from BDF is unclear.
Clarify and document how '30:03.0' is encoded as '3018' in the PMU device name for user understanding.
Comment on lines
+88
to
+89
| Each lane has the same event set and to avoid generating a list of hundreds | ||
| of events, the user need to specify the lane ID explicitly, e.g.:: |
There was a problem hiding this comment.
issue (typo): Grammatical error: 'need' should be 'needs'.
Change 'the user need' to 'the user needs' for correct grammar.
Suggested change
| Each lane has the same event set and to avoid generating a list of hundreds | |
| of events, the user need to specify the lane ID explicitly, e.g.:: | |
| Each lane has the same event set and to avoid generating a list of hundreds | |
| of events, the user needs to specify the lane ID explicitly, e.g.:: |
Member
|
Chekdepends: commit b94b054 commit e669388 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=8565
Link: https://gitee.com/anolis/cloud-kernel/pulls/2900
Summary by Sourcery
Add upstream support for Alibaba’s root port PMU on Yitian710 by providing a new DWC PCIe perf driver, refactoring PCI config helper APIs, and aligning vendor ID definitions.
New Features:
Enhancements:
Build:
Documentation: