diff mbox series

[net-next,2/5] net: dsa: mv88e6xxx: Hold mutex while doing stats operations

Message ID 1514988562-20079-3-git-send-email-andrew@lunn.ch
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series Export SERDES stats via ethtool -S | expand

Commit Message

Andrew Lunn Jan. 3, 2018, 2:09 p.m. UTC 
Until now, there has been no need to hold the reg mutex while getting
the count of statistics, or the strings, because the hardware was not
accessed. When adding support for SERDES statistics, it is necessary
to access the hardware, to determine if a port is using the SERDES
interface. So add mutex lock/unlocks.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Comments

Vivien Didelot Jan. 3, 2018, 2:32 p.m. UTC | #1 
Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> -static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
> +static int _mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  
> @@ -702,6 +706,19 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>  	return 0;
>  }

We worked to remove the old underscore prefix convention. Please don't
add it back... Simply rework the return statements of
mv88e6xxx_get_sset_count to lock/unlock there.

>  
> +static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	int ret;
> +
> +	mutex_lock(&chip->reg_lock);
> +	ret = _mv88e6xxx_get_sset_count(ds, port);
> +	mutex_unlock(&chip->reg_lock);
> +
> +	return ret;
> +}
> +
> +

Extra newline.

>  static void mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
>  				      uint64_t *data, int types,
>  				      u16 bank1_select, u16 histogram)

Thanks,

        Vivien
Andrew Lunn Jan. 3, 2018, 3:02 p.m. UTC | #2 
On Wed, Jan 03, 2018 at 09:32:42AM -0500, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > -static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
> > +static int _mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
> >  {
> >  	struct mv88e6xxx_chip *chip = ds->priv;
> >  
> > @@ -702,6 +706,19 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
> >  	return 0;
> >  }
> 
> We worked to remove the old underscore prefix convention. Please don't
> add it back... Simply rework the return statements of
> mv88e6xxx_get_sset_count to lock/unlock there.

Hi Vivien

That makes mv88e6xxx_get_sset_count quite complex, making it error
prone. Doing the locking in a separate function makes is very clear
the lock is held and then correctly released. So i will just rename
_mv88e6xxx_get_sset_count() to mv88e6xxx_get_sset_count_locked()

    Andrew
Vivien Didelot Jan. 3, 2018, 3:43 p.m. UTC | #3 
Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Wed, Jan 03, 2018 at 09:32:42AM -0500, Vivien Didelot wrote:
>> Hi Andrew,
>> 
>> Andrew Lunn <andrew@lunn.ch> writes:
>> 
>> > -static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>> > +static int _mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>> >  {
>> >  	struct mv88e6xxx_chip *chip = ds->priv;
>> >  
>> > @@ -702,6 +706,19 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>> >  	return 0;
>> >  }
>> 
>> We worked to remove the old underscore prefix convention. Please don't
>> add it back... Simply rework the return statements of
>> mv88e6xxx_get_sset_count to lock/unlock there.
>
> That makes mv88e6xxx_get_sset_count quite complex, making it error
> prone. Doing the locking in a separate function makes is very clear
> the lock is held and then correctly released. So i will just rename
> _mv88e6xxx_get_sset_count() to mv88e6xxx_get_sset_count_locked()

     static int mv88e6xxx_get_sset_count(struct dsa_switch *ds)
     {
            struct mv88e6xxx_chip *chip = ds->priv;
    +       int err;
     
    -       if (chip->info->ops->stats_get_sset_count)
    -               return chip->info->ops->stats_get_sset_count(chip);
    +       if (!chip->info->ops->stats_get_sset_count)
    +               return 0;
     
    -       return 0;
    +       mutex_lock(&chip->reg_lock);
    +       err = chip->info->ops->stats_get_sset_count(chip);
    +       mutex_unlock(&chip->reg_lock);
    +
    +       return err;
     }


This is quite complex and error prone, seriously?


     Vivien
diff mbox series

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 504407adc7aa..12e274a3ff24 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -662,8 +662,12 @@  static void mv88e6xxx_get_strings(struct dsa_switch *ds, int port,
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 
+	mutex_lock(&chip->reg_lock);
+
 	if (chip->info->ops->stats_get_strings)
 		chip->info->ops->stats_get_strings(chip, data);
+
+	mutex_unlock(&chip->reg_lock);
 }
 
 static int mv88e6xxx_stats_get_sset_count(struct mv88e6xxx_chip *chip,
@@ -692,7 +696,7 @@  static int mv88e6320_stats_get_sset_count(struct mv88e6xxx_chip *chip)
 					      STATS_TYPE_BANK1);
 }
 
-static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
+static int _mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 
@@ -702,6 +706,19 @@  static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
 	return 0;
 }
 
+static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int ret;
+
+	mutex_lock(&chip->reg_lock);
+	ret = _mv88e6xxx_get_sset_count(ds, port);
+	mutex_unlock(&chip->reg_lock);
+
+	return ret;
+}
+
+
 static void mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
 				      uint64_t *data, int types,
 				      u16 bank1_select, u16 histogram)