[PATCH net-next 0/6] Fixes for the MV88e6xxx interrupt code

[PATCH net-next 0/6] Fixes for the MV88e6xxx interrupt code

[Andrew Lunn]

The interrupt code was never tested with a board who's probing
resulted in an -EPROBE_DEFFERED. So the clean up paths never got
tested. I now do have -EPROBE_DEFFERED, and things break badly during
cleanup. These are the fixes.

This is fixing code in net-next.

Andrew Lunn (6):
  net: dsa: mv88e6xxx: Fix typos when removing g1 interrupts
  net: dsa: mv88e6xxx: Fix unconditional irq freeing
  net: dsa: mv88e6xxx: Mask g1 interrupts and free interrupt
  net: dsa: mv88e6xxx: Fix releasing for the global2 interrupts
  net: dsa: mv88e6xxx: Fix cleanup on error for g1 interrupt setup
  net: dsa: mv88e6xxx: Hold the mutex while freeing g1 interrupts

 drivers/net/dsa/mv88e6xxx/chip.c      | 58 ++++++++++++++++++++++++-----------
 drivers/net/dsa/mv88e6xxx/global2.c   | 25 ++++++++++-----
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h |  1 +
 3 files changed, 58 insertions(+), 26 deletions(-)

-- 
2.10.2

[PATCH net-next 1/6] net: dsa: mv88e6xxx: Fix typos when removing g1 interrupts

[Andrew Lunn]

Simple typos, s/g2/g1/

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index d6d9d66b81ce..6aa81d2d8f63 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -415,11 +415,11 @@ static void mv88e6xxx_g1_irq_free(struct mv88e6xxx_chip *chip)
 	int irq, virq;
 
 	for (irq = 0; irq < 16; irq++) {
-		virq = irq_find_mapping(chip->g2_irq.domain, irq);
+		virq = irq_find_mapping(chip->g1_irq.domain, irq);
 		irq_dispose_mapping(virq);
 	}
 
-	irq_domain_remove(chip->g2_irq.domain);
+	irq_domain_remove(chip->g1_irq.domain);
 }
 
 static int mv88e6xxx_g1_irq_setup(struct mv88e6xxx_chip *chip)
-- 
2.10.2

[PATCH net-next 2/6] net: dsa: mv88e6xxx: Fix unconditional irq freeing

[Andrew Lunn]

Trying to remove an IRQ domain that was not created results in an
Opps. Add the necessary checks that the irqs were created before
freeing them.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 6aa81d2d8f63..b843052d32bd 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3897,10 +3897,11 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 out_mdio:
 	mv88e6xxx_mdio_unregister(chip);
 out_g2_irq:
-	if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_G2_INT))
+	if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_G2_INT) && chip->irq > 0)
 		mv88e6xxx_g2_irq_free(chip);
 out_g1_irq:
-	mv88e6xxx_g1_irq_free(chip);
+	if (chip->irq > 0)
+		mv88e6xxx_g1_irq_free(chip);
 out:
 	return err;
 }
@@ -3914,9 +3915,11 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
 	mv88e6xxx_unregister_switch(chip);
 	mv88e6xxx_mdio_unregister(chip);
 
-	if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_G2_INT))
-		mv88e6xxx_g2_irq_free(chip);
-	mv88e6xxx_g1_irq_free(chip);
+	if (chip->irq > 0) {
+		if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_G2_INT))
+			mv88e6xxx_g2_irq_free(chip);
+		mv88e6xxx_g1_irq_free(chip);
+	}
 }
 
 static const struct of_device_id mv88e6xxx_of_match[] = {
-- 
2.10.2

[PATCH net-next 3/6] net: dsa: mv88e6xxx: Mask g1 interrupts and free interrupt

[Andrew Lunn]

Fix the g1 interrupt free code such that is masks any further
interrupts, and then releases the interrupt.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index b843052d32bd..8fcef7e0d3ba 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -413,6 +413,13 @@ static const struct irq_domain_ops mv88e6xxx_g1_irq_domain_ops = {
 static void mv88e6xxx_g1_irq_free(struct mv88e6xxx_chip *chip)
 {
 	int irq, virq;
+	u16 mask;
+
+	mv88e6xxx_g1_read(chip, GLOBAL_CONTROL, &mask);
+	mask |= GENMASK(chip->g1_irq.nirqs, 0);
+	mv88e6xxx_g1_write(chip, GLOBAL_CONTROL, mask);
+
+	free_irq(chip->irq, chip);
 
 	for (irq = 0; irq < 16; irq++) {
 		virq = irq_find_mapping(chip->g1_irq.domain, irq);
-- 
2.10.2

[PATCH net-next 4/6] net: dsa: mv88e6xxx: Fix releasing for the global2 interrupts

[Andrew Lunn]

It is not possible to use devm_request_threaded_irq() because we have
two stacked interrupt controllers in one device. The lower interrupt
controller cannot be removed until the upper is fully removed. This
happens too late with the devm API, resulting in error messages about
removing a domain while there is still an active interrupt. Swap to
using request_threaded_irq() and manage the release of the interrupt
manually.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/global2.c   | 25 +++++++++++++++++--------
 drivers/net/dsa/mv88e6xxx/mv88e6xxx.h |  1 +
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c
index 1a0b13521d13..703d68e92f76 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.c
+++ b/drivers/net/dsa/mv88e6xxx/global2.c
@@ -507,6 +507,9 @@ void mv88e6xxx_g2_irq_free(struct mv88e6xxx_chip *chip)
 {
 	int irq, virq;
 
+	free_irq(chip->device_irq, chip);
+	irq_dispose_mapping(chip->device_irq);
+
 	for (irq = 0; irq < 16; irq++) {
 		virq = irq_find_mapping(chip->g2_irq.domain, irq);
 		irq_dispose_mapping(virq);
@@ -518,7 +521,7 @@ void mv88e6xxx_g2_irq_free(struct mv88e6xxx_chip *chip)
 int mv88e6xxx_g2_irq_setup(struct mv88e6xxx_chip *chip)
 {
 	int device_irq;
-	int err, irq;
+	int err, irq, virq;
 
 	if (!chip->dev->of_node)
 		return -EINVAL;
@@ -534,22 +537,28 @@ int mv88e6xxx_g2_irq_setup(struct mv88e6xxx_chip *chip)
 	chip->g2_irq.chip = mv88e6xxx_g2_irq_chip;
 	chip->g2_irq.masked = ~0;
 
-	device_irq = irq_find_mapping(chip->g1_irq.domain,
-				      GLOBAL_STATUS_IRQ_DEVICE);
-	if (device_irq < 0) {
+	chip->device_irq = irq_find_mapping(chip->g1_irq.domain,
+					    GLOBAL_STATUS_IRQ_DEVICE);
+	if (chip->device_irq < 0) {
 		err = device_irq;
 		goto out;
 	}
 
-	err = devm_request_threaded_irq(chip->dev, device_irq, NULL,
-					mv88e6xxx_g2_irq_thread_fn,
-					IRQF_ONESHOT, "mv88e6xxx-g1", chip);
+	err = request_threaded_irq(chip->device_irq, NULL,
+				   mv88e6xxx_g2_irq_thread_fn,
+				   IRQF_ONESHOT, "mv88e6xxx-g1", chip);
 	if (err)
 		goto out;
 
 	return 0;
+
 out:
-	mv88e6xxx_g2_irq_free(chip);
+	for (irq = 0; irq < 16; irq++) {
+		virq = irq_find_mapping(chip->g2_irq.domain, irq);
+		irq_dispose_mapping(virq);
+	}
+
+	irq_domain_remove(chip->g2_irq.domain);
 
 	return err;
 }
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index 929613021eff..a3869504f881 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -714,6 +714,7 @@ struct mv88e6xxx_chip {
 	struct mv88e6xxx_irq g1_irq;
 	struct mv88e6xxx_irq g2_irq;
 	int irq;
+	int device_irq;
 };
 
 struct mv88e6xxx_bus_ops {
-- 
2.10.2

[PATCH net-next 5/6] net: dsa: mv88e6xxx: Fix cleanup on error for g1 interrupt setup

[Andrew Lunn]

On error, remask the interrupts, release all maps, and remove the
domain. This cannot be done using the mv88e6xxx_g1_irq_free() because
some of these actions are not idempotent.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 8fcef7e0d3ba..614b2f68d401 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -431,8 +431,8 @@ static void mv88e6xxx_g1_irq_free(struct mv88e6xxx_chip *chip)
 
 static int mv88e6xxx_g1_irq_setup(struct mv88e6xxx_chip *chip)
 {
-	int err, irq;
-	u16 reg;
+	int err, irq, virq;
+	u16 reg, mask;
 
 	chip->g1_irq.nirqs = chip->info->g1_irqs;
 	chip->g1_irq.domain = irq_domain_add_simple(
@@ -447,32 +447,41 @@ static int mv88e6xxx_g1_irq_setup(struct mv88e6xxx_chip *chip)
 	chip->g1_irq.chip = mv88e6xxx_g1_irq_chip;
 	chip->g1_irq.masked = ~0;
 
-	err = mv88e6xxx_g1_read(chip, GLOBAL_CONTROL, &reg);
+	err = mv88e6xxx_g1_read(chip, GLOBAL_CONTROL, &mask);
 	if (err)
-		goto out;
+		goto out_mapping;
 
-	reg &= ~GENMASK(chip->g1_irq.nirqs, 0);
+	mask &= ~GENMASK(chip->g1_irq.nirqs, 0);
 
-	err = mv88e6xxx_g1_write(chip, GLOBAL_CONTROL, reg);
+	err = mv88e6xxx_g1_write(chip, GLOBAL_CONTROL, mask);
 	if (err)
-		goto out;
+		goto out_disable;
 
 	/* Reading the interrupt status clears (most of) them */
 	err = mv88e6xxx_g1_read(chip, GLOBAL_STATUS, &reg);
 	if (err)
-		goto out;
+		goto out_disable;
 
 	err = request_threaded_irq(chip->irq, NULL,
 				   mv88e6xxx_g1_irq_thread_fn,
 				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
 				   dev_name(chip->dev), chip);
 	if (err)
-		goto out;
+		goto out_disable;
 
 	return 0;
 
-out:
-	mv88e6xxx_g1_irq_free(chip);
+out_disable:
+	mask |= GENMASK(chip->g1_irq.nirqs, 0);
+	mv88e6xxx_g1_write(chip, GLOBAL_CONTROL, 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;
 }
-- 
2.10.2

[PATCH net-next 6/6] net: dsa: mv88e6xxx: Hold the mutex while freeing g1 interrupts

[Andrew Lunn]

Freeing interrupts requires switch register access to mask the
interrupts. Hence we must hold the register mutex.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 614b2f68d401..e30d0eaf2b5f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3916,8 +3916,11 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_G2_INT) && chip->irq > 0)
 		mv88e6xxx_g2_irq_free(chip);
 out_g1_irq:
-	if (chip->irq > 0)
+	if (chip->irq > 0) {
+		mutex_lock(&chip->reg_lock);
 		mv88e6xxx_g1_irq_free(chip);
+		mutex_unlock(&chip->reg_lock);
+	}
 out:
 	return err;
 }
-- 
2.10.2

Re: [PATCH net-next 0/6] Fixes for the MV88e6xxx interrupt code

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Wed, 16 Nov 2016 01:56:50 +0100

> The interrupt code was never tested with a board who's probing
> resulted in an -EPROBE_DEFFERED. So the clean up paths never got
> tested. I now do have -EPROBE_DEFFERED, and things break badly during
> cleanup. These are the fixes.
> 
> This is fixing code in net-next.

Series applied, thanks Andrew.

Re: [PATCH net-next 0/6] Fixes for the MV88e6xxx interrupt code

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 16 Nov 2016 14:21:02 -0500 (EST)

> From: Andrew Lunn <andrew@lunn.ch>
> Date: Wed, 16 Nov 2016 01:56:50 +0100
> 
>> The interrupt code was never tested with a board who's probing
>> resulted in an -EPROBE_DEFFERED. So the clean up paths never got
>> tested. I now do have -EPROBE_DEFFERED, and things break badly during
>> cleanup. These are the fixes.
>> 
>> This is fixing code in net-next.
> 
> Series applied, thanks Andrew.

Actually, I reverted, there is a bug.

Take a look at how the 'device_irq' local variable is used in
mv88e6xxx_g2_irq_setup.  You assign it to 'err' in an error
path before it is ever set to anything.

I think you meant to use the structure's 'device_irq' member
instead.

Re: [PATCH net-next 0/6] Fixes for the MV88e6xxx interrupt code

[Andrew Lunn]

> Take a look at how the 'device_irq' local variable is used in
> mv88e6xxx_g2_irq_setup.  You assign it to 'err' in an error
> path before it is ever set to anything.
> 
> I think you meant to use the structure's 'device_irq' member
> instead.

Hi David

Agreed. Thanks for the review.

	Andrew