Message ID | 1446237342-5109-1-git-send-email-vivien.didelot@savoirfairelinux.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Hi Andrew, On Oct. Friday 30 (44) 09:41 PM, Andrew Lunn wrote: > On Fri, Oct 30, 2015 at 04:35:42PM -0400, Vivien Didelot wrote: > > It's easy to forget to lock the smi_mutex before calling the low-level > > _mv88e6xxx_reg_{read,write}, so add a assert_smi_lock function in them. > > > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> > > --- > > drivers/net/dsa/mv88e6xxx.c | 31 ++++++++++++++++++------------- > > 1 file changed, 18 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c > > index b1b14f5..70a0106 100644 > > --- a/drivers/net/dsa/mv88e6xxx.c > > +++ b/drivers/net/dsa/mv88e6xxx.c > > @@ -24,6 +24,16 @@ > > #include <net/switchdev.h> > > #include "mv88e6xxx.h" > > > > +static inline void assert_smi_lock(struct dsa_switch *ds) > > No need for inline. Gcc will automatically inline it, if it thinks it > is small enough. OK, I will respin this without the inline keyword then. > > > +{ > > + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds); > > + > > + if (unlikely(!mutex_is_locked(&ps->smi_mutex))) { > > + dev_err(ds->master_dev, "SMI lock not held!\n"); > > + dump_stack(); > > + } > > +} > > + > > > -/* Must be called with SMI mutex held */ > > static int _mv88e6xxx_reg_read(struct dsa_switch *ds, int addr, int reg) > > { > > - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); > > + struct mii_bus *bus; > > int ret; > > > > + assert_smi_lock(ds); > > + > > + bus = dsa_host_dev_to_mii_bus(ds->master_dev); > > Is this change of when bus is assigned actually required? No, but I found not necessary to issue this "mdio_bus" lookup if the lock is not held (see net/dsa/dsa.c:555). Do you prefer not to do that? Also are you OK with removing all the "Must be called with..." comments, which I found not necessary too (some function have this comment, some others don't). Thanks, -v -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > static int _mv88e6xxx_reg_read(struct dsa_switch *ds, int addr, int reg) > > > { > > > - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); > > > + struct mii_bus *bus; > > > int ret; > > > > > > + assert_smi_lock(ds); > > > + > > > + bus = dsa_host_dev_to_mii_bus(ds->master_dev); > > > > Is this change of when bus is assigned actually required? > > No, but I found not necessary to issue this "mdio_bus" lookup if the > lock is not held (see net/dsa/dsa.c:555). Do you prefer not to do that? You are optimising for an error condition. If this optimisation saves anything, it means we have a locking bug! As a separate patch, i would do this lookup once in a setup function and save it away in ps. We just need to watch out for the probe register accesses. > Also are you OK with removing all the "Must be called with..." comments, Yes, it will become a lot more clear when the kernel outputs a stack dump! Andrew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct. Friday 30 (44) 10:01 PM, Andrew Lunn wrote: > > > > static int _mv88e6xxx_reg_read(struct dsa_switch *ds, int addr, int reg) > > > > { > > > > - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); > > > > + struct mii_bus *bus; > > > > int ret; > > > > > > > > + assert_smi_lock(ds); > > > > + > > > > + bus = dsa_host_dev_to_mii_bus(ds->master_dev); > > > > > > Is this change of when bus is assigned actually required? > > > > No, but I found not necessary to issue this "mdio_bus" lookup if the > > lock is not held (see net/dsa/dsa.c:555). Do you prefer not to do that? > > You are optimising for an error condition. If this optimisation saves > anything, it means we have a locking bug! Very good point, I'll get rid of that then. > As a separate patch, i would do this lookup once in a setup function > and save it away in ps. We just need to watch out for the probe > register accesses. Yes, I already put that on a todo list ;-) > > Also are you OK with removing all the "Must be called with..." comments, > > Yes, it will become a lot more clear when the kernel outputs a stack dump! Good, sending a v2 right away then. Thanks, -v -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c index b1b14f5..70a0106 100644 --- a/drivers/net/dsa/mv88e6xxx.c +++ b/drivers/net/dsa/mv88e6xxx.c @@ -24,6 +24,16 @@ #include <net/switchdev.h> #include "mv88e6xxx.h" +static inline void assert_smi_lock(struct dsa_switch *ds) +{ + struct mv88e6xxx_priv_state *ps = ds_to_priv(ds); + + if (unlikely(!mutex_is_locked(&ps->smi_mutex))) { + dev_err(ds->master_dev, "SMI lock not held!\n"); + dump_stack(); + } +} + /* If the switch's ADDR[4:0] strap pins are strapped to zero, it will * use all 32 SMI bus addresses on its SMI bus, and all switch registers * will be directly accessible on some {device address,register address} @@ -80,12 +90,14 @@ int __mv88e6xxx_reg_read(struct mii_bus *bus, int sw_addr, int addr, int reg) return ret & 0xffff; } -/* Must be called with SMI mutex held */ static int _mv88e6xxx_reg_read(struct dsa_switch *ds, int addr, int reg) { - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); + struct mii_bus *bus; int ret; + assert_smi_lock(ds); + + bus = dsa_host_dev_to_mii_bus(ds->master_dev); if (bus == NULL) return -EINVAL; @@ -143,12 +155,14 @@ int __mv88e6xxx_reg_write(struct mii_bus *bus, int sw_addr, int addr, return 0; } -/* Must be called with SMI mutex held */ static int _mv88e6xxx_reg_write(struct dsa_switch *ds, int addr, int reg, u16 val) { - struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev); + struct mii_bus *bus; + + assert_smi_lock(ds); + bus = dsa_host_dev_to_mii_bus(ds->master_dev); if (bus == NULL) return -EINVAL; @@ -204,7 +218,6 @@ int mv88e6xxx_set_addr_indirect(struct dsa_switch *ds, u8 *addr) return 0; } -/* Must be called with SMI mutex held */ static int _mv88e6xxx_phy_read(struct dsa_switch *ds, int addr, int regnum) { if (addr >= 0) @@ -212,7 +225,6 @@ static int _mv88e6xxx_phy_read(struct dsa_switch *ds, int addr, int regnum) return 0xffff; } -/* Must be called with SMI mutex held */ static int _mv88e6xxx_phy_write(struct dsa_switch *ds, int addr, int regnum, u16 val) { @@ -538,7 +550,6 @@ out: mutex_unlock(&ps->smi_mutex); } -/* Must be called with SMI mutex held */ static int _mv88e6xxx_stats_wait(struct dsa_switch *ds) { int ret; @@ -553,7 +564,6 @@ static int _mv88e6xxx_stats_wait(struct dsa_switch *ds) return -ETIMEDOUT; } -/* Must be called with SMI mutex held */ static int _mv88e6xxx_stats_snapshot(struct dsa_switch *ds, int port) { int ret; @@ -576,7 +586,6 @@ static int _mv88e6xxx_stats_snapshot(struct dsa_switch *ds, int port) return 0; } -/* Must be called with SMI mutex held */ static void _mv88e6xxx_stats_read(struct dsa_switch *ds, int stat, u32 *val) { u32 _val; @@ -789,7 +798,6 @@ void mv88e6xxx_get_regs(struct dsa_switch *ds, int port, } } -/* Must be called with SMI lock held */ static int _mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset, u16 mask) { @@ -839,14 +847,12 @@ int mv88e6xxx_eeprom_busy_wait(struct dsa_switch *ds) GLOBAL2_EEPROM_OP_BUSY); } -/* Must be called with SMI lock held */ static int _mv88e6xxx_atu_wait(struct dsa_switch *ds) { return _mv88e6xxx_wait(ds, REG_GLOBAL, GLOBAL_ATU_OP, GLOBAL_ATU_OP_BUSY); } -/* Must be called with SMI mutex held */ static int _mv88e6xxx_phy_read_indirect(struct dsa_switch *ds, int addr, int regnum) { @@ -865,7 +871,6 @@ static int _mv88e6xxx_phy_read_indirect(struct dsa_switch *ds, int addr, return _mv88e6xxx_reg_read(ds, REG_GLOBAL2, GLOBAL2_SMI_DATA); } -/* Must be called with SMI mutex held */ static int _mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int addr, int regnum, u16 val) {
It's easy to forget to lock the smi_mutex before calling the low-level _mv88e6xxx_reg_{read,write}, so add a assert_smi_lock function in them. Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> --- drivers/net/dsa/mv88e6xxx.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)