Messages in this thread | | | Date | Wed, 26 Nov 2014 11:25:44 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH v11 1/2] irqchip: gicv2m: Add support for ARM GICv2m MSI(-X) doorbell |
| |
Hi Mark,
On 26/11/14 11:11, Mark Rutland wrote: > Hi Marc, > > On Tue, Nov 25, 2014 at 06:47:22PM +0000, Marc Zyngier wrote: >> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> >> ARM GICv2m specification extends GICv2 to support MSI(-X) with >> a new register frame. This allows a GICv2 based system to support >> MSI with minimal changes. >> >> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> [maz: converted the driver to use stacked irq domains, >> updated changelog] >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> arch/arm64/Kconfig | 1 + >> drivers/irqchip/Kconfig | 6 + >> drivers/irqchip/Makefile | 1 + >> drivers/irqchip/irq-gic-v2m.c | 333 ++++++++++++++++++++++++++++++++++++++++ >> drivers/irqchip/irq-gic.c | 4 + >> include/linux/irqchip/arm-gic.h | 2 + >> 6 files changed, 347 insertions(+) >> create mode 100644 drivers/irqchip/irq-gic-v2m.c > > [...] > >> +static int gicv2m_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, >> + unsigned int nr_irqs, void *args) >> +{ >> + struct v2m_data *v2m = domain->host_data; >> + int hwirq, offset, err = 0; >> + >> + spin_lock(&v2m->msi_cnt_lock); >> + offset = find_first_zero_bit(v2m->bm, v2m->nr_spis); >> + if (offset < v2m->nr_spis) >> + __set_bit(offset, v2m->bm); >> + else >> + err = -ENOSPC; >> + spin_unlock(&v2m->msi_cnt_lock); >> + >> + if (err) >> + return err; >> + >> + hwirq = v2m->spi_start + offset; >> + >> + err = gicv2m_irq_gic_domain_alloc(domain, virq, hwirq); >> + if (err) { >> + gicv2m_unalloc_msi(v2m, hwirq); >> + return err; >> + } >> + >> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, >> + &gicv2m_irq_chip, v2m); >> + >> + return 0; >> +} >> + >> +static void gicv2m_irq_domain_free(struct irq_domain *domain, >> + unsigned int virq, unsigned int nr_irqs) >> +{ >> + struct irq_data *d = irq_domain_get_irq_data(domain, virq); >> + struct v2m_data *v2m = irq_data_get_irq_chip_data(d); >> + >> + BUG_ON(nr_irqs != 1); > > We didn't check nr_irqs at all in gicv2m_irq_domain_alloc, which seems a > bit odd given this BUG_ON. > > Is the caller responsible for checking we only allocated one irq, or is > it only valid to ask for one?
We already have a strong guarantee from the alloc path that we'll never see nr_irqs != 1, as we don't support MULTI_MSI just yet.
This BUG_ON() is more a debug leftover.
>> + gicv2m_unalloc_msi(v2m, d->hwirq); >> + irq_domain_free_irqs_parent(domain, virq, nr_irqs); >> +} >> + >> +static const struct irq_domain_ops gicv2m_domain_ops = { >> + .alloc = gicv2m_irq_domain_alloc, >> + .free = gicv2m_irq_domain_free, >> +}; >> + >> +static bool is_msi_spi_valid(u32 base, u32 num) >> +{ >> + if (base < V2M_MIN_SPI) { >> + pr_err("Invalid MSI base SPI (base:%u)\n", base); >> + return false; >> + } >> + >> + if ((num == 0) || (base + num > V2M_MAX_SPI)) { >> + pr_err("Number of SPIs (%u) exceed maximum (%u)\n", >> + num, V2M_MAX_SPI - V2M_MIN_SPI + 1); > > That warning is a bit odd for the num == 0 case. Perhaps > s/exceed/invalid,/ ?
Sure.
>> + return false; >> + } >> + >> + return true; >> +} >> + > > [...] > >> +static int __init gicv2m_init_one(struct device_node *node, >> + struct irq_domain *parent) >> +{ >> + int ret; >> + struct v2m_data *v2m; >> + >> + v2m = kzalloc(sizeof(struct v2m_data), GFP_KERNEL); > > Minor nit: sizeof(*v2m) would be preferable. > >> + if (!v2m) { >> + pr_err("Failed to allocate struct v2m_data.\n"); >> + return -ENOMEM; >> + } >> + >> + ret = of_address_to_resource(node, 0, &v2m->res); >> + if (ret) { >> + pr_err("Failed to allocate v2m resource.\n"); >> + goto err_free_v2m; >> + } >> + >> + v2m->base = ioremap(v2m->res.start, resource_size(&v2m->res)); >> + if (!v2m->base) { >> + pr_err("Failed to map GICv2m resource\n"); >> + ret = -ENOMEM; >> + goto err_free_v2m; >> + } >> + >> + if (!of_property_read_u32(node, "arm,msi-base-spi", &v2m->spi_start) && >> + !of_property_read_u32(node, "arm,msi-num-spis", &v2m->nr_spis)) { >> + pr_info("Overriding V2M MSI_TYPER (base:%u, num:%u)\n", >> + v2m->spi_start, v2m->nr_spis); > > It would be nice if we could warn if only one of these properties is > present.
Yup. I'll tighten that in an additional patch, as Jason has already queued this.
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |