From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751879AbdJYQQA (ORCPT <rfc822;w@1wt.eu>);
        Wed, 25 Oct 2017 12:16:00 -0400
Received: from mout.web.de ([212.227.15.4]:63934 "EHLO mout.web.de"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751513AbdJYQP6 (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 25 Oct 2017 12:15:58 -0400
Subject: Re: [PATCH] iio/accel/bmc150: Improve unlocking of a mutex in two
 functions
To: Hans de Goede <hdegoede@redhat.com>, linux-iio@vger.kernel.org
Cc: Hartmut Knaack <knaack.h@gmx.de>, Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        LKML <linux-kernel@vger.kernel.org>, kernel-janitors@vger.kernel.org
References: <66d582a4-a77e-cd78-4215-49587ec2259e@users.sourceforge.net>
 <fb223eb2-ddab-396e-372e-e9496be8bf0f@redhat.com>
From: SF Markus Elfring <elfring@users.sourceforge.net>
Message-ID: <c32ea2ea-5961-6589-6b51-1184d9565a38@users.sourceforge.net>
Date: Wed, 25 Oct 2017 18:15:32 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <fb223eb2-ddab-396e-372e-e9496be8bf0f@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-GB
Content-Transfer-Encoding: 8bit
X-Provags-ID: V03:K0:1f0SMbmSr0UAlrZsYxKMN8bXBPEi07ItzOwtsSR6AsrKE4W6tg5
 IDVzY8ycPP+8PmI6Bc4FW9QHUGEZTBT/vVsTmDqjsUPPLVcDekipN1DfMcd2RZsqJrsKcpm
 Y3Y0OlwW57qHOCWhntxk72UJhF0f/OHoulhZFE4atq9/WpdjfWG6S+2bEuEeiWWrI79CKDJ
 AzxS6mXE74hw1EXOH6oRA==
X-UI-Out-Filterresults: notjunk:1;V01:K0:rClOmxTx5m8=:DYdM86mTA54dajBSRFHwzj
 n/w7c1arR4GQRVTt+SFeTnIkxaHxm+KBA14TclMEYhXAcWQ5uTb20cgOlDIlkaxZV5tsvQG3S
 hN9TngsTh0xSyfsCahGUxtMhM+wxEXlyeyy65e/U13AGe38TG/Ldmqd7NRTZgvmAkCTCwJssA
 mFwgnaAeYZMjJiKdUcG2lWMgsRx0O3IuOtOr83HTtHPbPdQmPMXe2gJidh9riBq20CrpouW1Y
 Wz70jD3ISZfJ3/t2iU6kN6wZ0p9L/BktKRX+hvWr9Va3Bu0S7RR7E+7fC6u6lln8N/mNUA7YQ
 9Rg4Jxfyi9F66si+3o6CxmwOyxZyRq9L2LS5Gfoi6oNXFzRMA9pFqgNMyQiqLfJE1P5gK2+Gm
 DDaURtW6e/yJjmdIpa9Y+ZDsgjJF61c2o0CnzXGGK8yPwqlrXh3Z+3LGkj95we+eCPV09H6fq
 iC4e++F1GWqNHe/YITkLJyQ7DBFWkloXeYWZdBEefxINrsAWk0/gZcT0Bgbuk37kCDupLfizS
 pNWvqUStieDDmUaWo7RxtOzVY6anMLfb/+gQ5dLukL7FdRN01lpswKgBE4LT1T2hm8025gb+S
 9TqT6foRLhXnH9zMIolAV3tY6yMqN66sdfKuLKtvBB++/3m+RPaoZ2xVn8RLAwaOtGJ9Gpn+L
 JqmeV7vkxp7VD3LBkxFhdwaU/suAGbDKTSGMHnEDXMt3ni00wj9aIJviWciHyKhgRSWaaGUR5
 1GEf+wf27374VpN0GhUc5/mwocE6NAMR27lezew1w9LedHAd6CVFv5pxz9X20Ulgm8YaVMLj3
 G4LIF+muOTavrXfIpCrbk2H7p/1c/IlnoRM9zwZAfo1Q0I3qIM=
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> IMHO, if you do this, you should rework the function so that there is a single unlock call
> at the end, not a separate one in in error label.

Thanks for your update suggestion.

Does it indicate that I may propose similar source code adjustments
in this software area?


> Could e.g. change this:
> 
>         ret = bmc150_accel_set_power_state(data, false);
>         mutex_unlock(&data->mutex);
>         if (ret < 0)
>                 return ret;
> 
>         return IIO_VAL_INT;
> }
> 
> To:
> 
>         ret = bmc150_accel_set_power_state(data, false);
>         if (ret < 0)
>                 goto unlock;
> 
>     ret = IIO_VAL_INT;

How do you think about to use the following code variant then?

	if (!ret)
		ret = IIO_VAL_INT;


> unlock:
>         mutex_unlock(&data->mutex);
> 
>         return ret;
> }
> 
> And also use the unlock label in the other cases, this is actually
> quite a normal pattern. I see little use in a patch like this if there
> are still 2 unlock paths after the patch.

How long should I wait for corresponding feedback before another small
source code adjustment will be appropriate?

Regards,
Markus