linux-kernel - Re: [PATCH 2/2] PCI: amd-mdb: Add AMD MDB Root Port driver

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: <20241129202202.GA2771092@bhelgaas>
Date: Fri, 29 Nov 2024 14:22:02 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Thippeswamy Havalige <thippeswamy.havalige@....com>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kw@...ux.com,
	manivannan.sadhasivam@...aro.org, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, linux-pci@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	jingoohan1@...il.com, michal.simek@....com,
	bharat.kumar.gogada@....com
Subject: Re: [PATCH 2/2] PCI: amd-mdb: Add AMD MDB Root Port driver

On Wed, Nov 27, 2024 at 05:28:04PM +0530, Thippeswamy Havalige wrote:
> Add support for AMD MDB(Multimedia DMA Bridge) IP core as Root Port.
> 
> The Versal2 devices include MDB Module. The integrated block for MDB along
> with the integrated bridge can function as PCIe Root Port controller at
> Gen5 speed.

What speed is Gen5?  Please include the numeric speed so we don't have
to Google it.

> Bridge error and legacy interrupts in Versal2 MDB are handled using Versal2
> MDB specific interrupt line.
> 
> Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@....com>
> ---
>  drivers/pci/controller/dwc/Kconfig        |  10 +
>  drivers/pci/controller/dwc/Makefile       |   1 +
>  drivers/pci/controller/dwc/pcie-amd-mdb.c | 455 ++++++++++++++++++++++
>  3 files changed, 466 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-amd-mdb.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index b6d6778b0698..e7ddab8da2c4 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -14,6 +14,16 @@ config PCIE_DW_EP
>  	bool
>  	select PCIE_DW
>  
> +config PCIE_AMD_MDB
> +	bool "AMD PCIe controller (host mode)"
> +	depends on OF || COMPILE_TEST
> +	depends on PCI && PCI_MSI
> +	select PCIE_DW_HOST
> +	help
> +	  Say Y here to enable PCIe controller support on AMD SoCs. The
> +	  PCIe controller is based on DesignWare Hardware and uses AMD
> +	  hardware wrappers.

Alphabetize by vendor name.  I suppose "Advanced" *would* sort before
"Amazon", but since "AMD" isn't spelled out, I think we have to sort
by the initialism and put "Ama" before "AMD".

>  config PCIE_AL
>  	bool "Amazon Annapurna Labs PCIe controller"
>  	depends on OF && (ARM64 || COMPILE_TEST)

> +static void amd_mdb_mask_leg_irq(struct irq_data *data)

s/_leg_/_intx_/

> +{
> +	struct dw_pcie_rp *port = irq_data_get_irq_chip_data(data);
> +	struct amd_mdb_pcie *pcie;
> +	unsigned long flags;
> +	u32 mask, val;
> +
> +	pcie = get_mdb_pcie(port);

Here and elsewhere, this could be done in the automatic variable list
above since this is non-interesting setup.

> +static void amd_mdb_unmask_leg_irq(struct irq_data *data)

Ditto.

> +static struct irq_chip amd_mdb_leg_irq_chip = {

Ditto.

> +static int amd_mdb_pcie_rp_intx_map(struct irq_domain *domain,
> +				    unsigned int irq, irq_hw_number_t hwirq)

"_rp_" in name unnecessary.

> +static irqreturn_t amd_mdb_pcie_rp_intr_handler(int irq, void *dev_id)
> +{
> +	struct dw_pcie_rp *port = dev_id;
> +	struct amd_mdb_pcie *pcie;
> +	struct device *dev;
> +	struct irq_data *d;
> +
> +	pcie = get_mdb_pcie(port);
> +	dev = pcie->pci.dev;
> +
> +	d = irq_domain_get_irq_data(pcie->mdb_domain, irq);
> +	if (intr_cause[d->hwirq].str)
> +		dev_warn(dev, "%s\n", intr_cause[d->hwirq].str);
> +	else
> +		dev_warn(dev, "Unknown IRQ %ld\n", d->hwirq);
> +
> +	return IRQ_HANDLED;

I see that some of these messages are "Correctable/Non-Fatal/Fatal
error message"; I assume this Root Port doesn't have an AER
Capability, and this interrupt is the "System Error" controlled by the
Root Control Error Enable bits in the PCIe Capability?  (See PCIe
r6.0, sec 6.2.6)

Is there any way to hook this into the AER handling so we can do
something about it, since the devices *below* the Root Port may
support AER and may have useful information logged?

Since this is DWC-based, I suppose these are general questions that
apply to all the similar drivers.

> +static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie,
> +				 struct platform_device *pdev)
> +{
> +	struct dw_pcie *pci = &pcie->pci;
> +	struct dw_pcie_rp *pp = &pci->pp;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	pp->ops = &amd_mdb_pcie_host_ops;

This is dw-related initialization; move it down just before the first
use at dw_pcie_host_init().

> +	pcie->mdb_base = devm_platform_ioremap_resource_byname(pdev, "mdb_pcie_slcr");
> +	if (IS_ERR(pcie->mdb_base))
> +		return PTR_ERR(pcie->mdb_base);
> +
> +	ret = amd_mdb_pcie_rp_init_irq_domain(pcie, pdev);

Other drivers use "*_pcie_init_irq_domain" (without "rp").  It's
helpful to use similar names so it's easier to compare
implementations.

Since amd_mdb_pcie_free_irq_domains() cleans this up, I think both
should end with "domains" (with an "s") so they match.

> +	if (ret)
> +		return ret;
> +
> +	amd_mdb_pcie_rp_init_port(pcie, pdev);

Other drivers use "*_pcie_init_port" (without "rp").

> +	ret = amd_mdb_setup_irq(pcie, pdev);
> +	if (ret) {
> +		dev_err(dev, "Failed to set up interrupts\n");
> +		goto out;
> +	}
> +
> +	ret = dw_pcie_host_init(pp);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize host\n");
> +		goto out;
> +	}
> +
> +	return 0;
> +
> +out:
> +	amd_mdb_pcie_free_irq_domains(pcie);
> +	return ret;
> +}

Bjorn