linux-kernel - Re: [PATCH v13 3/3] 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: <20250219181952.GA225098@bhelgaas>
Date: Wed, 19 Feb 2025 12:19:52 -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,
	michal.simek@....com, bharat.kumar.gogada@....com,
	jingoohan1@...il.com
Subject: Re: [PATCH v13 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

On Wed, Feb 19, 2025 at 03:51:24PM +0530, Thippeswamy Havalige wrote:
> Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root Port.

> +#define AMD_MDB_TLP_IR_STATUS_MISC		0x4C0
> +#define AMD_MDB_TLP_IR_MASK_MISC		0x4C4
> +#define AMD_MDB_TLP_IR_ENABLE_MISC		0x4C8
> +#define AMD_MDB_TLP_IR_DISABLE_MISC		0x4CC
> +
> +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)
> +
> +#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)	BIT((x) * 2)

> +#define AMD_MDB_PCIE_INTX_BIT(x) (1U << (2 * (x) + AMD_MDB_PCIE_INTR_INTX))

I don't think you need both AMD_MDB_PCIE_INTR_INTX_ASSERT() and
AMD_MDB_PCIE_INTX_BIT().

AMD_MDB_PCIE_INTX_BIT() is essentially the same as
FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, AMD_MDB_PCIE_INTR_INTX_ASSERT).

I would just do this:

  #define AMD_MDB_PCIE_INTR_INTX(x)     BIT((x) * 2)

and in amd_mdb_intx_irq_mask() and amd_mdb_intx_irq_unmask(), do this:

  val = FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK,
                   AMD_MDB_PCIE_INTR_INTX(data->hwirq));

Then all the users do FIELD_PREP() or FIELD_GET() at the point of use.

It's a little confusing for dw_pcie_rp_intx_flow() to do the
FIELD_GET() there, but amd_mdb_intx_irq_mask() and
amd_mdb_intx_irq_unmask() have the equivalent of FIELD_PREP() hidden
inside AMD_MDB_PCIE_INTX_BIT().

> +static void amd_mdb_intx_irq_mask(struct irq_data *data)
> +{
> +	struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(data);
> +	struct dw_pcie *pci = &pcie->pci;
> +	struct dw_pcie_rp *port = &pci->pp;
> +	unsigned long flags;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&port->lock, flags);
> +	val = AMD_MDB_PCIE_INTX_BIT(data->hwirq);
> +	pcie_write(pcie, val, AMD_MDB_TLP_IR_DISABLE_MISC);

I guess AMD_MDB_TLP_IR_DISABLE_MISC and AMD_MDB_TLP_IR_ENABLE_MISC are
set up so writing zero bits does nothing, and writing a one bit only
masks or unmasks that single interrupt?  It's somewhat unusual, so a
comment to that effect might be useful.

> +	raw_spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +static void amd_mdb_intx_irq_unmask(struct irq_data *data)
> +{
> +	struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(data);
> +	struct dw_pcie *pci = &pcie->pci;
> +	struct dw_pcie_rp *port = &pci->pp;
> +	unsigned long flags;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&port->lock, flags);
> +	val = AMD_MDB_PCIE_INTX_BIT(data->hwirq);
> +	pcie_write(pcie, val, AMD_MDB_TLP_IR_ENABLE_MISC);
> +	raw_spin_unlock_irqrestore(&port->lock, flags);
> +}

> +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args)
> +{
> +	struct amd_mdb_pcie *pcie = args;
> +	unsigned long val;
> +	int i;
> +
> +	val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> +			val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));

The "val = pcie_read()" looks like a useless assignment to val.  I
would do something like this:

  val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
  intx_status = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK, val);

  for (i = 0; i < PCI_NUM_INTX; i++) {
    if (intx_status & AMD_MDB_PCIE_INTR_INTX(i))
      generic_handle_domain_irq(pcie->intx_domain, i);
  }

> +	for (i = 0; i < PCI_NUM_INTX; i++) {
> +		if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
> +			generic_handle_domain_irq(pcie->intx_domain, i);
> +	}
> +
> +	return IRQ_HANDLED;
> +}