| Message ID | 20170926174837.15999-1-vivien.didelot@savoirfairelinux.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: dsa: mv88e6xxx: lock mutex when freeing IRQs | expand |
Hi Andrew, Andrew Lunn <andrew@lunn.ch> writes: > In general, i tried to keep the mutex out of the interrupt code. That > is not totally possible, the IRQ thread handler needs it. But > otherwise, the IRQ code assumes it is called with the mutex taken. > > So i think it is better to hold the mutex in mv88e6xxx_remove() when > calling mv88e6xxx_g1_irq_free(). I'd prefer that too. Respinning, thanks! Vivien
On Tue, Sep 26, 2017 at 01:48:37PM -0400, Vivien Didelot wrote: > mv88e6xxx_g2_irq_free locks the registers mutex, but not > mv88e6xxx_g1_irq_free, which results in a stack trace from > assert_reg_lock when unloading the mv88e6xxx module. Fix this. > > Fixes: 3460a5770ce9 ("net: dsa: mv88e6xxx: Mask g1 interrupts and free interrupt") > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> > --- > drivers/net/dsa/mv88e6xxx/chip.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index c6678aa9b4ef..b4359e4e5165 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -338,9 +338,11 @@ static void mv88e6xxx_g1_irq_free(struct mv88e6xxx_chip *chip) > int irq, virq; > u16 mask; > > + mutex_lock(&chip->reg_lock); > mv88e6xxx_g1_read(chip, MV88E6XXX_G1_CTL1, &mask); > mask |= GENMASK(chip->g1_irq.nirqs, 0); > mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, mask); > + mutex_unlock(&chip->reg_lock); > > free_irq(chip->irq, chip); > Hi Vivien static int mv88e6xxx_probe(struct mdio_device *mdiodev) { ... out_g1_irq: if (chip->irq > 0) { mutex_lock(&chip->reg_lock); mv88e6xxx_g1_irq_free(chip); mutex_unlock(&chip->reg_lock); } It looks like this will deadlock? In general, i tried to keep the mutex out of the interrupt code. That is not totally possible, the IRQ thread handler needs it. But otherwise, the IRQ code assumes it is called with the mutex taken. So i think it is better to hold the mutex in mv88e6xxx_remove() when calling mv88e6xxx_g1_irq_free(). Andrew
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index c6678aa9b4ef..b4359e4e5165 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -338,9 +338,11 @@ static void mv88e6xxx_g1_irq_free(struct mv88e6xxx_chip *chip) int irq, virq; u16 mask; + mutex_lock(&chip->reg_lock); mv88e6xxx_g1_read(chip, MV88E6XXX_G1_CTL1, &mask); mask |= GENMASK(chip->g1_irq.nirqs, 0); mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, mask); + mutex_unlock(&chip->reg_lock); free_irq(chip->irq, chip);
mv88e6xxx_g2_irq_free locks the registers mutex, but not mv88e6xxx_g1_irq_free, which results in a stack trace from assert_reg_lock when unloading the mv88e6xxx module. Fix this. Fixes: 3460a5770ce9 ("net: dsa: mv88e6xxx: Mask g1 interrupts and free interrupt") Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> --- drivers/net/dsa/mv88e6xxx/chip.c | 2 ++ 1 file changed, 2 insertions(+)