Re: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for handling mutex lock

[Andrew Ijano]

On Sat, Jun 14, 2025 at 9:01 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Wed, 11 Jun 2025 16:39:21 -0300
> Andrew Ijano <andrew.ijano@xxxxxxxxx> wrote:
>
> > Use guard(mutex)(&st->lock) for handling mutex lock instead of
> > manually locking and unlocking the mutex. This prevents forgotten
> > locks due to early exits and remove the need of gotos.
> >
> > Signed-off-by: Andrew Ijano <andrew.lopes@xxxxxxxxxxxxx>
> > Co-developed-by: Gustavo Bastos <gustavobastos@xxxxxx>
> > Signed-off-by: Gustavo Bastos <gustavobastos@xxxxxx>
> > Suggested-by: Jonathan Cameron <jic23@xxxxxxxxxx>
> > ---
> > For this one, there are two cases where the previous implementation
> > was a smalllocking portion of the code and now it's locking the whole
> > function. I don't know if this is a desired behavior.
>
> I'd call out more specifically that you are going from
>
> lock
> stuff
> unlock
> call which contains lock over whole useful scope
>
> to
> lock
> stuff
> call with lock no longer done inside
> unlock
>
> In at least one case.  Looks cleaner to me. I'd guess this is a silly
> bit of code evolution that you are tidying up as that lock dance looks
> pointless to me.

Yes! That's something I noticed when making this change and it looked
like a clean up for me too.
What bothered me were cases where we originally had a lock /
spi_w8r8() / unlock and then several operations like iio_push_event()
or sprintf(). I found these cases in sca3000_event_handler() and
sca3000_read_av_freq().

Maybe this isn't as problematic as I'm making them up to be, but it
seems that applying the suggestion of Nino to use scoped_guard()
instead in these cases would already solve this problem.

>
> Otherwise just the {} for cases thing needs fixing up.

Great! I'll fix that too.

Thanks,
Andrew