From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-17.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8ECF5C4338F for ; Sat, 24 Jul 2021 17:29:24 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 50D5B60E09 for ; Sat, 24 Jul 2021 17:29:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 50D5B60E09 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=deM5gtRigSkPrbBgjmV/9rJq/39uTl5nynP1FKrtq4g=; b=cAGQuas3rjyixq k9W7lEXG24/ZKtGc8irQiyzgmRkpZVvWqtK1dBHqz5SCs7NQop5YfYNG7kZQDFH6Uchv+eA++3dcW bCtAJwEOuu8M9Kub4Af6gbHiP6g6vcuwC/nf/JrU7HAT4WY619r/Vh/wwQzDNbp4m8C6LP9Sab/KS vjSTyLFKhX2CZlIvmGc0WkFVZ2zZ7IITzkU+oMWeKlB9Vqvs6SSaCxiO3IyL5RfoaWwgcFRtKr79O gEZegDS48/kcOqgLMcKG5c51xHWCxfTvzrABIVyS8EO8KzGm+hh4posFyUKwO5Nh0UaHs0dVkJqPv e/B7SjcwQT/98CLiFxnA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1m7LR8-007WZ1-Og; Sat, 24 Jul 2021 17:27:42 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1m7LR4-007WY1-2R; Sat, 24 Jul 2021 17:27:40 +0000 Received: from jic23-huawei (cpc108967-cmbg20-2-0-cust86.5-4.cable.virginm.net [81.101.6.87]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 82F5F60E8E; Sat, 24 Jul 2021 17:27:32 +0000 (UTC) Date: Sat, 24 Jul 2021 18:30:03 +0100 From: Jonathan Cameron To: "hui.liu" Cc: , , , , , , , , , , , , , , , Subject: Re: [PATCH] iio: mtk-auxadc: add mutex_destroy Message-ID: <20210724183003.6f3bc1d5@jic23-huawei> In-Reply-To: <1627042875.27985.15.camel@mhfsdcap03> References: <20210715093523.29844-1-hui.liu@mediatek.com> <20210715093523.29844-2-hui.liu@mediatek.com> <20210717174432.7e69e4e9@jic23-huawei> <1627042875.27985.15.camel@mhfsdcap03> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210724_102739_191899_EF1C805D X-CRM114-Status: GOOD ( 36.81 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, 23 Jul 2021 20:21:15 +0800 hui.liu wrote: > On Sat, 2021-07-17 at 17:44 +0100, Jonathan Cameron wrote: > > On Thu, 15 Jul 2021 17:35:23 +0800 > > Hui Liu wrote: > > > > > Add mutex_destroy when probe fail and remove device. > > > > > > Signed-off-by: Hui Liu > > Hi Hui Liu, > > > > We very very rarely bother to call mutex_destroy(). The reason is > > that it is only a non noop in when mutex debugging is enabled and > > that is only useful if there is a plausible route in which it could > > be used after the mutex_destroy. Given these are both at the ends > > of removal paths, I don't think this is useful. That's why you will > > rarely find mutex_destroy() being called. > > > > Thanks, > > > > Jonathan > > Hi Jonathon, > > I think this patch could assurance the integrity of code. > mutex_init will be called when driver probe. If driver probe fail or > device removed, mutex_destroy could set lock->magic to NULL. I'm not seeing the use case here given the location doesn't leave a huge amount of code that could have such a bug. There might have been something if we had any route to increment the reference count of the structure this mutex is ultimately embedded in and so have it outlast the remove function or error path. In this driver it looks like there is no such path. Hence you are protecting against a automated cleanup of core code (nothing in the driver itself) which is obviously not going to try taking a driver specific mutex. A few side notes: You are calling it wrong place in remove. The ordering in remove should be the opposite of that in probe so the mutex_destroy should either be a few lines earlier, or you should have a comment there to say why you are doing it where you have chosen to do so. The style of this probe is to do error handling in a block at the end. So this handling should be there, not in the if statement. Jonathan > > Thanks. > Hui > > > > > > --- > > > drivers/iio/adc/mt6577_auxadc.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/iio/adc/mt6577_auxadc.c b/drivers/iio/adc/mt6577_auxadc.c > > > index 79c1dd68b909..d57243037ad6 100644 > > > --- a/drivers/iio/adc/mt6577_auxadc.c > > > +++ b/drivers/iio/adc/mt6577_auxadc.c > > > @@ -289,6 +289,7 @@ static int mt6577_auxadc_probe(struct platform_device *pdev) > > > ret = iio_device_register(indio_dev); > > > if (ret < 0) { > > > dev_err(&pdev->dev, "failed to register iio device\n"); > > > + mutex_destroy(&adc_dev->lock); > > > goto err_power_off; > > > } > > > > > > @@ -313,6 +314,7 @@ static int mt6577_auxadc_remove(struct platform_device *pdev) > > > 0, MT6577_AUXADC_PDN_EN); > > > > > > clk_disable_unprepare(adc_dev->adc_clk); > > > + mutex_destroy(&adc_dev->lock); > > > > > > return 0; > > > } > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel