[PATCH v4 0/2] Tango PCIe controller support

[PATCH v4 0/2] Tango PCIe controller support

[Marc Gonzalez]

Hello,

This patch was split in two, to ease review of two orthogonal
parts (MSI controller and host bridge). NB: the patch is
just split in two where host bridge support starts.

Changes from v3 to v4

In the MSI part:
- Support 256 MSIs instead of only 32
- Define tango_{ack,mask,unmask} callbacks for the HW irq_chip
- Use a spinlock instead of a mutex
- Rename msi_mask register to msi_enable

In the host bridge part:
- Move several quirk-handling snippets out of the config space read function
- Check whether the PCIe link is up at probe-time

Other files
- Let the framework compute the bus-range from the config space width
- Be slightly more verbose in the Kconfig help


What has *not* changed, waiting from feedback from Bjorn:

- Pray that config and mem space accesses NEVER occur concurrently.
- Using of_device_is_compatible() vs of_device_get_match_data()


Marc Gonzalez (2):
  PCI: Add tango MSI controller support
  PCI: Add tango PCIe host bridge support

 Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 ++++
 drivers/pci/host/Kconfig                             |   8 +
 drivers/pci/host/Makefile                            |   1 +
 drivers/pci/host/pcie-tango.c                        | 393 +++++++++++++++++++++++++++++++++++++++
 include/linux/pci_ids.h                              |   2 +
 5 files changed, 436 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/tango-pcie.txt
 create mode 100644 drivers/pci/host/pcie-tango.c

-- 
2.11.0

[PATCH v4 1/2] PCI: Add tango MSI controller support

[Marc Gonzalez]

The MSI controller in Tango supports 256 message-signaled interrupts,
and a single doorbell address.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/pci/host/pcie-tango.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 232 insertions(+)

diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
new file mode 100644
index 000000000000..ada8d35066ab
--- /dev/null
+++ b/drivers/pci/host/pcie-tango.c
@@ -0,0 +1,232 @@
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/pci-ecam.h>
+#include <linux/delay.h>
+#include <linux/msi.h>
+
+#define MSI_MAX 256
+
+struct tango_pcie {
+	DECLARE_BITMAP(bitmap, MSI_MAX);
+	spinlock_t lock;
+	void __iomem *mux;
+	void __iomem *msi_status;
+	void __iomem *msi_enable;
+	phys_addr_t msi_doorbell;
+	struct irq_domain *irq_dom;
+	struct irq_domain *msi_dom;
+	int irq;
+};
+
+/*** MSI CONTROLLER SUPPORT ***/
+
+static void dispatch(struct tango_pcie *pcie, unsigned long status, int base)
+{
+	unsigned int pos, virq;
+
+	for_each_set_bit(pos, &status, 32) {
+		virq = irq_find_mapping(pcie->irq_dom, base + pos);
+		generic_handle_irq(virq);
+	}
+}
+
+static void tango_msi_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
+	unsigned int status, base, pos = 0;
+
+	chained_irq_enter(chip, desc);
+
+	while ((pos = find_next_bit(pcie->bitmap, MSI_MAX, pos)) < MSI_MAX) {
+		base = round_down(pos, 32);
+		status = readl_relaxed(pcie->msi_status + base / 8);
+		dispatch(pcie, status, base);
+		pos = base + 32;
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void tango_ack(struct irq_data *data)
+{
+	u32 bit = BIT(data->hwirq);
+	struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
+
+	writel_relaxed(bit, pcie->msi_status);
+}
+
+static void update_msi_enable(struct irq_data *data, bool unmask)
+{
+	unsigned long flags;
+	u32 val, bit = BIT(data->hwirq % 32);
+	int byte_offset = (data->hwirq / 32) * 4;
+	struct tango_pcie *pcie = data->chip_data;
+
+	spin_lock_irqsave(&pcie->lock, flags);
+	val = readl_relaxed(pcie->msi_enable + byte_offset);
+	val = unmask ? val | bit : val & ~bit;
+	writel_relaxed(val, pcie->msi_enable + byte_offset);
+	spin_unlock_irqrestore(&pcie->lock, flags);
+}
+
+static void tango_mask(struct irq_data *data)
+{
+	update_msi_enable(data, false);
+}
+
+static void tango_unmask(struct irq_data *data)
+{
+	update_msi_enable(data, true);
+}
+
+static int tango_set_affinity(struct irq_data *data,
+		const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static void tango_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
+
+	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
+	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
+	msg->data = data->hwirq;
+}
+
+static struct irq_chip tango_chip = {
+	.irq_ack		= tango_ack,
+	.irq_mask		= tango_mask,
+	.irq_unmask		= tango_unmask,
+	.irq_set_affinity	= tango_set_affinity,
+	.irq_compose_msi_msg	= tango_compose_msi_msg,
+};
+
+static void msi_ack(struct irq_data *data)
+{
+	irq_chip_ack_parent(data);
+}
+
+static void msi_mask(struct irq_data *data)
+{
+	pci_msi_mask_irq(data);
+	irq_chip_mask_parent(data);
+}
+
+static void msi_unmask(struct irq_data *data)
+{
+	pci_msi_unmask_irq(data);
+	irq_chip_unmask_parent(data);
+}
+
+static struct irq_chip msi_chip = {
+	.name = "MSI",
+	.irq_ack = msi_ack,
+	.irq_mask = msi_mask,
+	.irq_unmask = msi_unmask,
+};
+
+static struct msi_domain_info msi_dom_info = {
+	.flags	= MSI_FLAG_PCI_MSIX
+		| MSI_FLAG_USE_DEF_DOM_OPS
+		| MSI_FLAG_USE_DEF_CHIP_OPS,
+	.chip	= &msi_chip,
+};
+
+static int find_free_msi(struct irq_domain *dom, unsigned int virq)
+{
+	unsigned int pos;
+	struct tango_pcie *pcie = dom->host_data;
+
+	pos = find_first_zero_bit(pcie->bitmap, MSI_MAX);
+	if (pos >= MSI_MAX)
+		return -ENOSPC;
+	__set_bit(pos, pcie->bitmap);
+
+	irq_domain_set_info(dom, virq, pos, &tango_chip, pcie,
+			handle_edge_irq, NULL, NULL);
+
+	return 0;
+}
+
+static int tango_irq_domain_alloc(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs, void *args)
+{
+	int err;
+	unsigned long flags;
+	struct tango_pcie *pcie = dom->host_data;
+
+	spin_lock_irqsave(&pcie->lock, flags);
+	err = find_free_msi(dom, virq);
+	spin_unlock_irqrestore(&pcie->lock, flags);
+
+	return err;
+}
+
+static void tango_irq_domain_free(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs)
+{
+	unsigned long flags;
+	struct irq_data *data = irq_domain_get_irq_data(dom, virq);
+	struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
+
+	spin_lock_irqsave(&pcie->lock, flags);
+	__clear_bit(data->hwirq, pcie->bitmap);
+	spin_unlock_irqrestore(&pcie->lock, flags);
+}
+
+static const struct irq_domain_ops irq_dom_ops = {
+	.alloc	= tango_irq_domain_alloc,
+	.free	= tango_irq_domain_free,
+};
+
+static int tango_msi_remove(struct platform_device *pdev)
+{
+	struct tango_pcie *pcie = platform_get_drvdata(pdev);
+
+	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
+	irq_domain_remove(pcie->msi_dom);
+	irq_domain_remove(pcie->irq_dom);
+
+	return 0;
+}
+
+static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie)
+{
+	int i, virq;
+	struct irq_domain *msi_dom, *irq_dom;
+	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
+
+	spin_lock_init(&pcie->lock);
+	for (i = 0; i < MSI_MAX / 32; ++i)
+		writel_relaxed(0, pcie->msi_enable + i * 4);
+
+	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &irq_dom_ops, pcie);
+	if (!irq_dom) {
+		pr_err("Failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_dom_info, irq_dom);
+	if (!msi_dom) {
+		pr_err("Failed to create MSI domain\n");
+		irq_domain_remove(irq_dom);
+		return -ENOMEM;
+	}
+
+	virq = platform_get_irq(pdev, 1);
+	if (virq <= 0) {
+		pr_err("Failed to map IRQ\n");
+		irq_domain_remove(msi_dom);
+		irq_domain_remove(irq_dom);
+		return -ENXIO;
+	}
+
+	pcie->irq_dom = irq_dom;
+	pcie->msi_dom = msi_dom;
+	pcie->irq = virq;
+	irq_set_chained_handler_and_data(virq, tango_msi_isr, pcie);
+
+	return 0;
+}
-- 
2.11.0

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Marc Gonzalez]

On 20/04/2017 16:28, Marc Gonzalez wrote:

> +static int tango_set_affinity(struct irq_data *data,
> +		const struct cpumask *mask, bool force)
> +{
> +	return -EINVAL;
> +}
> +
> +static struct irq_chip tango_chip = {
> +	.irq_ack		= tango_ack,
> +	.irq_mask		= tango_mask,
> +	.irq_unmask		= tango_unmask,
> +	.irq_set_affinity	= tango_set_affinity,
> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
> +};

Hmmm... I'm wondering why .irq_set_affinity is required.

static int setup_affinity(struct irq_desc *desc, struct cpumask *mask)
first calls __irq_can_set_affinity() to check whether
desc->irq_data.chip->irq_set_affinity) exists.

then calls irq_do_set_affinity(&desc->irq_data, mask, false);
which calls chip->irq_set_affinity(data, mask, force);
= msi_domain_set_affinity()
which calls parent->chip->irq_set_affinity() unconditionally.

Would it make sense to test that the callback is implemented
before calling it?


[    0.723895] Unable to handle kernel NULL pointer dereference at virtual address 00000000
...
[    1.135809] [<c0160eb4>] (msi_domain_set_affinity) from [<c015a864>] (irq_do_set_affinity+0x18/0x48)
[    1.144990] [<c015a864>] (irq_do_set_affinity) from [<c015a918>] (setup_affinity+0x84/0xd4)
[    1.153384] [<c015a918>] (setup_affinity) from [<c015b20c>] (__setup_irq+0x40c/0x5d4)
[    1.161254] [<c015b20c>] (__setup_irq) from [<c015b57c>] (request_threaded_irq+0xe4/0x184)
[    1.169569] [<c015b57c>] (request_threaded_irq) from [<c03102e0>] (aer_probe+0x9c/0x218)
[    1.177704] [<c03102e0>] (aer_probe) from [<c030e4bc>] (pcie_port_probe_service+0x34/0x70)
[    1.186017] [<c030e4bc>] (pcie_port_probe_service) from [<c0351cd0>] (really_probe+0x1c4/0x250)
[    1.194763] [<c0351cd0>] (really_probe) from [<c0351ec8>] (__device_attach_driver+0xa4/0xe0)
[    1.203245] [<c0351ec8>] (__device_attach_driver) from [<c0350274>] (bus_for_each_drv+0x60/0x94)
[    1.212076] [<c0350274>] (bus_for_each_drv) from [<c0351abc>] (__device_attach+0x9c/0xdc)
[    1.220296] [<c0351abc>] (__device_attach) from [<c0351ff0>] (device_initial_probe+0xc/0x10)
[    1.228777] [<c0351ff0>] (device_initial_probe) from [<c0351090>] (bus_probe_device+0x84/0x8c)
[    1.237433] [<c0351090>] (bus_probe_device) from [<c034f484>] (device_add+0x3cc/0x548)
[    1.245390] [<c034f484>] (device_add) from [<c034f614>] (device_register+0x14/0x18)
[    1.253086] [<c034f614>] (device_register) from [<c030e8b8>] (pcie_port_device_register+0x3b0/0x450)
[    1.262267] [<c030e8b8>] (pcie_port_device_register) from [<c030ecfc>] (pcie_portdrv_probe+0x2c/0x50)
[    1.271539] [<c030ecfc>] (pcie_portdrv_probe) from [<c0302ba8>] (pci_device_probe+0x7c/0xc8)
[    1.280022] [<c0302ba8>] (pci_device_probe) from [<c0351c84>] (really_probe+0x178/0x250)
[    1.288154] [<c0351c84>] (really_probe) from [<c0351ec8>] (__device_attach_driver+0xa4/0xe0)
[    1.296635] [<c0351ec8>] (__device_attach_driver) from [<c0350274>] (bus_for_each_drv+0x60/0x94)
[    1.305465] [<c0350274>] (bus_for_each_drv) from [<c0351abc>] (__device_attach+0x9c/0xdc)
[    1.313684] [<c0351abc>] (__device_attach) from [<c0351b08>] (device_attach+0xc/0x10)
[    1.321559] [<c0351b08>] (device_attach) from [<c02f9a20>] (pci_bus_add_device+0x44/0x90)
[    1.329778] [<c02f9a20>] (pci_bus_add_device) from [<c02f9aa8>] (pci_bus_add_devices+0x3c/0x80)
[    1.338523] [<c02f9aa8>] (pci_bus_add_devices) from [<c0312dfc>] (pci_host_common_probe+0x100/0x314)
[    1.347703] [<c0312dfc>] (pci_host_common_probe) from [<c0313550>] (tango_pcie_probe+0x138/0x340)
[    1.356624] [<c0313550>] (tango_pcie_probe) from [<c03532c4>] (platform_drv_probe+0x34/0x6c)

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Bjorn Helgaas]

On Wed, May 17, 2017 at 04:56:08PM +0200, Marc Gonzalez wrote:
> On 20/04/2017 16:28, Marc Gonzalez wrote:
> 
> > +static int tango_set_affinity(struct irq_data *data,
> > +		const struct cpumask *mask, bool force)
> > +{
> > +	return -EINVAL;
> > +}
> > +
> > +static struct irq_chip tango_chip = {
> > +	.irq_ack		= tango_ack,
> > +	.irq_mask		= tango_mask,
> > +	.irq_unmask		= tango_unmask,
> > +	.irq_set_affinity	= tango_set_affinity,
> > +	.irq_compose_msi_msg	= tango_compose_msi_msg,
> > +};
> 
> Hmmm... I'm wondering why .irq_set_affinity is required.
> 
> static int setup_affinity(struct irq_desc *desc, struct cpumask *mask)
> first calls __irq_can_set_affinity() to check whether
> desc->irq_data.chip->irq_set_affinity) exists.
> 
> then calls irq_do_set_affinity(&desc->irq_data, mask, false);
> which calls chip->irq_set_affinity(data, mask, force);
> = msi_domain_set_affinity()
> which calls parent->chip->irq_set_affinity() unconditionally.
> 
> Would it make sense to test that the callback is implemented
> before calling it?
> 
> 
> [    0.723895] Unable to handle kernel NULL pointer dereference at virtual address 00000000

I'm not sure what you're asking.

Is this a bug report for the v4 tango driver?

Or are you asking whether msi_domain_set_affinity() should be changed
to check whether parent->chip->irq_set_affinity is implemented?

msi_domain_set_affinity() has called parent->chip->irq_set_affinity()
without checking since it was added in 2014 by f3cf8bb0d6c3 ("genirq: Add
generic msi irq domain support"), so if there's a problem here, it's most
likely in the tango code.

> ...
> [    1.135809] [<c0160eb4>] (msi_domain_set_affinity) from [<c015a864>] (irq_do_set_affinity+0x18/0x48)
> [    1.144990] [<c015a864>] (irq_do_set_affinity) from [<c015a918>] (setup_affinity+0x84/0xd4)
> [    1.153384] [<c015a918>] (setup_affinity) from [<c015b20c>] (__setup_irq+0x40c/0x5d4)
> [    1.161254] [<c015b20c>] (__setup_irq) from [<c015b57c>] (request_threaded_irq+0xe4/0x184)
> [    1.169569] [<c015b57c>] (request_threaded_irq) from [<c03102e0>] (aer_probe+0x9c/0x218)
> [    1.177704] [<c03102e0>] (aer_probe) from [<c030e4bc>] (pcie_port_probe_service+0x34/0x70)
> [    1.186017] [<c030e4bc>] (pcie_port_probe_service) from [<c0351cd0>] (really_probe+0x1c4/0x250)
> [    1.194763] [<c0351cd0>] (really_probe) from [<c0351ec8>] (__device_attach_driver+0xa4/0xe0)
> [    1.203245] [<c0351ec8>] (__device_attach_driver) from [<c0350274>] (bus_for_each_drv+0x60/0x94)
> [    1.212076] [<c0350274>] (bus_for_each_drv) from [<c0351abc>] (__device_attach+0x9c/0xdc)
> [    1.220296] [<c0351abc>] (__device_attach) from [<c0351ff0>] (device_initial_probe+0xc/0x10)
> [    1.228777] [<c0351ff0>] (device_initial_probe) from [<c0351090>] (bus_probe_device+0x84/0x8c)
> [    1.237433] [<c0351090>] (bus_probe_device) from [<c034f484>] (device_add+0x3cc/0x548)
> [    1.245390] [<c034f484>] (device_add) from [<c034f614>] (device_register+0x14/0x18)
> [    1.253086] [<c034f614>] (device_register) from [<c030e8b8>] (pcie_port_device_register+0x3b0/0x450)
> [    1.262267] [<c030e8b8>] (pcie_port_device_register) from [<c030ecfc>] (pcie_portdrv_probe+0x2c/0x50)
> [    1.271539] [<c030ecfc>] (pcie_portdrv_probe) from [<c0302ba8>] (pci_device_probe+0x7c/0xc8)
> [    1.280022] [<c0302ba8>] (pci_device_probe) from [<c0351c84>] (really_probe+0x178/0x250)
> [    1.288154] [<c0351c84>] (really_probe) from [<c0351ec8>] (__device_attach_driver+0xa4/0xe0)
> [    1.296635] [<c0351ec8>] (__device_attach_driver) from [<c0350274>] (bus_for_each_drv+0x60/0x94)
> [    1.305465] [<c0350274>] (bus_for_each_drv) from [<c0351abc>] (__device_attach+0x9c/0xdc)
> [    1.313684] [<c0351abc>] (__device_attach) from [<c0351b08>] (device_attach+0xc/0x10)
> [    1.321559] [<c0351b08>] (device_attach) from [<c02f9a20>] (pci_bus_add_device+0x44/0x90)
> [    1.329778] [<c02f9a20>] (pci_bus_add_device) from [<c02f9aa8>] (pci_bus_add_devices+0x3c/0x80)
> [    1.338523] [<c02f9aa8>] (pci_bus_add_devices) from [<c0312dfc>] (pci_host_common_probe+0x100/0x314)
> [    1.347703] [<c0312dfc>] (pci_host_common_probe) from [<c0313550>] (tango_pcie_probe+0x138/0x340)
> [    1.356624] [<c0313550>] (tango_pcie_probe) from [<c03532c4>] (platform_drv_probe+0x34/0x6c)
> 

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Mason]

On 23/05/2017 19:03, Bjorn Helgaas wrote:
> On Wed, May 17, 2017 at 04:56:08PM +0200, Marc Gonzalez wrote:
>> On 20/04/2017 16:28, Marc Gonzalez wrote:
>>
>>> +static int tango_set_affinity(struct irq_data *data,
>>> +		const struct cpumask *mask, bool force)
>>> +{
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static struct irq_chip tango_chip = {
>>> +	.irq_ack		= tango_ack,
>>> +	.irq_mask		= tango_mask,
>>> +	.irq_unmask		= tango_unmask,
>>> +	.irq_set_affinity	= tango_set_affinity,
>>> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
>>> +};
>>
>> Hmmm... I'm wondering why .irq_set_affinity is required.
>>
>> static int setup_affinity(struct irq_desc *desc, struct cpumask *mask)
>> first calls __irq_can_set_affinity() to check whether
>> desc->irq_data.chip->irq_set_affinity) exists.
>>
>> then calls irq_do_set_affinity(&desc->irq_data, mask, false);
>> which calls chip->irq_set_affinity(data, mask, force);
>> = msi_domain_set_affinity()
>> which calls parent->chip->irq_set_affinity() unconditionally.
>>
>> Would it make sense to test that the callback is implemented
>> before calling it?
>>
>>
>> [    0.723895] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> 
> I'm not sure what you're asking.
> 
> Is this a bug report for the v4 tango driver?

No.

> Or are you asking whether msi_domain_set_affinity() should be changed
> to check whether parent->chip->irq_set_affinity is implemented?

Yes. The way things are implemented now, drivers are forced
to define an irq_set_affinity callback, even if it just returns
an error, otherwise, the kernel crashes, because of the
unconditional function pointer deref. 

> msi_domain_set_affinity() has called parent->chip->irq_set_affinity()
> without checking since it was added in 2014 by f3cf8bb0d6c3 ("genirq: Add
> generic msi irq domain support"), so if there's a problem here, it's most
> likely in the tango code.

The issue is having to define an "empty" function.
(Unnecessary code bloat and maintenance.)

I'll send a patch illustrating exactly what I intended.

Regards.

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Robin Murphy]

On 23/05/17 18:54, Mason wrote:
> On 23/05/2017 19:03, Bjorn Helgaas wrote:
>> On Wed, May 17, 2017 at 04:56:08PM +0200, Marc Gonzalez wrote:
>>> On 20/04/2017 16:28, Marc Gonzalez wrote:
>>>
>>>> +static int tango_set_affinity(struct irq_data *data,
>>>> +		const struct cpumask *mask, bool force)
>>>> +{
>>>> +	return -EINVAL;
>>>> +}
>>>> +
>>>> +static struct irq_chip tango_chip = {
>>>> +	.irq_ack		= tango_ack,
>>>> +	.irq_mask		= tango_mask,
>>>> +	.irq_unmask		= tango_unmask,
>>>> +	.irq_set_affinity	= tango_set_affinity,
>>>> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
>>>> +};
>>>
>>> Hmmm... I'm wondering why .irq_set_affinity is required.
>>>
>>> static int setup_affinity(struct irq_desc *desc, struct cpumask *mask)
>>> first calls __irq_can_set_affinity() to check whether
>>> desc->irq_data.chip->irq_set_affinity) exists.
>>>
>>> then calls irq_do_set_affinity(&desc->irq_data, mask, false);
>>> which calls chip->irq_set_affinity(data, mask, force);
>>> = msi_domain_set_affinity()
>>> which calls parent->chip->irq_set_affinity() unconditionally.
>>>
>>> Would it make sense to test that the callback is implemented
>>> before calling it?
>>>
>>>
>>> [    0.723895] Unable to handle kernel NULL pointer dereference at virtual address 00000000
>>
>> I'm not sure what you're asking.
>>
>> Is this a bug report for the v4 tango driver?
> 
> No.
> 
>> Or are you asking whether msi_domain_set_affinity() should be changed
>> to check whether parent->chip->irq_set_affinity is implemented?
> 
> Yes. The way things are implemented now, drivers are forced
> to define an irq_set_affinity callback, even if it just returns
> an error, otherwise, the kernel crashes, because of the
> unconditional function pointer deref. 
> 
>> msi_domain_set_affinity() has called parent->chip->irq_set_affinity()
>> without checking since it was added in 2014 by f3cf8bb0d6c3 ("genirq: Add
>> generic msi irq domain support"), so if there's a problem here, it's most
>> likely in the tango code.
> 
> The issue is having to define an "empty" function.
> (Unnecessary code bloat and maintenance.)

AFAICS, only one driver (other than this one) implements a "do nothing"
set_affinity callback - everyone else who doesn't do anything hardware
specific just passes it along via irq_chip_set_affinity_parent().

Robin.

> 
> I'll send a patch illustrating exactly what I intended.
> 
> Regards.
> 

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Mason]

On 23/05/2017 20:03, Robin Murphy wrote:
> On 23/05/17 18:54, Mason wrote:
>> On 23/05/2017 19:03, Bjorn Helgaas wrote:
>>> On Wed, May 17, 2017 at 04:56:08PM +0200, Marc Gonzalez wrote:
>>>> On 20/04/2017 16:28, Marc Gonzalez wrote:
>>>>
>>>>> +static int tango_set_affinity(struct irq_data *data,
>>>>> +		const struct cpumask *mask, bool force)
>>>>> +{
>>>>> +	return -EINVAL;
>>>>> +}
>>>>> +
>>>>> +static struct irq_chip tango_chip = {
>>>>> +	.irq_ack		= tango_ack,
>>>>> +	.irq_mask		= tango_mask,
>>>>> +	.irq_unmask		= tango_unmask,
>>>>> +	.irq_set_affinity	= tango_set_affinity,
>>>>> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
>>>>> +};
>>>>
>>>> Hmmm... I'm wondering why .irq_set_affinity is required.
>>>>
>>>> static int setup_affinity(struct irq_desc *desc, struct cpumask *mask)
>>>> first calls __irq_can_set_affinity() to check whether
>>>> desc->irq_data.chip->irq_set_affinity) exists.
>>>>
>>>> then calls irq_do_set_affinity(&desc->irq_data, mask, false);
>>>> which calls chip->irq_set_affinity(data, mask, force);
>>>> = msi_domain_set_affinity()
>>>> which calls parent->chip->irq_set_affinity() unconditionally.
>>>>
>>>> Would it make sense to test that the callback is implemented
>>>> before calling it?
>>>>
>>>>
>>>> [    0.723895] Unable to handle kernel NULL pointer dereference at virtual address 00000000
>>>
>>> I'm not sure what you're asking.
>>>
>>> Is this a bug report for the v4 tango driver?
>>
>> No.
>>
>>> Or are you asking whether msi_domain_set_affinity() should be changed
>>> to check whether parent->chip->irq_set_affinity is implemented?
>>
>> Yes. The way things are implemented now, drivers are forced
>> to define an irq_set_affinity callback, even if it just returns
>> an error, otherwise, the kernel crashes, because of the
>> unconditional function pointer deref. 
>>
>>> msi_domain_set_affinity() has called parent->chip->irq_set_affinity()
>>> without checking since it was added in 2014 by f3cf8bb0d6c3 ("genirq: Add
>>> generic msi irq domain support"), so if there's a problem here, it's most
>>> likely in the tango code.
>>
>> The issue is having to define an "empty" function.
>> (Unnecessary code bloat and maintenance.)
> 
> AFAICS, only one driver (other than this one) implements a "do nothing"
> set_affinity callback - everyone else who doesn't do anything hardware
> specific just passes it along via irq_chip_set_affinity_parent().

I counted 4. Where did I mess up?

advk_msi_set_affinity
altera_msi_set_affinity
nwl_msi_set_affinity
vmd_irq_set_affinity
tango_set_affinity

Regards.

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Robin Murphy]

On 23/05/17 20:15, Mason wrote:
> On 23/05/2017 20:03, Robin Murphy wrote:
>> On 23/05/17 18:54, Mason wrote:
>>> On 23/05/2017 19:03, Bjorn Helgaas wrote:
>>>> On Wed, May 17, 2017 at 04:56:08PM +0200, Marc Gonzalez wrote:
>>>>> On 20/04/2017 16:28, Marc Gonzalez wrote:
>>>>>
>>>>>> +static int tango_set_affinity(struct irq_data *data,
>>>>>> +		const struct cpumask *mask, bool force)
>>>>>> +{
>>>>>> +	return -EINVAL;
>>>>>> +}
>>>>>> +
>>>>>> +static struct irq_chip tango_chip = {
>>>>>> +	.irq_ack		= tango_ack,
>>>>>> +	.irq_mask		= tango_mask,
>>>>>> +	.irq_unmask		= tango_unmask,
>>>>>> +	.irq_set_affinity	= tango_set_affinity,
>>>>>> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
>>>>>> +};
>>>>>
>>>>> Hmmm... I'm wondering why .irq_set_affinity is required.
>>>>>
>>>>> static int setup_affinity(struct irq_desc *desc, struct cpumask *mask)
>>>>> first calls __irq_can_set_affinity() to check whether
>>>>> desc->irq_data.chip->irq_set_affinity) exists.
>>>>>
>>>>> then calls irq_do_set_affinity(&desc->irq_data, mask, false);
>>>>> which calls chip->irq_set_affinity(data, mask, force);
>>>>> = msi_domain_set_affinity()
>>>>> which calls parent->chip->irq_set_affinity() unconditionally.
>>>>>
>>>>> Would it make sense to test that the callback is implemented
>>>>> before calling it?
>>>>>
>>>>>
>>>>> [    0.723895] Unable to handle kernel NULL pointer dereference at virtual address 00000000
>>>>
>>>> I'm not sure what you're asking.
>>>>
>>>> Is this a bug report for the v4 tango driver?
>>>
>>> No.
>>>
>>>> Or are you asking whether msi_domain_set_affinity() should be changed
>>>> to check whether parent->chip->irq_set_affinity is implemented?
>>>
>>> Yes. The way things are implemented now, drivers are forced
>>> to define an irq_set_affinity callback, even if it just returns
>>> an error, otherwise, the kernel crashes, because of the
>>> unconditional function pointer deref. 
>>>
>>>> msi_domain_set_affinity() has called parent->chip->irq_set_affinity()
>>>> without checking since it was added in 2014 by f3cf8bb0d6c3 ("genirq: Add
>>>> generic msi irq domain support"), so if there's a problem here, it's most
>>>> likely in the tango code.
>>>
>>> The issue is having to define an "empty" function.
>>> (Unnecessary code bloat and maintenance.)
>>
>> AFAICS, only one driver (other than this one) implements a "do nothing"
>> set_affinity callback - everyone else who doesn't do anything hardware
>> specific just passes it along via irq_chip_set_affinity_parent().
> 
> I counted 4. Where did I mess up?
> 
> advk_msi_set_affinity
> altera_msi_set_affinity
> nwl_msi_set_affinity
> vmd_irq_set_affinity
> tango_set_affinity

Fair point - I went through drivers/irqchip and the various
arch-specific instances and found ls_scfg_msi_set_affinity(), but
somehow skipped over drivers/pci. Anyway, I think the question stands of
why are these handful of drivers *not* using irq_chip_set_affinity_parent()?

As an outsider, it naively seems logical that the affinity of an MSI
which just gets translated to a wired interrupt should propagate through
to the affinity of that wired interrupt, but maybe there are reasons not
to; I really don't know.

Robin.

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Marc Zyngier]

On 24/05/17 11:00, Robin Murphy wrote:
> On 23/05/17 20:15, Mason wrote:
>> On 23/05/2017 20:03, Robin Murphy wrote:
>>> On 23/05/17 18:54, Mason wrote:
>>>> On 23/05/2017 19:03, Bjorn Helgaas wrote:
>>>>> On Wed, May 17, 2017 at 04:56:08PM +0200, Marc Gonzalez wrote:
>>>>>> On 20/04/2017 16:28, Marc Gonzalez wrote:
>>>>>>
>>>>>>> +static int tango_set_affinity(struct irq_data *data,
>>>>>>> +		const struct cpumask *mask, bool force)
>>>>>>> +{
>>>>>>> +	return -EINVAL;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static struct irq_chip tango_chip = {
>>>>>>> +	.irq_ack		= tango_ack,
>>>>>>> +	.irq_mask		= tango_mask,
>>>>>>> +	.irq_unmask		= tango_unmask,
>>>>>>> +	.irq_set_affinity	= tango_set_affinity,
>>>>>>> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
>>>>>>> +};
>>>>>>
>>>>>> Hmmm... I'm wondering why .irq_set_affinity is required.
>>>>>>
>>>>>> static int setup_affinity(struct irq_desc *desc, struct cpumask *mask)
>>>>>> first calls __irq_can_set_affinity() to check whether
>>>>>> desc->irq_data.chip->irq_set_affinity) exists.
>>>>>>
>>>>>> then calls irq_do_set_affinity(&desc->irq_data, mask, false);
>>>>>> which calls chip->irq_set_affinity(data, mask, force);
>>>>>> = msi_domain_set_affinity()
>>>>>> which calls parent->chip->irq_set_affinity() unconditionally.
>>>>>>
>>>>>> Would it make sense to test that the callback is implemented
>>>>>> before calling it?
>>>>>>
>>>>>>
>>>>>> [    0.723895] Unable to handle kernel NULL pointer dereference at virtual address 00000000
>>>>>
>>>>> I'm not sure what you're asking.
>>>>>
>>>>> Is this a bug report for the v4 tango driver?
>>>>
>>>> No.
>>>>
>>>>> Or are you asking whether msi_domain_set_affinity() should be changed
>>>>> to check whether parent->chip->irq_set_affinity is implemented?
>>>>
>>>> Yes. The way things are implemented now, drivers are forced
>>>> to define an irq_set_affinity callback, even if it just returns
>>>> an error, otherwise, the kernel crashes, because of the
>>>> unconditional function pointer deref. 
>>>>
>>>>> msi_domain_set_affinity() has called parent->chip->irq_set_affinity()
>>>>> without checking since it was added in 2014 by f3cf8bb0d6c3 ("genirq: Add
>>>>> generic msi irq domain support"), so if there's a problem here, it's most
>>>>> likely in the tango code.
>>>>
>>>> The issue is having to define an "empty" function.
>>>> (Unnecessary code bloat and maintenance.)
>>>
>>> AFAICS, only one driver (other than this one) implements a "do nothing"
>>> set_affinity callback - everyone else who doesn't do anything hardware
>>> specific just passes it along via irq_chip_set_affinity_parent().
>>
>> I counted 4. Where did I mess up?
>>
>> advk_msi_set_affinity
>> altera_msi_set_affinity
>> nwl_msi_set_affinity
>> vmd_irq_set_affinity
>> tango_set_affinity
> 
> Fair point - I went through drivers/irqchip and the various
> arch-specific instances and found ls_scfg_msi_set_affinity(), but
> somehow skipped over drivers/pci. Anyway, I think the question stands of
> why are these handful of drivers *not* using irq_chip_set_affinity_parent()?

Probably because they don't have a parent, in the hierarchical sense.
All they have is a chained interrupt (*puke*). Which implies that
changing the affinity of one MSI would affect all of them, completely
confusing unsuspecting userspace such as irqbalance.

> As an outsider, it naively seems logical that the affinity of an MSI
> which just gets translated to a wired interrupt should propagate through
> to the affinity of that wired interrupt, but maybe there are reasons not
> to; I really don't know.

See above. The main issue is that HW folks haven't understood the actual
use of MSIs, and persist in implementing them as an afterthought, based
on some cascading interrupt controller design.

Sad. So sad.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Mason]

On 24/05/2017 12:22, Marc Zyngier wrote:
> On 24/05/17 11:00, Robin Murphy wrote:
>> On 23/05/17 20:15, Mason wrote:
>>> On 23/05/2017 20:03, Robin Murphy wrote:
>>>> On 23/05/17 18:54, Mason wrote:
>>>>> On 23/05/2017 19:03, Bjorn Helgaas wrote:
>>>>>> On Wed, May 17, 2017 at 04:56:08PM +0200, Marc Gonzalez wrote:
>>>>>>> On 20/04/2017 16:28, Marc Gonzalez wrote:
>>>>>>>
>>>>>>>> +static int tango_set_affinity(struct irq_data *data,
>>>>>>>> +		const struct cpumask *mask, bool force)
>>>>>>>> +{
>>>>>>>> +	return -EINVAL;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static struct irq_chip tango_chip = {
>>>>>>>> +	.irq_ack		= tango_ack,
>>>>>>>> +	.irq_mask		= tango_mask,
>>>>>>>> +	.irq_unmask		= tango_unmask,
>>>>>>>> +	.irq_set_affinity	= tango_set_affinity,
>>>>>>>> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
>>>>>>>> +};
>>>>>>>
>>>>>>> Hmmm... I'm wondering why .irq_set_affinity is required.
>>>>>>>
>>>>>>> static int setup_affinity(struct irq_desc *desc, struct cpumask *mask)
>>>>>>> first calls __irq_can_set_affinity() to check whether
>>>>>>> desc->irq_data.chip->irq_set_affinity) exists.
>>>>>>>
>>>>>>> then calls irq_do_set_affinity(&desc->irq_data, mask, false);
>>>>>>> which calls chip->irq_set_affinity(data, mask, force);
>>>>>>> = msi_domain_set_affinity()
>>>>>>> which calls parent->chip->irq_set_affinity() unconditionally.
>>>>>>>
>>>>>>> Would it make sense to test that the callback is implemented
>>>>>>> before calling it?
>>>>>>>
>>>>>>>
>>>>>>> [    0.723895] Unable to handle kernel NULL pointer dereference at virtual address 00000000
>>>>>>
>>>>>> I'm not sure what you're asking.
>>>>>>
>>>>>> Is this a bug report for the v4 tango driver?
>>>>>
>>>>> No.
>>>>>
>>>>>> Or are you asking whether msi_domain_set_affinity() should be changed
>>>>>> to check whether parent->chip->irq_set_affinity is implemented?
>>>>>
>>>>> Yes. The way things are implemented now, drivers are forced
>>>>> to define an irq_set_affinity callback, even if it just returns
>>>>> an error, otherwise, the kernel crashes, because of the
>>>>> unconditional function pointer deref. 
>>>>>
>>>>>> msi_domain_set_affinity() has called parent->chip->irq_set_affinity()
>>>>>> without checking since it was added in 2014 by f3cf8bb0d6c3 ("genirq: Add
>>>>>> generic msi irq domain support"), so if there's a problem here, it's most
>>>>>> likely in the tango code.
>>>>>
>>>>> The issue is having to define an "empty" function.
>>>>> (Unnecessary code bloat and maintenance.)
>>>>
>>>> AFAICS, only one driver (other than this one) implements a "do nothing"
>>>> set_affinity callback - everyone else who doesn't do anything hardware
>>>> specific just passes it along via irq_chip_set_affinity_parent().
>>>
>>> I counted 4. Where did I mess up?
>>>
>>> advk_msi_set_affinity
>>> altera_msi_set_affinity
>>> nwl_msi_set_affinity
>>> vmd_irq_set_affinity
>>> tango_set_affinity
>>
>> Fair point - I went through drivers/irqchip and the various
>> arch-specific instances and found ls_scfg_msi_set_affinity(), but
>> somehow skipped over drivers/pci. Anyway, I think the question stands of
>> why are these handful of drivers *not* using irq_chip_set_affinity_parent()?
> 
> Probably because they don't have a parent, in the hierarchical sense.
> All they have is a chained interrupt (*puke*). Which implies that
> changing the affinity of one MSI would affect all of them, completely
> confusing unsuspecting userspace such as irqbalance.
> 
>> As an outsider, it naively seems logical that the affinity of an MSI
>> which just gets translated to a wired interrupt should propagate through
>> to the affinity of that wired interrupt, but maybe there are reasons not
>> to; I really don't know.
> 
> See above. The main issue is that HW folks haven't understood the actual
> use of MSIs, and persist in implementing them as an afterthought, based
> on some cascading interrupt controller design.
> 
> Sad. So sad.

For the record, below is the patch I had in mind:

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 8a3e872798f3..edfc95575a37 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -91,9 +91,11 @@ int msi_domain_set_affinity(struct irq_data *irq_data,
 {
        struct irq_data *parent = irq_data->parent_data;
        struct msi_msg msg;
-       int ret;
+       int ret = -EINVAL;
+
+       if (parent->chip->irq_set_affinity)
+               ret = parent->chip->irq_set_affinity(parent, mask, force);
 
-       ret = parent->chip->irq_set_affinity(parent, mask, force);
        if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
                BUG_ON(irq_chip_compose_msi_msg(irq_data, &msg));
                irq_chip_write_msi_msg(irq_data, &msg);



Then, it would be safe to remove "do-nothing" .irq_set_affinity
callbacks in drivers/pci/host

Regards.

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Bjorn Helgaas]

On Wed, May 31, 2017 at 04:18:44PM +0200, Mason wrote:
> On 24/05/2017 12:22, Marc Zyngier wrote:
> > On 24/05/17 11:00, Robin Murphy wrote:
> >> On 23/05/17 20:15, Mason wrote:
> >>> On 23/05/2017 20:03, Robin Murphy wrote:
> >>>> On 23/05/17 18:54, Mason wrote:
> >>>>> On 23/05/2017 19:03, Bjorn Helgaas wrote:
> >>>>>> On Wed, May 17, 2017 at 04:56:08PM +0200, Marc Gonzalez wrote:
> >>>>>>> On 20/04/2017 16:28, Marc Gonzalez wrote:
> >>>>>>>
> >>>>>>>> +static int tango_set_affinity(struct irq_data *data,
> >>>>>>>> +		const struct cpumask *mask, bool force)
> >>>>>>>> +{
> >>>>>>>> +	return -EINVAL;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static struct irq_chip tango_chip = {
> >>>>>>>> +	.irq_ack		= tango_ack,
> >>>>>>>> +	.irq_mask		= tango_mask,
> >>>>>>>> +	.irq_unmask		= tango_unmask,
> >>>>>>>> +	.irq_set_affinity	= tango_set_affinity,
> >>>>>>>> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
> >>>>>>>> +};
> >>>>>>>
> >>>>>>> Hmmm... I'm wondering why .irq_set_affinity is required.
> >>>>>>>
> >>>>>>> static int setup_affinity(struct irq_desc *desc, struct cpumask *mask)
> >>>>>>> first calls __irq_can_set_affinity() to check whether
> >>>>>>> desc->irq_data.chip->irq_set_affinity) exists.
> >>>>>>>
> >>>>>>> then calls irq_do_set_affinity(&desc->irq_data, mask, false);
> >>>>>>> which calls chip->irq_set_affinity(data, mask, force);
> >>>>>>> = msi_domain_set_affinity()
> >>>>>>> which calls parent->chip->irq_set_affinity() unconditionally.
> >>>>>>>
> >>>>>>> Would it make sense to test that the callback is implemented
> >>>>>>> before calling it?
> >>>>>>>
> >>>>>>>
> >>>>>>> [    0.723895] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> >>>>>>
> >>>>>> I'm not sure what you're asking.
> >>>>>>
> >>>>>> Is this a bug report for the v4 tango driver?
> >>>>>
> >>>>> No.
> >>>>>
> >>>>>> Or are you asking whether msi_domain_set_affinity() should be changed
> >>>>>> to check whether parent->chip->irq_set_affinity is implemented?
> >>>>>
> >>>>> Yes. The way things are implemented now, drivers are forced
> >>>>> to define an irq_set_affinity callback, even if it just returns
> >>>>> an error, otherwise, the kernel crashes, because of the
> >>>>> unconditional function pointer deref. 
> >>>>>
> >>>>>> msi_domain_set_affinity() has called parent->chip->irq_set_affinity()
> >>>>>> without checking since it was added in 2014 by f3cf8bb0d6c3 ("genirq: Add
> >>>>>> generic msi irq domain support"), so if there's a problem here, it's most
> >>>>>> likely in the tango code.
> >>>>>
> >>>>> The issue is having to define an "empty" function.
> >>>>> (Unnecessary code bloat and maintenance.)
> >>>>
> >>>> AFAICS, only one driver (other than this one) implements a "do nothing"
> >>>> set_affinity callback - everyone else who doesn't do anything hardware
> >>>> specific just passes it along via irq_chip_set_affinity_parent().
> >>>
> >>> I counted 4. Where did I mess up?
> >>>
> >>> advk_msi_set_affinity
> >>> altera_msi_set_affinity
> >>> nwl_msi_set_affinity
> >>> vmd_irq_set_affinity
> >>> tango_set_affinity
> >>
> >> Fair point - I went through drivers/irqchip and the various
> >> arch-specific instances and found ls_scfg_msi_set_affinity(), but
> >> somehow skipped over drivers/pci. Anyway, I think the question stands of
> >> why are these handful of drivers *not* using irq_chip_set_affinity_parent()?
> > 
> > Probably because they don't have a parent, in the hierarchical sense.
> > All they have is a chained interrupt (*puke*). Which implies that
> > changing the affinity of one MSI would affect all of them, completely
> > confusing unsuspecting userspace such as irqbalance.
> > 
> >> As an outsider, it naively seems logical that the affinity of an MSI
> >> which just gets translated to a wired interrupt should propagate through
> >> to the affinity of that wired interrupt, but maybe there are reasons not
> >> to; I really don't know.
> > 
> > See above. The main issue is that HW folks haven't understood the actual
> > use of MSIs, and persist in implementing them as an afterthought, based
> > on some cascading interrupt controller design.
> > 
> > Sad. So sad.
> 
> For the record, below is the patch I had in mind:
> 
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 8a3e872798f3..edfc95575a37 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -91,9 +91,11 @@ int msi_domain_set_affinity(struct irq_data *irq_data,
>  {
>         struct irq_data *parent = irq_data->parent_data;
>         struct msi_msg msg;
> -       int ret;
> +       int ret = -EINVAL;
> +
> +       if (parent->chip->irq_set_affinity)
> +               ret = parent->chip->irq_set_affinity(parent, mask, force);
>  
> -       ret = parent->chip->irq_set_affinity(parent, mask, force);
>         if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
>                 BUG_ON(irq_chip_compose_msi_msg(irq_data, &msg));
>                 irq_chip_write_msi_msg(irq_data, &msg);
> 
> 
> 
> Then, it would be safe to remove "do-nothing" .irq_set_affinity
> callbacks in drivers/pci/host

I assume it's obvious that nobody else is going to take this and run
with it, so nothing will happen with this unless you finish it up by
doing the cleanup (removing the "do-nothing" callbacks) and adding a
changelog and signed-off-by.

This would be more an IRQ patch than a PCI patch, but if I were
reviewing it, I would look for assurance that *all* the no-op
.irq_set_affinity callbacks were cleaned up, not just those in
drivers/pci/host.

Bjorn

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Mason]

On 31/05/2017 19:34, Bjorn Helgaas wrote:
> On Wed, May 31, 2017 at 04:18:44PM +0200, Mason wrote:
>
>> For the record, below is the patch I had in mind:
>>
>> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
>> index 8a3e872798f3..edfc95575a37 100644
>> --- a/kernel/irq/msi.c
>> +++ b/kernel/irq/msi.c
>> @@ -91,9 +91,11 @@ int msi_domain_set_affinity(struct irq_data *irq_data,
>>  {
>>         struct irq_data *parent = irq_data->parent_data;
>>         struct msi_msg msg;
>> -       int ret;
>> +       int ret = -EINVAL;
>> +
>> +       if (parent->chip->irq_set_affinity)
>> +               ret = parent->chip->irq_set_affinity(parent, mask, force);
>>  
>> -       ret = parent->chip->irq_set_affinity(parent, mask, force);
>>         if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
>>                 BUG_ON(irq_chip_compose_msi_msg(irq_data, &msg));
>>                 irq_chip_write_msi_msg(irq_data, &msg);
>>
>>
>>
>> Then, it would be safe to remove "do-nothing" .irq_set_affinity
>> callbacks in drivers/pci/host
> 
> I assume it's obvious that nobody else is going to take this and run
> with it, so nothing will happen with this unless you finish it up by
> doing the cleanup (removing the "do-nothing" callbacks) and adding a
> changelog and signed-off-by.

Smirk.

> This would be more an IRQ patch than a PCI patch, but if I were
> reviewing it, I would look for assurance that *all* the no-op
> .irq_set_affinity callbacks were cleaned up, not just those in
> drivers/pci/host.

Are you saying the patch is *wrong* if not all "do-nothing"
callbacks are cleaned up?

Regards.

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Bjorn Helgaas]

On Wed, May 31, 2017 at 08:49:04PM +0200, Mason wrote:
> On 31/05/2017 19:34, Bjorn Helgaas wrote:
> ...

> > This would be more an IRQ patch than a PCI patch, but if I were
> > reviewing it, I would look for assurance that *all* the no-op
> > .irq_set_affinity callbacks were cleaned up, not just those in
> > drivers/pci/host.
> 
> Are you saying the patch is *wrong* if not all "do-nothing"
> callbacks are cleaned up?

I'm saying that (1) this probably wouldn't be applied via the PCI
tree, and (2) if it *were* applied via PCI, I would ask that all the
no-op callbacks were cleaned up at the same time.

Huh, that sounds a lot like what I wrote above.  Was I unclear?

Bjorn

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Bjorn Helgaas]

On Wed, May 31, 2017 at 02:00:37PM -0500, Bjorn Helgaas wrote:
> On Wed, May 31, 2017 at 08:49:04PM +0200, Mason wrote:
> > On 31/05/2017 19:34, Bjorn Helgaas wrote:
> > ...
> 
> > > This would be more an IRQ patch than a PCI patch, but if I were
> > > reviewing it, I would look for assurance that *all* the no-op
> > > .irq_set_affinity callbacks were cleaned up, not just those in
> > > drivers/pci/host.
> > 
> > Are you saying the patch is *wrong* if not all "do-nothing"
> > callbacks are cleaned up?
> 
> I'm saying that (1) this probably wouldn't be applied via the PCI
> tree, and (2) if it *were* applied via PCI, I would ask that all the
> no-op callbacks were cleaned up at the same time.
> 
> Huh, that sounds a lot like what I wrote above.  Was I unclear?

I'm afraid this sounded snarky, which isn't my intention.  It seems
like there's a useful patch here, and I didn't want to see it get
ignored for lack of following the usual process.  If this is all
obvious to you, my apologies and please ignore my suggestion.

Bjorn

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Mason]

On 31/05/2017 21:12, Bjorn Helgaas wrote:
> On Wed, May 31, 2017 at 02:00:37PM -0500, Bjorn Helgaas wrote:
>> On Wed, May 31, 2017 at 08:49:04PM +0200, Mason wrote:
>>> On 31/05/2017 19:34, Bjorn Helgaas wrote:
>>> ...
>>
>>>> This would be more an IRQ patch than a PCI patch, but if I were
>>>> reviewing it, I would look for assurance that *all* the no-op
>>>> .irq_set_affinity callbacks were cleaned up, not just those in
>>>> drivers/pci/host.
>>>
>>> Are you saying the patch is *wrong* if not all "do-nothing"
>>> callbacks are cleaned up?
>>
>> I'm saying that (1) this probably wouldn't be applied via the PCI
>> tree, and (2) if it *were* applied via PCI, I would ask that all the
>> no-op callbacks were cleaned up at the same time.
>>
>> Huh, that sounds a lot like what I wrote above.  Was I unclear?
> 
> I'm afraid this sounded snarky, which isn't my intention.  It seems
> like there's a useful patch here, and I didn't want to see it get
> ignored for lack of following the usual process.  If this is all
> obvious to you, my apologies and please ignore my suggestion.

Thanks for clearing things up. I had indeed assumed from
your first reply that the patch was pointless.

Writing a script locating all candidates will be an
interesting exercise.

Regards.

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Bjorn Helgaas]

On Wed, May 31, 2017 at 09:27:50PM +0200, Mason wrote:
> On 31/05/2017 21:12, Bjorn Helgaas wrote:
> > On Wed, May 31, 2017 at 02:00:37PM -0500, Bjorn Helgaas wrote:
> >> On Wed, May 31, 2017 at 08:49:04PM +0200, Mason wrote:
> >>> On 31/05/2017 19:34, Bjorn Helgaas wrote:
> >>> ...
> >>
> >>>> This would be more an IRQ patch than a PCI patch, but if I were
> >>>> reviewing it, I would look for assurance that *all* the no-op
> >>>> .irq_set_affinity callbacks were cleaned up, not just those in
> >>>> drivers/pci/host.
> >>>
> >>> Are you saying the patch is *wrong* if not all "do-nothing"
> >>> callbacks are cleaned up?
> >>
> >> I'm saying that (1) this probably wouldn't be applied via the PCI
> >> tree, and (2) if it *were* applied via PCI, I would ask that all the
> >> no-op callbacks were cleaned up at the same time.
> >>
> >> Huh, that sounds a lot like what I wrote above.  Was I unclear?
> > 
> > I'm afraid this sounded snarky, which isn't my intention.  It seems
> > like there's a useful patch here, and I didn't want to see it get
> > ignored for lack of following the usual process.  If this is all
> > obvious to you, my apologies and please ignore my suggestion.
> 
> Thanks for clearing things up. I had indeed assumed from
> your first reply that the patch was pointless.
> 
> Writing a script locating all candidates will be an
> interesting exercise.

Cscope only sees 94 definitions of irq_set_affinity.  I know *I* could
never write a script faster than looking at them manually.  While
doing that, I noticed irq_chip_set_affinity_parent(), which is used in
14 cases and appears similar to your patch.

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Mason]

On 31/05/2017 22:04, Bjorn Helgaas wrote:

> Cscope only sees 94 definitions of irq_set_affinity.  I know *I* could
> never write a script faster than looking at them manually.

for F in $(find -name '*.c'); do
  if grep -q pci_msi_create_irq_domain $F; then
    grep 'irq_set_affinity\s*=' $F | cut -f2 -d= | tr -d ',;' | while read CALLBACK
    do
      Y=$(grep -A4 "$CALLBACK(struct" $F)
      echo $Y | grep static
    done
  fi
done

static int ls_scfg_msi_set_affinity(struct irq_data *irq_data, const struct cpumask *mask, bool force) { return -EINVAL; }
static int armada_370_xp_msi_set_affinity(struct irq_data *irq_data, const struct cpumask *mask, bool force) { return -EINVAL; }
static int armada_xp_set_affinity(struct irq_data *d, const struct cpumask *mask_val, bool force) { irq_hw_number_t hwirq = irqd_to_hwirq(d);
static int nwl_msi_set_affinity(struct irq_data *irq_data, const struct cpumask *mask, bool force) { return -EINVAL; }
static int hv_set_affinity(struct irq_data *data, const struct cpumask *dest, bool force) { struct irq_data *parent = data->parent_data;
static int xgene_msi_set_affinity(struct irq_data *irqdata, const struct cpumask *mask, bool force) { int target_cpu = cpumask_first(mask); int curr_cpu;
static int vmd_irq_set_affinity(struct irq_data *data, const struct cpumask *dest, bool force) { return -EINVAL; }
static int altera_msi_set_affinity(struct irq_data *irq_data, const struct cpumask *mask, bool force) { return -EINVAL; }
static int advk_msi_set_affinity(struct irq_data *irq_data, const struct cpumask *mask, bool force) { return -EINVAL; }
static int iproc_msi_irq_set_affinity(struct irq_data *data, const struct cpumask *mask, bool force) { struct iproc_msi *msi = irq_data_get_irq_chip_data(data);

> While doing that, I noticed irq_chip_set_affinity_parent(), which is
> used in 14 cases and appears similar to your patch.

Indeed. I suppose msi_domain_set_affinity() could look like below,
if we don't mind changing EINVAL to ENOSYS. What do you think?

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index ddc2f5427f75..d37d2ba790df 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -87,11 +87,10 @@ static inline void irq_chip_write_msi_msg(struct irq_data *data,
 int msi_domain_set_affinity(struct irq_data *irq_data,
                            const struct cpumask *mask, bool force)
 {
-       struct irq_data *parent = irq_data->parent_data;
        struct msi_msg msg;
        int ret;
 
-       ret = parent->chip->irq_set_affinity(parent, mask, force);
+       ret = irq_chip_set_affinity_parent(irq_data, mask, force);
        if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
                BUG_ON(irq_chip_compose_msi_msg(irq_data, &msg));
                irq_chip_write_msi_msg(irq_data, &msg);

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Marc Zyngier]

On 20/04/17 15:28, Marc Gonzalez wrote:
> The MSI controller in Tango supports 256 message-signaled interrupts,
> and a single doorbell address.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  drivers/pci/host/pcie-tango.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 232 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
> new file mode 100644
> index 000000000000..ada8d35066ab
> --- /dev/null
> +++ b/drivers/pci/host/pcie-tango.c

As Bjorn mentioned elsewhere, the ordering of the patch is backward.
Please have the PCIe host controller driver in the first patch, then the
MSI stuff. Otherwise, it is impossible to see how the various bits are
wired.

And please have a separate patch the the DT binding, which needs
separate Acks from the DT folks.

> @@ -0,0 +1,232 @@
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/pci-ecam.h>
> +#include <linux/delay.h>
> +#include <linux/msi.h>
> +
> +#define MSI_MAX 256
> +
> +struct tango_pcie {
> +	DECLARE_BITMAP(bitmap, MSI_MAX);
> +	spinlock_t lock;
> +	void __iomem *mux;
> +	void __iomem *msi_status;
> +	void __iomem *msi_enable;
> +	phys_addr_t msi_doorbell;

Init of these three fields should be in this patch.

> +	struct irq_domain *irq_dom;
> +	struct irq_domain *msi_dom;
> +	int irq;
> +};
> +
> +/*** MSI CONTROLLER SUPPORT ***/
> +
> +static void dispatch(struct tango_pcie *pcie, unsigned long status, int base)
> +{
> +	unsigned int pos, virq;
> +
> +	for_each_set_bit(pos, &status, 32) {
> +		virq = irq_find_mapping(pcie->irq_dom, base + pos);
> +		generic_handle_irq(virq);
> +	}
> +}
> +
> +static void tango_msi_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
> +	unsigned int status, base, pos = 0;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	while ((pos = find_next_bit(pcie->bitmap, MSI_MAX, pos)) < MSI_MAX) {
> +		base = round_down(pos, 32);
> +		status = readl_relaxed(pcie->msi_status + base / 8);
> +		dispatch(pcie, status, base);

Just inline dispatch here.

> +		pos = base + 32;
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void tango_ack(struct irq_data *data)
> +{
> +	u32 bit = BIT(data->hwirq);

How does this work when hwirq is >= 32 (from what I gather, it can range
from 0 to 255...).

> +	struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
> +
> +	writel_relaxed(bit, pcie->msi_status);
> +}
> +
> +static void update_msi_enable(struct irq_data *data, bool unmask)
> +{
> +	unsigned long flags;
> +	u32 val, bit = BIT(data->hwirq % 32);
> +	int byte_offset = (data->hwirq / 32) * 4;
> +	struct tango_pcie *pcie = data->chip_data;
> +
> +	spin_lock_irqsave(&pcie->lock, flags);
> +	val = readl_relaxed(pcie->msi_enable + byte_offset);

As I already mentioned in one of the previous series, this is a fairly
pointless MMIO access. You could maintain a SW version of the enable
register, update that copy and write it.

But that's only a performance optimization, and it won't affect the
behaviour. Your call.

> +	val = unmask ? val | bit : val & ~bit;
> +	writel_relaxed(val, pcie->msi_enable + byte_offset);
> +	spin_unlock_irqrestore(&pcie->lock, flags);
> +}
> +
> +static void tango_mask(struct irq_data *data)
> +{
> +	update_msi_enable(data, false);
> +}
> +
> +static void tango_unmask(struct irq_data *data)
> +{
> +	update_msi_enable(data, true);
> +}
> +
> +static int tango_set_affinity(struct irq_data *data,
> +		const struct cpumask *mask, bool force)
> +{
> +	return -EINVAL;
> +}
> +
> +static void tango_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> +	struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
> +
> +	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
> +	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
> +	msg->data = data->hwirq;
> +}
> +
> +static struct irq_chip tango_chip = {
> +	.irq_ack		= tango_ack,
> +	.irq_mask		= tango_mask,
> +	.irq_unmask		= tango_unmask,
> +	.irq_set_affinity	= tango_set_affinity,
> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
> +};
> +
> +static void msi_ack(struct irq_data *data)
> +{
> +	irq_chip_ack_parent(data);
> +}
> +
> +static void msi_mask(struct irq_data *data)
> +{
> +	pci_msi_mask_irq(data);
> +	irq_chip_mask_parent(data);
> +}
> +
> +static void msi_unmask(struct irq_data *data)
> +{
> +	pci_msi_unmask_irq(data);
> +	irq_chip_unmask_parent(data);
> +}
> +
> +static struct irq_chip msi_chip = {
> +	.name = "MSI",
> +	.irq_ack = msi_ack,
> +	.irq_mask = msi_mask,
> +	.irq_unmask = msi_unmask,
> +};
> +
> +static struct msi_domain_info msi_dom_info = {
> +	.flags	= MSI_FLAG_PCI_MSIX
> +		| MSI_FLAG_USE_DEF_DOM_OPS
> +		| MSI_FLAG_USE_DEF_CHIP_OPS,
> +	.chip	= &msi_chip,
> +};
> +
> +static int find_free_msi(struct irq_domain *dom, unsigned int virq)
> +{
> +	unsigned int pos;
> +	struct tango_pcie *pcie = dom->host_data;
> +
> +	pos = find_first_zero_bit(pcie->bitmap, MSI_MAX);
> +	if (pos >= MSI_MAX)
> +		return -ENOSPC;
> +	__set_bit(pos, pcie->bitmap);
> +
> +	irq_domain_set_info(dom, virq, pos, &tango_chip, pcie,
> +			handle_edge_irq, NULL, NULL);
> +
> +	return 0;
> +}
> +
> +static int tango_irq_domain_alloc(struct irq_domain *dom,
> +		unsigned int virq, unsigned int nr_irqs, void *args)
> +{
> +	int err;
> +	unsigned long flags;
> +	struct tango_pcie *pcie = dom->host_data;
> +
> +	spin_lock_irqsave(&pcie->lock, flags);
> +	err = find_free_msi(dom, virq);

Just inline find_free_msi here. it is not called from anywhere else, and
seeing the locking close to the use of the bitmap is definitely better.

> +	spin_unlock_irqrestore(&pcie->lock, flags);
> +
> +	return err;
> +}
> +
> +static void tango_irq_domain_free(struct irq_domain *dom,
> +		unsigned int virq, unsigned int nr_irqs)
> +{
> +	unsigned long flags;
> +	struct irq_data *data = irq_domain_get_irq_data(dom, virq);
> +	struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
> +
> +	spin_lock_irqsave(&pcie->lock, flags);
> +	__clear_bit(data->hwirq, pcie->bitmap);
> +	spin_unlock_irqrestore(&pcie->lock, flags);
> +}
> +
> +static const struct irq_domain_ops irq_dom_ops = {
> +	.alloc	= tango_irq_domain_alloc,
> +	.free	= tango_irq_domain_free,
> +};
> +
> +static int tango_msi_remove(struct platform_device *pdev)
> +{
> +	struct tango_pcie *pcie = platform_get_drvdata(pdev);
> +
> +	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
> +	irq_domain_remove(pcie->msi_dom);
> +	irq_domain_remove(pcie->irq_dom);
> +
> +	return 0;
> +}
> +
> +static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie)
> +{
> +	int i, virq;
> +	struct irq_domain *msi_dom, *irq_dom;
> +	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
> +
> +	spin_lock_init(&pcie->lock);
> +	for (i = 0; i < MSI_MAX / 32; ++i)
> +		writel_relaxed(0, pcie->msi_enable + i * 4);
> +
> +	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &irq_dom_ops, pcie);
> +	if (!irq_dom) {
> +		pr_err("Failed to create IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_dom_info, irq_dom);
> +	if (!msi_dom) {
> +		pr_err("Failed to create MSI domain\n");
> +		irq_domain_remove(irq_dom);
> +		return -ENOMEM;
> +	}
> +
> +	virq = platform_get_irq(pdev, 1);
> +	if (virq <= 0) {
> +		pr_err("Failed to map IRQ\n");
> +		irq_domain_remove(msi_dom);
> +		irq_domain_remove(irq_dom);
> +		return -ENXIO;
> +	}

Maybe start the probe with this. No need to start allocating data
structures if the DT is wrong.

> +
> +	pcie->irq_dom = irq_dom;
> +	pcie->msi_dom = msi_dom;
> +	pcie->irq = virq;
> +	irq_set_chained_handler_and_data(virq, tango_msi_isr, pcie);
> +
> +	return 0;
> +}
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

[Marc Gonzalez]

On 25/05/2017 10:37, Marc Zyngier wrote:

> On 20/04/17 15:28, Marc Gonzalez wrote:
>
>> The MSI controller in Tango supports 256 message-signaled interrupts,
>> and a single doorbell address.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>>  drivers/pci/host/pcie-tango.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 232 insertions(+)
>>
>> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
>> new file mode 100644
>> index 000000000000..ada8d35066ab
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-tango.c
> 
> As Bjorn mentioned elsewhere, the ordering of the patch is backward.
> Please have the PCIe host controller driver in the first patch, then the
> MSI stuff. Otherwise, it is impossible to see how the various bits are
> wired.

Done.

> And please have a separate patch the the DT binding, which needs
> separate Acks from the DT folks.

Done.

>> @@ -0,0 +1,232 @@
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/pci-ecam.h>
>> +#include <linux/delay.h>
>> +#include <linux/msi.h>
>> +
>> +#define MSI_MAX 256
>> +
>> +struct tango_pcie {
>> +	DECLARE_BITMAP(bitmap, MSI_MAX);
>> +	spinlock_t lock;
>> +	void __iomem *mux;
>> +	void __iomem *msi_status;
>> +	void __iomem *msi_enable;
>> +	phys_addr_t msi_doorbell;
> 
> Init of these three fields should be in this patch.

Done.

>> +	struct irq_domain *irq_dom;
>> +	struct irq_domain *msi_dom;
>> +	int irq;
>> +};
>> +
>> +/*** MSI CONTROLLER SUPPORT ***/
>> +
>> +static void dispatch(struct tango_pcie *pcie, unsigned long status, int base)
>> +{
>> +	unsigned int pos, virq;
>> +
>> +	for_each_set_bit(pos, &status, 32) {
>> +		virq = irq_find_mapping(pcie->irq_dom, base + pos);
>> +		generic_handle_irq(virq);
>> +	}
>> +}
>> +
>> +static void tango_msi_isr(struct irq_desc *desc)
>> +{
>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>> +	struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
>> +	unsigned int status, base, pos = 0;
>> +
>> +	chained_irq_enter(chip, desc);
>> +
>> +	while ((pos = find_next_bit(pcie->bitmap, MSI_MAX, pos)) < MSI_MAX) {
>> +		base = round_down(pos, 32);
>> +		status = readl_relaxed(pcie->msi_status + base / 8);
>> +		dispatch(pcie, status, base);
> 
> Just inline dispatch here.

Done.

>> +		pos = base + 32;
>> +	}
>> +
>> +	chained_irq_exit(chip, desc);
>> +}
>> +
>> +static void tango_ack(struct irq_data *data)
>> +{
>> +	u32 bit = BIT(data->hwirq);
> 
> How does this work when hwirq is >= 32 (from what I gather, it can range
> from 0 to 255...).

It blows up. Fixed now.

>> +	struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
>> +
>> +	writel_relaxed(bit, pcie->msi_status);
>> +}
>> +
>> +static void update_msi_enable(struct irq_data *data, bool unmask)
>> +{
>> +	unsigned long flags;
>> +	u32 val, bit = BIT(data->hwirq % 32);
>> +	int byte_offset = (data->hwirq / 32) * 4;
>> +	struct tango_pcie *pcie = data->chip_data;
>> +
>> +	spin_lock_irqsave(&pcie->lock, flags);
>> +	val = readl_relaxed(pcie->msi_enable + byte_offset);
> 
> As I already mentioned in one of the previous series, this is a fairly
> pointless MMIO access. You could maintain a SW version of the enable
> register, update that copy and write it.
> 
> But that's only a performance optimization, and it won't affect the
> behaviour. Your call.

Thanks for pointing it out.
I'll implement the optimization in a future patch.

>> +	val = unmask ? val | bit : val & ~bit;
>> +	writel_relaxed(val, pcie->msi_enable + byte_offset);
>> +	spin_unlock_irqrestore(&pcie->lock, flags);
>> +}
>> +
>> +static void tango_mask(struct irq_data *data)
>> +{
>> +	update_msi_enable(data, false);
>> +}
>> +
>> +static void tango_unmask(struct irq_data *data)
>> +{
>> +	update_msi_enable(data, true);
>> +}
>> +
>> +static int tango_set_affinity(struct irq_data *data,
>> +		const struct cpumask *mask, bool force)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static void tango_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>> +{
>> +	struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
>> +
>> +	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
>> +	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
>> +	msg->data = data->hwirq;
>> +}
>> +
>> +static struct irq_chip tango_chip = {
>> +	.irq_ack		= tango_ack,
>> +	.irq_mask		= tango_mask,
>> +	.irq_unmask		= tango_unmask,
>> +	.irq_set_affinity	= tango_set_affinity,
>> +	.irq_compose_msi_msg	= tango_compose_msi_msg,
>> +};
>> +
>> +static void msi_ack(struct irq_data *data)
>> +{
>> +	irq_chip_ack_parent(data);
>> +}
>> +
>> +static void msi_mask(struct irq_data *data)
>> +{
>> +	pci_msi_mask_irq(data);
>> +	irq_chip_mask_parent(data);
>> +}
>> +
>> +static void msi_unmask(struct irq_data *data)
>> +{
>> +	pci_msi_unmask_irq(data);
>> +	irq_chip_unmask_parent(data);
>> +}
>> +
>> +static struct irq_chip msi_chip = {
>> +	.name = "MSI",
>> +	.irq_ack = msi_ack,
>> +	.irq_mask = msi_mask,
>> +	.irq_unmask = msi_unmask,
>> +};
>> +
>> +static struct msi_domain_info msi_dom_info = {
>> +	.flags	= MSI_FLAG_PCI_MSIX
>> +		| MSI_FLAG_USE_DEF_DOM_OPS
>> +		| MSI_FLAG_USE_DEF_CHIP_OPS,
>> +	.chip	= &msi_chip,
>> +};
>> +
>> +static int find_free_msi(struct irq_domain *dom, unsigned int virq)
>> +{
>> +	unsigned int pos;
>> +	struct tango_pcie *pcie = dom->host_data;
>> +
>> +	pos = find_first_zero_bit(pcie->bitmap, MSI_MAX);
>> +	if (pos >= MSI_MAX)
>> +		return -ENOSPC;
>> +	__set_bit(pos, pcie->bitmap);
>> +
>> +	irq_domain_set_info(dom, virq, pos, &tango_chip, pcie,
>> +			handle_edge_irq, NULL, NULL);
>> +
>> +	return 0;
>> +}
>> +
>> +static int tango_irq_domain_alloc(struct irq_domain *dom,
>> +		unsigned int virq, unsigned int nr_irqs, void *args)
>> +{
>> +	int err;
>> +	unsigned long flags;
>> +	struct tango_pcie *pcie = dom->host_data;
>> +
>> +	spin_lock_irqsave(&pcie->lock, flags);
>> +	err = find_free_msi(dom, virq);
> 
> Just inline find_free_msi here. it is not called from anywhere else, and
> seeing the locking close to the use of the bitmap is definitely better.

Done.

>> +	spin_unlock_irqrestore(&pcie->lock, flags);
>> +
>> +	return err;
>> +}
>> +
>> +static void tango_irq_domain_free(struct irq_domain *dom,
>> +		unsigned int virq, unsigned int nr_irqs)
>> +{
>> +	unsigned long flags;
>> +	struct irq_data *data = irq_domain_get_irq_data(dom, virq);
>> +	struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
>> +
>> +	spin_lock_irqsave(&pcie->lock, flags);
>> +	__clear_bit(data->hwirq, pcie->bitmap);
>> +	spin_unlock_irqrestore(&pcie->lock, flags);
>> +}
>> +
>> +static const struct irq_domain_ops irq_dom_ops = {
>> +	.alloc	= tango_irq_domain_alloc,
>> +	.free	= tango_irq_domain_free,
>> +};
>> +
>> +static int tango_msi_remove(struct platform_device *pdev)
>> +{
>> +	struct tango_pcie *pcie = platform_get_drvdata(pdev);
>> +
>> +	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
>> +	irq_domain_remove(pcie->msi_dom);
>> +	irq_domain_remove(pcie->irq_dom);
>> +
>> +	return 0;
>> +}
>> +
>> +static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie)
>> +{
>> +	int i, virq;
>> +	struct irq_domain *msi_dom, *irq_dom;
>> +	struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
>> +
>> +	spin_lock_init(&pcie->lock);
>> +	for (i = 0; i < MSI_MAX / 32; ++i)
>> +		writel_relaxed(0, pcie->msi_enable + i * 4);
>> +
>> +	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &irq_dom_ops, pcie);
>> +	if (!irq_dom) {
>> +		pr_err("Failed to create IRQ domain\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	msi_dom = pci_msi_create_irq_domain(fwnode, &msi_dom_info, irq_dom);
>> +	if (!msi_dom) {
>> +		pr_err("Failed to create MSI domain\n");
>> +		irq_domain_remove(irq_dom);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	virq = platform_get_irq(pdev, 1);
>> +	if (virq <= 0) {
>> +		pr_err("Failed to map IRQ\n");
>> +		irq_domain_remove(msi_dom);
>> +		irq_domain_remove(irq_dom);
>> +		return -ENXIO;
>> +	}
> 
> Maybe start the probe with this. No need to start allocating data
> structures if the DT is wrong.

Done.

[PATCH v4 2/2] PCI: Add tango PCIe host bridge support

[Marc Gonzalez]

This driver is required to work around several hardware bugs in the
PCIe controller.

NB: Revision 1 does not support legacy interrupts, or IO space.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 ++++++++
 drivers/pci/host/Kconfig                             |   8 ++
 drivers/pci/host/Makefile                            |   1 +
 drivers/pci/host/pcie-tango.c                        | 161 +++++++++++++++++++++++++++++++++++++++
 include/linux/pci_ids.h                              |   2 +
 5 files changed, 204 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt
new file mode 100644
index 000000000000..3353b4e77309
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
@@ -0,0 +1,32 @@
+Sigma Designs Tango PCIe controller
+
+Required properties:
+
+- compatible: "sigma,smp8759-pcie"
+- reg: address/size of PCI configuration space, address/size of register area
+- device_type: "pci"
+- #size-cells: <2>
+- #address-cells: <3>
+- #interrupt-cells: <1>
+- ranges: translation from system to bus addresses
+- interrupts: spec for misc interrupts, spec for MSI
+- msi-controller
+
+http://elinux.org/Device_Tree_Usage#PCI_Address_Translation
+http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
+
+Example:
+
+	pcie@2e000 {
+		compatible = "sigma,smp8759-pcie";
+		reg = <0x50000000 SZ_4M>, <0x2e000 0x100>;
+		device_type = "pci";
+		#size-cells = <2>;
+		#address-cells = <3>;
+		#interrupt-cells = <1>;
+		ranges = <0x02000000 0x0 0x00400000  0x50400000  0x0 SZ_60M>;
+		msi-controller;
+		interrupts =
+			<54 IRQ_TYPE_LEVEL_HIGH>, /* misc interrupts */
+			<55 IRQ_TYPE_LEVEL_HIGH>; /* MSI */
+	};
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index d7e7c0a827c3..5183d9095c3a 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -285,6 +285,14 @@ config PCIE_ROCKCHIP
 	  There is 1 internal PCIe port available to support GEN2 with
 	  4 slots.
 
+config PCIE_TANGO
+	bool "Tango PCIe controller"
+	depends on ARCH_TANGO && PCI_MSI && OF
+	select PCI_HOST_COMMON
+	help
+	  Say Y here to enable PCIe controller support for Sigma Designs
+	  Tango systems, such as SMP8759 and later chips.
+
 config VMD
 	depends on PCI_MSI && X86_64
 	tristate "Intel Volume Management Device Driver"
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 084cb4983645..fc7ea90196f3 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -32,4 +32,5 @@ obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
 obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
+obj-$(CONFIG_PCIE_TANGO) += pcie-tango.o
 obj-$(CONFIG_VMD) += vmd.o
diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
index ada8d35066ab..7eb74e82d325 100644
--- a/drivers/pci/host/pcie-tango.c
+++ b/drivers/pci/host/pcie-tango.c
@@ -230,3 +230,164 @@ static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie
 
 	return 0;
 }
+
+/*** HOST BRIDGE SUPPORT ***/
+
+static int smp8759_config_read(struct pci_bus *bus,
+		unsigned int devfn, int where, int size, u32 *val)
+{
+	int ret;
+	struct pci_config_window *cfg = bus->sysdata;
+	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
+
+	/*
+	 * QUIRK #1
+	 * Reads in configuration space outside devfn 0 return garbage.
+	 */
+	if (devfn != 0)
+		return PCIBIOS_FUNC_NOT_SUPPORTED;
+
+	/*
+	 * QUIRK #2
+	 * Unfortunately, config and mem spaces are muxed.
+	 * Linux does not support such a setting, since drivers are free
+	 * to access mem space directly, at any time.
+	 * Therefore, we can only PRAY that config and mem space accesses
+	 * NEVER occur concurrently.
+	 */
+	writel_relaxed(1, pcie->mux);
+	ret = pci_generic_config_read(bus, devfn, where, size, val);
+	writel_relaxed(0, pcie->mux);
+
+	return ret;
+}
+
+static int smp8759_config_write(struct pci_bus *bus,
+		unsigned int devfn, int where, int size, u32 val)
+{
+	int ret;
+	struct pci_config_window *cfg = bus->sysdata;
+	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
+
+	writel_relaxed(1, pcie->mux);
+	ret = pci_generic_config_write(bus, devfn, where, size, val);
+	writel_relaxed(0, pcie->mux);
+
+	return ret;
+}
+
+static struct pci_ecam_ops smp8759_ecam_ops = {
+	.bus_shift	= 20,
+	.pci_ops	= {
+		.map_bus	= pci_ecam_map_bus,
+		.read		= smp8759_config_read,
+		.write		= smp8759_config_write,
+	}
+};
+
+static const struct of_device_id tango_pcie_ids[] = {
+	{ .compatible = "sigma,smp8759-pcie" },
+	{ /* sentinel */ },
+};
+
+static int tango_check_pcie_link(void __iomem *test_out)
+{
+	int i;
+
+	writel_relaxed(16, test_out);
+	for (i = 0; i < 10; ++i) {
+		u32 ltssm_state = readl_relaxed(test_out) >> 8;
+		if ((ltssm_state & 0x1f) == 0xf) /* L0 */
+			return 0;
+		usleep_range(3000, 4000);
+	}
+
+	return -ENODEV;
+}
+
+static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
+{
+	pcie->mux		= base + 0x48;
+	pcie->msi_status	= base + 0x80;
+	pcie->msi_enable	= base + 0xa0;
+	pcie->msi_doorbell	= 0xa0000000 + 0x2e07c;
+
+	return tango_check_pcie_link(base + 0x74);
+}
+
+static int tango_pcie_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	void __iomem *base;
+	struct resource *res;
+	struct tango_pcie *pcie;
+	struct device *dev = &pdev->dev;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pcie);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	if (of_device_is_compatible(dev->of_node, "sigma,smp8759-pcie"))
+		ret = smp8759_init(pcie, base);
+
+	if (ret)
+		return ret;
+
+	ret = tango_msi_probe(pdev, pcie);
+	if (ret)
+		return ret;
+
+	return pci_host_common_probe(pdev, &smp8759_ecam_ops);
+}
+
+static int tango_pcie_remove(struct platform_device *pdev)
+{
+	return tango_msi_remove(pdev);
+}
+
+static struct platform_driver tango_pcie_driver = {
+	.probe	= tango_pcie_probe,
+	.remove	= tango_pcie_remove,
+	.driver	= {
+		.name = KBUILD_MODNAME,
+		.of_match_table = tango_pcie_ids,
+	},
+};
+
+builtin_platform_driver(tango_pcie_driver);
+
+/*
+ * QUIRK #3
+ * The root complex advertizes the wrong device class.
+ * Header Type 1 is for PCI-to-PCI bridges.
+ */
+static void tango_fixup_class(struct pci_dev *dev)
+{
+	dev->class = PCI_CLASS_BRIDGE_PCI << 8;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x24, tango_fixup_class);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x28, tango_fixup_class);
+
+/*
+ * QUIRK #4
+ * The root complex exposes a "fake" BAR, which is used to filter
+ * bus-to-system accesses. Only accesses within the range defined
+ * by this BAR are forwarded to the host, others are ignored.
+ *
+ * By default, the DMA framework expects an identity mapping,
+ * and DRAM0 is mapped at 0x80000000.
+ */
+static void tango_fixup_bar(struct pci_dev *dev)
+{
+	dev->non_compliant_bars = true;
+	pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, 0x80000000);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x24, tango_fixup_bar);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x28, tango_fixup_bar);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index f020ab4079d3..b577dbe46f8f 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -1369,6 +1369,8 @@
 #define PCI_DEVICE_ID_TTI_HPT374	0x0008
 #define PCI_DEVICE_ID_TTI_HPT372N	0x0009	/* apparently a 372N variant? */
 
+#define PCI_VENDOR_ID_SIGMA		0x1105
+
 #define PCI_VENDOR_ID_VIA		0x1106
 #define PCI_DEVICE_ID_VIA_8763_0	0x0198
 #define PCI_DEVICE_ID_VIA_8380_0	0x0204
-- 
2.11.0

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

[Bjorn Helgaas]

On Thu, Apr 20, 2017 at 04:31:25PM +0200, Marc Gonzalez wrote:
> This driver is required to work around several hardware bugs in the
> PCIe controller.
> 
> NB: Revision 1 does not support legacy interrupts, or IO space.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 ++++++++
>  drivers/pci/host/Kconfig                             |   8 ++
>  drivers/pci/host/Makefile                            |   1 +
>  drivers/pci/host/pcie-tango.c                        | 161 +++++++++++++++++++++++++++++++++++++++
>  include/linux/pci_ids.h                              |   2 +
>  5 files changed, 204 insertions(+)
> ...

> +static int smp8759_config_read(struct pci_bus *bus,
> +		unsigned int devfn, int where, int size, u32 *val)
> +{
> +	int ret;
> +	struct pci_config_window *cfg = bus->sysdata;
> +	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
> +
> +	/*
> +	 * QUIRK #1
> +	 * Reads in configuration space outside devfn 0 return garbage.
> +	 */
> +	if (devfn != 0)
> +		return PCIBIOS_FUNC_NOT_SUPPORTED;
> +
> +	/*
> +	 * QUIRK #2
> +	 * Unfortunately, config and mem spaces are muxed.
> +	 * Linux does not support such a setting, since drivers are free
> +	 * to access mem space directly, at any time.
> +	 * Therefore, we can only PRAY that config and mem space accesses
> +	 * NEVER occur concurrently.
> +	 */
> +	writel_relaxed(1, pcie->mux);
> +	ret = pci_generic_config_read(bus, devfn, where, size, val);
> +	writel_relaxed(0, pcie->mux);

This is a major issue and possibly even a security problem.
Unprivileged users can cause config accesses via lspci/setpci.

I don't have a good suggestion for addressing it, but if you can't
make this work reliably, you need at least a dev_err() in the probe
function and probably a taint of the kernel (see add_taint()).

> +	return ret;
> +}
> +
> +static int smp8759_config_write(struct pci_bus *bus,
> +		unsigned int devfn, int where, int size, u32 val)
> +{
> +	int ret;
> +	struct pci_config_window *cfg = bus->sysdata;
> +	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
> +
> +	writel_relaxed(1, pcie->mux);
> +	ret = pci_generic_config_write(bus, devfn, where, size, val);
> +	writel_relaxed(0, pcie->mux);
> +
> +	return ret;
> +}
> +
> +static struct pci_ecam_ops smp8759_ecam_ops = {
> +	.bus_shift	= 20,
> +	.pci_ops	= {
> +		.map_bus	= pci_ecam_map_bus,
> +		.read		= smp8759_config_read,
> +		.write		= smp8759_config_write,
> +	}
> +};

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

[Mason]

On 23/05/2017 19:24, Bjorn Helgaas wrote:
> On Thu, Apr 20, 2017 at 04:31:25PM +0200, Marc Gonzalez wrote:
>> This driver is required to work around several hardware bugs in the
>> PCIe controller.
>>
>> NB: Revision 1 does not support legacy interrupts, or IO space.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 ++++++++
>>  drivers/pci/host/Kconfig                             |   8 ++
>>  drivers/pci/host/Makefile                            |   1 +
>>  drivers/pci/host/pcie-tango.c                        | 161 +++++++++++++++++++++++++++++++++++++++
>>  include/linux/pci_ids.h                              |   2 +
>>  5 files changed, 204 insertions(+)
>> ...
> 
>> +static int smp8759_config_read(struct pci_bus *bus,
>> +		unsigned int devfn, int where, int size, u32 *val)
>> +{
>> +	int ret;
>> +	struct pci_config_window *cfg = bus->sysdata;
>> +	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
>> +
>> +	/*
>> +	 * QUIRK #1
>> +	 * Reads in configuration space outside devfn 0 return garbage.
>> +	 */
>> +	if (devfn != 0)
>> +		return PCIBIOS_FUNC_NOT_SUPPORTED;
>> +
>> +	/*
>> +	 * QUIRK #2
>> +	 * Unfortunately, config and mem spaces are muxed.
>> +	 * Linux does not support such a setting, since drivers are free
>> +	 * to access mem space directly, at any time.
>> +	 * Therefore, we can only PRAY that config and mem space accesses
>> +	 * NEVER occur concurrently.
>> +	 */
>> +	writel_relaxed(1, pcie->mux);
>> +	ret = pci_generic_config_read(bus, devfn, where, size, val);
>> +	writel_relaxed(0, pcie->mux);
> 
> This is a major issue and possibly even a security problem.
> Unprivileged users can cause config accesses via lspci/setpci.

Since the host bridge doesn't support hotplug in any way,
how about setting a flag once enumeration is complete,
to prevent all future config accesses?

> I don't have a good suggestion for addressing it, but if you can't
> make this work reliably, you need at least a dev_err() in the probe
> function and probably a taint of the kernel (see add_taint()).

There is a chance that the issue will get fixed in rev2
of the bridge. But obviously, that won't help for rev1
already in the wild.

Regards.

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

[Bjorn Helgaas]

On Tue, May 23, 2017 at 07:43:16PM +0200, Mason wrote:
> On 23/05/2017 19:24, Bjorn Helgaas wrote:
> > On Thu, Apr 20, 2017 at 04:31:25PM +0200, Marc Gonzalez wrote:
> >> This driver is required to work around several hardware bugs in the
> >> PCIe controller.
> >>
> >> NB: Revision 1 does not support legacy interrupts, or IO space.
> >>
> >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> >> ---
> >>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 ++++++++
> >>  drivers/pci/host/Kconfig                             |   8 ++
> >>  drivers/pci/host/Makefile                            |   1 +
> >>  drivers/pci/host/pcie-tango.c                        | 161 +++++++++++++++++++++++++++++++++++++++
> >>  include/linux/pci_ids.h                              |   2 +
> >>  5 files changed, 204 insertions(+)
> >> ...
> > 
> >> +static int smp8759_config_read(struct pci_bus *bus,
> >> +		unsigned int devfn, int where, int size, u32 *val)
> >> +{
> >> +	int ret;
> >> +	struct pci_config_window *cfg = bus->sysdata;
> >> +	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
> >> +
> >> +	/*
> >> +	 * QUIRK #1
> >> +	 * Reads in configuration space outside devfn 0 return garbage.
> >> +	 */
> >> +	if (devfn != 0)
> >> +		return PCIBIOS_FUNC_NOT_SUPPORTED;
> >> +
> >> +	/*
> >> +	 * QUIRK #2
> >> +	 * Unfortunately, config and mem spaces are muxed.
> >> +	 * Linux does not support such a setting, since drivers are free
> >> +	 * to access mem space directly, at any time.
> >> +	 * Therefore, we can only PRAY that config and mem space accesses
> >> +	 * NEVER occur concurrently.
> >> +	 */
> >> +	writel_relaxed(1, pcie->mux);
> >> +	ret = pci_generic_config_read(bus, devfn, where, size, val);
> >> +	writel_relaxed(0, pcie->mux);
> > 
> > This is a major issue and possibly even a security problem.
> > Unprivileged users can cause config accesses via lspci/setpci.
> 
> Since the host bridge doesn't support hotplug in any way,
> how about setting a flag once enumeration is complete,
> to prevent all future config accesses?

I thought about that, too, but I'm not sure it will work becuase I
think drivers will need to do at least a few config accesses, e.g.,
pci_enable_device() may write PCI_COMMAND to set PCI_COMMAND_MEMORY,
it may read PCI_INTERRUPT_PIN, etc.  And MSI setup requires config
access, too.

> > I don't have a good suggestion for addressing it, but if you can't
> > make this work reliably, you need at least a dev_err() in the probe
> > function and probably a taint of the kernel (see add_taint()).
> 
> There is a chance that the issue will get fixed in rev2
> of the bridge. But obviously, that won't help for rev1
> already in the wild.

Hopefully there will be a way to distinguish rev2 from rev1.

Bjorn

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

[Mason]

On 23/05/2017 20:35, Bjorn Helgaas wrote:
> On Tue, May 23, 2017 at 07:43:16PM +0200, Mason wrote:
>> On 23/05/2017 19:24, Bjorn Helgaas wrote:
>>> On Thu, Apr 20, 2017 at 04:31:25PM +0200, Marc Gonzalez wrote:
>>>> This driver is required to work around several hardware bugs in the
>>>> PCIe controller.
>>>>
>>>> NB: Revision 1 does not support legacy interrupts, or IO space.
>>>>
>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 ++++++++
>>>>  drivers/pci/host/Kconfig                             |   8 ++
>>>>  drivers/pci/host/Makefile                            |   1 +
>>>>  drivers/pci/host/pcie-tango.c                        | 161 +++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/pci_ids.h                              |   2 +
>>>>  5 files changed, 204 insertions(+)
>>>> ...
>>>
>>>> +static int smp8759_config_read(struct pci_bus *bus,
>>>> +		unsigned int devfn, int where, int size, u32 *val)
>>>> +{
>>>> +	int ret;
>>>> +	struct pci_config_window *cfg = bus->sysdata;
>>>> +	struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
>>>> +
>>>> +	/*
>>>> +	 * QUIRK #1
>>>> +	 * Reads in configuration space outside devfn 0 return garbage.
>>>> +	 */
>>>> +	if (devfn != 0)
>>>> +		return PCIBIOS_FUNC_NOT_SUPPORTED;
>>>> +
>>>> +	/*
>>>> +	 * QUIRK #2
>>>> +	 * Unfortunately, config and mem spaces are muxed.
>>>> +	 * Linux does not support such a setting, since drivers are free
>>>> +	 * to access mem space directly, at any time.
>>>> +	 * Therefore, we can only PRAY that config and mem space accesses
>>>> +	 * NEVER occur concurrently.
>>>> +	 */
>>>> +	writel_relaxed(1, pcie->mux);
>>>> +	ret = pci_generic_config_read(bus, devfn, where, size, val);
>>>> +	writel_relaxed(0, pcie->mux);
>>>
>>> This is a major issue and possibly even a security problem.
>>> Unprivileged users can cause config accesses via lspci/setpci.
>>
>> Since the host bridge doesn't support hotplug in any way,
>> how about setting a flag once enumeration is complete,
>> to prevent all future config accesses?
> 
> I thought about that, too, but I'm not sure it will work becuase I
> think drivers will need to do at least a few config accesses, e.g.,
> pci_enable_device() may write PCI_COMMAND to set PCI_COMMAND_MEMORY,
> it may read PCI_INTERRUPT_PIN, etc.  And MSI setup requires config
> access, too.
> 
>>> I don't have a good suggestion for addressing it, but if you can't
>>> make this work reliably, you need at least a dev_err() in the probe
>>> function and probably a taint of the kernel (see add_taint()).
>>
>> There is a chance that the issue will get fixed in rev2
>> of the bridge. But obviously, that won't help for rev1
>> already in the wild.
> 
> Hopefully there will be a way to distinguish rev2 from rev1.

rev2 is embedded in a new SoC, so the DT node will specify a
different compatible string. A "preview" of the intent was
given in an older patch:
"[RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge"

Regards.

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

[Marc Zyngier]

On 20/04/17 15:31, Marc Gonzalez wrote:
> This driver is required to work around several hardware bugs in the
> PCIe controller.
> 
> NB: Revision 1 does not support legacy interrupts, or IO space.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 ++++++++
>  drivers/pci/host/Kconfig                             |   8 ++
>  drivers/pci/host/Makefile                            |   1 +
>  drivers/pci/host/pcie-tango.c                        | 161 +++++++++++++++++++++++++++++++++++++++
>  include/linux/pci_ids.h                              |   2 +
>  5 files changed, 204 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt
> new file mode 100644
> index 000000000000..3353b4e77309
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
> @@ -0,0 +1,32 @@
> +Sigma Designs Tango PCIe controller
> +
> +Required properties:
> +
> +- compatible: "sigma,smp8759-pcie"
> +- reg: address/size of PCI configuration space, address/size of register area
> +- device_type: "pci"
> +- #size-cells: <2>
> +- #address-cells: <3>
> +- #interrupt-cells: <1>

What is the point of having an #interrupt-cells when this is *not* an
interrupt controller (as it doesn't support legacy interrupts)?

> +- ranges: translation from system to bus addresses
> +- interrupts: spec for misc interrupts, spec for MSI
> +- msi-controller
> +
> +http://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> +http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
> +
> +Example:
> +
> +	pcie@2e000 {
> +		compatible = "sigma,smp8759-pcie";
> +		reg = <0x50000000 SZ_4M>, <0x2e000 0x100>;
> +		device_type = "pci";
> +		#size-cells = <2>;
> +		#address-cells = <3>;
> +		#interrupt-cells = <1>;
> +		ranges = <0x02000000 0x0 0x00400000  0x50400000  0x0 SZ_60M>;
> +		msi-controller;
> +		interrupts =
> +			<54 IRQ_TYPE_LEVEL_HIGH>, /* misc interrupts */
> +			<55 IRQ_TYPE_LEVEL_HIGH>; /* MSI */
> +	};

As mentioned earlier, this needs to be a separate patch to be reviewed
by the Keepers of the Faith (aka the DT maintainers).

[...]

> +static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
> +{
> +	pcie->mux		= base + 0x48;
> +	pcie->msi_status	= base + 0x80;
> +	pcie->msi_enable	= base + 0xa0;
> +	pcie->msi_doorbell	= 0xa0000000 + 0x2e07c;
> +
> +	return tango_check_pcie_link(base + 0x74);

Please have some defines for these magic values.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

[Mason]

On 25/05/2017 10:48, Marc Zyngier wrote:

> On 20/04/17 15:31, Marc Gonzalez wrote:
>
>> This driver is required to work around several hardware bugs in the
>> PCIe controller.
>>
>> NB: Revision 1 does not support legacy interrupts, or IO space.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 ++++++++
>>  drivers/pci/host/Kconfig                             |   8 ++
>>  drivers/pci/host/Makefile                            |   1 +
>>  drivers/pci/host/pcie-tango.c                        | 161 +++++++++++++++++++++++++++++++++++++++
>>  include/linux/pci_ids.h                              |   2 +
>>  5 files changed, 204 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>> new file mode 100644
>> index 000000000000..3353b4e77309
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>> @@ -0,0 +1,32 @@
>> +Sigma Designs Tango PCIe controller
>> +
>> +Required properties:
>> +
>> +- compatible: "sigma,smp8759-pcie"
>> +- reg: address/size of PCI configuration space, address/size of register area
>> +- device_type: "pci"
>> +- #size-cells: <2>
>> +- #address-cells: <3>
>> +- #interrupt-cells: <1>
> 
> What is the point of having an #interrupt-cells when this is *not* an
> interrupt controller (as it doesn't support legacy interrupts)?

My mistake.

Thanks for kindly pointing out that the #interrupt-cells property
is not needed when a controller doesn't support legacy interrupts.

If a controller does support legacy interrupts, then I see other
bindings define #interrupt-cells and interrupt-map.
Is interrupt-controller also required?
Is that redundant with msi-controller?

(Rev2 will support legacy interrupts.)

References for my own information:
http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/host-generic-pci.txt
http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/altera-pcie.txt
http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping

> As mentioned earlier, this needs to be a separate patch to be reviewed
> by the Keepers of the Faith (aka the DT maintainers).

robh already acked v3 two months ago, but can split it up,
and CC the DT folks for v5.

>> +static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
>> +{
>> +	pcie->mux		= base + 0x48;
>> +	pcie->msi_status	= base + 0x80;
>> +	pcie->msi_enable	= base + 0xa0;
>> +	pcie->msi_doorbell	= 0xa0000000 + 0x2e07c;
>> +
>> +	return tango_check_pcie_link(base + 0x74);
> 
> Please have some defines for these magic values.

Typical driver do
#define MUX_OFFSET 0x48
and then access the register's value through
readl_relaxed(pcie->base + MUX_OFFSET);

I can't do that because the registers were shuffled around
between revision 1 and revision 2. Thus, instead of an
explicitly-named macro (MUX_OFFSET), I used an explicitly-
named field (pcie->mux) and access the register's value
through readl_relaxed(pcie->mux);

This is equivalent to providing the offset definitions in the
init functions, instead of at the top of the file.

Regards.

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

[Marc Zyngier]

On 25/05/17 13:00, Mason wrote:
> On 25/05/2017 10:48, Marc Zyngier wrote:
> 
>> On 20/04/17 15:31, Marc Gonzalez wrote:
>>
>>> This driver is required to work around several hardware bugs in the
>>> PCIe controller.
>>>
>>> NB: Revision 1 does not support legacy interrupts, or IO space.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 ++++++++
>>>  drivers/pci/host/Kconfig                             |   8 ++
>>>  drivers/pci/host/Makefile                            |   1 +
>>>  drivers/pci/host/pcie-tango.c                        | 161 +++++++++++++++++++++++++++++++++++++++
>>>  include/linux/pci_ids.h                              |   2 +
>>>  5 files changed, 204 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>>> new file mode 100644
>>> index 000000000000..3353b4e77309
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>>> @@ -0,0 +1,32 @@
>>> +Sigma Designs Tango PCIe controller
>>> +
>>> +Required properties:
>>> +
>>> +- compatible: "sigma,smp8759-pcie"
>>> +- reg: address/size of PCI configuration space, address/size of register area
>>> +- device_type: "pci"
>>> +- #size-cells: <2>
>>> +- #address-cells: <3>
>>> +- #interrupt-cells: <1>
>>
>> What is the point of having an #interrupt-cells when this is *not* an
>> interrupt controller (as it doesn't support legacy interrupts)?
> 
> My mistake.
> 
> Thanks for kindly pointing out that the #interrupt-cells property
> is not needed when a controller doesn't support legacy interrupts.
> 
> If a controller does support legacy interrupts, then I see other
> bindings define #interrupt-cells and interrupt-map.
> Is interrupt-controller also required?

Probably.

> Is that redundant with msi-controller?

No.

> (Rev2 will support legacy interrupts.)
> 
> References for my own information:
> http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/altera-pcie.txt
> http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
> 
>> As mentioned earlier, this needs to be a separate patch to be reviewed
>> by the Keepers of the Faith (aka the DT maintainers).
> 
> robh already acked v3 two months ago, but can split it up,
> and CC the DT folks for v5.

You didn't add the Acked-by to this patch, making Rob's effort pretty
useless.

>>> +static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
>>> +{
>>> +	pcie->mux		= base + 0x48;
>>> +	pcie->msi_status	= base + 0x80;
>>> +	pcie->msi_enable	= base + 0xa0;
>>> +	pcie->msi_doorbell	= 0xa0000000 + 0x2e07c;
>>> +
>>> +	return tango_check_pcie_link(base + 0x74);
>>
>> Please have some defines for these magic values.
> 
> Typical driver do
> #define MUX_OFFSET 0x48
> and then access the register's value through
> readl_relaxed(pcie->base + MUX_OFFSET);
> 
> I can't do that because the registers were shuffled around
> between revision 1 and revision 2. Thus, instead of an
> explicitly-named macro (MUX_OFFSET), I used an explicitly-
> named field (pcie->mux) and access the register's value
> through readl_relaxed(pcie->mux);

That doesn't prevent you from having a TANGO_V1_MUX_OFFSET define, which
you can supplement with a V2 at some point.

> This is equivalent to providing the offset definitions in the
> init functions, instead of at the top of the file.

Sorry, my brain parses text far better than hex number.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

[Mason]

On 25/05/2017 14:23, Marc Zyngier wrote:
> On 25/05/17 13:00, Mason wrote:
>> On 25/05/2017 10:48, Marc Zyngier wrote:
>>> Please have some defines for these magic values.
>>
>> Typical driver do
>> #define MUX_OFFSET 0x48
>> and then access the register's value through
>> readl_relaxed(pcie->base + MUX_OFFSET);
>>
>> I can't do that because the registers were shuffled around
>> between revision 1 and revision 2. Thus, instead of an
>> explicitly-named macro (MUX_OFFSET), I used an explicitly-
>> named field (pcie->mux) and access the register's value
>> through readl_relaxed(pcie->mux);
> 
> That doesn't prevent you from having a TANGO_V1_MUX_OFFSET define, which
> you can supplement with a V2 at some point.
> 
>> This is equivalent to providing the offset definitions in the
>> init functions, instead of at the top of the file.
> 
> Sorry, my brain parses text far better than hex number.

Well, the hex numbers do need to show up somewhere :-)

IIUC, you're saying that
#define MUX_OFFSET 0x48
is clearer than
pcie->mux = base + 0x48;

OK, I can accept that. Maybe our brains have been trained
to easily recognize and ingest the macro, or maybe it's
the caps, or maybe the fact that the statement does
several things (addition and assignment and hex).

Out of curiosity, how would you feel about
pcie->MUX_OFFSET = 0x48;
and then using
readl_relaxed(pcie->base + pcie->MUX_OFFSET);

It feels weird to me, I think mostly because it is
an unusual pattern.

Anyway, I'll add the macros, if that improves review and
maintenance.

Regards.

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

[Marc Zyngier]

On 25/05/17 13:41, Mason wrote:
> On 25/05/2017 14:23, Marc Zyngier wrote:
>> On 25/05/17 13:00, Mason wrote:
>>> On 25/05/2017 10:48, Marc Zyngier wrote:
>>>> Please have some defines for these magic values.
>>>
>>> Typical driver do
>>> #define MUX_OFFSET 0x48
>>> and then access the register's value through
>>> readl_relaxed(pcie->base + MUX_OFFSET);
>>>
>>> I can't do that because the registers were shuffled around
>>> between revision 1 and revision 2. Thus, instead of an
>>> explicitly-named macro (MUX_OFFSET), I used an explicitly-
>>> named field (pcie->mux) and access the register's value
>>> through readl_relaxed(pcie->mux);
>>
>> That doesn't prevent you from having a TANGO_V1_MUX_OFFSET define, which
>> you can supplement with a V2 at some point.
>>
>>> This is equivalent to providing the offset definitions in the
>>> init functions, instead of at the top of the file.
>>
>> Sorry, my brain parses text far better than hex number.
> 
> Well, the hex numbers do need to show up somewhere :-)
> 
> IIUC, you're saying that
> #define MUX_OFFSET 0x48
> is clearer than
> pcie->mux = base + 0x48;

yes.

> 
> OK, I can accept that. Maybe our brains have been trained
> to easily recognize and ingest the macro, or maybe it's
> the caps, or maybe the fact that the statement does
> several things (addition and assignment and hex).
> 
> Out of curiosity, how would you feel about
> pcie->MUX_OFFSET = 0x48;
> and then using
> readl_relaxed(pcie->base + pcie->MUX_OFFSET);
> 
> It feels weird to me, I think mostly because it is
> an unusual pattern.

Exactly. Use existing practices help the reviewers quite a lot.

> 
> Anyway, I'll add the macros, if that improves review and
> maintenance.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 0/2] Tango PCIe controller support

[Marc Gonzalez]

On 20/04/2017 16:24, Marc Gonzalez wrote:

> This patch was split in two, to ease review of two orthogonal
> parts (MSI controller and host bridge). NB: the patch is
> just split in two where host bridge support starts.
> 
> Changes from v3 to v4
> 
> In the MSI part:
> - Support 256 MSIs instead of only 32
> - Define tango_{ack,mask,unmask} callbacks for the HW irq_chip
> - Use a spinlock instead of a mutex
> - Rename msi_mask register to msi_enable
> 
> In the host bridge part:
> - Move several quirk-handling snippets out of the config space read function
> - Check whether the PCIe link is up at probe-time
> 
> Other files
> - Let the framework compute the bus-range from the config space width
> - Be slightly more verbose in the Kconfig help
> 
> 
> What has *not* changed, waiting from feedback from Bjorn:
> 
> - Pray that config and mem space accesses NEVER occur concurrently.
> - Using of_device_is_compatible() vs of_device_get_match_data()
> 
> 
> Marc Gonzalez (2):
>   PCI: Add tango MSI controller support
>   PCI: Add tango PCIe host bridge support
> 
>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 ++++
>  drivers/pci/host/Kconfig                             |   8 +
>  drivers/pci/host/Makefile                            |   1 +
>  drivers/pci/host/pcie-tango.c                        | 393 +++++++++++++++++++++++++++++++++++++++
>  include/linux/pci_ids.h                              |   2 +
>  5 files changed, 436 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/tango-pcie.txt
>  create mode 100644 drivers/pci/host/pcie-tango.c

Now that Linus has closed the merge window for v4.12, I hope that this
driver can be accepted in v4.13 ?

Regards.

Re: [PATCH v4 0/2] Tango PCIe controller support

[Bjorn Helgaas]

On Thu, Apr 20, 2017 at 04:24:39PM +0200, Marc Gonzalez wrote:
> Hello,
> 
> This patch was split in two, to ease review of two orthogonal
> parts (MSI controller and host bridge). NB: the patch is
> just split in two where host bridge support starts.

The split is backwards.  The first patch should add basic host bridge
support that is functional, though lacking MSI support.  The second should
add the MSI support.

> Changes from v3 to v4
> 
> In the MSI part:
> - Support 256 MSIs instead of only 32
> - Define tango_{ack,mask,unmask} callbacks for the HW irq_chip
> - Use a spinlock instead of a mutex
> - Rename msi_mask register to msi_enable
> 
> In the host bridge part:
> - Move several quirk-handling snippets out of the config space read function
> - Check whether the PCIe link is up at probe-time
> 
> Other files
> - Let the framework compute the bus-range from the config space width
> - Be slightly more verbose in the Kconfig help
> 
> 
> What has *not* changed, waiting from feedback from Bjorn:
> 
> - Pray that config and mem space accesses NEVER occur concurrently.

This is a pretty major issue that needs to be addressed in a more
concrete way than praying.  

> - Using of_device_is_compatible() vs of_device_get_match_data()
> 
> 
> Marc Gonzalez (2):
>   PCI: Add tango MSI controller support
>   PCI: Add tango PCIe host bridge support
> 
>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 ++++
>  drivers/pci/host/Kconfig                             |   8 +
>  drivers/pci/host/Makefile                            |   1 +
>  drivers/pci/host/pcie-tango.c                        | 393 +++++++++++++++++++++++++++++++++++++++
>  include/linux/pci_ids.h                              |   2 +
>  5 files changed, 436 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/tango-pcie.txt
>  create mode 100644 drivers/pci/host/pcie-tango.c
> 
> -- 
> 2.11.0

Re: [PATCH v4 0/2] Tango PCIe controller support

[Mason]

On 23/05/2017 19:17, Bjorn Helgaas wrote:
>
> On Thu, Apr 20, 2017 at 04:24:39PM +0200, Marc Gonzalez wrote:
>
>> This patch was split in two, to ease review of two orthogonal
>> parts (MSI controller and host bridge). NB: the patch is
>> just split in two where host bridge support starts.
> 
> The split is backwards.  The first patch should add basic host bridge
> support that is functional, though lacking MSI support.  The second should
> add the MSI support.

OK, I'll invert the patches.

Note: the bridge does not support legacy interrupts.
Without MSI support, EPs can't interrupt the host.
Is that still considered functional?

Regards.

Re: [PATCH v4 0/2] Tango PCIe controller support

[Bjorn Helgaas]

On Tue, May 23, 2017 at 08:07:53PM +0200, Mason wrote:
> On 23/05/2017 19:17, Bjorn Helgaas wrote:
> >
> > On Thu, Apr 20, 2017 at 04:24:39PM +0200, Marc Gonzalez wrote:
> >
> >> This patch was split in two, to ease review of two orthogonal
> >> parts (MSI controller and host bridge). NB: the patch is
> >> just split in two where host bridge support starts.
> > 
> > The split is backwards.  The first patch should add basic host bridge
> > support that is functional, though lacking MSI support.  The second should
> > add the MSI support.
> 
> OK, I'll invert the patches.
> 
> Note: the bridge does not support legacy interrupts.
> Without MSI support, EPs can't interrupt the host.
> Is that still considered functional?

Yes.  The driver can at least claim the host bridge and enumerate PCI
devices.  If you add the MSI support first, that code is completely
dead and can never be called until the second patch is merged.

Bjorn