Re: [PATCH v6 3/4] drivers/perf: add DesignWare PCIe PMU driver

[Yicong Yang]

On 2023/10/9 21:08, Shuai Xue wrote:
> 
> 
> On 2023/8/4 11:09, Shuai Xue wrote:
>>
>>
>> On 2023/8/4 10:28, Yicong Yang wrote:
>>> On 2023/8/4 9:39, Shuai Xue wrote:
>>>>
>>>>
>>>> On 2023/8/1 19:46, Yicong Yang wrote:
>>>>> On 2023/7/28 20:41, Shuai Xue wrote:
>>>>>>
>>>>>>
>>>>>> On 2023/7/27 17:39, Jonathan Cameron wrote:
>>>>>>> On Tue, 6 Jun 2023 23:14:07 +0800
>>>>>>> Yicong Yang <yangyicong@xxxxxxxxxx> wrote:
>>>>>>>
>>>>>>>> On 2023/6/6 15:49, Shuai Xue wrote:
>>>>>>>>> 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 not a PCIe
>>>>>>>>> Root Complex integrated End Point(RCiEP) device but only register counters
>>>>>>>>> provided by each PCIe Root Port.
>>>>>>>>>
>>>>>>>>> To facilitate collection of statistics the controller provides the
>>>>>>>>> following two features for each Root Port:
>>>>>>>>>
>>>>>>>>> - Time Based Analysis (RX/TX data throughput and time spent in each
>>>>>>>>>   low-power LTSSM state)
>>>>>>>>> - Event counters (Error and Non-Error for lanes)
>>>>>>>>>
>>>>>>>>> Note, only one counter for each type and does not overflow interrupt.
>>>>>>>>>
>>>>>>>>> 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 16 bytes)::
>>>>>>>>>
>>>>>>>>>     $# perf stat -a -e dwc_rootport_3018/Rx_PCIe_TLP_Data_Payload/
>>>>>>>>>
>>>>>>>>> average RX bandwidth can be calculated like this:
>>>>>>>>>
>>>>>>>>>     PCIe TX Bandwidth = PCIE_TX_DATA * 16B / Measure_Time_Window
>>>>>>>>>
>>>>>>>>> Signed-off-by: Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx>
>>>>>>>>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>>>>>>>>> Link: https://lore.kernel.org/oe-kbuild-all/202305170639.XU3djFZX-lkp@xxxxxxxxx/
>>>>>>>>> Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
>>>>>>>
>>>>>>> I'll review on top to avoid any duplication with Yicong.
>>>>>>
>>>>>> Thank you! It also served as a reminder that I missed Yicong's email. It appears
>>>>>> that Thunderbird mistakenly moved his email to the junk folder, resulting in me
>>>>>> overlooking it.
>>>>>>
>>>>>>>
>>>>>>> Note I've cropped the stuff neither of us commented on so it's
>>>>>>> easier to spot the feedback.
>>>>>>
>>>>>> Thank you for noting that. My feedback is replied inline.
>>>>>>
>>>>>>>
>>>>>>> Jonathan
>>>>>>>
>>>>>>>>> ---
>>>>>>>>>  drivers/perf/Kconfig        |   7 +
>>>>>>>>>  drivers/perf/Makefile       |   1 +
>>>>>>>>>  drivers/perf/dwc_pcie_pmu.c | 706 ++++++++++++++++++++++++++++++++++++
>>>>>>>>>  3 files changed, 714 insertions(+)
>>>>>>>>>  create mode 100644 drivers/perf/dwc_pcie_pmu.c
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
>>>>>>>>> index 711f82400086..6ff3921d7a62 100644
>>>>>>>>> --- a/drivers/perf/Kconfig
>>>>>>>>> +++ b/drivers/perf/Kconfig
>>>>>>>>> @@ -209,6 +209,13 @@ config MARVELL_CN10K_DDR_PMU
>>>>>>>>>  	  Enable perf support for Marvell DDR Performance monitoring
>>>>>>>>>  	  event on CN10K platform.
>>>>>>>>>  
>>>>>>>>> +config DWC_PCIE_PMU
>>>>>>>>> +	tristate "Enable Synopsys DesignWare PCIe PMU Support"
>>>>>>>>> +	depends on (ARM64 && PCI)
>>>>>>>>> +	help
>>>>>>>>> +	  Enable perf support for Synopsys DesignWare PCIe PMU Performance
>>>>>>>>> +	  monitoring event on Yitian 710 platform.
>>>>>>>
>>>>>>> The documentation kind of implies this isn't platform specific.
>>>>>>> If some parts are (such as which events exist) then you may want to push
>>>>>>> that to userspace / perftool with appropriate matching against specific SoC.
>>>>>>>
>>>>>>> If it is generic, then change this text to "event on platform including the Yitian 710."
>>>>>>
>>>>>> It is generic without any platform specific, so I will change it as you expected.
>>>>>>
>>>>>>>
>>>>>>>>> +
>>>>>>>>>  source "drivers/perf/arm_cspmu/Kconfig"
>>>>>>>>>  
>>>>>>>>>  source "drivers/perf/amlogic/Kconfig"
>>>>>>>
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000000000000..8bfcf6e0662d
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/drivers/perf/dwc_pcie_pmu.c
>>>>>>>>> @@ -0,0 +1,706 @@
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>>>> +
>>>>>>>>> +struct dwc_pcie_pmu {
>>>>>>>>> +	struct pci_dev		*pdev;		/* Root Port device */  
>>>>>>>>
>>>>>>>> If the root port removed after the probe of this PCIe PMU driver, we'll access the NULL
>>>>>>>> pointer. I didn't see you hold the root port to avoid the removal.
>>>>>>
>>>>>> Do you mean that I should have a reference count of rootport by pci_dev_get() when allocating
>>>>>> pcie_pmu?
>>>>>>
>>>>>>      pcie_pmu->pdev = pci_dev_get();
>>>>>
>>>>> It could be one option, but will block the removal of device from userspace. Another option
>>>>> is to register a PCI bus notifier then on removal/added the driver can get notified and handle
>>>>> it, for example, remove the related PMU on the removal of the root ports.
>>>>
>>>> I see, but can root port be removed from userspace? I check the hotplug slot interface, no root
>>>> port is available to power off.
>>>>
>>>
>>> For hotplug maybe not, but user can remove certian device through sysfs:
>>>
>>> echo 1 > /sys/bus/pci/devices/<root port>/remove
>>>
>>
>> Thank you, I will add a notifier for removal/added action.
>>
>> Best Regards,
>> Shuai
> 
> Hi, Yicong,
> 
> I am confused when adding a notifier by bus_register_notifier(). If I have added a action to
> pdev->dev to unregister pmu:
> 
> 		ret = perf_pmu_register(&pcie_pmu->pmu, name, -1);
> 		if (ret) {
> 			pci_err(pcie_pmu->pdev,
> 				"Error %d registering PMU @%x\n", ret, bdf);
> 			goto out;
> 		}
> 		ret = devm_add_action_or_reset(
> 			&pdev->dev, dwc_pcie_pmu_unregister_pmu, &pcie_pmu->pmu);
> 
> the pmu will be unregister when the port removes, so accessing the NULL pointer will never happen,
> right? Do we still need the bus_register_notifier()?
> 

Not necessary, a notifier is used to notice the device removal and avoid dereferencing the
NULL pointer. If you find another way like above to avoid the issue it will be ok. Since
your pmu is 1:1 related to the root port, add a devm action to unregister the PMU on root
port removal looks better.

Thanks.