| Message ID | 53b49df8-53ed-704f-9197-230b18d83090@bell.net |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phylink: dsa: mv88e6xxx: Revise irq setup ordering | expand |
On Mon, Feb 04, 2019 at 01:37:13PM -0500, John David Anglin wrote: > This change fixes a race condition in the setup of hardware irqs and the > code enabling PHY link > detection. > > This was observed on the espressobin board where the GPIO interrupt > controller only supports edge > interrupts. If the INTn output pin goes low before the GPIO interrupt > is enabled, PHY link interrupts > are not detected. Hi David Please break this up into two patches. Masking interrupts in the setup code before enabling the interrupt i'm happy with. The change to the interrupt handler i'm pretty sure is wrong. You have to accept with edge interrupts you are going to loose interrupts. Andrew
Hi Andrew, On 2019-02-04 2:35 p.m., Andrew Lunn wrote: > The change to the interrupt handler i'm pretty sure is wrong. You have > to accept with edge interrupts you are going to loose interrupts. Can you be more specific regarding what you think is wrong with this hunk? I can see that an interrupt might be handled twice if the source wasn't cleared before interrupts are re-enabled but I think that would also occur with level interrupts. Dave
> Can you be more specific regarding what you think is wrong with this hunk?
Hi David
The IRQ core would do this if it was needed.
How many other irq thread work functions can you point to which do
something similar?
Andrew
On 2019-02-04 3:19 p.m., Andrew Lunn wrote: > The IRQ core would do this if it was needed. > > How many other irq thread work functions can you point to which do > something similar? This is comment for handle_edge_irq: /** * handle_edge_irq - edge type IRQ handler * @desc: the interrupt description structure for this irq * * Interrupt occures on the falling and/or rising edge of a hardware * signal. The occurrence is latched into the irq controller hardware * and must be acked in order to be reenabled. After the ack another * interrupt can happen on the same source even before the first one * is handled by the associated event handler. If this happens it * might be necessary to disable (mask) the interrupt depending on the * controller hardware. This requires to reenable the interrupt inside * of the loop which handles the interrupts which have arrived while * the handler was running. If all pending interrupts are handled, the * loop is left. */ As can be seen, the above comment suggests that it may be necessary to disable (mask) interrupt as I proposed. I see no evidence from the Marvell functional specifications for the 88E6341 that it sequences interrupts from the various sources although it might be that device interrupts are sequenced so INTn rises and falls. I haven't seen any ports fail to link without the hunk on espressobin but it is hard to stress test the code. Disabling and re-enabling interrupts in the global control register does not affect their status. Thus, at worst, the hunk adds a bit of unnecessary code. It could be skipped if we knew we were using level interrupts. Dave
On Mon, Feb 04, 2019 at 04:38:53PM -0500, John David Anglin wrote: > On 2019-02-04 3:19 p.m., Andrew Lunn wrote: > > The IRQ core would do this if it was needed. > > > > How many other irq thread work functions can you point to which do > > something similar? > This is comment for handle_edge_irq: > > /** > * handle_edge_irq - edge type IRQ handler > * @desc: the interrupt description structure for this irq > * > * Interrupt occures on the falling and/or rising edge of a hardware > * signal. The occurrence is latched into the irq controller hardware > * and must be acked in order to be reenabled. After the ack another > * interrupt can happen on the same source even before the first one > * is handled by the associated event handler. If this happens it > * might be necessary to disable (mask) the interrupt depending on the > * controller hardware. This requires to reenable the interrupt inside > * of the loop which handles the interrupts which have arrived while > * the handler was running. If all pending interrupts are handled, the > * loop is left. > */ > > As can be seen, the above comment suggests that it may be necessary to > disable (mask) interrupt > as I proposed. Hi Dave This comment is describing what handle_edge_irq() actually does. Read the code. It does not say anything about that the handling thread function should do. Andrew
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index b2a0e59b6252..0dcbc25c1b4b 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -260,7 +260,7 @@ static irqreturn_t mv88e6xxx_g1_irq_thread_work(struct mv88e6xxx_chip *chip) unsigned int nhandled = 0; unsigned int sub_irq; unsigned int n; - u16 reg; + u16 mask, reg; int err;
This change fixes a race condition in the setup of hardware irqs and the code enabling PHY link detection. This was observed on the espressobin board where the GPIO interrupt controller only supports edge interrupts. If the INTn output pin goes low before the GPIO interrupt is enabled, PHY link interrupts are not detected. With this change, we 1) force INTn high by clearing all interrupt enables in global 1 control1, 2) setup the hardware irq, and 3) perform the remaining common setup. This in fact simplifies the setup and allows some unnecessary code to be removed. In order to prevent races in mv88e6xxx_g1_irq_thread_work(), we clear and restore the interrupt enables in the normal path just prior to exit. mutex_lock(&chip->reg_lock); @@ -277,6 +277,14 @@ static irqreturn_t mv88e6xxx_g1_irq_thread_work(struct mv88e6xxx_chip *chip) ++nhandled; } } + + mutex_lock(&chip->reg_lock); + mv88e6xxx_g1_read(chip, MV88E6XXX_G1_CTL1, ®); + mask = ~GENMASK(chip->g1_irq.nirqs, 0); + mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, (reg & mask)); + mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, reg); + mutex_unlock(&chip->reg_lock); + out: return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE); } @@ -374,10 +382,30 @@ static void mv88e6xxx_g1_irq_free(struct mv88e6xxx_chip *chip) mutex_unlock(&chip->reg_lock); } +static int mv88e6xxx_g1_irq_setup_masks(struct mv88e6xxx_chip *chip) +{ + int err; + u16 reg; + + /* The INTn output must be high when hardware interrupts are setup. + The EEPROM done interrupt enable is set on reset, so clear all + interrupt enable bits to ensure INTn is not driven low */ + err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_CTL1, ®); + if (err) + return err; + reg &= ~GENMASK(chip->info->g1_irqs, 0); + err = mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, reg); + if (err) + return err; + + /* Reading the interrupt status clears (most of) them */ + err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_STS, ®); + return err; +} + static int mv88e6xxx_g1_irq_setup_common(struct mv88e6xxx_chip *chip) { - int err, irq, virq; - u16 reg, mask; + int irq; chip->g1_irq.nirqs = chip->info->g1_irqs; chip->g1_irq.domain = irq_domain_add_simple( @@ -392,43 +420,14 @@ static int mv88e6xxx_g1_irq_setup_common(struct mv88e6xxx_chip *chip) chip->g1_irq.chip = mv88e6xxx_g1_irq_chip; chip->g1_irq.masked = ~0; - err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_CTL1, &mask); - if (err) - goto out_mapping; - - mask &= ~GENMASK(chip->g1_irq.nirqs, 0); - - err = mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, mask); - if (err) - goto out_disable; - - /* Reading the interrupt status clears (most of) them */ - err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_STS, ®); - if (err) - goto out_disable; - return 0; - -out_disable: - mask &= ~GENMASK(chip->g1_irq.nirqs, 0); - mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, mask); - -out_mapping: - for (irq = 0; irq < 16; irq++) { - virq = irq_find_mapping(chip->g1_irq.domain, irq); - irq_dispose_mapping(virq); - } - - irq_domain_remove(chip->g1_irq.domain); - - return err; } static int mv88e6xxx_g1_irq_setup(struct mv88e6xxx_chip *chip) { int err; - err = mv88e6xxx_g1_irq_setup_common(chip); + err = mv88e6xxx_g1_irq_setup_masks(chip); if (err) return err; @@ -437,9 +436,9 @@ static int mv88e6xxx_g1_irq_setup(struct mv88e6xxx_chip *chip) IRQF_ONESHOT | IRQF_SHARED, dev_name(chip->dev), chip); if (err) - mv88e6xxx_g1_irq_free_common(chip); + return err; - return err; + return mv88e6xxx_g1_irq_setup_common(chip); } static void mv88e6xxx_irq_poll(struct kthread_work *work) @@ -457,6 +456,10 @@ static int mv88e6xxx_irq_poll_setup(struct mv88e6xxx_chip *chip) { int err; + err = mv88e6xxx_g1_irq_setup_masks(chip); + if (err) + return err; + err = mv88e6xxx_g1_irq_setup_common(chip); if (err) return err; Signed-off-by: John David Anglin <dave.anglin@bell.net>