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

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <and...@lunn.ch> writes:

> On Wed, Jan 03, 2018 at 09:32:42AM -0500, Vivien Didelot wrote:
>> Hi Andrew,
>> 
>> Andrew Lunn <and...@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