Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1664783 > unrolled thread

Re: [PATCH v5 3/3] PCI: Add tango MSI controller support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 3/3] PCI: Add tango MSI controller support Marc Zyngier <marc.zyngier@arm.com> - 2017-06-13 14:20 +0200
    [PATCH v6 3/3] PCI: Add tango MSI controller support Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-06-13 16:10 +0200
      Re: [PATCH v6 3/3] PCI: Add tango MSI controller support Marc Zyngier <marc.zyngier@arm.com> - 2017-06-13 16:30 +0200
        Re: [PATCH v6 3/3] PCI: Add tango MSI controller support Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-06-13 16:50 +0200
          Re: [PATCH v6 3/3] PCI: Add tango MSI controller support Marc Zyngier <marc.zyngier@arm.com> - 2017-06-13 19:00 +0200
            [PATCH v7 3/3] PCI: Add tango MSI controller support Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-06-14 11:10 +0200
              Re: [PATCH v7 3/3] PCI: Add tango MSI controller support Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-06-14 11:30 +0200
                Re: [PATCH v7 3/3] PCI: Add tango MSI controller support Marc Zyngier <marc.zyngier@arm.com> - 2017-06-14 11:40 +0200
                  [PATCH v8 3/3] PCI: Add tango MSI controller support Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-06-14 13:10 +0200

#1664783 — Re: [PATCH v5 3/3] PCI: Add tango MSI controller support

On 31/05/17 14:35, 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 | 222 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 222 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
> index 67aaadcc1c5e..7d99ef26173f 100644
> --- a/drivers/pci/host/pcie-tango.c
> +++ b/drivers/pci/host/pcie-tango.c
> @@ -1,16 +1,225 @@
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
>  #include <linux/pci-ecam.h>
>  #include <linux/delay.h>
> +#include <linux/msi.h>
>  #include <linux/of.h>
>  
>  #define MSI_MAX 256
>  
>  #define SMP8759_MUX		0x48
>  #define SMP8759_TEST_OUT	0x74
> +#define SMP8759_STATUS		0x80
> +#define SMP8759_ENABLE		0xa0
> +#define SMP8759_DOORBELL	0xa002e07c
>  
>  struct tango_pcie {
> +	DECLARE_BITMAP(used, MSI_MAX);
> +	spinlock_t lock;

These two fields have pretty generic names. Consider naming them in a
way that indicates their purpose (used_msi? used_msi_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 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 long status, base, virq, idx, pos = 0;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	while ((pos = find_next_bit(pcie->used, MSI_MAX, pos)) < MSI_MAX) {

The pcie->used bitmap read can race against another CPU accessing this
bitmap in the alloc/free paths.

> +		base = round_down(pos, 32);
> +		status = readl_relaxed(pcie->msi_status + base / 8);
> +		for_each_set_bit(idx, &status, 32) {
> +			virq = irq_find_mapping(pcie->irq_dom, base + idx);
> +			generic_handle_irq(virq);
> +		}
> +		pos = base + 32;
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void tango_ack(struct irq_data *d)
> +{
> +	struct tango_pcie *pcie = d->chip_data;
> +	u32 offset = (d->hwirq / 32) * 4;
> +	u32 bit = BIT(d->hwirq % 32);
> +
> +	writel_relaxed(bit, pcie->msi_status + offset);
> +}
> +
> +static void update_msi_enable(struct irq_data *d, bool unmask)
> +{
> +	unsigned long flags;
> +	struct tango_pcie *pcie = d->chip_data;
> +	u32 offset = (d->hwirq / 32) * 4;
> +	u32 bit = BIT(d->hwirq % 32);
> +	u32 val;
> +
> +	spin_lock_irqsave(&pcie->lock, flags);
> +	val = readl_relaxed(pcie->msi_enable + offset);
> +	val = unmask ? val | bit : val & ~bit;
> +	writel_relaxed(val, pcie->msi_enable + offset);
> +	spin_unlock_irqrestore(&pcie->lock, flags);
> +}
> +
> +static void tango_mask(struct irq_data *d)
> +{
> +	update_msi_enable(d, false);
> +}
> +
> +static void tango_unmask(struct irq_data *d)
> +{
> +	update_msi_enable(d, true);
> +}
> +
> +static int tango_set_affinity(struct irq_data *d,
> +		const struct cpumask *mask, bool force)
> +{
> +	return -EINVAL;
> +}
> +
> +static void tango_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
> +{
> +	struct tango_pcie *pcie = d->chip_data;
> +	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
> +	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
> +	msg->data = d->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 *d)
> +{
> +	irq_chip_ack_parent(d);
> +}
> +
> +static void msi_mask(struct irq_data *d)
> +{
> +	pci_msi_mask_irq(d);
> +	irq_chip_mask_parent(d);
> +}
> +
> +static void msi_unmask(struct irq_data *d)
> +{
> +	pci_msi_unmask_irq(d);
> +	irq_chip_unmask_parent(d);
> +}
> +
> +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 tango_irq_domain_alloc(struct irq_domain *dom,
> +		unsigned int virq, unsigned int nr_irqs, void *args)
> +{
> +	unsigned long flags;
> +	int pos, err = -ENOSPC;
> +	struct tango_pcie *pcie = dom->host_data;
> +
> +	spin_lock_irqsave(&pcie->lock, flags);
> +	pos = find_first_zero_bit(pcie->used, MSI_MAX);
> +	if (pos < MSI_MAX) {
> +		err = 0;
> +		__set_bit(pos, pcie->used);
> +		irq_domain_set_info(dom, virq, pos,
> +				&tango_chip, pcie, handle_edge_irq, NULL, NULL);
> +	}
> +	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 *d = irq_domain_get_irq_data(dom, virq);
> +	struct tango_pcie *pcie = d->chip_data;
> +
> +	spin_lock_irqsave(&pcie->lock, flags);
> +	__clear_bit(d->hwirq, pcie->used);
> +	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);
> +
> +	virq = platform_get_irq(pdev, 1);
> +	if (virq <= 0) {
> +		pr_err("Failed to map IRQ\n");
> +		return -ENXIO;
> +	}
> +
> +	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");
> +		return -ENOMEM;

You're now leaking irq_dom.

> +	}
> +
> +	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;
> +}
> +
>  /*** HOST BRIDGE SUPPORT ***/
>  
>  static int smp8759_config_read(struct pci_bus *bus,
> @@ -88,6 +297,9 @@ static int tango_check_pcie_link(void __iomem *test_out)
>  static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
>  {
>  	pcie->mux		= base + SMP8759_MUX;
> +	pcie->msi_status	= base + SMP8759_STATUS;
> +	pcie->msi_enable	= base + SMP8759_ENABLE;
> +	pcie->msi_doorbell	= SMP8759_DOORBELL;
>  
>  	return tango_check_pcie_link(base + SMP8759_TEST_OUT);
>  }
> @@ -121,11 +333,21 @@ static int tango_pcie_probe(struct platform_device *pdev)
>  	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,
> 

Thanks,

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

[toc] | [next] | [standalone]

#1664899 — [PATCH v6 3/3] PCI: Add tango MSI controller support

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

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
Changes from v5 to v6
o Rename 'used' bitmap to 'used_msi'
o Rename 'lock' spinlock to 'used_msi_lock'
o Take lock in interrupt handler
o Remove irq_dom in error path
---
 drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 225 insertions(+)

diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
index 67aaadcc1c5e..b06446b23bc8 100644
--- a/drivers/pci/host/pcie-tango.c
+++ b/drivers/pci/host/pcie-tango.c
@@ -1,16 +1,228 @@
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/pci-ecam.h>
 #include <linux/delay.h>
+#include <linux/msi.h>
 #include <linux/of.h>
 
 #define MSI_MAX 256
 
 #define SMP8759_MUX		0x48
 #define SMP8759_TEST_OUT	0x74
+#define SMP8759_STATUS		0x80
+#define SMP8759_ENABLE		0xa0
+#define SMP8759_DOORBELL	0xa002e07c
 
 struct tango_pcie {
+	DECLARE_BITMAP(used_msi, MSI_MAX);
+	spinlock_t used_msi_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 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 long flags, status, base, virq, idx, pos = 0;
+
+	chained_irq_enter(chip, desc);
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+
+	while ((pos = find_next_bit(pcie->used_msi, MSI_MAX, pos)) < MSI_MAX) {
+		base = round_down(pos, 32);
+		status = readl_relaxed(pcie->msi_status + base / 8);
+		for_each_set_bit(idx, &status, 32) {
+			virq = irq_find_mapping(pcie->irq_dom, base + idx);
+			generic_handle_irq(virq);
+		}
+		pos = base + 32;
+	}
+
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+	chained_irq_exit(chip, desc);
+}
+
+static void tango_ack(struct irq_data *d)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+
+	writel_relaxed(bit, pcie->msi_status + offset);
+}
+
+static void update_msi_enable(struct irq_data *d, bool unmask)
+{
+	unsigned long flags;
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+	u32 val;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	val = readl_relaxed(pcie->msi_enable + offset);
+	val = unmask ? val | bit : val & ~bit;
+	writel_relaxed(val, pcie->msi_enable + offset);
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+}
+
+static void tango_mask(struct irq_data *d)
+{
+	update_msi_enable(d, false);
+}
+
+static void tango_unmask(struct irq_data *d)
+{
+	update_msi_enable(d, true);
+}
+
+static int tango_set_affinity(struct irq_data *d,
+		const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static void tango_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
+	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
+	msg->data = d->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 *d)
+{
+	irq_chip_ack_parent(d);
+}
+
+static void msi_mask(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void msi_unmask(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
+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 tango_irq_domain_alloc(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs, void *args)
+{
+	unsigned long flags;
+	int pos, err = -ENOSPC;
+	struct tango_pcie *pcie = dom->host_data;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	pos = find_first_zero_bit(pcie->used_msi, MSI_MAX);
+	if (pos < MSI_MAX) {
+		err = 0;
+		__set_bit(pos, pcie->used_msi);
+		irq_domain_set_info(dom, virq, pos,
+				&tango_chip, pcie, handle_edge_irq, NULL, NULL);
+	}
+	spin_unlock_irqrestore(&pcie->used_msi_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 *d = irq_domain_get_irq_data(dom, virq);
+	struct tango_pcie *pcie = d->chip_data;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	__clear_bit(d->hwirq, pcie->used_msi);
+	spin_unlock_irqrestore(&pcie->used_msi_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->used_msi_lock);
+	for (i = 0; i < MSI_MAX / 32; ++i)
+		writel_relaxed(0, pcie->msi_enable + i * 4);
+
+	virq = platform_get_irq(pdev, 1);
+	if (virq <= 0) {
+		pr_err("Failed to map IRQ\n");
+		return -ENXIO;
+	}
+
+	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;
+	}
+
+	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;
+}
+
 /*** HOST BRIDGE SUPPORT ***/
 
 static int smp8759_config_read(struct pci_bus *bus,
@@ -88,6 +300,9 @@ static int tango_check_pcie_link(void __iomem *test_out)
 static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
 {
 	pcie->mux		= base + SMP8759_MUX;
+	pcie->msi_status	= base + SMP8759_STATUS;
+	pcie->msi_enable	= base + SMP8759_ENABLE;
+	pcie->msi_doorbell	= SMP8759_DOORBELL;
 
 	return tango_check_pcie_link(base + SMP8759_TEST_OUT);
 }
@@ -121,11 +336,21 @@ static int tango_pcie_probe(struct platform_device *pdev)
 	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,
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1664924 — Re: [PATCH v6 3/3] PCI: Add tango MSI controller support

On 13/06/17 15:01, 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>
> ---
> Changes from v5 to v6
> o Rename 'used' bitmap to 'used_msi'
> o Rename 'lock' spinlock to 'used_msi_lock'
> o Take lock in interrupt handler
> o Remove irq_dom in error path
> ---
>  drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 225 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
> index 67aaadcc1c5e..b06446b23bc8 100644
> --- a/drivers/pci/host/pcie-tango.c
> +++ b/drivers/pci/host/pcie-tango.c
> @@ -1,16 +1,228 @@
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
>  #include <linux/pci-ecam.h>
>  #include <linux/delay.h>
> +#include <linux/msi.h>
>  #include <linux/of.h>
>  
>  #define MSI_MAX 256
>  
>  #define SMP8759_MUX		0x48
>  #define SMP8759_TEST_OUT	0x74
> +#define SMP8759_STATUS		0x80
> +#define SMP8759_ENABLE		0xa0
> +#define SMP8759_DOORBELL	0xa002e07c
>  
>  struct tango_pcie {
> +	DECLARE_BITMAP(used_msi, MSI_MAX);
> +	spinlock_t used_msi_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 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 long flags, status, base, virq, idx, pos = 0;
> +
> +	chained_irq_enter(chip, desc);
> +	spin_lock_irqsave(&pcie->used_msi_lock, flags);

You're already in interrupt context, so there is no need to disable
interrupts any further. spin_lock() should do the trick

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1664941 — Re: [PATCH v6 3/3] PCI: Add tango MSI controller support

On 13/06/2017 16:22, Marc Zyngier wrote:
> On 13/06/17 15:01, 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>
>> ---
>> Changes from v5 to v6
>> o Rename 'used' bitmap to 'used_msi'
>> o Rename 'lock' spinlock to 'used_msi_lock'
>> o Take lock in interrupt handler
>> o Remove irq_dom in error path
>> ---
>>  drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 225 insertions(+)
>>
>> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
>> index 67aaadcc1c5e..b06446b23bc8 100644
>> --- a/drivers/pci/host/pcie-tango.c
>> +++ b/drivers/pci/host/pcie-tango.c
>> @@ -1,16 +1,228 @@
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>>  #include <linux/pci-ecam.h>
>>  #include <linux/delay.h>
>> +#include <linux/msi.h>
>>  #include <linux/of.h>
>>  
>>  #define MSI_MAX 256
>>  
>>  #define SMP8759_MUX		0x48
>>  #define SMP8759_TEST_OUT	0x74
>> +#define SMP8759_STATUS		0x80
>> +#define SMP8759_ENABLE		0xa0
>> +#define SMP8759_DOORBELL	0xa002e07c
>>  
>>  struct tango_pcie {
>> +	DECLARE_BITMAP(used_msi, MSI_MAX);
>> +	spinlock_t used_msi_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 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 long flags, status, base, virq, idx, pos = 0;
>> +
>> +	chained_irq_enter(chip, desc);
>> +	spin_lock_irqsave(&pcie->used_msi_lock, flags);
> 
> You're already in interrupt context, so there is no need to disable
> interrupts any further. spin_lock() should do the trick

Thanks for the hint.

I am confused, because Documentation/locking/spinlocks.txt states:

> If you have a case where you have to protect a data structure across
> several CPU's and you want to use spinlocks you can potentially use
> cheaper versions of the spinlocks. IFF you know that the spinlocks are
> never used in interrupt handlers, you can use the non-irq versions:
> 
> 	spin_lock(&lock);
> 	...
> 	spin_unlock(&lock);
> 
> (and the equivalent read-write versions too, of course). The spinlock will
> guarantee the same kind of exclusive access, and it will be much faster.
> This is useful if you know that the data in question is only ever
> manipulated from a "process context", ie no interrupts involved.
> 
> The reasons you mustn't use these versions if you have interrupts that
> play with the spinlock is that you can get deadlocks:
> 
> 	spin_lock(&lock);
> 	...
> 		<- interrupt comes in:
> 			spin_lock(&lock);
> 
> where an interrupt tries to lock an already locked variable. This is ok if
> the other interrupt happens on another CPU, but it is _not_ ok if the
> interrupt happens on the same CPU that already holds the lock, because the
> lock will obviously never be released (because the interrupt is waiting
> for the lock, and the lock-holder is interrupted by the interrupt and will
> not continue until the interrupt has been processed).
> 
> (This is also the reason why the irq-versions of the spinlocks only need
> to disable the _local_ interrupts - it's ok to use spinlocks in interrupts
> on other CPU's, because an interrupt on another CPU doesn't interrupt the
> CPU that holds the lock, so the lock-holder can continue and eventually
> releases the lock).

Isn't this saying that it is not safe to call spin_lock() from
the interrupt handler? (Sorry if I misunderstood.)

Regards.

[toc] | [prev] | [next] | [standalone]

#1665023 — Re: [PATCH v6 3/3] PCI: Add tango MSI controller support

On 13/06/17 15:47, Marc Gonzalez wrote:
> On 13/06/2017 16:22, Marc Zyngier wrote:
>> On 13/06/17 15:01, 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>
>>> ---
>>> Changes from v5 to v6
>>> o Rename 'used' bitmap to 'used_msi'
>>> o Rename 'lock' spinlock to 'used_msi_lock'
>>> o Take lock in interrupt handler
>>> o Remove irq_dom in error path
>>> ---
>>>  drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 225 insertions(+)
>>>
>>> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
>>> index 67aaadcc1c5e..b06446b23bc8 100644
>>> --- a/drivers/pci/host/pcie-tango.c
>>> +++ b/drivers/pci/host/pcie-tango.c
>>> @@ -1,16 +1,228 @@
>>> +#include <linux/irqchip/chained_irq.h>
>>> +#include <linux/irqdomain.h>
>>>  #include <linux/pci-ecam.h>
>>>  #include <linux/delay.h>
>>> +#include <linux/msi.h>
>>>  #include <linux/of.h>
>>>  
>>>  #define MSI_MAX 256
>>>  
>>>  #define SMP8759_MUX		0x48
>>>  #define SMP8759_TEST_OUT	0x74
>>> +#define SMP8759_STATUS		0x80
>>> +#define SMP8759_ENABLE		0xa0
>>> +#define SMP8759_DOORBELL	0xa002e07c
>>>  
>>>  struct tango_pcie {
>>> +	DECLARE_BITMAP(used_msi, MSI_MAX);
>>> +	spinlock_t used_msi_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 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 long flags, status, base, virq, idx, pos = 0;
>>> +
>>> +	chained_irq_enter(chip, desc);
>>> +	spin_lock_irqsave(&pcie->used_msi_lock, flags);
>>
>> You're already in interrupt context, so there is no need to disable
>> interrupts any further. spin_lock() should do the trick
> 
> Thanks for the hint.
> 
> I am confused, because Documentation/locking/spinlocks.txt states:
> 
>> If you have a case where you have to protect a data structure across
>> several CPU's and you want to use spinlocks you can potentially use
>> cheaper versions of the spinlocks. IFF you know that the spinlocks are
>> never used in interrupt handlers, you can use the non-irq versions:
>>
>> 	spin_lock(&lock);
>> 	...
>> 	spin_unlock(&lock);
>>
>> (and the equivalent read-write versions too, of course). The spinlock will
>> guarantee the same kind of exclusive access, and it will be much faster.
>> This is useful if you know that the data in question is only ever
>> manipulated from a "process context", ie no interrupts involved.
>>
>> The reasons you mustn't use these versions if you have interrupts that
>> play with the spinlock is that you can get deadlocks:
>>
>> 	spin_lock(&lock);
>> 	...
>> 		<- interrupt comes in:
>> 			spin_lock(&lock);
>>
>> where an interrupt tries to lock an already locked variable. This is ok if
>> the other interrupt happens on another CPU, but it is _not_ ok if the
>> interrupt happens on the same CPU that already holds the lock, because the
>> lock will obviously never be released (because the interrupt is waiting
>> for the lock, and the lock-holder is interrupted by the interrupt and will
>> not continue until the interrupt has been processed).
>>
>> (This is also the reason why the irq-versions of the spinlocks only need
>> to disable the _local_ interrupts - it's ok to use spinlocks in interrupts
>> on other CPU's, because an interrupt on another CPU doesn't interrupt the
>> CPU that holds the lock, so the lock-holder can continue and eventually
>> releases the lock).
> 
> Isn't this saying that it is not safe to call spin_lock() from
> the interrupt handler? (Sorry if I misunderstood.)

It is saying exactly the opposite.

If you take a spinlock and can be interrupted by an interrupt that takes
the same spinlock, then you must use the irq-safe version *outside of
the interrupt handler*. That's because Linux interrupts are not
preemptible (well, in general -- it is different with RT, but let's not
get there). If you're guaranteed that no interrupt handler will take
this spinlock, then you don't have to use the irq-safe version.

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

[toc] | [prev] | [next] | [standalone]

#1665598 — [PATCH v7 3/3] PCI: Add tango MSI controller support

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

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
Changes from v6 to v7
o Call spin_lock() not spin_lock_irqsave() in the ISR
---
 drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 225 insertions(+)

diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
index 67aaadcc1c5e..d4b3520b5a03 100644
--- a/drivers/pci/host/pcie-tango.c
+++ b/drivers/pci/host/pcie-tango.c
@@ -1,16 +1,228 @@
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/pci-ecam.h>
 #include <linux/delay.h>
+#include <linux/msi.h>
 #include <linux/of.h>
 
 #define MSI_MAX 256
 
 #define SMP8759_MUX		0x48
 #define SMP8759_TEST_OUT	0x74
+#define SMP8759_STATUS		0x80
+#define SMP8759_ENABLE		0xa0
+#define SMP8759_DOORBELL	0xa002e07c
 
 struct tango_pcie {
+	DECLARE_BITMAP(used_msi, MSI_MAX);
+	spinlock_t used_msi_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 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 long status, base, virq, idx, pos = 0;
+
+	chained_irq_enter(chip, desc);
+	spin_lock(&pcie->used_msi_lock);
+
+	while ((pos = find_next_bit(pcie->used_msi, MSI_MAX, pos)) < MSI_MAX) {
+		base = round_down(pos, 32);
+		status = readl_relaxed(pcie->msi_status + base / 8);
+		for_each_set_bit(idx, &status, 32) {
+			virq = irq_find_mapping(pcie->irq_dom, base + idx);
+			generic_handle_irq(virq);
+		}
+		pos = base + 32;
+	}
+
+	spin_unlock(&pcie->used_msi_lock);
+	chained_irq_exit(chip, desc);
+}
+
+static void tango_ack(struct irq_data *d)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+
+	writel_relaxed(bit, pcie->msi_status + offset);
+}
+
+static void update_msi_enable(struct irq_data *d, bool unmask)
+{
+	unsigned long flags;
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+	u32 val;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	val = readl_relaxed(pcie->msi_enable + offset);
+	val = unmask ? val | bit : val & ~bit;
+	writel_relaxed(val, pcie->msi_enable + offset);
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+}
+
+static void tango_mask(struct irq_data *d)
+{
+	update_msi_enable(d, false);
+}
+
+static void tango_unmask(struct irq_data *d)
+{
+	update_msi_enable(d, true);
+}
+
+static int tango_set_affinity(struct irq_data *d,
+		const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static void tango_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
+	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
+	msg->data = d->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 *d)
+{
+	irq_chip_ack_parent(d);
+}
+
+static void msi_mask(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void msi_unmask(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
+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 tango_irq_domain_alloc(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs, void *args)
+{
+	unsigned long flags;
+	int pos, err = -ENOSPC;
+	struct tango_pcie *pcie = dom->host_data;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	pos = find_first_zero_bit(pcie->used_msi, MSI_MAX);
+	if (pos < MSI_MAX) {
+		err = 0;
+		__set_bit(pos, pcie->used_msi);
+		irq_domain_set_info(dom, virq, pos,
+				&tango_chip, pcie, handle_edge_irq, NULL, NULL);
+	}
+	spin_unlock_irqrestore(&pcie->used_msi_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 *d = irq_domain_get_irq_data(dom, virq);
+	struct tango_pcie *pcie = d->chip_data;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	__clear_bit(d->hwirq, pcie->used_msi);
+	spin_unlock_irqrestore(&pcie->used_msi_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->used_msi_lock);
+	for (i = 0; i < MSI_MAX / 32; ++i)
+		writel_relaxed(0, pcie->msi_enable + i * 4);
+
+	virq = platform_get_irq(pdev, 1);
+	if (virq <= 0) {
+		pr_err("Failed to map IRQ\n");
+		return -ENXIO;
+	}
+
+	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;
+	}
+
+	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;
+}
+
 /*** HOST BRIDGE SUPPORT ***/
 
 static int smp8759_config_read(struct pci_bus *bus,
@@ -88,6 +300,9 @@ static int tango_check_pcie_link(void __iomem *test_out)
 static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
 {
 	pcie->mux		= base + SMP8759_MUX;
+	pcie->msi_status	= base + SMP8759_STATUS;
+	pcie->msi_enable	= base + SMP8759_ENABLE;
+	pcie->msi_doorbell	= SMP8759_DOORBELL;
 
 	return tango_check_pcie_link(base + SMP8759_TEST_OUT);
 }
@@ -121,11 +336,21 @@ static int tango_pcie_probe(struct platform_device *pdev)
 	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,
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1665629 — Re: [PATCH v7 3/3] PCI: Add tango MSI controller support

On 14/06/2017 11:00, 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>
> ---
> Changes from v6 to v7
> o Call spin_lock() not spin_lock_irqsave() in the ISR
> ---
>  drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 225 insertions(+)

Someone on IRC suggested testing the driver with LOCKDEP.

If I understand the warning below correctly, I am not supposed
to call irq_domain_set_info() while holding used_msi_lock?

NB: in probe, my driver calls

	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);

This should not break LOCKDEP analysis, right?

Regards.


[    0.685615] ======================================================
[    0.685621] [ INFO: possible circular locking dependency detected ]
[    0.685632] 4.9.20-1-rc3 #2 Tainted: G          I    
[    0.685638] -------------------------------------------------------
[    0.685644] swapper/0/1 is trying to acquire lock:
[    0.685650]  (&(&pcie->used_msi_lock)->rlock){......}, at: [<c0352e5c>] update_msi_enable+0x2c/0x58

[    0.685692] but task is already holding lock:
[    0.685698]  (&irq_desc_lock_class){......}, at: [<c017586c>] __setup_irq+0xa4/0x5f0

[    0.685718] which lock already depends on the new lock.
[    0.685718] 
[    0.685725] 
[    0.685725] the existing dependency chain (in reverse order) is:
[    0.685731] 
[    0.685731] -> #1 (&irq_desc_lock_class){......}:
[    0.685758]        __irq_get_desc_lock+0x54/0x94
[    0.685768]        __irq_set_handler+0x24/0x54
[    0.685778]        irq_domain_set_info+0x38/0x4c
[    0.685786]        tango_irq_domain_alloc+0x98/0xbc
[    0.685795]        msi_domain_alloc+0x68/0x130
[    0.685802]        __irq_domain_alloc_irqs+0x11c/0x2ac
[    0.685809]        msi_domain_alloc_irqs+0x78/0x188
[    0.685816]        __pci_enable_msi_range+0x200/0x37c
[    0.685824]        pcie_port_device_register+0x3cc/0x494
[    0.685831]        pcie_portdrv_probe+0x2c/0xa4
[    0.685844]        pci_device_probe+0x8c/0xe8
[    0.685852]        driver_probe_device+0x1c8/0x2ac
[    0.685863]        bus_for_each_drv+0x60/0x94
[    0.685870]        __device_attach+0xb4/0x118
[    0.685883]        pci_bus_add_device+0x44/0x90
[    0.685891]        pci_bus_add_devices+0x3c/0x80
[    0.685898]        pci_host_common_probe+0x100/0x314
[    0.685906]        platform_drv_probe+0x50/0xb0
[    0.685912]        driver_probe_device+0x21c/0x2ac
[    0.685918]        __driver_attach+0xc0/0xc4
[    0.685926]        bus_for_each_dev+0x68/0x9c
[    0.685933]        bus_add_driver+0x108/0x214
[    0.685939]        driver_register+0x78/0xf4
[    0.685947]        do_one_initcall+0x44/0x174
[    0.685961]        kernel_init_freeable+0x158/0x1e8
[    0.685971]        kernel_init+0x8/0x10c
[    0.685980]        ret_from_fork+0x14/0x24
[    0.685985] 
[    0.685985] -> #0 (&(&pcie->used_msi_lock)->rlock){......}:
[    0.686003]        _raw_spin_lock_irqsave+0x44/0x58
[    0.686012]        update_msi_enable+0x2c/0x58
[    0.686019]        irq_enable+0x30/0x44
[    0.686025]        irq_startup+0x80/0x84
[    0.686031]        __setup_irq+0x558/0x5f0
[    0.686038]        request_threaded_irq+0xe4/0x184
[    0.686044]        pcie_pme_probe+0xc4/0x1f0
[    0.686051]        pcie_port_probe_service+0x34/0x70
[    0.686057]        driver_probe_device+0x21c/0x2ac
[    0.686065]        bus_for_each_drv+0x60/0x94
[    0.686071]        __device_attach+0xb4/0x118
[    0.686079]        bus_probe_device+0x88/0x90
[    0.686085]        device_add+0x3f4/0x584
[    0.686092]        pcie_port_device_register+0x228/0x494
[    0.686099]        pcie_portdrv_probe+0x2c/0xa4
[    0.686106]        pci_device_probe+0x8c/0xe8
[    0.686113]        driver_probe_device+0x1c8/0x2ac
[    0.686120]        bus_for_each_drv+0x60/0x94
[    0.686126]        __device_attach+0xb4/0x118
[    0.686134]        pci_bus_add_device+0x44/0x90
[    0.686141]        pci_bus_add_devices+0x3c/0x80
[    0.686149]        pci_host_common_probe+0x100/0x314
[    0.686155]        platform_drv_probe+0x50/0xb0
[    0.686161]        driver_probe_device+0x21c/0x2ac
[    0.686167]        __driver_attach+0xc0/0xc4
[    0.686175]        bus_for_each_dev+0x68/0x9c
[    0.686182]        bus_add_driver+0x108/0x214
[    0.686188]        driver_register+0x78/0xf4
[    0.686194]        do_one_initcall+0x44/0x174
[    0.686203]        kernel_init_freeable+0x158/0x1e8
[    0.686210]        kernel_init+0x8/0x10c
[    0.686218]        ret_from_fork+0x14/0x24
[    0.686222] 
[    0.686222] other info that might help us debug this:
[    0.686222] 
[    0.686229]  Possible unsafe locking scenario:
[    0.686229] 
[    0.686235]        CPU0                    CPU1
[    0.686239]        ----                    ----
[    0.686243]   lock(&irq_desc_lock_class);
[    0.686251]                                lock(&(&pcie->used_msi_lock)->rlock);
[    0.686260]                                lock(&irq_desc_lock_class);
[    0.686267]   lock(&(&pcie->used_msi_lock)->rlock);
[    0.686275] 
[    0.686275]  *** DEADLOCK ***
[    0.686275] 
[    0.686283] 5 locks held by swapper/0/1:
[    0.686288]  #0:  (&dev->mutex){......}, at: [<c03939e4>] __driver_attach+0x50/0xc4
[    0.686303]  #1:  (&dev->mutex){......}, at: [<c03939f4>] __driver_attach+0x60/0xc4
[    0.686318]  #2:  (&dev->mutex){......}, at: [<c0393574>] __device_attach+0x20/0x118
[    0.686333]  #3:  (&dev->mutex){......}, at: [<c0393574>] __device_attach+0x20/0x118
[    0.686348]  #4:  (&irq_desc_lock_class){......}, at: [<c017586c>] __setup_irq+0xa4/0x5f0
[    0.686363] 
[    0.686363] stack backtrace:
[    0.686373] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G          I     4.9.20-1-rc3 #2
[    0.686379] Hardware name: Sigma Tango DT
[    0.686398] [<c010f8a0>] (unwind_backtrace) from [<c010b540>] (show_stack+0x10/0x14)
[    0.686411] [<c010b540>] (show_stack) from [<c0309f40>] (dump_stack+0x98/0xc4)
[    0.686423] [<c0309f40>] (dump_stack) from [<c0165ce4>] (print_circular_bug+0x214/0x334)
[    0.686432] [<c0165ce4>] (print_circular_bug) from [<c016934c>] (__lock_acquire+0x171c/0x1a28)
[    0.686441] [<c016934c>] (__lock_acquire) from [<c0169a28>] (lock_acquire+0x6c/0x88)
[    0.686452] [<c0169a28>] (lock_acquire) from [<c0533004>] (_raw_spin_lock_irqsave+0x44/0x58)
[    0.686464] [<c0533004>] (_raw_spin_lock_irqsave) from [<c0352e5c>] (update_msi_enable+0x2c/0x58)
[    0.686475] [<c0352e5c>] (update_msi_enable) from [<c0177614>] (irq_enable+0x30/0x44)
[    0.686484] [<c0177614>] (irq_enable) from [<c01776a8>] (irq_startup+0x80/0x84)
[    0.686493] [<c01776a8>] (irq_startup) from [<c0175d20>] (__setup_irq+0x558/0x5f0)
[    0.686502] [<c0175d20>] (__setup_irq) from [<c0175f60>] (request_threaded_irq+0xe4/0x184)
[    0.686511] [<c0175f60>] (request_threaded_irq) from [<c034fed0>] (pcie_pme_probe+0xc4/0x1f0)
[    0.686520] [<c034fed0>] (pcie_pme_probe) from [<c034d8a8>] (pcie_port_probe_service+0x34/0x70)
[    0.686530] [<c034d8a8>] (pcie_port_probe_service) from [<c0393904>] (driver_probe_device+0x21c/0x2ac)
[    0.686540] [<c0393904>] (driver_probe_device) from [<c0391d18>] (bus_for_each_drv+0x60/0x94)
[    0.686550] [<c0391d18>] (bus_for_each_drv) from [<c0393608>] (__device_attach+0xb4/0x118)
[    0.686560] [<c0393608>] (__device_attach) from [<c0392b14>] (bus_probe_device+0x88/0x90)
[    0.686570] [<c0392b14>] (bus_probe_device) from [<c0390ef0>] (device_add+0x3f4/0x584)
[    0.686580] [<c0390ef0>] (device_add) from [<c034dba4>] (pcie_port_device_register+0x228/0x494)
[    0.686589] [<c034dba4>] (pcie_port_device_register) from [<c034e26c>] (pcie_portdrv_probe+0x2c/0xa4)
[    0.686600] [<c034e26c>] (pcie_portdrv_probe) from [<c0341ff0>] (pci_device_probe+0x8c/0xe8)
[    0.686611] [<c0341ff0>] (pci_device_probe) from [<c03938b0>] (driver_probe_device+0x1c8/0x2ac)
[    0.686620] [<c03938b0>] (driver_probe_device) from [<c0391d18>] (bus_for_each_drv+0x60/0x94)
[    0.686630] [<c0391d18>] (bus_for_each_drv) from [<c0393608>] (__device_attach+0xb4/0x118)
[    0.686641] [<c0393608>] (__device_attach) from [<c03381fc>] (pci_bus_add_device+0x44/0x90)
[    0.686651] [<c03381fc>] (pci_bus_add_device) from [<c0338284>] (pci_bus_add_devices+0x3c/0x80)
[    0.686662] [<c0338284>] (pci_bus_add_devices) from [<c0352bf4>] (pci_host_common_probe+0x100/0x314)
[    0.686673] [<c0352bf4>] (pci_host_common_probe) from [<c03950dc>] (platform_drv_probe+0x50/0xb0)
[    0.686682] [<c03950dc>] (platform_drv_probe) from [<c0393904>] (driver_probe_device+0x21c/0x2ac)
[    0.686690] [<c0393904>] (driver_probe_device) from [<c0393a54>] (__driver_attach+0xc0/0xc4)
[    0.686700] [<c0393a54>] (__driver_attach) from [<c0391c70>] (bus_for_each_dev+0x68/0x9c)
[    0.686711] [<c0391c70>] (bus_for_each_dev) from [<c0392d2c>] (bus_add_driver+0x108/0x214)
[    0.686721] [<c0392d2c>] (bus_add_driver) from [<c0394170>] (driver_register+0x78/0xf4)
[    0.686730] [<c0394170>] (driver_register) from [<c01017dc>] (do_one_initcall+0x44/0x174)
[    0.686742] [<c01017dc>] (do_one_initcall) from [<c0800dc0>] (kernel_init_freeable+0x158/0x1e8)
[    0.686754] [<c0800dc0>] (kernel_init_freeable) from [<c052c918>] (kernel_init+0x8/0x10c)
[    0.686765] [<c052c918>] (kernel_init) from [<c01077d0>] (ret_from_fork+0x14/0x24)

[    0.686824] pcieport 0000:00:00.0: Signaling PME through PCIe PME interrupt
[    0.686835] pci 0000:01:00.0: Signaling PME through PCIe PME interrupt
[    0.686849] pcie_pme 0000:00:00.0:pcie001: service driver pcie_pme loaded
[    0.687049] aer 0000:00:00.0:pcie002: service driver aer loaded
[    0.687276] pci 0000:01:00.0: enabling device (0140 -> 0142)

[toc] | [prev] | [next] | [standalone]

#1665635 — Re: [PATCH v7 3/3] PCI: Add tango MSI controller support

On 14/06/17 10:19, Marc Gonzalez wrote:
> On 14/06/2017 11:00, 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>
>> ---
>> Changes from v6 to v7
>> o Call spin_lock() not spin_lock_irqsave() in the ISR
>> ---
>>  drivers/pci/host/pcie-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 225 insertions(+)
> 
> Someone on IRC suggested testing the driver with LOCKDEP.
> 
> If I understand the warning below correctly, I am not supposed
> to call irq_domain_set_info() while holding used_msi_lock?

Indeed. This creates an AB/BA situation, which will eventually deadlock.
Once again, lockdep saves the day.

> NB: in probe, my driver calls
> 
> 	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
> 
> This should not break LOCKDEP analysis, right?

It doesn't. The code is provably wrong, and lockdep proved that it is wrong.

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

[toc] | [prev] | [next] | [standalone]

#1665717 — [PATCH v8 3/3] PCI: Add tango MSI controller support

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

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
Changes from v7 to v8
o Reorganize tango_irq_domain_alloc() so as not to call irq_domain_set_info()
  with pcie->used_msi_lock held => Bug diagnosed by LOCKDEP
---
 drivers/pci/host/pcie-tango.c | 226 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 226 insertions(+)

diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
index 67aaadcc1c5e..5c47a4cc03e3 100644
--- a/drivers/pci/host/pcie-tango.c
+++ b/drivers/pci/host/pcie-tango.c
@@ -1,16 +1,229 @@
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/pci-ecam.h>
 #include <linux/delay.h>
+#include <linux/msi.h>
 #include <linux/of.h>
 
 #define MSI_MAX 256
 
 #define SMP8759_MUX		0x48
 #define SMP8759_TEST_OUT	0x74
+#define SMP8759_STATUS		0x80
+#define SMP8759_ENABLE		0xa0
+#define SMP8759_DOORBELL	0xa002e07c
 
 struct tango_pcie {
+	DECLARE_BITMAP(used_msi, MSI_MAX);
+	spinlock_t used_msi_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 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 long status, base, virq, idx, pos = 0;
+
+	chained_irq_enter(chip, desc);
+	spin_lock(&pcie->used_msi_lock);
+
+	while ((pos = find_next_bit(pcie->used_msi, MSI_MAX, pos)) < MSI_MAX) {
+		base = round_down(pos, 32);
+		status = readl_relaxed(pcie->msi_status + base / 8);
+		for_each_set_bit(idx, &status, 32) {
+			virq = irq_find_mapping(pcie->irq_dom, base + idx);
+			generic_handle_irq(virq);
+		}
+		pos = base + 32;
+	}
+
+	spin_unlock(&pcie->used_msi_lock);
+	chained_irq_exit(chip, desc);
+}
+
+static void tango_ack(struct irq_data *d)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+
+	writel_relaxed(bit, pcie->msi_status + offset);
+}
+
+static void update_msi_enable(struct irq_data *d, bool unmask)
+{
+	unsigned long flags;
+	struct tango_pcie *pcie = d->chip_data;
+	u32 offset = (d->hwirq / 32) * 4;
+	u32 bit = BIT(d->hwirq % 32);
+	u32 val;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	val = readl_relaxed(pcie->msi_enable + offset);
+	val = unmask ? val | bit : val & ~bit;
+	writel_relaxed(val, pcie->msi_enable + offset);
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+}
+
+static void tango_mask(struct irq_data *d)
+{
+	update_msi_enable(d, false);
+}
+
+static void tango_unmask(struct irq_data *d)
+{
+	update_msi_enable(d, true);
+}
+
+static int tango_set_affinity(struct irq_data *d,
+		const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static void tango_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
+{
+	struct tango_pcie *pcie = d->chip_data;
+	msg->address_lo = lower_32_bits(pcie->msi_doorbell);
+	msg->address_hi = upper_32_bits(pcie->msi_doorbell);
+	msg->data = d->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 *d)
+{
+	irq_chip_ack_parent(d);
+}
+
+static void msi_mask(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void msi_unmask(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
+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 tango_irq_domain_alloc(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs, void *args)
+{
+	int pos;
+	unsigned long flags;
+	struct tango_pcie *pcie = dom->host_data;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	pos = find_first_zero_bit(pcie->used_msi, MSI_MAX);
+	if (pos >= MSI_MAX) {
+		spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+		return -ENOSPC;
+	}
+	__set_bit(pos, pcie->used_msi);
+	spin_unlock_irqrestore(&pcie->used_msi_lock, flags);
+	irq_domain_set_info(dom, virq, pos, &tango_chip,
+			pcie, handle_edge_irq, NULL, NULL);
+
+	return 0;
+}
+
+static void tango_irq_domain_free(struct irq_domain *dom,
+		unsigned int virq, unsigned int nr_irqs)
+{
+	unsigned long flags;
+	struct irq_data *d = irq_domain_get_irq_data(dom, virq);
+	struct tango_pcie *pcie = d->chip_data;
+
+	spin_lock_irqsave(&pcie->used_msi_lock, flags);
+	__clear_bit(d->hwirq, pcie->used_msi);
+	spin_unlock_irqrestore(&pcie->used_msi_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->used_msi_lock);
+	for (i = 0; i < MSI_MAX / 32; ++i)
+		writel_relaxed(0, pcie->msi_enable + i * 4);
+
+	virq = platform_get_irq(pdev, 1);
+	if (virq <= 0) {
+		pr_err("Failed to map IRQ\n");
+		return -ENXIO;
+	}
+
+	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;
+	}
+
+	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;
+}
+
 /*** HOST BRIDGE SUPPORT ***/
 
 static int smp8759_config_read(struct pci_bus *bus,
@@ -88,6 +301,9 @@ static int tango_check_pcie_link(void __iomem *test_out)
 static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
 {
 	pcie->mux		= base + SMP8759_MUX;
+	pcie->msi_status	= base + SMP8759_STATUS;
+	pcie->msi_enable	= base + SMP8759_ENABLE;
+	pcie->msi_doorbell	= SMP8759_DOORBELL;
 
 	return tango_check_pcie_link(base + SMP8759_TEST_OUT);
 }
@@ -121,11 +337,21 @@ static int tango_pcie_probe(struct platform_device *pdev)
 	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,
-- 
2.11.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web