Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 16 Jun 2022 17:02:08 +0000
Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:



> > > +       err = -EINVAL;
> > > +       mutex_lock(&msa311->lock);
> > > +       for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); ++odr)
> > > +               if (val == msa311_odr_table[odr].val &&
> > > +                   val2 == msa311_odr_table[odr].val2) {
> > > +                       err = msa311_set_odr(msa311, odr);  
> >   
> > > +                       if (err) {
> > > +                               dev_err(dev, "cannot update freq (%d)\n", err);
> > > +                               goto failed;
> > > +                       }  
> > 
> > Why is this inside the loop and more important under lock? Also you
> > may cover the initial error code by this message when moving it out of
> > the loop and lock.
> > 
> > Ditto for other code snippets in other function(s) where applicable.
> >   
> 
> Yes, I can move dev_err() outside of loop. But all ODR search loop
> should be under lock fully, because other msa311 operations should not
> be executed when we search proper ODR place.

I don't see why?  The search itself is for a match of the input to const data.
That can occur before taking the lock to do the actual write.

I don't see any additional race beyond the one that is always there of
a thread updating ODR whilst another is accessing the device.  Which order
those events happen in is not controlled by the driver, but the output
will be consistent with one or other order of those two accesses.

Jonathan