linux-kernel - Re: [PATCH v2 5/6] PCI: brcmstb: add MSI capability

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]

Date:   Thu, 21 Nov 2019 18:19:12 +0100
From:   Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
To:     Andrew Murray <andrew.murray@....com>
Cc:     maz@...nel.org, linux-kernel@...r.kernel.org,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Florian Fainelli <f.fainelli@...il.com>,
        bcm-kernel-feedback-list@...adcom.com,
        Eric Anholt <eric@...olt.net>,
        Stefan Wahren <wahrenst@....net>, james.quinlan@...adcom.com,
        mbrugger@...e.com, phil@...pberrypi.org, jeremy.linton@....com,
        Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
        linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 5/6] PCI: brcmstb: add MSI capability

On Thu, 2019-11-21 at 15:38 +0000, Andrew Murray wrote:
> >  #define PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_SHIFT		0x4
> >  #define PCIE_MISC_PCIE_STATUS_PCIE_LINK_IN_L23_MASK		0x40
> >  #define PCIE_MISC_PCIE_STATUS_PCIE_LINK_IN_L23_SHIFT		0x6
> > +#define PCIE_MISC_REVISION_MAJMIN_MASK				0xffff
> > +#define PCIE_MISC_REVISION_MAJMIN_SHIFT				0
> 
> I don't think these two are used.

Yes, in brcm_pcie_setup(), when we grab the PCIe hw revision number.

[...]
> > +static struct msi_domain_info brcm_msi_domain_info = {
> > +	/* TODO: Multi MSI is technically supported by the controller */
> 
> As I understand we're not supporting MSI_FLAG_MULTI_PCI_MSI, not because there
> is no hardware capability, but because the current use-case (RPi EPs) have no
> need for it. It wouldn't be a stretch to add this support (compare your alloc
> implementation with nwl_irq_domain_alloc from pcie-xilinx-nwl.c) - though I
> appreciate the difficulity you may have with testing.
> 
> I'd probably replace the TODO line with:
> 
> /* Multi MSI is supported by the controller, but not by this driver */

I'll replace the comment for now.

I've already seen people who unsoldered the XHCI chip on the RPi4 to then add a
proper PCI connector. If someone shows up with such setup, I'll be happy to
work out the MultiMSI support.

[...]
> > +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > +		   MSI_FLAG_PCI_MSIX),
> 
> Why the MSIX flag if the commit message says "It does not add MSIX since that
> functionality is not in the HW." in the commit message?

That's plain wrong, sorry.

[...]
> > +	.chip	= &brcm_msi_irq_chip,
> > +};
> > +
> > +static void brcm_pcie_msi_isr(struct irq_desc *desc)
> > +{
> > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +	unsigned long status, virq;
> > +	struct brcm_msi *msi;
> > +	u32 mask, bit, hwirq;
> > +	struct device *dev;
> > +
> > +	chained_irq_enter(chip, desc);
> > +	msi = irq_desc_get_handler_data(desc);
> > +	mask = msi->intr_legacy_mask;
> 
> NIT: As you only use this variable once, you could get rid of it.

OK

[...]
> > +
> > +static int brcm_pcie_enable_msi(struct brcm_pcie *pcie)
> > +{
> > +	struct brcm_msi *msi;
> > +	int irq, ret;
> > +	struct device *dev = pcie->dev;
> > +
> > +	irq = irq_of_parse_and_map(dev->of_node, 1);
> > +	if (irq <= 0) {
> > +		dev_err(dev, "cannot map msi intr\n");
> 
> NIT: I think we can spare a few more characters and replace intr with
> interrupt.

Of course.

[...]
> > +	/*
> > +	 * We ideally want the MSI target address to be located in the 32bit
> > +	 * addressable memory area. Some devices might depend on it. This is
> > +	 * possible either when the inbound window is located above the lower
> > +	 * 4GB or when the inbound and outbound areas fit in the lower 4GB of
> > +	 * memory.
> > +	 */
> > +	if (rc_bar2_offset >= SZ_4G || (rc_bar2_size + rc_bar2_offset) <= SZ_4G)
> > +		pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_LT_4GB;
> > +	else
> > +		pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_GT_4GB;
> > +
> 
> Can this above hunk me moved into brcm_pcie_enable_msi? You could then avoid
> having the pcie->msi_target_addr and just have a single msi->target_addr
> variable?

As it depends on rc_bar2_offset and rc_bar2_size it's not really possible
without having to store those values in exchange, which IMO amounts to
negative benefit.

Regards,
Nicolas


Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)