netdev - [PATCH] net: phylink: dsa: mv88e6xxx: Revise irq setup ordering

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <53b49df8-53ed-704f-9197-230b18d83090@bell.net>
Date:   Mon, 4 Feb 2019 13:37:13 -0500
From:   John David Anglin <dave.anglin@...l.net>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Russell King <linux@....linux.org.uk>,
        Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org
Subject: [PATCH] net: phylink: dsa: mv88e6xxx: Revise irq setup ordering

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.

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;
 
     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, &reg);
+    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, &reg);
+    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, &reg);
+    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, &reg);
-    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@...l.net>
-- 

John David Anglin  dave.anglin@...l.net