linux-kernel - Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d370b69a-3b70-4e3b-94a3-43e0bc1305cd@163.com>
Date: Mon, 24 Mar 2025 22:29:09 +0800
From: Hans Zhang <18255117159@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: lpieralisi@...nel.org, kw@...ux.com, manivannan.sadhasivam@...aro.org,
 robh@...nel.org, bhelgaas@...gle.com, jingoohan1@...il.com,
 thomas.richard@...tlin.com, linux-pci@...r.kernel.org,
 LKML <linux-kernel@...r.kernel.org>
Subject: Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for
 finding the capabilities



On 2025/3/24 21:44, Ilpo Järvinen wrote:
> On Mon, 24 Mar 2025, Hans Zhang wrote:
> 
>> Since the PCI core is now exposing generic APIs for the host bridges to
> 
> No need to say "since ... is now exposing". Just say "Use ..." as if the
> API has always existed even if you just added it.
> 

Hi Ilpo,

Thanks your for reply. Will change.


>> search for the PCIe capabilities, make use of them in the CDNS driver.
>>
>> Signed-off-by: Hans Zhang <18255117159@....com>
>> ---
>> Changes since v5:
>> https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@163.com
>>
>> - Kconfig add "select PCI_HOST_HELPERS"
>> ---
>>   drivers/pci/controller/cadence/Kconfig        |  1 +
>>   drivers/pci/controller/cadence/pcie-cadence.c | 25 +++++++++++++++++++
>>   drivers/pci/controller/cadence/pcie-cadence.h |  3 +++
>>   3 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
>> index 8a0044bb3989..0a4f245bbeb0 100644
>> --- a/drivers/pci/controller/cadence/Kconfig
>> +++ b/drivers/pci/controller/cadence/Kconfig
>> @@ -5,6 +5,7 @@ menu "Cadence-based PCIe controllers"
>>   
>>   config PCIE_CADENCE
>>   	bool
>> +	select PCI_HOST_HELPERS
>>   
>>   config PCIE_CADENCE_HOST
>>   	bool
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
>> index 204e045aed8c..329dab4ff813 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence.c
>> @@ -8,6 +8,31 @@
>>   
>>   #include "pcie-cadence.h"
>>   
>> +static u32 cdns_pcie_read_cfg(void *priv, int where, int size)
>> +{
>> +	struct cdns_pcie *pcie = priv;
>> +	u32 val;
>> +
>> +	if (size == 4)
>> +		val = readl(pcie->reg_base + where);
> 
> Should this use cdns_pcie_readl() ?

pci_host_bridge_find_*capability required to read two or four bytes.

reg = read_cfg(priv, cap_ptr, 2);
or
header = read_cfg(priv, pos, 4);

Here I mainly want to write it the same way as size == 2 and size == 1.
Or size == 4 should I write it as cdns_pcie_readl() ?

> 
>> +	else if (size == 2)
>> +		val = readw(pcie->reg_base + where);
>> +	else if (size == 1)
>> +		val = readb(pcie->reg_base + where);
>> +
>> +	return val;
>> +}
>> +
>> +u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
>> +{
>> +	return pci_host_bridge_find_capability(pcie, cdns_pcie_read_cfg, cap);
>> +}
>> +
>> +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
>> +{
>> +	return pci_host_bridge_find_ext_capability(pcie, cdns_pcie_read_cfg, cap);
>> +}
> 
> I'm really wondering why the read config function is provided directly as
> an argument. Shouldn't struct pci_host_bridge have some ops that can read
> config so wouldn't it make much more sense to pass it and use the func
> from there? There seems to ops in pci_host_bridge that has read(), does
> that work? If not, why?
> 

No effect. Because we need to get the offset of the capability before 
PCIe enumerates the device. I originally added a separate find 
capability related function for CDNS in the following patch. It's also 
copied directly from DWC. Mani felt there was too much duplicate code 
and also suggested passing a callback function that could manipulate the 
registers of the root port of DWC or CDNS.

https://patchwork.kernel.org/project/linux-pci/patch/20250308133903.322216-1-18255117159@163.com/

The original function is in the following file:
drivers/pci/controller/dwc/pcie-designware.c
u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)

CDNS has the same need to find the offset of the capability.

>> +
>>   void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie)
>>   {
>>   	u32 delay = 0x3;
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
>> index f5eeff834ec1..6f4981fccb94 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>> @@ -557,6 +557,9 @@ static inline int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
>>   }
>>   #endif
>>   
>> +u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap);
>> +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap);
>> +
>>   void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);
>>   
>>   void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr, u8 fn,
>>
> 

Best regards,
Hans