Re: [PATCH v7] iio: imu: kmx61: Use guard(mutex)() over manual locking

[Jonathan Cameron]

On Fri, 22 May 2026 10:38:09 -0500
Maxwell Doose <m32285159@xxxxxxxxx> wrote:

> On Fri, May 22, 2026 at 7:46 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Thu, 21 May 2026 17:30:43 -0500
> > Maxwell Doose <m32285159@xxxxxxxxx> wrote:
> >  
> > > Include linux/cleanup.h to take advantage of new macros.
> > >
> > > Replace manual mutex_lock() and mutex_unlock() calls across the file
> > > with guard(mutex)() and scoped_guard() where appropriate to simplify
> > > error paths and eliminate manual locking calls.
> > >
> > > Add new helper function kmx61_read_for_each_active_channel() to mitigate
> > > certain style issues and to prevent notifying that the IRQ is finished
> > > whilst holding the lock.
> > >
> > > Update certain returns, and add default case to return -EINVAL in
> > > kmx61_read_raw().
> > >
> > > Remove now-redundant gotos and ret variables, as the new RAII macros
> > > make them unneeded.  
> >
> > A few remaining things inline.  I'll tweak whilst applying.
> >
> > Tweaked as:
> > diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
> > index 52a6e044e1e5..b8a8297b39af 100644
> > --- a/drivers/iio/imu/kmx61.c
> > +++ b/drivers/iio/imu/kmx61.c
> > @@ -827,15 +827,17 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
> >                 default:
> >                         return -EINVAL;
> >                 }
> > -       case IIO_CHAN_INFO_SAMP_FREQ:
> > +       case IIO_CHAN_INFO_SAMP_FREQ: {
> >                 if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
> >                         return -EINVAL;
> >
> > -               scoped_guard(mutex, &data->lock)
> > -                       ret = kmx61_get_odr(data, val, val2, chan->address);
> > +               guard(mutex)(&data->lock);
> > +
> > +               ret = kmx61_get_odr(data, val, val2, chan->address);
> >                 if (ret)
> >                         return -EINVAL;
> >                 return IIO_VAL_INT_PLUS_MICRO;
> > +       }
> >         default:
> >                 return -EINVAL;
> >         }
> > @@ -1178,8 +1180,6 @@ static irqreturn_t kmx61_data_rdy_trig_poll(int irq, void *private)
> >   * @indio_dev: IIO Device struct to read from
> >   * @buffer: Destination buffer to write to, the array must be of at least size 8
> >   *
> > - * Intended only for use in kmx61_trigger_handler().
> > - *
> >   * Return:
> >   * 0 on success,
> >   */
> >
> > See below for why.

I failed to say I've applied it with those tweaks.

> > > @@ -834,41 +831,40 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
> > >               if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
> > >                       return -EINVAL;
> > >
> > > -             mutex_lock(&data->lock);
> > > -             ret = kmx61_get_odr(data, val, val2, chan->address);
> > > -             mutex_unlock(&data->lock);
> > > +             scoped_guard(mutex, &data->lock)
> > > +                     ret = kmx61_get_odr(data, val, val2, chan->address);  
> >
> > Why is this one a scoped_guard()?  Seems like it would be more consistent
> > if it was done like the one above with {} and guard()
> >  
> 
> I'm guessing you're talking about the {} in the switch-case. I guess
> it's likely because the guard()() in that specific case goes for the
> whole scope of the case, whereas we only need to hold the lock for
> this call (I tend to keep section length as close as possible when
> doing these things, and typically I'd use scoped_guard() for a single
> call).

	case IIO_CHAN_INFO_SAMP_FREQ:
		if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
			return -EINVAL;

		mutex_lock(&data->lock);
		ret = kmx61_get_odr(data, val, val2, chan->address);
		mutex_unlock(&data->lock);
		if (ret)
			return -EINVAL;
		return IIO_VAL_INT_PLUS_MICRO;

Is the block in question.  Given nothing else happens after this point,
other than returning.  I'd always favour guard as it keeps the error
check at same level as the source of the error.


> 
> >  
> > >               if (ret)
> > >                       return -EINVAL;  
> > Unconnected to this patch but why are we eating the error code from kmx61_get_odr()?
> > In practice makes no difference as -EINVAL is the only error returned.
> >
> > Still nice to do
> >                 if (ret)
> >                         return ret;
> > so we don't need to go check that.
> > I'd be fine with you sneaking this in this patch with a brief not in the commit
> > message if you want to. Or can leave for another day.
> >  
> 
> Seems like a better idea to do this in a separate patch, perhaps where
> we refresh some of the returns/control paths in the file. But at the
> same time not opposed to sneaking this in if I end up touching that
> function in a different patch.

Sure.
> 
> >  
> > >               return IIO_VAL_INT_PLUS_MICRO;
> > > +     default:
> > > +             return -EINVAL;
> > >       }
> > > -     return -EINVAL;
> > >  }
> > >  

> >