2017-09-26 17:52:25

by Vivien Didelot

Subject: [PATCH net] net: dsa: mv88e6xxx: lock mutex when freeing IRQs

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 <[email protected]>
---
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);

--
2.14.1

2017-09-26 18:01:42

by Andrew Lunn

Subject: Re: [PATCH net] net: dsa: mv88e6xxx: lock mutex when freeing IRQs

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 <[email protected]>
> ---
> 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

2017-09-26 18:04:50

by Vivien Didelot

Subject: Re: [PATCH net] net: dsa: mv88e6xxx: lock mutex when freeing IRQs

Hi Andrew,

Andrew Lunn <[email protected]> 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