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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANZih_RErpueDYAC--XUnq=wUY72WWDsC9mj0n1bfqC0aiRHYg@mail.gmail.com>
Date: Sat, 14 Jun 2025 18:40:14 -0300
From: Andrew Ijano <andrew.ijano@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: andrew.lopes@...mni.usp.br, gustavobastos@....br, dlechner@...libre.com, 
	nuno.sa@...log.com, andy@...nel.org, jstephan@...libre.com, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/3] iio: accel: sca3000: use guard(mutex)() for
 handling mutex lock

On Sat, Jun 14, 2025 at 9:01 AM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Wed, 11 Jun 2025 16:39:21 -0300
> Andrew Ijano <andrew.ijano@...il.com> 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@...mni.usp.br>
> > Co-developed-by: Gustavo Bastos <gustavobastos@....br>
> > Signed-off-by: Gustavo Bastos <gustavobastos@....br>
> > Suggested-by: Jonathan Cameron <jic23@...nel.org>
> > ---
> > 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