linux-kernel - Re: [PATCH v10 04/10] PCI: cadence: Add support for High Perf Architecture (HPA) controller

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: <jmxdju5aon3biunji6rplxmapb6j7ozet37olxtcknznqekw7p@a3bj7glbxc4n>
Date: Sun, 2 Nov 2025 20:38:15 +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 05:51:05AM +0000, Manikandan Karunakaran Pillai wrote:
> Hi Mani,
> 
> Pls find my comments below.
> 
> >> >> +			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.
> 
> The very first patch  for this feature included an ops struct  which was registered (very similar to a callback). Are are asking me to again implement the same design which was earlier rejected ?
> 

You didn't provide any link to the discussion, so how can I decide without
looking into it?

> Secondly except the names of the functions, the registers and their offset written and the sequence also changes for the implementations.
> 

I don't think so. From a quick look, at least cdns_pcie_hpa_host_bar_config(),
cdns_pcie_hpa_host_map_dma_ranges() are mostly similar. You can keep the
functions having different register configurations, but should try to move
others.

- Mani

-- 
மணிவண்ணன் சதாசிவம்