| 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
| ||
|
Message-ID: <2aanerkp7c4qd4mukz6oaxafe22assjyah2kdbdmyuich5hzha@k6hlzvarixxo>
Date: Sun, 2 Nov 2025 11:10:02 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Manikandan Karunakaran Pillai <mpillai@...ence.com>
Cc: "hans.zhang@...tech.com" <hans.zhang@...tech.com>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>, "helgaas@...nel.org" <helgaas@...nel.org>,
"lpieralisi@...nel.org" <lpieralisi@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
"robh@...nel.org" <robh@...nel.org>, "kwilczynski@...nel.org" <kwilczynski@...nel.org>,
"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
"fugang.duan@...tech.com" <fugang.duan@...tech.com>, "guoyin.chen@...tech.com" <guoyin.chen@...tech.com>,
"peter.chen@...tech.com" <peter.chen@...tech.com>,
"cix-kernel-upstream@...tech.com" <cix-kernel-upstream@...tech.com>, "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v10 04/10] PCI: cadence: Add support for High Perf
Architecture (HPA) controller
On Sun, Nov 02, 2025 at 04:15:03AM +0000, Manikandan Karunakaran Pillai wrote:
> Hi Mani,
>
> Pls find my comments inline
>
> Regards
>
> >> +
> >> + /* Clear AXI link-down status */
> >> + regval = cdns_pcie_hpa_readl(pcie, REG_BANK_AXI_SLAVE,
> >CDNS_PCIE_HPA_AT_LINKDOWN);
> >> + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> >CDNS_PCIE_HPA_AT_LINKDOWN,
> >> + (regval & ~GENMASK(0, 0)));
> >> +
> >> + /* Update Output registers for AXI region 0 */
> >> + addr0 = CDNS_PCIE_HPA_AT_OB_REGION_PCI_ADDR0_NBITS(12) |
> >> + CDNS_PCIE_HPA_AT_OB_REGION_PCI_ADDR0_DEVFN(devfn) |
> >> + CDNS_PCIE_HPA_AT_OB_REGION_PCI_ADDR0_BUS(busn);
> >> + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> >> + CDNS_PCIE_HPA_AT_OB_REGION_PCI_ADDR0(0),
> >addr0);
> >> +
> >> + desc1 = cdns_pcie_hpa_readl(pcie, REG_BANK_AXI_SLAVE,
> >> +
> >CDNS_PCIE_HPA_AT_OB_REGION_DESC1(0));
> >> + desc1 &= ~CDNS_PCIE_HPA_AT_OB_REGION_DESC1_DEVFN_MASK;
> >> + desc1 |= CDNS_PCIE_HPA_AT_OB_REGION_DESC1_DEVFN(0);
> >> + ctrl0 = CDNS_PCIE_HPA_AT_OB_REGION_CTRL0_SUPPLY_BUS |
> >> + CDNS_PCIE_HPA_AT_OB_REGION_CTRL0_SUPPLY_DEV_FN;
> >> +
> >> + if (busn == bridge->busnr + 1)
> >> + desc0 =
> >CDNS_PCIE_HPA_AT_OB_REGION_DESC0_TYPE_CONF_TYPE0;
> >> + else
> >> + desc0 =
> >CDNS_PCIE_HPA_AT_OB_REGION_DESC0_TYPE_CONF_TYPE1;
> >> +
> >> + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> >> + CDNS_PCIE_HPA_AT_OB_REGION_DESC0(0), desc0);
> >> + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> >> + CDNS_PCIE_HPA_AT_OB_REGION_DESC1(0), desc1);
> >> + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> >> + CDNS_PCIE_HPA_AT_OB_REGION_CTRL0(0), ctrl0);
> >> +
> >> + return rc->cfg_base + (where & 0xfff);
> >> +}
> >> +
> >> +static int cdns_pcie_hpa_host_wait_for_link(struct cdns_pcie *pcie)
> >> +{
> >> + struct device *dev = pcie->dev;
> >> + struct cdns_pcie_rc *rc;
> >> + int retries, ret;
> >> +
> >> + rc = container_of(pcie, struct cdns_pcie_rc, pcie);
> >> +
> >> + /* Check if the link is up or not */
> >> + for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> >> + if (cdns_pcie_hpa_link_up(pcie)) {
> >> + dev_info(dev, "Link up\n");
> >> + return 0;
> >> + }
> >> + usleep_range(LINK_WAIT_USLEEP_MIN,
> >LINK_WAIT_USLEEP_MAX);
> >> + }
> >> + if (rc->quirk_retrain_flag)
> >> + ret = cdns_pcie_retrain(pcie);
> >> + return ret;
> >
> >If 'quirk_retrain_flag' was not set, you are 'ret' will be uninitialized.
>
> Ok, Patch set 11 will have the fix.
>
> >
> >> +}
> >> +
> >> +static struct pci_ops cdns_pcie_hpa_host_ops = {
> >> + .map_bus = cdns_pci_hpa_map_bus,
> >> + .read = pci_generic_config_read,
> >> + .write = pci_generic_config_write,
> >> +};
> >> +
> >> +static void cdns_pcie_hpa_host_enable_ptm_response(struct cdns_pcie
> >*pcie)
> >> +{
> >> + u32 val;
> >> +
> >> + val = cdns_pcie_hpa_readl(pcie, REG_BANK_IP_REG,
> >CDNS_PCIE_HPA_LM_PTM_CTRL);
> >> + cdns_pcie_hpa_writel(pcie, REG_BANK_IP_REG,
> >CDNS_PCIE_HPA_LM_PTM_CTRL,
> >> + val | CDNS_PCIE_HPA_LM_PTM_CTRL_PTMRSEN);
> >> +}
> >> +
> >> +static int cdns_pcie_hpa_host_bar_ib_config(struct cdns_pcie_rc *rc,
> >> + enum cdns_pcie_rp_bar bar,
> >> + u64 cpu_addr, u64 size,
> >> + unsigned long flags)
> >> +{
> >> + struct cdns_pcie *pcie = &rc->pcie;
> >> + u32 addr0, addr1, aperture, value;
> >> +
> >> + if (!rc->avail_ib_bar[bar])
> >> + return -ENODEV;
> >> +
> >> + rc->avail_ib_bar[bar] = false;
> >> +
> >> + aperture = ilog2(size);
> >> + if (bar == RP_NO_BAR) {
> >> + addr0 =
> >CDNS_PCIE_HPA_AT_IB_RP_BAR_ADDR0_NBITS(aperture) |
> >> + (lower_32_bits(cpu_addr) & GENMASK(31, 8));
> >> + addr1 = upper_32_bits(cpu_addr);
> >> + } else {
> >> + addr0 = lower_32_bits(cpu_addr);
> >> + addr1 = upper_32_bits(cpu_addr);
> >> + }
> >> + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_MASTER,
> >> + CDNS_PCIE_HPA_AT_IB_RP_BAR_ADDR0(bar),
> >addr0);
> >> + cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_MASTER,
> >> + CDNS_PCIE_HPA_AT_IB_RP_BAR_ADDR1(bar),
> >addr1);
> >> +
> >> + if (bar == RP_NO_BAR)
> >> + bar = (enum cdns_pcie_rp_bar)BAR_0;
> >> +
> >> + value = cdns_pcie_hpa_readl(pcie, REG_BANK_IP_CFG_CTRL_REG,
> >CDNS_PCIE_HPA_LM_RC_BAR_CFG);
> >> + value &= ~(HPA_LM_RC_BAR_CFG_CTRL_MEM_64BITS(bar) |
> >> + HPA_LM_RC_BAR_CFG_CTRL_PREF_MEM_64BITS(bar) |
> >> + HPA_LM_RC_BAR_CFG_CTRL_MEM_32BITS(bar) |
> >> + HPA_LM_RC_BAR_CFG_CTRL_PREF_MEM_32BITS(bar) |
> >> + HPA_LM_RC_BAR_CFG_APERTURE(bar,
> >bar_aperture_mask[bar] + 7));
> >> + if (size + cpu_addr >= SZ_4G) {
> >> + value |= HPA_LM_RC_BAR_CFG_CTRL_MEM_64BITS(bar);
> >> + if ((flags & IORESOURCE_PREFETCH))
> >> + value |=
> >HPA_LM_RC_BAR_CFG_CTRL_PREF_MEM_64BITS(bar);
> >> + } else {
> >> + value |= HPA_LM_RC_BAR_CFG_CTRL_MEM_32BITS(bar);
> >> + if ((flags & IORESOURCE_PREFETCH))
> >> + value |=
> >HPA_LM_RC_BAR_CFG_CTRL_PREF_MEM_32BITS(bar);
> >> + }
> >> +
> >> + value |= HPA_LM_RC_BAR_CFG_APERTURE(bar, aperture);
> >> + cdns_pcie_hpa_writel(pcie, REG_BANK_IP_CFG_CTRL_REG,
> >CDNS_PCIE_HPA_LM_RC_BAR_CFG, value);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int cdns_pcie_hpa_host_bar_config(struct cdns_pcie_rc *rc,
> >> + struct resource_entry *entry)
> >
> >This and other functions are almost same as in 'pcie-cadence-host'. Why don't
> >you reuse them in a common library?
>
> The function cdns_pcie_hpa_host_bar_config() calls functions cdns_pcie_hpa_host_bar_ib_config()
> which is not common. All functions that are common between the two architecture are moved to the
> common library file based on earlier comments.
>
This is not a good reason to duplicate the whole function. You could just make
the common function accept the callback ib_config() and pass either
cdns_pcie_host_bar_ib_config() or cdns_pcie_hpa_host_bar_ib_config().
This pattern could be done for other functions as well. Please audit all of them
and move them to common library. Currently, I could see a lot of duplications
that could be avoided.
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists