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 24584CF34A9 for ; Thu, 3 Oct 2024 14:37:14 +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-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=YzHBY8uemC96dyPvlpz0g43Vs4ghZWmRlUr6suCIVOE=; b=PyObrsmCoD0f3j7/wYth38GAgf 7EOfVDKjWutZgvT5omTRcUzDYQ+R1c+nT5owpa7Obvcbp3OFiAfjOZkUafaaw+Xty75mKO9shvZXv aRWld3+MpshRp/P5B+RHZg9262XhaOpMmC2Lsv5kAbCLXFnIevn65d+XK4Q56WIrlUJtYJdahJJfI OvQ7EtBAI/g8Exn9B3tNh+gvDK4sdv27P2Fagx/cakII7fuXVIWMUpaoUItgpWopjKrGk/bHVfpvy GI7oFKtokszVpUh4MGIiUxt1XGziwOpddXzZEeMyRK2LN3iNnrafQRJbPTBpognn6D84Tr1cCCgIv pOr2smNg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1swMwv-00000009Pkh-4Bk6; Thu, 03 Oct 2024 14:37:02 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1swMvf-00000009PTM-2p0V for linux-arm-kernel@lists.infradead.org; Thu, 03 Oct 2024 14:35:45 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 5E0C85C5D2B; Thu, 3 Oct 2024 14:35:38 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1CB0DC4CEC5; Thu, 3 Oct 2024 14:35:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1727966142; bh=U8icIK3BE1bI7+nO+YidN+i9hjIvX2G+tPxNCsSRFd0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=oKrn8L84tWxFREL5btCJr75fz4lqrxEjGiFna35gdvpWSAHQuM5hWrX9aHKPUrJry LhM0FzuDCbK8K3osrJqxj4eQJ961r9Y4/eS6Ut7RagUr/XJraPcylGkoqjuUdKlELK m5cPQtK8qUkaA/jO4o+VtHutDtpZJtJV/89KKb2JXeLwDC6z5MUe2uzXKPv2WoK5wm ief+mLnhy28/d/5g7LVL3N5B7uAAyPpHcLAhckuxGTdH39NV55Q7OgQt+RdDU45eUy EvY1vQu7qx6GiTUcwoVC26oBlrmFVjT+hgl4od2//YapTFAZm4zPV4IQIO5HVWvt8N 9LJ+OHW/WvlNQ== Date: Thu, 3 Oct 2024 15:35:37 +0100 From: Simon Horman To: Fedor Pchelkin Cc: Vitalii Mordan , Alexandre Torgue , Jose Abreu , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Maxime Coquelin , netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, lvc-project@linuxtesting.org, Alexey Khoroshilov , Vadim Mutilin , Andrew Lunn Subject: Re: [PATCH net] stmmac: dwmac-intel-plat: fix call balance of tx_clk handling routines Message-ID: <20241003143537.GQ1310185@kernel.org> References: <20240930183715.2112075-1-mordan@ispras.ru> <20241003111811.GJ1310185@kernel.org> <20241003-31f0aab72f4bccce9337303f-pchelkin@ispras.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20241003-31f0aab72f4bccce9337303f-pchelkin@ispras.ru> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241003_073543_828344_183AEB6E X-CRM114-Status: GOOD ( 40.33 ) 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 + Andrew Lunn On Thu, Oct 03, 2024 at 03:55:35PM +0300, Fedor Pchelkin wrote: > Hello, > > On Thu, 03. Oct 12:18, Simon Horman wrote: > > On Mon, Sep 30, 2024 at 09:37:15PM +0300, Vitalii Mordan wrote: > > > If the clock dwmac->tx_clk was not enabled in intel_eth_plat_probe, > > > it should not be disabled in any path. > > > > > > Conversely, if it was enabled in intel_eth_plat_probe, it must be disabled > > > in all error paths to ensure proper cleanup. > > > > > > Found by Linux Verification Center (linuxtesting.org) with Klever. > > > > > > Fixes: 9efc9b2b04c7 ("net: stmmac: Add dwmac-intel-plat for GBE driver") > > > Signed-off-by: Vitalii Mordan > > > --- > > > .../ethernet/stmicro/stmmac/dwmac-intel-plat.c | 16 +++++++++++++--- > > > 1 file changed, 13 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c > > > index d68f0c4e7835..2a2893f2f2a8 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel-plat.c > > > @@ -108,7 +108,12 @@ static int intel_eth_plat_probe(struct platform_device *pdev) > > > if (IS_ERR(dwmac->tx_clk)) > > > return PTR_ERR(dwmac->tx_clk); > > > > > > - clk_prepare_enable(dwmac->tx_clk); > > > + ret = clk_prepare_enable(dwmac->tx_clk); > > > + if (ret) { > > > + dev_err(&pdev->dev, > > > + "Failed to enable tx_clk\n"); > > > + return ret; > > > + } > > > > > > /* Check and configure TX clock rate */ > > > rate = clk_get_rate(dwmac->tx_clk); > > > @@ -117,6 +122,7 @@ static int intel_eth_plat_probe(struct platform_device *pdev) > > > rate = dwmac->data->tx_clk_rate; > > > ret = clk_set_rate(dwmac->tx_clk, rate); > > > if (ret) { > > > + clk_disable_unprepare(dwmac->tx_clk); > > > dev_err(&pdev->dev, > > > "Failed to set tx_clk\n"); > > > return ret; > > > > Hi Vitalii, > > > > I think that unwinding using a goto label would be more idiomatic here > > and in the following changes to intel_eth_plat_probe(). > > > > > @@ -131,6 +137,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev) > > > rate = dwmac->data->ptp_ref_clk_rate; > > > ret = clk_set_rate(plat_dat->clk_ptp_ref, rate); > > > if (ret) { > > > + if (dwmac->data->tx_clk_en) > > > + clk_disable_unprepare(dwmac->tx_clk); > > > dev_err(&pdev->dev, > > > "Failed to set clk_ptp_ref\n"); > > > return ret; > > > @@ -150,7 +158,8 @@ static int intel_eth_plat_probe(struct platform_device *pdev) > > > > > > ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res); > > > if (ret) { > > > - clk_disable_unprepare(dwmac->tx_clk); > > > + if (dwmac->data->tx_clk_en) > > > + clk_disable_unprepare(dwmac->tx_clk); > > > > Smatch warns that dwmac->data may be NULL here. > > FWIW, there is a patch [1] targeted at net-next which removes the seemingly > redundant check for dwmac->data. > > [1]: https://lore.kernel.org/netdev/20240930183926.2112546-1-mordan@ispras.ru/ > > At the moment device_get_match_data() can't return NULL in probe function > of this driver - it gets the data from static const intel_eth_plat_match[] > table where every entry has defined non-NULL .data. > > It's not expected (at least currently) that there would be any code changes > to the driver match table so it looks worthwhile to remove the check in > order to reduce additional complexity in error paths and > intel_eth_plat_remove(). Thanks, I hadn't correlated that patch with this one. I agree that it address my comment about needing to check wmac->data here and below. > That said, maybe it would be more safe now to rearrange the check to fail > at probe stage in case dwmac->data is NULL. Just not to confuse the static > analysis tools :) I see that Andrew Lunn responded that a comment is appropriate, rather than writing code to please the checkers. And, FWIIW, I agree with his suggestion. > > Thanks! > > > > > > return ret; > > > } > > > > > > @@ -162,7 +171,8 @@ static void intel_eth_plat_remove(struct platform_device *pdev) > > > struct intel_dwmac *dwmac = get_stmmac_bsp_priv(&pdev->dev); > > > > > > stmmac_pltfr_remove(pdev); > > > - clk_disable_unprepare(dwmac->tx_clk); > > > + if (dwmac->data->tx_clk_en) > > > > And I wonder if it can be NULL here too. > > > > > + clk_disable_unprepare(dwmac->tx_clk); > > > } > > > > > > static struct platform_driver intel_eth_plat_driver = { > > > -- > > > 2.25.1 > > > > > > >