* [PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock
@ 2015-10-30 20:35 Vivien Didelot
2015-10-30 20:41 ` Andrew Lunn
0 siblings, 1 reply; 5+ messages in thread
From: Vivien Didelot @ 2015-10-30 20:35 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Guenter Roeck, Neil Armstrong, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock
2015-10-30 20:35 [PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock Vivien Didelot
@ 2015-10-30 20:41 ` Andrew Lunn
2015-10-30 20:50 ` Vivien Didelot
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Lunn @ 2015-10-30 20:41 UTC (permalink / raw)
To: Vivien Didelot
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Guenter Roeck, Neil Armstrong
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock
2015-10-30 20:41 ` Andrew Lunn
@ 2015-10-30 20:50 ` Vivien Didelot
2015-10-30 21:01 ` Andrew Lunn
0 siblings, 1 reply; 5+ messages in thread
From: Vivien Didelot @ 2015-10-30 20:50 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Guenter Roeck, Neil Armstrong
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock
2015-10-30 20:50 ` Vivien Didelot
@ 2015-10-30 21:01 ` Andrew Lunn
2015-10-30 21:11 ` Vivien Didelot
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Lunn @ 2015-10-30 21:01 UTC (permalink / raw)
To: Vivien Didelot
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Guenter Roeck, Neil Armstrong
> > > 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock
2015-10-30 21:01 ` Andrew Lunn
@ 2015-10-30 21:11 ` Vivien Didelot
0 siblings, 0 replies; 5+ messages in thread
From: Vivien Didelot @ 2015-10-30 21:11 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli,
Guenter Roeck, Neil Armstrong
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-10-30 21:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-30 20:35 [PATCH net-next] net: dsa: mv88e6xxx: assert SMI lock Vivien Didelot
2015-10-30 20:41 ` Andrew Lunn
2015-10-30 20:50 ` Vivien Didelot
2015-10-30 21:01 ` Andrew Lunn
2015-10-30 21:11 ` Vivien Didelot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).