| Message ID | 1471874464-16142-4-git-send-email-andrew@lunn.ch |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Andrew, Andrew Lunn <andrew@lunn.ch> writes: > The PPU method of accessing PHYs makes use of a timer. Make sure this > timer is deleted before unloading the driver. > > Reported-by: Jamie Lentin <jm@lentin.co.uk> > Signed-off-by: Andrew Lunn <andrew@lunn.ch> I'm wondering if the PPU shouldn't be disabled before unloading the module... Otherwise: Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> Thanks, Vivien
On Mon, Aug 22, 2016 at 10:51:03AM -0400, Vivien Didelot wrote: > Hi Andrew, > > Andrew Lunn <andrew@lunn.ch> writes: > > > The PPU method of accessing PHYs makes use of a timer. Make sure this > > timer is deleted before unloading the driver. > > > > Reported-by: Jamie Lentin <jm@lentin.co.uk> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > I'm wondering if the PPU shouldn't be disabled before unloading the > module... Humm, i did not thing about that. Actually, we have a general issue here. On unload, we leave the switch 'running'. The SF2 driver disables all the ports. The b53 seems to leave the switch running. We should define a general policy about this, and implement it in all drivers. As such, your comment is valid, but a different patch set. Andrew
Hi Andrew, Andrew Lunn <andrew@lunn.ch> writes: > On Mon, Aug 22, 2016 at 10:51:03AM -0400, Vivien Didelot wrote: >> Hi Andrew, >> >> Andrew Lunn <andrew@lunn.ch> writes: >> >> > The PPU method of accessing PHYs makes use of a timer. Make sure this >> > timer is deleted before unloading the driver. >> > >> > Reported-by: Jamie Lentin <jm@lentin.co.uk> >> > Signed-off-by: Andrew Lunn <andrew@lunn.ch> >> >> I'm wondering if the PPU shouldn't be disabled before unloading the >> module... > > Humm, i did not thing about that. > > Actually, we have a general issue here. On unload, we leave the switch > 'running'. The SF2 driver disables all the ports. The b53 seems to > leave the switch running. That is true. We also certainly want to put the common logic in the DSA layer. > We should define a general policy about this, and implement it in all > drivers. As such, your comment is valid, but a different patch set. I agree, that's why I added my tag to that patch ;-) Thanks, Vivien
Hello. On 8/22/2016 5:01 PM, Andrew Lunn wrote: > The PPU method of accessing PHYs makes use of a timer. Make sure this > timer is deleted before unloading the driver. > > Reported-by: Jamie Lentin <jm@lentin.co.uk> > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/dsa/mv88e6xxx/chip.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index b315769aa5be..1d5f9576e62a 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c [...] > @@ -3892,6 +3897,13 @@ static void mv88e6xxx_phy_init(struct mv88e6xxx_chip *chip) > } > } > > +static void mv88e6xxx_phy_destroy(struct mv88e6xxx_chip *chip) > +{ > + if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_PPU)) { > + mv88e6xxx_ppu_state_destroy(chip); > + } {} not needed here. See Documentation/CodingStyle, chapter 3. > +} > + > static int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip, > struct mii_bus *bus, int sw_addr) > { [...] MBR, Sergei
> >+static void mv88e6xxx_phy_destroy(struct mv88e6xxx_chip *chip) > >+{ > >+ if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_PPU)) { > >+ mv88e6xxx_ppu_state_destroy(chip); > >+ } > > {} not needed here. See Documentation/CodingStyle, chapter 3. Agreed, I will send a follow up patch, since the patchset has been accepted. Thanks Andrew
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index b315769aa5be..1d5f9576e62a 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -486,6 +486,11 @@ static void mv88e6xxx_ppu_state_init(struct mv88e6xxx_chip *chip) chip->ppu_timer.function = mv88e6xxx_ppu_reenable_timer; } +static void mv88e6xxx_ppu_state_destroy(struct mv88e6xxx_chip *chip) +{ + del_timer_sync(&chip->ppu_timer); +} + static int mv88e6xxx_phy_ppu_read(struct mv88e6xxx_chip *chip, int addr, int reg, u16 *val) { @@ -3892,6 +3897,13 @@ static void mv88e6xxx_phy_init(struct mv88e6xxx_chip *chip) } } +static void mv88e6xxx_phy_destroy(struct mv88e6xxx_chip *chip) +{ + if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_PPU)) { + mv88e6xxx_ppu_state_destroy(chip); + } +} + static int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip, struct mii_bus *bus, int sw_addr) { @@ -4080,6 +4092,7 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev) struct dsa_switch *ds = dev_get_drvdata(&mdiodev->dev); struct mv88e6xxx_chip *chip = ds_to_priv(ds); + mv88e6xxx_phy_destroy(chip); mv88e6xxx_unregister_switch(chip); mv88e6xxx_mdio_unregister(chip); }
The PPU method of accessing PHYs makes use of a timer. Make sure this timer is deleted before unloading the driver. Reported-by: Jamie Lentin <jm@lentin.co.uk> Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/dsa/mv88e6xxx/chip.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)