linux-kernel - RE: [PATCH v8 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] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <SN7PR12MB720115611148E80D555A1A1E8BF72@SN7PR12MB7201.namprd12.prod.outlook.com>
Date: Wed, 5 Feb 2025 11:53:53 +0000
From: "Havalige, Thippeswamy" <thippeswamy.havalige@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "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>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"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>,
	"jingoohan1@...il.com" <jingoohan1@...il.com>, "Simek, Michal"
	<michal.simek@....com>, "Gogada, Bharat Kumar" <bharat.kumar.gogada@....com>
Subject: RE: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

Hi Bjorn,

> -----Original Message-----
> From: Havalige, Thippeswamy
> Sent: Wednesday, February 5, 2025 5:08 PM
> To: Bjorn Helgaas <helgaas@...nel.org>
> 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; Simek, Michal
> <michal.simek@....com>; Gogada, Bharat Kumar
> <bharat.kumar.gogada@....com>
> Subject: RE: [PATCH v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
> 
> Hi Bjorn,
> > > > It's kind of weird that these skip the odd-numbered bits, since
> > > > dw_pcie_rp_intx_flow(), amd_mdb_mask_intx_irq(),
> > > > amd_mdb_unmask_intx_irq() only use bits 19:16.  Something seems
> > > > wrong and needs either a fix or a comment about why this is the way it
> is.
> > >
> > > ... the odd bits are meant for deasserting inta, intb intc & intd I
> > > ll include this in my next patch
> >
> > > > > +#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x)
> > > > > +#define AMD_MDB_PCIE_IMR_ALL_MASK			\
> > > > > +	(						\
> > > > > +		IMR(CMPL_TIMEOUT)	|		\
> > > > > +		IMR(INTA_ASSERT)	|		\
> > > > > +		IMR(INTB_ASSERT)	|		\
> > > > > +		IMR(INTC_ASSERT)	|		\
> > > > > +		IMR(INTD_ASSERT)	|		\
> > > > > +		IMR(PM_PME_RCVD)	|		\
> > > > > +		IMR(PME_TO_ACK_RCVD)	|		\
> > > > > +		IMR(MISC_CORRECTABLE)	|		\
> > > > > +		IMR(NONFATAL)		|		\
> > > > > +		IMR(FATAL)				\
> > > > > +	)
> > > > > +
> > > > > +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)
> > > >
> > > > I would drop AMD_MDB_PCIE_INTR_INTA_ASSERT, etc, and just use
> > > > AMD_MDB_TLP_PCIE_INTX_MASK in the
> > AMD_MDB_PCIE_IMR_ALL_MASK
> > > > definition.
> > > >
> > > > If there are really eight bits of INTx-related things here for the
> > > > four INTx interrupts, I think you should make two #defines to
> > > > separate them out.
> >
> > > Yes, there are 8 intx related bits I ll define them in my next patch.
> > > I was in confusion here regarding "PCI_NUM_INTX " since this macro
> > > indicates INTA INTB INTC INTD bits so I discarded deassert bits here.
> >
> > It seems like what you have is a single 8-bit field that contains both
> > assert and deassert info, interspersed.  GENMASK()/FIELD_GET() isn't
> > enough to really separate them.  Maybe you can do something like this:
> >
> >   #define AMD_MDB_TLP_PCIE_INTX_MASK          GENMASK(23, 16)
> >
> >   #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(1 << x)
> >
> > If you don't need the deassert bits, a comment would be useful, but
> > there's no point in adding a #define for them.  If you do need them, maybe
> this:
> >
> >   #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)  BIT((1 << x) + 1)
> >
> > > > > +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,
> > > > > +			pcie_read(pcie,
> AMD_MDB_TLP_IR_STATUS_MISC));
> > > > > +
> > > > > +	for_each_set_bit(i, &val, 4)
> > > >
> > > >   for_each_set_bit(..., PCI_NUM_INTX)
> >
> > > In next patch I will update value to 8 here.
> >
> > And here you could do:
> >
> >   val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> >                   pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> >
> >   for (i = 0; i < PCI_NUM_INTX; i++) {
> >     if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
> - Thanks for reviewing, This condition never met observing zero here.
To satisfy this condition need to modify macros as following.
#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(x)
#define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)    BIT(x+1)

> >       generic_handle_domain_irq(pcie->intx_domain, i);
> >
> > > > > +		generic_handle_domain_irq(pcie->intx_domain, i);
> >
> > > > > +	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_once(dev, "Unknown IRQ %ld\n", d-
> >hwirq);
> > > >
> > > > What's the point of an interrupt handler that only logs it?
> > >
> > > At this stage, our objective is to notify the user of the occurrence
> > > of an event. While we intend to integrate these events with the AER
> > > subsystem in the future, for the time being, we will limit the
> > > functionality to notifying the user.
> >
> > OK, just add a comment to that effect here.
> - Thanks for reviewing, will add this in next patch.
> >
> > Bjorn