On Mon, 20 Sep 2021 11:05:26 +0100,
Daniel Palmer <daniel@xxxxxxxx> wrote:
>
> Hi Marc,
>
> On Mon, 20 Sept 2021 at 18:45, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > > +static void ssd20xd_gpi_unmask_irq(struct irq_data *data)
> > > +{
> > > + irq_hw_number_t hwirq = irqd_to_hwirq(data);
> > > + struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data);
> > > + int offset_reg = REG_OFFSET(hwirq);
> > > + int offset_bit = BIT_OFFSET(hwirq);
> > > +
> > > + regmap_update_bits(gpi->regmap, REG_MASK + offset_reg, offset_bit, 0);
> >
> > Is this regmap call atomic? When running this, you are holding a
> > raw_spinlock already. From what I can see, this is unlikely to work
> > correctly with the current state of regmap.
>
> I didn't even think about it. I will check.
You may want to enable lockdep to verify that.
>
> > > +static void ssd20x_gpi_chainedhandler(struct irq_desc *desc)
> > > +{
> > > + struct ssd20xd_gpi *intc = irq_desc_get_handler_data(desc);
> > > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > > + unsigned int irqbit, hwirq, virq, status;
> > > + int i;
> > > +
> > > + chained_irq_enter(chip, desc);
> > > +
> > > + for (i = 0; i < NUM_IRQ / IRQS_PER_REG; i++) {
> > > + int offset_reg = STRIDE * i;
> > > + int offset_irq = IRQS_PER_REG * i;
> > > +
> > > + regmap_read(intc->regmap, REG_STATUS + offset_reg, &status);
> >
> > Does this act as an ACK in the HW?
>
> Not that I'm aware of. The status registers have the interrupt bits
> set until the EOI callback is called from what I can tell.
Then this doesn't work for edge signalling, as you will lose
interrupts that occur between the handling and EOI.
> Technically I think the EOI callback should actually be ACK instead
> but from my fuzzy memory with the stack of IRQ controllers that
> resulted in a null pointer dereference.
That's probably because you are using the wrong flow handler. You
should turn this irq_eoi into an irq_ack, because that's really what
it is, and use handle_edge_irq() as the flow handler.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
