Re: [PATCH 06/10] Input: sparcspkr - use cleanup facility for device_node

Javier Carrasco
2024-10-10T22:28:14Z

On 11/10/2024 00:22, Al Viro wrote:
> On Fri, Oct 11, 2024 at 12:09:01AM +0200, Javier Carrasco wrote:
>
>> I think that the issue you are talking about is that the goto will
>> trigger the cleanup function of the device_node, which will not be
>> initialized at that point.
>
> ... and gcc will compile that without an error. Clang will not, but
> you need to watch out for build coverage in arch-specific code -
> clang doesn't cover every architecture (and won't cover some of them,
> no matter what - alpha, for example).
>
> As for the scope changes... note that you've delayed the moment of
> of_node_put() in some of those. It's harmless for device_node, but
> try something of that sort with e.g.
>
> mutex_lock(&lock);
> something();
> mutex_unlock(&lock);
> foo();
> return 0;
>
> where foo() itself grabs the same lock and it's not harmless at all -
>
> guard(mutex)(&lock);
> something();
> foo();
> return 0;
>
> is equivalent to moving mutex_unlock() to the end of scope, i.e. past
> the call of foo(), resulting in
>
> mutex_lock(&lock);
> something();
> foo(); // deadlock
> mutex_unlock(&lock);
> return 0;
>
> __cleanup *is* a useful tool, when used carefully, but you really
> have to watch out for crap of that sort.

For cases like the one you are mentioning, scoped_guard() is the real
one to be used, but I get your point.

I just overlooked the goto as it just goes to a return, and I processed
in my mind as a direct return, which is not! I have even reviewed such
issues in the past... karma.

The goto in that case is meaningless anyway, and a direct return would
be more readable anyway.

Thanks again.