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.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 AE98FC433DB for ; Tue, 22 Dec 2020 04:36:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7F9D02313C for ; Tue, 22 Dec 2020 04:36:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726030AbgLVEfw (ORCPT ); Mon, 21 Dec 2020 23:35:52 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33180 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725945AbgLVEfv (ORCPT ); Mon, 21 Dec 2020 23:35:51 -0500 Received: from mail-pg1-x529.google.com (mail-pg1-x529.google.com [IPv6:2607:f8b0:4864:20::529]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0CFA6C06179C for ; Mon, 21 Dec 2020 20:35:11 -0800 (PST) Received: by mail-pg1-x529.google.com with SMTP id x1so2015905pgh.12 for ; Mon, 21 Dec 2020 20:35:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=efVgO12y6qfjlIavBoObF8HNxu4Hvm/8sbVusrJEyQg=; b=gcGcsrRKLFZqUishLYUK2rrAojhE9KbDilt+wO3akI1bUTMp6W4g6QDuvluwKINvEL LPQJwcQesfB7tbcvglhfu/eDlPpLMgIABhch24FsZA3KmDR5YnK4Jf8Ij8kyMStXn0PD eWrEAmPy2T8neKZkvacTysW0dP1CPT0DnemUt9jwi+Nx4w2NaxhUEDnLXb1QS18HYoks r2JGrQ7jj61jt/hpzITPtrXoSmH3d5GRx/0w2CbDGyKlSWKXNYq53HXZCg8RcdvG2LID +lGBlkcfXuKgcYgzovKXbGfJNYLh782NpxjOHrljhj2ug57mZoDd8Nu++wsQhMiCD6G4 yKWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=efVgO12y6qfjlIavBoObF8HNxu4Hvm/8sbVusrJEyQg=; b=q5eI6s3mHIg/jiUCNvIx8BGSOyyCpIiLrEl1MYx5UTTZfpANjiRU4NK6cEKsc8+q3i JeJVxeHUTQIG4scNdvXQUgw671ve+Gqw5HCJjOr/dg7MuE3guZGn/aBl9lCu8YT/dTB2 /trYDJQaSX8CBve1M1e01cV/BI28yo4+IFk1ar/7VvT1a1LcZDdf9yXXhtI/7fagc1OE /z+PklhWcwpRmiJbR1j30fFtw/SLCQxTEbCV9bvZLj+lJKVcnAlzoUgZeeYtl7xThmNq /8OywJBzF0LOJRpZq3nM1adDa3CE9bq569yB0VY8daHFlRxakL7qiXfvFBFhOWqeFz4Q ZG8g== X-Gm-Message-State: AOAM531fx27BYZBzXlj93oXTBzoBkVmsJhjNJdAMReEm7s0inPZTLMMy 55rHkswBy/ixYcT7FyH7AYrrZA== X-Google-Smtp-Source: ABdhPJyIViesLe5L57QCib4Tc30kaHqp521NAFEgLLhIV79uAFs5gXWHG0AJbrltGyBakJ301c0CIQ== X-Received: by 2002:a65:5b47:: with SMTP id y7mr16390380pgr.221.1608611710462; Mon, 21 Dec 2020 20:35:10 -0800 (PST) Received: from localhost ([122.172.20.109]) by smtp.gmail.com with ESMTPSA id 6sm18090487pfj.216.2020.12.21.20.35.09 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 21 Dec 2020 20:35:09 -0800 (PST) Date: Tue, 22 Dec 2020 10:05:05 +0530 From: Viresh Kumar To: Christophe JAILLET Cc: mmayer@broadcom.com, bcm-kernel-feedback-list@broadcom.com, rjw@rjwysocki.net, f.fainelli@gmail.com, linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: Re: [PATCH] cpufreq: brcmstb-avs-cpufreq: Fix some resource leaks in the error handling path of the probe function Message-ID: <20201222043505.rq3cmajc3mxv3p2z@vireshk-i7> References: <20201219101751.181783-1-christophe.jaillet@wanadoo.fr> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201219101751.181783-1-christophe.jaillet@wanadoo.fr> User-Agent: NeoMutt/20180716-391-311a52 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 19-12-20, 11:17, Christophe JAILLET wrote: > If 'cpufreq_register_driver()' fails, we must release the resources > allocated in 'brcm_avs_prepare_init()' as already done in the remove > function. > > To do that, introduce a new function 'brcm_avs_prepare_uninit()' in order > to avoid code duplication. This also makes the code more readable (IMHO). > > Fixes: de322e085995 ("cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs") > Signed-off-by: Christophe JAILLET > --- > I'm not sure that the existing error handling in the remove function is > correct and/or needed. > --- > drivers/cpufreq/brcmstb-avs-cpufreq.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c > index 3e31e5d28b79..750ca7cfccb0 100644 > --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c > +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c > @@ -597,6 +597,16 @@ static int brcm_avs_prepare_init(struct platform_device *pdev) > return ret; > } > > +static void brcm_avs_prepare_uninit(struct platform_device *pdev) > +{ > + struct private_data *priv; > + > + priv = platform_get_drvdata(pdev); > + > + iounmap(priv->avs_intr_base); > + iounmap(priv->base); > +} > + > static int brcm_avs_cpufreq_init(struct cpufreq_policy *policy) > { > struct cpufreq_frequency_table *freq_table; > @@ -732,21 +742,26 @@ static int brcm_avs_cpufreq_probe(struct platform_device *pdev) > > brcm_avs_driver.driver_data = pdev; > > - return cpufreq_register_driver(&brcm_avs_driver); > + ret = cpufreq_register_driver(&brcm_avs_driver); > + if (ret) > + goto err_uninit; > + > + return 0; > + > +err_uninit: > + brcm_avs_prepare_uninit(pdev); > + return ret; Maybe rewrite as: ret = cpufreq_register_driver(&brcm_avs_driver); if (ret) brcm_avs_prepare_uninit(pdev); return ret; > } > > static int brcm_avs_cpufreq_remove(struct platform_device *pdev) > { > - struct private_data *priv; > int ret; > > ret = cpufreq_unregister_driver(&brcm_avs_driver); > if (ret) > return ret; Instead of returning here, it can be just WARN_ON(ret); and then go on and free the resources and this needs to be done in a separate patch. > > - priv = platform_get_drvdata(pdev); > - iounmap(priv->base); > - iounmap(priv->avs_intr_base); > + brcm_avs_prepare_uninit(pdev); > > return 0; > } -- viresh From mboxrd@z Thu Jan 1 00:00:00 1970 From: Viresh Kumar Date: Tue, 22 Dec 2020 04:47:05 +0000 Subject: Re: [PATCH] cpufreq: brcmstb-avs-cpufreq: Fix some resource leaks in the error handling path of the Message-Id: <20201222043505.rq3cmajc3mxv3p2z@vireshk-i7> List-Id: References: <20201219101751.181783-1-christophe.jaillet@wanadoo.fr> In-Reply-To: <20201219101751.181783-1-christophe.jaillet@wanadoo.fr> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Christophe JAILLET Cc: f.fainelli@gmail.com, linux-pm@vger.kernel.org, kernel-janitors@vger.kernel.org, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com, mmayer@broadcom.com, linux-arm-kernel@lists.infradead.org On 19-12-20, 11:17, Christophe JAILLET wrote: > If 'cpufreq_register_driver()' fails, we must release the resources > allocated in 'brcm_avs_prepare_init()' as already done in the remove > function. > > To do that, introduce a new function 'brcm_avs_prepare_uninit()' in order > to avoid code duplication. This also makes the code more readable (IMHO). > > Fixes: de322e085995 ("cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs") > Signed-off-by: Christophe JAILLET > --- > I'm not sure that the existing error handling in the remove function is > correct and/or needed. > --- > drivers/cpufreq/brcmstb-avs-cpufreq.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c > index 3e31e5d28b79..750ca7cfccb0 100644 > --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c > +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c > @@ -597,6 +597,16 @@ static int brcm_avs_prepare_init(struct platform_device *pdev) > return ret; > } > > +static void brcm_avs_prepare_uninit(struct platform_device *pdev) > +{ > + struct private_data *priv; > + > + priv = platform_get_drvdata(pdev); > + > + iounmap(priv->avs_intr_base); > + iounmap(priv->base); > +} > + > static int brcm_avs_cpufreq_init(struct cpufreq_policy *policy) > { > struct cpufreq_frequency_table *freq_table; > @@ -732,21 +742,26 @@ static int brcm_avs_cpufreq_probe(struct platform_device *pdev) > > brcm_avs_driver.driver_data = pdev; > > - return cpufreq_register_driver(&brcm_avs_driver); > + ret = cpufreq_register_driver(&brcm_avs_driver); > + if (ret) > + goto err_uninit; > + > + return 0; > + > +err_uninit: > + brcm_avs_prepare_uninit(pdev); > + return ret; Maybe rewrite as: ret = cpufreq_register_driver(&brcm_avs_driver); if (ret) brcm_avs_prepare_uninit(pdev); return ret; > } > > static int brcm_avs_cpufreq_remove(struct platform_device *pdev) > { > - struct private_data *priv; > int ret; > > ret = cpufreq_unregister_driver(&brcm_avs_driver); > if (ret) > return ret; Instead of returning here, it can be just WARN_ON(ret); and then go on and free the resources and this needs to be done in a separate patch. > > - priv = platform_get_drvdata(pdev); > - iounmap(priv->base); > - iounmap(priv->avs_intr_base); > + brcm_avs_prepare_uninit(pdev); > > return 0; > } -- viresh 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=-15.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 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 A60A1C433E0 for ; Tue, 22 Dec 2020 04:37:02 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 62D6222B2D for ; Tue, 22 Dec 2020 04:37:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 62D6222B2D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject: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=cbkoSqhXy2rgPM8U0IiTP/Gj35YA5LlKo5IC3dYbP2Q=; b=NugWykoEdQhBf52Vtg7Ar6i6v KK5jP/D9SobDlF5yRuWSPDJ3794NetktpbwjdGBZiXcbqKyPmInnODuVW2dlIw2AvZd8hsLLyi4V1 VGm1NaHC7Ugg7cXlrN8Li0ahuX7sWe70jfQ9NSXYL6I2WOzkuG+k8lvsN6KzzVVuo2qboyjiR+4z3 4PU9oI+pXgV+6QG9LeeUyLK8Ennd9BQOHFh8q2Uoeip+Ya38GBpszpK3ekj8AlrRbqb/+XSBxrid3 +P3uzWQc9rbuHOP3uB2Qn42Sca9dUmhDY6ApVL1wg4Vxi8lS/slaW5CAybPmGf1BOhW9h+KZF7w48 +LIZGgKNg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1krZOH-0006xd-09; Tue, 22 Dec 2020 04:35:17 +0000 Received: from mail-pg1-x52d.google.com ([2607:f8b0:4864:20::52d]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1krZOE-0006xA-As for linux-arm-kernel@lists.infradead.org; Tue, 22 Dec 2020 04:35:15 +0000 Received: by mail-pg1-x52d.google.com with SMTP id e2so7602364pgi.5 for ; Mon, 21 Dec 2020 20:35:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=efVgO12y6qfjlIavBoObF8HNxu4Hvm/8sbVusrJEyQg=; b=gcGcsrRKLFZqUishLYUK2rrAojhE9KbDilt+wO3akI1bUTMp6W4g6QDuvluwKINvEL LPQJwcQesfB7tbcvglhfu/eDlPpLMgIABhch24FsZA3KmDR5YnK4Jf8Ij8kyMStXn0PD eWrEAmPy2T8neKZkvacTysW0dP1CPT0DnemUt9jwi+Nx4w2NaxhUEDnLXb1QS18HYoks r2JGrQ7jj61jt/hpzITPtrXoSmH3d5GRx/0w2CbDGyKlSWKXNYq53HXZCg8RcdvG2LID +lGBlkcfXuKgcYgzovKXbGfJNYLh782NpxjOHrljhj2ug57mZoDd8Nu++wsQhMiCD6G4 yKWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=efVgO12y6qfjlIavBoObF8HNxu4Hvm/8sbVusrJEyQg=; b=UXBYNLIcJ9bjHnMNG/VdcKcEq00AZBdGO4bSKJTGD5m0pQPbWdO0Tdrivd4WPlomGZ CzflcZlKPaYOnQ3F4ygzZCujFzAlu8xL/9yFLPZBfefvPWQDLnhXpMgGHIBAmq72fq8S s0H+P8LiWG+MSicWfZCv+pXGTLjmpPyfxbq3gB1Jzozk9ZgOZpmu2SNrDdBN6OULStDU N+N0uA/pnRd2S9tykppMrOVyn8YPN5lqRuklXKQ64ebyP0YT/9dA0IPmmGttrD2/SZrD r+UV/3t0VQw9Os3RROvm6so92+aRq/LhePXbvx0E6/uoLKEufbJeUOM41E5JryEpTwa4 wX1w== X-Gm-Message-State: AOAM531iTpoD3NjnhPpaLxuta5CucOl5Zo03deMwbMnpPlupt7sNqccw EpStMgDn1iz/ljMEVNZ8qp4PMcJ+bOyK6Q== X-Google-Smtp-Source: ABdhPJyIViesLe5L57QCib4Tc30kaHqp521NAFEgLLhIV79uAFs5gXWHG0AJbrltGyBakJ301c0CIQ== X-Received: by 2002:a65:5b47:: with SMTP id y7mr16390380pgr.221.1608611710462; Mon, 21 Dec 2020 20:35:10 -0800 (PST) Received: from localhost ([122.172.20.109]) by smtp.gmail.com with ESMTPSA id 6sm18090487pfj.216.2020.12.21.20.35.09 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 21 Dec 2020 20:35:09 -0800 (PST) Date: Tue, 22 Dec 2020 10:05:05 +0530 From: Viresh Kumar To: Christophe JAILLET Subject: Re: [PATCH] cpufreq: brcmstb-avs-cpufreq: Fix some resource leaks in the error handling path of the probe function Message-ID: <20201222043505.rq3cmajc3mxv3p2z@vireshk-i7> References: <20201219101751.181783-1-christophe.jaillet@wanadoo.fr> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201219101751.181783-1-christophe.jaillet@wanadoo.fr> User-Agent: NeoMutt/20180716-391-311a52 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201221_233514_682108_14239DD6 X-CRM114-Status: GOOD ( 26.19 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: f.fainelli@gmail.com, linux-pm@vger.kernel.org, kernel-janitors@vger.kernel.org, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com, mmayer@broadcom.com, linux-arm-kernel@lists.infradead.org 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 19-12-20, 11:17, Christophe JAILLET wrote: > If 'cpufreq_register_driver()' fails, we must release the resources > allocated in 'brcm_avs_prepare_init()' as already done in the remove > function. > > To do that, introduce a new function 'brcm_avs_prepare_uninit()' in order > to avoid code duplication. This also makes the code more readable (IMHO). > > Fixes: de322e085995 ("cpufreq: brcmstb-avs-cpufreq: AVS CPUfreq driver for Broadcom STB SoCs") > Signed-off-by: Christophe JAILLET > --- > I'm not sure that the existing error handling in the remove function is > correct and/or needed. > --- > drivers/cpufreq/brcmstb-avs-cpufreq.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c > index 3e31e5d28b79..750ca7cfccb0 100644 > --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c > +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c > @@ -597,6 +597,16 @@ static int brcm_avs_prepare_init(struct platform_device *pdev) > return ret; > } > > +static void brcm_avs_prepare_uninit(struct platform_device *pdev) > +{ > + struct private_data *priv; > + > + priv = platform_get_drvdata(pdev); > + > + iounmap(priv->avs_intr_base); > + iounmap(priv->base); > +} > + > static int brcm_avs_cpufreq_init(struct cpufreq_policy *policy) > { > struct cpufreq_frequency_table *freq_table; > @@ -732,21 +742,26 @@ static int brcm_avs_cpufreq_probe(struct platform_device *pdev) > > brcm_avs_driver.driver_data = pdev; > > - return cpufreq_register_driver(&brcm_avs_driver); > + ret = cpufreq_register_driver(&brcm_avs_driver); > + if (ret) > + goto err_uninit; > + > + return 0; > + > +err_uninit: > + brcm_avs_prepare_uninit(pdev); > + return ret; Maybe rewrite as: ret = cpufreq_register_driver(&brcm_avs_driver); if (ret) brcm_avs_prepare_uninit(pdev); return ret; > } > > static int brcm_avs_cpufreq_remove(struct platform_device *pdev) > { > - struct private_data *priv; > int ret; > > ret = cpufreq_unregister_driver(&brcm_avs_driver); > if (ret) > return ret; Instead of returning here, it can be just WARN_ON(ret); and then go on and free the resources and this needs to be done in a separate patch. > > - priv = platform_get_drvdata(pdev); > - iounmap(priv->base); > - iounmap(priv->avs_intr_base); > + brcm_avs_prepare_uninit(pdev); > > return 0; > } -- viresh _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel