From: Marc Zyngier <maz@kernel.org> To: Daniel Palmer <daniel@0x0f.com> Cc: DTML <devicetree@vger.kernel.org>, Rob Herring <robh+dt@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, Romain Perier <romain.perier@gmail.com> Subject: Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi Date: Mon, 20 Sep 2021 12:24:27 +0100 [thread overview] Message-ID: <87wnnbv6ac.wl-maz@kernel.org> (raw) In-Reply-To: <CAFr9PXnC4hQw5_0TtciKvqF7s=4axJ5Yrq80RXGcY4VvT1Ac2A@mail.gmail.com> On Mon, 20 Sep 2021 11:05:26 +0100, Daniel Palmer <daniel@0x0f.com> wrote: > > Hi Marc, > > On Mon, 20 Sept 2021 at 18:45, Marc Zyngier <maz@kernel.org> 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.
WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org> To: Daniel Palmer <daniel@0x0f.com> Cc: DTML <devicetree@vger.kernel.org>, Rob Herring <robh+dt@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, Romain Perier <romain.perier@gmail.com> Subject: Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi Date: Mon, 20 Sep 2021 12:24:27 +0100 [thread overview] Message-ID: <87wnnbv6ac.wl-maz@kernel.org> (raw) In-Reply-To: <CAFr9PXnC4hQw5_0TtciKvqF7s=4axJ5Yrq80RXGcY4VvT1Ac2A@mail.gmail.com> On Mon, 20 Sep 2021 11:05:26 +0100, Daniel Palmer <daniel@0x0f.com> wrote: > > Hi Marc, > > On Mon, 20 Sept 2021 at 18:45, Marc Zyngier <maz@kernel.org> 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-09-20 11:24 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-14 10:04 [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller Daniel Palmer 2021-09-14 10:04 ` Daniel Palmer 2021-09-14 10:04 ` [PATCH 1/3] dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi Daniel Palmer 2021-09-14 10:04 ` Daniel Palmer 2021-09-21 20:36 ` Rob Herring 2021-09-21 20:36 ` Rob Herring 2021-09-14 10:04 ` [PATCH 2/3] irqchip: " Daniel Palmer 2021-09-14 10:04 ` Daniel Palmer 2021-09-20 9:45 ` Marc Zyngier 2021-09-20 9:45 ` Marc Zyngier 2021-09-20 10:05 ` Daniel Palmer 2021-09-20 10:05 ` Daniel Palmer 2021-09-20 11:24 ` Marc Zyngier [this message] 2021-09-20 11:24 ` Marc Zyngier 2021-09-21 4:16 ` Daniel Palmer 2021-09-21 4:16 ` Daniel Palmer 2021-09-21 8:27 ` Marc Zyngier 2021-09-21 8:27 ` Marc Zyngier 2021-09-21 18:23 ` Linus Walleij 2021-09-21 18:23 ` Linus Walleij 2021-09-30 12:39 ` Daniel Palmer 2021-09-30 12:39 ` Daniel Palmer 2021-09-30 13:07 ` Marc Zyngier 2021-09-30 13:07 ` Marc Zyngier 2021-09-30 13:10 ` Daniel Palmer 2021-09-30 13:10 ` Daniel Palmer 2021-09-30 13:06 ` Marc Zyngier 2021-09-30 13:06 ` Marc Zyngier 2021-09-30 13:36 ` Daniel Palmer 2021-09-30 13:36 ` Daniel Palmer 2021-09-30 13:53 ` Marc Zyngier 2021-09-30 13:53 ` Marc Zyngier 2021-09-30 13:59 ` Daniel Palmer 2021-09-30 13:59 ` Daniel Palmer 2021-09-30 14:11 ` Marc Zyngier 2021-09-30 14:11 ` Marc Zyngier 2021-09-30 16:10 ` Linus Walleij 2021-09-30 16:10 ` Linus Walleij 2021-09-30 16:13 ` Linus Walleij 2021-09-30 16:13 ` Linus Walleij 2021-10-01 12:33 ` Daniel Palmer 2021-10-01 12:33 ` Daniel Palmer 2021-10-02 3:08 ` Daniel Palmer 2021-10-02 3:08 ` Daniel Palmer 2021-09-21 6:11 ` Daniel Palmer 2021-09-21 6:11 ` Daniel Palmer 2021-09-14 10:04 ` [PATCH 3/3] ARM: dts: mstar: Add gpi interrupt controller to i2m Daniel Palmer 2021-09-14 10:04 ` Daniel Palmer 2021-09-14 15:59 ` [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller Andrew Lunn 2021-09-14 15:59 ` Andrew Lunn 2021-09-15 9:06 ` Daniel Palmer 2021-09-15 9:06 ` Daniel Palmer 2021-09-15 20:34 ` Andrew Lunn 2021-09-15 20:34 ` Andrew Lunn 2021-09-20 8:36 ` Daniel Palmer 2021-09-20 8:36 ` Daniel Palmer
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87wnnbv6ac.wl-maz@kernel.org \ --to=maz@kernel.org \ --cc=daniel@0x0f.com \ --cc=devicetree@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=robh+dt@kernel.org \ --cc=romain.perier@gmail.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.