linux-kernel - Re: [PATCH] pci: cdns : Function to read controller architecture

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: <20250131183835.GA688678@bhelgaas>
Date: Fri, 31 Jan 2025 12:38:35 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Manikandan Karunakaran Pillai <mpillai@...ence.com>
Cc: "lpieralisi@...nel.org" <lpieralisi@...nel.org>,
	"manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>,
	"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
	"robh@...nel.org" <robh@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pci: cdns : Function to read controller architecture

Look at previous subject lines for changes to these files and follow
the pattern.

On Fri, Jan 31, 2025 at 11:58:07AM +0000, Manikandan Karunakaran Pillai wrote:
> Add support for getting the architecture for Cadence PCIe controllers
> Store the architecture type in controller structure.

This needs to be part of a series that uses pcie->is_hpa for
something.  This patch all by itself isn't useful for anything.

Please post the resulting series with a cover letter and the patches
as responses to it:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/5.Posting.rst?id=v6.13#n333

You can look at previous postings to see the style, e.g.,
https://lore.kernel.org/linux-pci/20250115074301.3514927-1-pandoh@google.com/T/#t

> +static void cdns_pcie_ctlr_set_arch(struct cdns_pcie *pcie)
> +{
> +	/* Read register at offset 0xE4 of the config space
> +	 * The value for architecture is in the lower 4 bits
> +	 * Legacy-b'0010 and b'1111 for HPA-high performance architecture
> +	 */

Don't include the hex register offset in the comment.  That's what
CDNS_PCIE_CTRL_ARCH is for.  It doesn't need the bit values either.

Use the conventional comment style:

  /*
   * Text ...
   */

> +	u32 arch, reg;
> +
> +	reg = cdns_pcie_readl(pcie, CDNS_PCIE_CTRL_ARCH);
> +	arch = FIELD_GET(CDNS_PCIE_CTRL_ARCH_MASK, reg);

Thanks for using GENMASK() and FIELD_GET().

> +	if (arch == CDNS_PCIE_CTRL_HPA) {
> +		pcie->is_hpa = true;
> +	} else {
> +		pcie->is_hpa = false;
> +	}
> +}

> +/*
> + * Read completion time out reset value to decode controller architecture
> + */
> +#define CDNS_PCIE_CTRL_ARCH		0xE4

Is this another name for the PCI_EXP_DEVCTL2 in the PCIe Capability?
Or maybe PCI_EXP_DEVCAP2?  If so, use those existing #defines and the
related masks (if it's DEVCAP2, you'd probably have to add a new one
for the Completion Timeout Ranges Supported field).

There's something similar in cdns_pcie_retrain(), where
CDNS_PCIE_RP_CAP_OFFSET is apparently the config space offset of the
PCIe Capability.

> +#define CDNS_PCIE_CTRL_ARCH_MASK	GENMASK(3, 0)
> +#define CDNS_PCIE_CTRL_HPA		0xF