Re: iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions

[Jonathan Cameron]

On Sun, 18 Mar 2018 09:19:47 +0100
SF Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx> wrote:

> Am 17.03.2018 um 20:54 schrieb Jonathan Cameron:
> > On Wed, 14 Mar 2018 16:15:32 +0100
> > SF Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >   
> >> From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> >> Date: Wed, 14 Mar 2018 16:06:49 +0100
> >>
> >> * Add jump targets so that a call of the function "mutex_unlock" is stored
> >>   only once in these function implementations.
> >>
> >> * Replace 19 calls by goto statements.
> >>
> >> This issue was detected by using the Coccinelle software.
> >>
> >> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>  
> > 
> > Hi Markus,
> > 
> > Some of these are good and sensible changes  
> 
> Such feedback is nice.
> 
> 
> > - others break the code.  
> 
> Which concrete places do you find questionable here?
> 
> 
> >> -		return ret;
> >> +
> >> +		goto set_power_state;
> >>  	default:
> >>  		return -EINVAL;  
> > We exit with the mutex locked now and it should not be.  
> 
> I wonder about your source code interpretation here.
> The mutex was (and is still only) locked within case branches, isn't it?
> 
You are correct, this does however reflect the issue with the resulting
lack of balance here.  I saw the mutex was getting unlocked outside
the local scope and so assumed that it was also take outside the local
scope.  That isn't true, so we have hurt readability.

It might make sense to move the lock and unlock outside the switch statement
but we certainly don't want to the the confusion that the lack of balance is
causing here.

I read it quickly and got the wrong idea which generally implies it is not
as clear as we would like.

Hence this change isn't going anywhere I'm afraid.

Jonathan

> 
> >   
> >>  	}
> >>  
> >>  	return -EINVAL;  
> > Mutex is still locked here and the return is wrong.  
> 
> Should this statement get any more software development attention?
> 
> Regards,
> Markus