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: <3d9b2fa9-98bf-4f47-aa76-640a4f82cb2f@163.com>
Date: Tue, 25 Mar 2025 10:59:40 +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 23:02, Ilpo Järvinen wrote:
>>>>    +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() ?
> 
> As is, it seems two functions are added for the same thing for the case
> with size == 4 with different names which feels duplication. One could add
> cdns_pcie_readw() and cdns_pcie_readb() too but perhaps cdns_pcie_readl()
> should just call this new function instead?

Hi Ilpo,

Redefine a function with reference to DWC?

u32 dw_pcie_read_dbi(struct dw_pcie *pci, u32 reg, size_t size)
   dw_pcie_read(pci->dbi_base + reg, size, &val);
     dw_pcie_read

int dw_pcie_read(void __iomem *addr, int size, u32 *val)
{
	if (!IS_ALIGNED((uintptr_t)addr, size)) {
		*val = 0;
		return PCIBIOS_BAD_REGISTER_NUMBER;
	}

	if (size == 4) {
		*val = readl(addr);
	} else if (size == 2) {
		*val = readw(addr);
	} else if (size == 1) {
		*val = readb(addr);
	} else {
		*val = 0;
		return PCIBIOS_BAD_REGISTER_NUMBER;
	}

	return PCIBIOS_SUCCESSFUL;
}
EXPORT_SYMBOL_GPL(dw_pcie_read);

> 
>>>> +	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.
> 
> I'm not sure what you meant?
> 
>> Because we need to get the offset of the capability before PCIe
>> enumerates the device.
> 
> Is this to say it is needed before the struct pci_host_bridge is created?
> 
>> 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.
> 
> I very much like the direction this patchset is moving (moving shared
> part of controllers code to core), I just feel this doesn't go far enough
> when it's passing function pointer to the read function.
> 
> I admit I've never written a controller driver so perhaps there's
> something detail I lack knowledge of but I'd want to understand why
> struct pci_ops (which exists both in pci_host_bridge and pci_bus) cannot
> be used?
> 


I don't know if the following code can make it clear to you.

static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
	.host_init	= qcom_pcie_host_init,
                   pcie->cfg->ops->post_init(pcie);
                     qcom_pcie_post_init_2_3_3
                       dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
};

int dw_pcie_host_init(struct dw_pcie_rp *pp)
   bridge = devm_pci_alloc_host_bridge(dev, 0);
   if (pp->ops->host_init)
     pp->ops = &qcom_pcie_dw_ops;  // qcom here needs to find capability

   pci_host_probe(bridge); // pcie enumerate flow
     pci_scan_root_bus_bridge(bridge);
       pci_register_host_bridge(bridge);
         bus->ops = bridge->ops;   // Only pci bus ops can be used


Best regards,
Hans