Re: [PATCH 10/14] irqchip/mips-gic: Add a IPI hierarchy domain

From: Qais Yousef
Date: Mon Nov 09 2015 - 06:17:33 EST

On 11/07/2015 02:51 PM, Thomas Gleixner wrote:
On Tue, 3 Nov 2015, Qais Yousef wrote:

Add a new ipi domain on top of the normal domain.

MIPS GIC now supports dynamic allocation of an IPI.
I don't think you make use of the power of hierarchical irq
domains. You just whacked the current code into submission.

Let me explain it to you how that should look like and how that's
going to make the code way simpler.

The root domain is the GIC itself. It provides an allocation mechanism
for all GIC interrupts, global, ipi and per cpu plus the basic
management of them.

So the GIC domain looks at the complete hardware irq space. Now that
irq space is first partioned into local and shared interrupts.

------------- hwirq MAX

Shared interrupts

------------- hwirq 6

Local interrupts

------------- hwirq 0

So that shared interrupt space is where your device interrupts and the
ipi interrupts come from. That local interrupt space seems to be
hardwired, so it'd be overkill to handle that in an extra domain.

I assume that that shared interrupt space is partitioned as well
because the potential device interrupts on the SoC are hardwired. So
the picture looks like this:

------------- hwirq MAX

Shared assignable interrupts

------------- hwirq X

Shared device interrupts

------------- hwirq 6

Local interrupts

------------- hwirq 0


So if we look at the resulting hierarchy it looks like this:

|----- [IPI domain]
[ GIC domain] -
|----- [device domain]

The GIC domain manages a bitmap of the full irq space. The IPI domain
and the device domain request N interrupts from the GIC domain at
allocation time.

So when you allocate from the device domain, you tell the parent
domain, that this is actually a device interrupt, and from the IPI
domain you tell it it's an IPI.

So the allocator in the root domain can decide from which space of the
bitmap to allocate.

if (device) {
hwirq = translate_from_dt(arg);
if (test_and_set_bit(&allocated_irqs, hwirq))
return -EBUSY;
} else {
start = first_ipi_irq;
end = last_ipi_irq + 1;
hwirq = bitmap_find_next_zero_area(allocated_irqs, start, end,
nrirqs, 0);
}
....

So that gives you a consecutive hw irq space for your IPI.


This is potentially dangerous. If a device allocation happens after an IPI allocation, and that device wants the hwirq at the region that IPI just allocated because it thought it's free, it'd fail.

Generally it's hard to know whether a real device is connected to a hwirq or not. I am saving a patch where we get a set of free hwirqs from DT as only the SoC designer knows what hwirq are actually free and safe to use for IPI. I'll send this patch with the DT IPI changes or the rproc driver that I will be send once these changes are merged.

The current code assumes that the last 2 * NR_CPUs hwirqs are always free to use for Linux SMP.


That makes a lot of things simpler. You don't have to keep a mapping
of the hwirq to the target cpu. You just can use the base hwirq and
calculate the destination hwirq from there when sending an IPI
(general Linux ones). The coprocessor one will just be a natural
fallout.

Are you suggesting here to remove the whole new mapping API from the generic code or just that it's not necessary to use it in my case?


So if you have the following in the generic ipi code:

void ipi_send_single(unsigned int irq, unsigned int cpu)
{
struct irq_desc *desc = irq_to_desc(irq);
struct irq_data *data = irq_desc_get_irq_data(desc);
struct irq_chip *chip = irq_data_get_irq_chip(data);

if (chip->ipi_send_single)
chip->ipi_send_single(data, cpu);
else
chip->ipi_send_mask(data, cpumask_of(cpu));
}

void ipi_send_mask(unsigned int irq, const struct cpumask *dest)
{
struct irq_desc *desc = irq_to_desc(irq);
struct irq_data *data = irq_desc_get_irq_data(desc);
struct irq_chip *chip = irq_data_get_irq_chip(data);
int cpu;

if (!chip->ipi_send_mask) {
for_each_cpu(cpu, dest)
chip->ipi_send_single(data, cpu);
} else {
chip->ipi_send_mask(data, dest);
}
}

void ipi_send_coproc_mask(unsigned int irq, const struct ipi_mask *dest)
{
Fill in the obvious code.
}

And your ipi_send_single() callback just boils down to:
{
target = data->hw_irq + cpu;

tweak_chip_regs(target);
}

Sanity checks omitted for brevity.

I'm confused here as well. Is this a complementary API or are you suggesting replacing the one this patch introduces?


And that whole thing works for your case and for the case where we
only have a single per cpu interrupt descriptor allocated. The irq
descriptor based variants are exactly the same thing.

So now for the irq chips of your device and IPI domains. You can
either set the proper GIC chip variant or in case you need some extra
magic for one of the domains, you implement your own special chip
which can have some of its callback implemented by the existing
irq_***_parent() variants.

That gets rid of all your extra mappings, bitmaps and whatever add ons
you have duct taped into the existing GIC code.

This is all new territory for me. I thought I did it the correct way and it seemed simple to me.

I'll follow your idea through and see what I come up with. I'm getting a bit confused about how the generic API should look like now though. I'll keep what I added and add any new needed stuff and hopefully once I start doing the work and when the next series is ready it'd be more obvious what's needed and what's not.

Thanks,
Qais
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/