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

[Javier Carrasco <javier.carrasco.cruz@gmail.com>]

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.