linux-kernel - Re: [PATCH] iio/pressure/mpl3115: Improve unlocking of a mutex in mpl3115_trigger

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] [day] [month] [year] [list]

Message-ID: <20171026172118.6343072c@archlinux>
Date:   Thu, 26 Oct 2017 17:21:18 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     SF Markus Elfring <elfring@...rs.sourceforge.net>
Cc:     linux-iio@...r.kernel.org, Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Peter Rosin <peda@...ntia.se>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] iio/pressure/mpl3115: Improve unlocking of a mutex in
 mpl3115_trigger_handler()

On Thu, 26 Oct 2017 16:23:45 +0200
SF Markus Elfring <elfring@...rs.sourceforge.net> wrote:

> From: Markus Elfring <elfring@...rs.sourceforge.net>
> Date: Thu, 26 Oct 2017 16:10:46 +0200
> 
> Adjust jump targets so that a call of the function "mutex_unlock" is stored
> at the end of this function implementation.
> Replace three calls by goto statements.
> 
> This issue was detected by using the Coccinelle software.
> 

Markus,

I would suggest only sending on patch like this out at a time in order
to see if there are issues with the approach before you waste your time
generating lots of them.

This has the same issue as in one of the earlier patches.  The resulting
code is harder to read offsetting any gains by unlocking in one place.

Jonathan

> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> ---
>  drivers/iio/pressure/mpl3115.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index 7537547fb7ee..f39e6f42a9c0 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -152,38 +152,36 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
>  
>  	mutex_lock(&data->lock);
>  	ret = mpl3115_request(data);
> -	if (ret < 0) {
> -		mutex_unlock(&data->lock);
> -		goto done;
> -	}
> +	if (ret < 0)
> +		goto unlock;
>  
>  	memset(buffer, 0, sizeof(buffer));
>  	if (test_bit(0, indio_dev->active_scan_mask)) {
>  		ret = i2c_smbus_read_i2c_block_data(data->client,
>  			MPL3115_OUT_PRESS, 3, &buffer[pos]);
> -		if (ret < 0) {
> -			mutex_unlock(&data->lock);
> -			goto done;
> -		}
> +		if (ret < 0)
> +			goto unlock;
> +
>  		pos += 4;
>  	}
>  
>  	if (test_bit(1, indio_dev->active_scan_mask)) {
>  		ret = i2c_smbus_read_i2c_block_data(data->client,
>  			MPL3115_OUT_TEMP, 2, &buffer[pos]);
> -		if (ret < 0) {
> -			mutex_unlock(&data->lock);
> -			goto done;
> -		}
> +		if (ret < 0)
> +			goto unlock;
>  	}
>  	mutex_unlock(&data->lock);
>  
>  	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
>  		iio_get_time_ns(indio_dev));
> -
> -done:
> +notify_trigger:
>  	iio_trigger_notify_done(indio_dev->trig);
>  	return IRQ_HANDLED;
> +
> +unlock:

Absolutely not.  This is much less readable than the original.

> +	mutex_unlock(&data->lock);
> +	goto notify_trigger;

>  }
>  
>  static const struct iio_chan_spec mpl3115_channels[] = {