| 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: <20250129152621.GA460594@bhelgaas>
Date: Wed, 29 Jan 2025 09:26:21 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Manikandan Karunakaran Pillai <mpillai@...ence.com>
Cc: "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-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: Re: [PATCH] Enhance PCIe cadence controller driver for supporting
HPA architecture
Take a look at the additions of previous drivers and follow the style.
For example:
2bdd5238e756 ("PCI: mt7621: Add MediaTek MT7621 PCIe host controller driver")
0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
da36024a4e83 ("PCI: visconti: Add Toshiba Visconti PCIe host controller driver")
f3e25911a430 ("PCI: j721e: Add TI J721E PCIe driver")
After skimming the patch, it looks like this doesn't actually add a
new driver, but adds support to an existing driver for some kind of
different surrounding platform architecture.
On Wed, Jan 29, 2025 at 07:24:04AM +0000, Manikandan Karunakaran Pillai wrote:
> This patch enhances the Cadence PCIe controller driver to support HPA
> architecture. The Kconfig is added for PCIE_CADENCE_HPA config, which
> needs to be selected when HPA compatible PCIe controller is supported.
> The support for Legacy PCIe controller does not change.
Use imperative mood ("Enhance" instead of "This patch enhances ...").
Expand "HPA".
> drivers/pci/controller/cadence/Kconfig | 8 +
> .../pci/controller/cadence/pcie-cadence-ep.c | 12 +-
> .../controller/cadence/pcie-cadence-host.c | 44 ++-
> .../pci/controller/cadence/pcie-cadence-hpa.h | 260 ++++++++++++++++++
> .../controller/cadence/pcie-cadence-legacy.h | 243 ++++++++++++++++
> drivers/pci/controller/cadence/pcie-cadence.c | 42 ++-
> drivers/pci/controller/cadence/pcie-cadence.h | 230 +---------------
This looks like it should be 5+ separate patches to make this
reviewable. Each patch should do only *one* thing. Obviously
everything must build and work correctly after each patch is added.
> +++ b/drivers/pci/controller/cadence/Kconfig
> @@ -6,6 +6,14 @@ menu "Cadence-based PCIe controllers"
> config PCIE_CADENCE
> bool
>
> +config PCIE_CADENCE_HPA
> + bool "PCIE controller HPA architecture"
s/PCIE/PCIe/ to match other entries.
Change the menu item to match surrounding entries (include vendor,
host/endpoint, etc).
But I don't think this is a *new* driver; it looks like it might be
optional functionality for the PCIE_CADENCE_PLAT and/or PCI_J721E
drivers? Maybe it needs to depend on PCIE_CADENCE?
> + help
> + Say Y here if you want to support Cadence PCIe Platform controller
> + on HPA architecture
> + The option should be deselected if the Cadence PCIe controller
> + is of legacy architecture
"HPA" must be expanded here too.
Omit the "deselect part". A user has no way to identify what "legacy
architecture" means here.
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -121,7 +121,11 @@ 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);
> +#ifdef CONFIG_PCIE_CADENCE_HPA
Nope. I don't want #ifdefs like this littered through the cadence
generic code. The driver should be able to support both HPA and
non-HPA in the same kernel image. You should be able to decide at
run-time, not at build-time.
> + b = (bar < BAR_3) ? bar : bar - BAR_3;
> +#else
> b = (bar < BAR_4) ? bar : bar - BAR_4;
> +#endif
> +++ b/drivers/pci/controller/cadence/pcie-cadence-hpa.h
> @@ -0,0 +1,260 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +// Copyright (c) 2017 Cadence
> +// Cadence PCIe Gen5(HPA) controller driver defines
Space before "(". Expand HPA.
> + * This file contains the updates/changes for PCIE Gen5 Controller
s/PCIE/PCIe/ everywhere in commit logs and comments.
> +#define CDNS_PCIE_IP_REG_BANK (0x01000000)
> +#define CDNS_PCIE_IP_CFG_CTRL_REG_BANK (0x01003C00)
> +#define CDNS_PCIE_IP_AXI_MASTER_COMMON (0x02020000)
No parens needed for bare numbers.
> +/* Vendor ID Register */
> +#define CDNS_PCIE_LM_ID (CDNS_PCIE_IP_REG_BANK + 0x1420)
> +#define CDNS_PCIE_LM_ID_VENDOR_MASK GENMASK(15, 0)
> +#define CDNS_PCIE_LM_ID_VENDOR_SHIFT 0
Omit _SHIFT definitions and use GENMASK()/FIELD_PREP()/FIELD_GET()
instead.
But it looks like this is mainly a move from one file to another. A
move like that should be strictly a move and shouldn't change the
content at the same time.
The move should be in its own patch that does nothing other than the
move, so you can ignore some of these comments for now.
> +#define CDNS_PCIE_LM_EP_FUNC_CFG (CDNS_PCIE_IP_REG_BANK + 0x02c0)
Pick either upper-case or lower-case for hex numbers and stick to it.
> +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR0_ENABLE BIT(0)
> +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR0_IO BIT(1)
> +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR0_MEM (0)
> +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR0_32BITS (0)
> +#define CDNS_PCIE_LM_RC_BAR_CFG_BAR0_PREFETCH_MEM_DISABLE (0)
These are unused. Don't add #defines that are not used.
When you do add them, indent the BIT() parts to they line up.
> +++ b/drivers/pci/controller/cadence/pcie-cadence.c
> ...
> /*
> * Whatever Bit [23] is set or not inside DESC0 register of the outbound
> * PCIe descriptor, the PCI function number must be set into
> * Bits [26:24] of DESC0 anyway.
> + * for HPA, Bit [26] is set or not inside CTRL0 register of the outbound
> + * PCI descriptor, the PCI function num must be set into Bits [31:24]
> + * of DESC1 anyway.
Start sentences with a capital letter.
Add blank lines between paragraphs.
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -10,213 +10,11 @@
> #include <linux/pci.h>
> #include <linux/pci-epf.h>
> #include <linux/phy/phy.h>
> -
> -/* Parameters for the waiting for link up routine */
> -#define LINK_WAIT_MAX_RETRIES 10
> -#define LINK_WAIT_USLEEP_MIN 90000
> -#define LINK_WAIT_USLEEP_MAX 100000
This looks like a move of a lot of things out of pcie-cadence.h to
somewhere else. A move like this should be its own patch with its own
commit log.
It looks suspect because this is a generic header used by several
drivers.
Bjorn
Powered by blists - more mailing lists