linux-kernel - [PATCH 4/7] PCI: cadence: Add support for PCIe Endpoint HPA controllers

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] [day] [month] [year] [list]

Message-ID:
 <CH2PPF4D26F8E1CA2D74FDA7DAD29C33518A2B62@CH2PPF4D26F8E1C.namprd07.prod.outlook.com>
Date: Fri, 11 Apr 2025 04:23:09 +0000
From: Manikandan Karunakaran Pillai <mpillai@...ence.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "lpieralisi@...nel.org"
	<lpieralisi@...nel.org>,
        "kw@...ux.com" <kw@...ux.com>,
        "manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>,
        "robh@...nel.org" <robh@...nel.org>,
        "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: [PATCH 4/7] PCI: cadence: Add support for PCIe Endpoint HPA
 controllers

>
>On Thu, Mar 27, 2025 at 11:40:36AM +0000, Manikandan Karunakaran Pillai
>wrote:
>> Add support for the second generation(HPA) Cadence PCIe endpoint
>> controller by adding the required functions based on the HPA registers
>> and register bit definitions
>
>Add period.
>

Ok

>> @@ -93,7 +93,10 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc,
>u8 fn, u8 vfn,
>>  	 * for 64bit values.
>>  	 */
>>  	sz = 1ULL << fls64(sz - 1);
>> -	aperture = ilog2(sz) - 7; /* 128B -> 0, 256B -> 1, 512B -> 2, ... */
>> +	/*
>> +	 * 128B -> 0, 256B -> 1, 512B -> 2, ...
>> +	 */
>> +	aperture = ilog2(sz) - 7;
>
>Unclear exactly how this is related to HPA and whether it affects
>non-HPA.
>

Removed in the next patch series


>> @@ -121,7 +124,7 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc,
>u8 fn, u8 vfn,
>>  		reg = CDNS_PCIE_LM_EP_VFUNC_BAR_CFG(bar, fn);
>>  	else
>>  		reg = CDNS_PCIE_LM_EP_FUNC_BAR_CFG(bar, fn);
>> -	b = (bar < BAR_4) ? bar : bar - BAR_4;
>> +	b = (bar < BAR_3) ? bar : bar - BAR_3;
>
>Unclear what's going on here because this doesn't look specific to
>HPA.  Should this be a separate patch that fixes an existing defect?
>
>>  	if (vfn == 0 || vfn == 1) {
>>  		cfg = cdns_pcie_readl(pcie, reg);
>> @@ -158,7 +161,7 @@ static void cdns_pcie_ep_clear_bar(struct pci_epc
>*epc, u8 fn, u8 vfn,
>>  		reg = CDNS_PCIE_LM_EP_VFUNC_BAR_CFG(bar, fn);
>>  	else
>>  		reg = CDNS_PCIE_LM_EP_FUNC_BAR_CFG(bar, fn);
>> -	b = (bar < BAR_4) ? bar : bar - BAR_4;
>> +	b = (bar < BAR_3) ? bar : bar - BAR_3;
>
>And here.

Removed in the next patch series and will be submitted as patch for bug fix

>
>> @@ -569,7 +572,11 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
>>  	 * BIT(0) is hardwired to 1, hence function 0 is always enabled
>>  	 * and can't be disabled anyway.
>>  	 */
>> -	cdns_pcie_writel(pcie, CDNS_PCIE_LM_EP_FUNC_CFG, epc-
>>function_num_map);
>> +	if (pcie->is_hpa)
>> +		cdns_pcie_hpa_writel(pcie, REG_BANK_IP_REG,
>> +				     CDNS_PCIE_HPA_LM_EP_FUNC_CFG, epc-
>>function_num_map);
>> +	else
>> +		cdns_pcie_writel(pcie, CDNS_PCIE_LM_EP_FUNC_CFG, epc-
>>function_num_map);
>
>Sprinkling tests of "is_hpa" around is not very extensible.  When the
>next generation after HPA shows up, then it gets really messy.
>Sometimes generation-specific function pointers can make this simpler.
>

The "is_hpa" are used at very few places in the code where putting a generation specific
functions might be an overkill

>> +static int cdns_pcie_hpa_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
>> +				    struct pci_epf_bar *epf_bar)
>> +{
>> +	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>> +	struct cdns_pcie_epf *epf = &ep->epf[fn];
>> +	struct cdns_pcie *pcie = &ep->pcie;
>> +	dma_addr_t bar_phys = epf_bar->phys_addr;
>> +	enum pci_barno bar = epf_bar->barno;
>> +	int flags = epf_bar->flags;
>> +	u32 addr0, addr1, reg, cfg, b, aperture, ctrl;
>> +	u64 sz;
>> +
>> +	/*
>> +	 * BAR size is 2^(aperture + 7)
>> +	 */
>> +	sz = max_t(size_t, epf_bar->size, CDNS_PCIE_EP_MIN_APERTURE);
>> +	/*
>
>Add blank line between code and comment.

Ok

>
>> +	 * roundup_pow_of_two() returns an unsigned long, which is not
>suited
>> +	 * for 64bit values.
>> +	 */
>> +	sz = 1ULL << fls64(sz - 1);
>> +	/*
>
>Again.  Check for other places in this series.
>
>> +	 * 128B -> 0, 256B -> 1, 512B -> 2, ...
>> +	 */
>> +	aperture = ilog2(sz) - 7;