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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id C3C1FC3DA49 for ; Fri, 26 Jul 2024 05:05:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References: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=jVzY9HXY97cczgYDJULvCCMUVKhL5nbeKKOuLjBsycM=; b=XFLP2RYb/Q7CPt8Km/6WS/5I/i AdF6UfD5YPUPmjxRnLC/RE5Qk+2GpsztFCoKYg25gbKFFG07BzZZiaeb46ErZsTon9QTwNmYfWs9W 292WGasCdYGlHXFVWzzH/5cmnOPjyIkQZiEhz7QuVusFmssiaPW6HiivykTKo8RLfoAVg5w2YT5Me o563EXDWvxvDN0bYc2LKSYJ4AUtnLLGKQbbxLsJ6XmgGkeIjrDHwMwtyzRKLgFGXKRPEbk/h2Dg/8 ta1JUzftKhpUn9umfSrjEeM807fCm02o23ooy7uGILi9x+csh7EDtF3GX69JRPsjm+tcfSr658Mu4 54OQHf9g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sXD8S-00000002uqS-0Eba; Fri, 26 Jul 2024 05:04:56 +0000 Received: from mail-pl1-x62b.google.com ([2607:f8b0:4864:20::62b]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sXD83-00000002ulq-0efo for linux-arm-kernel@lists.infradead.org; Fri, 26 Jul 2024 05:04:33 +0000 Received: by mail-pl1-x62b.google.com with SMTP id d9443c01a7336-1fd69e44596so2001465ad.1 for ; Thu, 25 Jul 2024 22:04:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1721970270; x=1722575070; darn=lists.infradead.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=jVzY9HXY97cczgYDJULvCCMUVKhL5nbeKKOuLjBsycM=; b=qDe8JczFfSxLtUnrdtiHchWIBwC8NYWXMCb/Qc/S8QyffYgE9Nryk1o4d5uVv3zj5Y RZyJMbqQuBQgjsJjnhW1atRmjCuAOHRQy6SKocAJyNpxkpEOqn/c9qudUhHjEnFCsKQe sWbkBgsRLEpV5d1voj04nxXdLFjMKT78Q4VV3RKJLJGzAfuJgeOuUizJBB2vV5K7Hj1p 0Zwy2kBC/KrzDM1i8WxOLcMFiDUIt4MZlkVnzN7lTrme96eQzFWYx+6ujz8B9JtoOd0s 3uIwFebTCkLjGcAfXxXhc41dDZ+Y0MvLggolC/bW4oXJFeHKkLLevbFSzA5o2V6RxVGx qhAQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721970270; x=1722575070; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=jVzY9HXY97cczgYDJULvCCMUVKhL5nbeKKOuLjBsycM=; b=aKnvkRQGUTUd4bDyGsBrMwK+kyGiHfZ6ncfvzT48hujl6nYpkDLuss3qOKmvi6SGF+ joi+jbKpYiDbzRhaOmqsNx7IH/2H97i918571dO1yDuxAp897LaDba3sGdMLrAOdj+PI YrS2dUz0Vb/0L5nRav6DbNVh6kofhqFQZ/HtqcJag8RwYV/BJmpuNa5UQrZfmr2MEgzk SmKwtk0L0hQOSvbLkVVkmKUB59xjdFuZoswpXotE7YpZS5kW8d5C4rQ4uxrPaNTm1lje 3zLvIpG3TZo3yA5Za9H6jRkmjAFa5lKyNG6QKn9IlgJ3tY86XtzN6jnD66CFoFAXmHC6 FktQ== X-Forwarded-Encrypted: i=1; AJvYcCUgjXdrXbF8Tcm7HLyEDgyi4Jc2154Y8yAS2XQyAsfFoTCNBgK5fTqvXJFVVYi0BbO9gQybRJvt+9SrmJ0gj7MpOchpqprlWGx1iGa9XhXu41bKVRs= X-Gm-Message-State: AOJu0YzpecLCpC1/n5XRI0h8Et4Ld4aot7WY/aEkJX58NUR+KwEZGOHS j/uNzoiZVUHIOMCgvnEVFhufQPcVPQJM8slocWbXj7PAQX5M3QKnJjy3AijquA== X-Google-Smtp-Source: AGHT+IEv9bnCjkKaaT/mVatGZz8Er2pi/NLYy/srtwCMJgijY/eyFSkIBNPHBAyhamVGhdhOUZ8Aqg== X-Received: by 2002:a17:903:1ce:b0:1fc:41c0:7a82 with SMTP id d9443c01a7336-1fed248c8c0mr82068615ad.0.1721970269543; Thu, 25 Jul 2024 22:04:29 -0700 (PDT) Received: from thinkpad ([220.158.156.199]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-1fed7c88aa1sm22840805ad.37.2024.07.25.22.04.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 25 Jul 2024 22:04:29 -0700 (PDT) Date: Fri, 26 Jul 2024 10:34:23 +0530 From: Manivannan Sadhasivam To: Jim Quinlan Cc: linux-pci@vger.kernel.org, Nicolas Saenz Julienne , Bjorn Helgaas , Lorenzo Pieralisi , Cyril Brulebois , Stanimir Varbanov , Krzysztof Kozlowski , bcm-kernel-feedback-list@broadcom.com, jim2101024@gmail.com, Florian Fainelli , Lorenzo Pieralisi , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Rob Herring , "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" , "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" , open list Subject: Re: [PATCH v4 03/12] PCI: brcmstb: Use common error handling code in brcm_pcie_probe() Message-ID: <20240726050423.GB2628@thinkpad> References: <20240716213131.6036-1-james.quinlan@broadcom.com> <20240716213131.6036-4-james.quinlan@broadcom.com> <20240725043111.GD2317@thinkpad> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240725_220431_256371_7293094B X-CRM114-Status: GOOD ( 26.31 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Jul 25, 2024 at 03:45:59PM -0400, Jim Quinlan wrote: > On Thu, Jul 25, 2024 at 12:31 AM Manivannan Sadhasivam > wrote: > > > > On Tue, Jul 16, 2024 at 05:31:18PM -0400, Jim Quinlan wrote: > > > o Move the clk_prepare_enable() below the resource allocations. > > > o Add a jump target (clk_out) so that a bit of exception handling can be > > > better reused at the end of this function implementation. > > > > > > Signed-off-by: Jim Quinlan > > > Reviewed-by: Stanimir Varbanov > > > Reviewed-by: Florian Fainelli > > > --- > > > drivers/pci/controller/pcie-brcmstb.c | 29 +++++++++++++++------------ > > > 1 file changed, 16 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > > index c08683febdd4..c257434edc08 100644 > > > --- a/drivers/pci/controller/pcie-brcmstb.c > > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > > @@ -1613,31 +1613,30 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > > > > > pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc"); > > > > > > - ret = clk_prepare_enable(pcie->clk); > > > - if (ret) { > > > - dev_err(&pdev->dev, "could not enable clock\n"); > > > - return ret; > > > - } > > > pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal"); > > > - if (IS_ERR(pcie->rescal)) { > > > - clk_disable_unprepare(pcie->clk); > > > + if (IS_ERR(pcie->rescal)) > > > return PTR_ERR(pcie->rescal); > > > - } > > > + > > > pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst"); > > > - if (IS_ERR(pcie->perst_reset)) { > > > - clk_disable_unprepare(pcie->clk); > > > + if (IS_ERR(pcie->perst_reset)) > > > return PTR_ERR(pcie->perst_reset); > > > + > > > + ret = clk_prepare_enable(pcie->clk); > > > + if (ret) { > > > + dev_err(&pdev->dev, "could not enable clock\n"); > > > + return ret; > > > } > > > > > > ret = reset_control_reset(pcie->rescal); > > > - if (ret) > > > + if (ret) { > > > dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); > > > + goto clk_out; > > > > Please use a descriptive name for the err labels. Here this err path disables > > and unprepares the clk, so use 'clk_disable_unprepare'. > ack > > > > > + } > > > > > > ret = brcm_phy_start(pcie); > > > if (ret) { > > > reset_control_rearm(pcie->rescal); > > > - clk_disable_unprepare(pcie->clk); > > > - return ret; > > > + goto clk_out; > > > } > > > > > > ret = brcm_pcie_setup(pcie); > > > @@ -1676,6 +1675,10 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > > > > > return 0; > > > > > > +clk_out: > > > + clk_disable_unprepare(pcie->clk); > > > + return ret; > > > + > > > > This is leaking the resources. Move this new label below 'fail'. > What resources is it leaking? At "clk_out" the return value will be negative > and only managed resources have been allocated at that juncture. > Right, but what about the err path below this one? If that path is taken, then clks won't be released, right? It is not a good design to return from each err labels. There should be only one return for all err labels at the end and those labels need to be in reverse order w.r.t the actual code. - Mani -- மணிவண்ணன் சதாசிவம்