On 29/03/2017 14:29, Marc Zyngier wrote:
> On 29/03/17 12:29, 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@xxxxxxxxxxxxxxxx>
>> ---
>> Changes since v0.2
>> - Support 256 MSIs instead of only 32
>> - Use spinlock_t instead of struct mutex
>> - Add MSI_FLAG_PCI_MSIX flag
>>
>> IRQs are acked in tango_msi_isr because handle_simple_irq leaves
>> ack, clear, mask and unmask up to the driver. For the same reason,
>> interrupt enable mask is updated from tango_irq_domain_alloc/free.
>
> I've asked you to move this to individual methods. You've decided not
> to, and that's your call. But I now wonder why I'm even bothering to
> review this, as you've so far just wasted my time.
I misunderstood what you wrote. When you pointed out the comment at
the top of handle_simple_irq (which I mentioned in my above blurb)
I took that to mean that I had to follow those instructions.
Judging by what you wrote below, I must replace handle_simple_irq
with handle_edge_irq, which will call the irq_chip callbacks.
But I don't understand how to get my pcie pointer back in irq_ack
or irq_unmask, or the relevant msi. Can you throw me a clue?
>> +static struct irq_chip tango_msi_irq_chip = {
>> + .name = "MSI",
>> + .irq_mask = pci_msi_mask_irq,
>> + .irq_unmask = pci_msi_unmask_irq,
>
> How do you make that work if the PCI device doesn't support per-MSI masking?
It seems you're saying this code is broken. Is it functional
in the Altera driver, and I did something to break it?
>> +static int find_free_msi(struct irq_domain *dom, unsigned int virq)
>> +{
>> + u32 val;
>> + struct tango_pcie *pcie = dom->host_data;
>> + unsigned int offset, pos;
>> +
>> + pos = find_first_zero_bit(pcie->bitmap, MSI_MAX);
>> + if (pos >= MSI_MAX)
>> + return -ENOSPC;
>> +
>> + offset = (pos / 32) * 4;
>> + val = readl_relaxed(pcie->msi_mask + offset);
>> + writel_relaxed(val | BIT(pos % 32), pcie->msi_mask + offset);
>
> Great. I'm now in a position where I can take an interrupt (because of
> the broken locking that doesn't disable interrupts), but the bitmap
> doesn't indicate it yet. With a bit of luck, I'll never make any forward
> progress.
Is this the Yoda way to say:
"Hey moron, use spin_lock_irqsave instead of spin_lock"?
>> + irq_domain_set_info(dom, virq, pos, &tango_msi_chip,
>> + dom->host_data, handle_simple_irq, NULL, NULL);
>
> I've told you a number of times that PCI MSIs are edge triggered...
I will register handle_edge_irq.
> So there is not much progress from the previous version. It is just
> broken in a different ways, and ignores most of the work that is already
> done in the irqchip core.
I wish nothing more than to be able to use as much infrastructure
as possible, in order to write as little code as possible.
Regards.
