[PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock

[PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock

[Vivien Didelot]

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)
+{
+	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)
 {
-- 
2.6.2

Re: [PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock

[Andrew Lunn]

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.

> +{
> +	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?

Thanks
	Andrew

Re: [PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock

[Vivien Didelot]

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

Re: [PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock

[Andrew Lunn]

> > >  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

Re: [PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock

[Vivien Didelot]

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