linux-kernel - Re: [PATCH 3/6] PCI: tegra: Add support for PCIe endpoint mode in Tegra194

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: <20191126213718.GA185422@google.com>
Date:   Tue, 26 Nov 2019 15:37:18 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Vidya Sagar <vidyas@...dia.com>
Cc:     lorenzo.pieralisi@....com, robh+dt@...nel.org,
        thierry.reding@...il.com, jonathanh@...dia.com,
        andrew.murray@....com, kishon@...com,
        gustavo.pimentel@...opsys.com, linux-pci@...r.kernel.org,
        devicetree@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        kthota@...dia.com, mmaddireddy@...dia.com, sagar.tv@...il.com
Subject: Re: [PATCH 3/6] PCI: tegra: Add support for PCIe endpoint mode in
 Tegra194

On Fri, Nov 22, 2019 at 04:15:02PM +0530, Vidya Sagar wrote:
> Add support for the endpoint mode of Synopsys DesignWare core based
> dual mode PCIe controllers present in Tegra194 SoC.

> +static irqreturn_t tegra_pcie_ep_irq_handler(struct tegra_pcie_dw *pcie)
> +{
> +	struct dw_pcie_ep *ep = &pcie->pci.ep;
> +	u32 val, tmp;
> +
> +	val = appl_readl(pcie, APPL_INTR_STATUS_L0);
> +	if (val & APPL_INTR_STATUS_L0_LINK_STATE_INT) {
> +		val = appl_readl(pcie, APPL_INTR_STATUS_L1_0_0);
> +		appl_writel(pcie, val, APPL_INTR_STATUS_L1_0_0);
> +		if (val & APPL_INTR_STATUS_L1_0_0_HOT_RESET_DONE) {
> +			/* clear any stale PEX_RST interrupt */
> +			if (!kfifo_put(&pcie->event_fifo, EP_HOT_RST_DONE)) {
> +				dev_err(pcie->dev, "EVENT FIFO is full\n");
> +				return IRQ_HANDLED;
> +			}
> +			wake_up(&pcie->wq);
> +		}
> +		if (val & APPL_INTR_STATUS_L1_0_0_RDLH_LINK_UP_CHGED) {
> +			tmp = appl_readl(pcie, APPL_LINK_STATUS);
> +			if (tmp & APPL_LINK_STATUS_RDLH_LINK_UP) {
> +				dev_info(pcie->dev, "Link is up with Host\n");
> +				dw_pcie_ep_linkup(ep);
> +			}
> +		}
> +	} else if (val & APPL_INTR_STATUS_L0_PCI_CMD_EN_INT) {

Is it really the case that only one of
APPL_INTR_STATUS_L0_LINK_STATE_INT and
APPL_INTR_STATUS_L0_PCI_CMD_EN_INT can be set?

If it's possible that both could be set, maybe this should be
something like this?

  int spurious = 1;

  if (val & APPL_INTR_STATUS_L0_LINK_STATE_INT) {
    ...
    spurious = 0;
  }
  if (val & APPL_INTR_STATUS_L0_PCI_CMD_EN_INT) {
    ...
    spurious = 0;
  }

  if (spurious) {
    dev_warn(...)
  }

> +		val = appl_readl(pcie, APPL_INTR_STATUS_L1_15);
> +		appl_writel(pcie, val, APPL_INTR_STATUS_L1_15);
> +		if (val & APPL_INTR_STATUS_L1_15_CFG_BME_CHGED) {
> +			if (!kfifo_put(&pcie->event_fifo, EP_BME_CHANGE)) {
> +				dev_err(pcie->dev, "EVENT FIFO is full\n");
> +				return IRQ_HANDLED;
> +			}
> +			wake_up(&pcie->wq);
> +		}
> +	} else {
> +		dev_warn(pcie->dev, "Random interrupt (STATUS = 0x%08X)\n",
> +			 val);
> +		appl_writel(pcie, val, APPL_INTR_STATUS_L0);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

> +static int tegra_pcie_ep_work_thread(void *p)
> +{
> +	struct tegra_pcie_dw *pcie = (struct tegra_pcie_dw *)p;
> +	u32 event;
> +
> +	while (true) {
> +		wait_event_interruptible(pcie->wq,
> +					 !kfifo_is_empty(&pcie->event_fifo));
> +
> +		if (kthread_should_stop())
> +			break;
> +
> +		if (!kfifo_get(&pcie->event_fifo, &event)) {
> +			dev_warn(pcie->dev, "EVENT FIFO is empty\n");
> +			continue;
> +		}
> +
> +		switch (event) {
> +		case EP_PEX_RST_DEASSERT:
> +			dev_info(pcie->dev, "EVENT: EP_PEX_RST_DEASSERT\n");
> +			pex_ep_event_pex_rst_deassert(pcie);
> +			break;
> +
> +		case EP_PEX_RST_ASSERT:
> +			dev_info(pcie->dev, "EVENT: EP_PEX_RST_ASSERT\n");
> +			pex_ep_event_pex_rst_assert(pcie);
> +			break;
> +
> +		case EP_HOT_RST_DONE:
> +			dev_info(pcie->dev, "EVENT: EP_HOT_RST_DONE\n");
> +			pex_ep_event_hot_rst_done(pcie);
> +			break;
> +
> +		case EP_BME_CHANGE:
> +			dev_info(pcie->dev, "EVENT: EP_BME_CHANGE\n");
> +			pex_ep_event_bme_change(pcie);
> +			break;
> +
> +		case EP_EVENT_EXIT:
> +			dev_info(pcie->dev, "EVENT: EP_EVENT_EXIT\n");
> +			return 0;
> +
> +		default:
> +			dev_warn(pcie->dev, "Invalid PCIe EP event\n");

Maybe include the invalid event value in the message?

> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}