linux-kernel - RE: [PATCH v2 2/2] PCI: altera: add suport for Agilex Family FPGA

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 8 Sep 2023 09:15:09 +0000
From:   "D M, Sharath Kumar" <sharath.kumar.d.m@...el.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     "lpieralisi@...nel.org" <lpieralisi@...nel.org>,
        "kw@...ux.com" <kw@...ux.com>, "robh@...nel.org" <robh@...nel.org>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "dinguyen@...nel.org" <dinguyen@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 2/2] PCI: altera: add suport for Agilex Family FPGA



> -----Original Message-----
> From: Bjorn Helgaas <helgaas@...nel.org>
> Sent: Wednesday, September 6, 2023 10:42 PM
> To: D M, Sharath Kumar <sharath.kumar.d.m@...el.com>
> Cc: lpieralisi@...nel.org; kw@...ux.com; robh@...nel.org;
> bhelgaas@...gle.com; linux-pci@...r.kernel.org; dinguyen@...nel.org;
> linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v2 2/2] PCI: altera: add suport for Agilex Family FPGA
> 
> Capitalize subject line similarly.
ok
> 
> s/suport/support/
ok
> 
> On Wed, Sep 06, 2023 at 04:39:18PM +0530, sharath.kumar.d.m@...el.com
> wrote:
> > From: D M Sharath Kumar <sharath.kumar.d.m@...el.com>
> 
> Needs a commit log.  It's ok to repeat the subject line.
ok
> 
> > +#define AGLX_BDF_REG 0x00002004
> > +#define AGLX_ROOT_PORT_IRQ_STATUS 0x14c #define
> > +AGLX_ROOT_PORT_IRQ_ENABLE 0x150
> > +#define CFG_AER                   (1<<4)
> 
> This seems to be AGLX-specific so maybe should have a prefix?
Will change to AGLX_CFG_AER
> 
> > +static u32 port_conf_off;
> 
> port_conf_off looks like something that should be per-controller.
Specific to agilex, will rename to "aglx_port_conf_off"
> 
> > +static int aglx_rp_read_cfg(struct altera_pcie *pcie, u8 busno, u32 devfn,
> > +			int where, int size, u32 *value)
> > +{
> > +	void __iomem *addr = AGLX_RP_CFG_ADDR(pcie, where);
> > +
> > +	switch (size) {
> > +	case 1:
> > +		*value = readb(addr);
> > +		break;
> > +	case 2:
> > +		*value = readw(addr);
> > +		break;
> > +	default:
> > +		*value = readl(addr);
> > +		break;
> > +	}
> > +
> > +	/* interrupt pin not programmed in hardware
> > +	 */
> 
> Use single-line comment style:
ok
> 
>   /* interrupt pin not programmed in hardware */
> 
> > +	if (where == 0x3d)
> > +		*value = 0x01;
> > +	if (where == 0x3c)
> > +		*value |= 0x0100;
> 
> Use PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN.
ok
> 
> > +	return PCIBIOS_SUCCESSFUL;
> > +}
> 
> > +static void aglx_isr(struct irq_desc *desc) {
> > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +	struct altera_pcie *pcie;
> > +	struct device *dev;
> > +	u32 status;
> > +	int ret;
> > +
> > +	chained_irq_enter(chip, desc);
> > +	pcie = irq_desc_get_handler_data(desc);
> > +	dev = &pcie->pdev->dev;
> > +
> > +	status = readl((pcie->hip_base + port_conf_off
> > +		+ AGLX_ROOT_PORT_IRQ_STATUS));
> > +	if (status & CFG_AER) {
> > +		ret = generic_handle_domain_irq(pcie->irq_domain, 0);
> > +		if (ret)
> > +			dev_err_ratelimited(dev, "unexpected IRQ,\n");
> 
> Remove the comma at end (or maybe you meant to add something else?)
> Looks like the place it was copied from had "unexpected IRQ, INT%d".
ok
> 
> > +	if (pcie->pcie_data->version == ALTERA_PCIE_V3) {
> > +		pcie->cs_base =
> > +			devm_platform_ioremap_resource_byname(pdev,
> "Cs");
> > +		if (IS_ERR(pcie->cs_base))
> > +			return PTR_ERR(pcie->cs_base);
> > +		of_property_read_u32(pcie->pdev->dev.of_node,
> "port_conf_stat",
> > +			&port_conf_off);
> > +		dev_info(&pcie->pdev->dev, "port_conf_stat_off =%x\n",
> > +port_conf_off);
> 
> Is this a debug message?  Doesn't look like something we need all the time.  If
> you want it all the time, use %#x so it's clear that it's hex.
ok
> 
> > +static const struct altera_pcie_data altera_pcie_3_0_data = {
> > +	.ops = &altera_pcie_ops_3_0,
> > +	.version = ALTERA_PCIE_V3,
> > +	.cap_offset = 0x70,
> 
> > +	.cfgrd0 = 0,
> > +	.cfgrd1 = 0,
> > +	.cfgwr0 = 0,
> > +	.cfgwr1 = 0,
> 
> cfgrd0, ..., cfgwr1 aren't used here, so no need to initialize them.
ok