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 <[email protected]>
> ---
> 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...
The MSI controller in Tango supports 256 message-signaled interrupts,
and a single doorbell address.
Signed-off-by: Marc Gonzalez <[email protected]>
---
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
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 <[email protected]>
> ---
> 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...
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 <[email protected]>
>> ---
>> 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.
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 <[email protected]>
>>> ---
>>> 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...
The MSI controller in Tango supports 256 message-signaled interrupts,
and a single doorbell address.
Signed-off-by: Marc Gonzalez <[email protected]>
---
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
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 <[email protected]>
> ---
> 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)
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 <[email protected]>
>> ---
>> 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...
The MSI controller in Tango supports 256 message-signaled interrupts,
and a single doorbell address.
Signed-off-by: Marc Gonzalez <[email protected]>
---
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