From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1C23B38F244 for ; Thu, 15 Jan 2026 12:34:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768480462; cv=none; b=oYcPQR//z5Xf7Nb9udmcYcI6RaZ82keh8fTkXeQvpVyDoc5oMIjNyr4rTo/7tFCZJkXCqcaUrfYVAK+mgFBRP3XXz8OiYrHpqpEDruKthukbcdtTBuXkk9GscLm1Vol3MPODgryeaAXoO9frRFkoZ9O7V0/v9841YGaC4Vc/HGc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768480462; c=relaxed/simple; bh=0DhpSjvfpByGwWWxOmMrQayWYnhrw3lEnX0Y7SfxVQs=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=FQo7q3UaSodghtKlii5LdNA/qdswmOGP/OTMJKqWFbUiLnG+BqGg7GkEewkR3JDcoR9grfHB8HLEPS8B5PijRm91IDqB4gUTbJPrNOsN2jW0SIuELgyGFxVoHqDZCoYCIc+m+v0xx4Kzru7of+vgnD4oR6J4KdNJ+oS2SgPHjfQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=HsDNlDm5; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=Qw+tz2if; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="HsDNlDm5"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="Qw+tz2if" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1768480460; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=QNPf7AyEdd66U2b9EqbJGKRCPDJ9Ni2qNv6cJ5EgabI=; b=HsDNlDm5ZBCiwHfDfPfyhpvcqa6cRkx9B9xHEQzgDlrkHPPo8Lh3Vei+t42bsglg5MBEzg l6d/wZbaq1Zjnymby3RG3pj/c9kERHOu+rSENrbox94DFCoP62VHTIRd14dcdIz88RDW5h TRPmyuCNGYa5PUOMglbd8xT8R6QMh2U= Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-594-DlclEArvMTO7LrdmrzPaWg-1; Thu, 15 Jan 2026 07:34:19 -0500 X-MC-Unique: DlclEArvMTO7LrdmrzPaWg-1 X-Mimecast-MFC-AGG-ID: DlclEArvMTO7LrdmrzPaWg_1768480458 Received: by mail-qt1-f199.google.com with SMTP id d75a77b69052e-50142190becso27745351cf.3 for ; Thu, 15 Jan 2026 04:34:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1768480458; x=1769085258; darn=vger.kernel.org; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=QNPf7AyEdd66U2b9EqbJGKRCPDJ9Ni2qNv6cJ5EgabI=; b=Qw+tz2ifzQPjemQjnFDUDfXRx/FnNaAHOjbjkoQm6P6RzTIUcv8N+RDjUeQfw0ar6V R/P4FssNKwnJnWqpd1kq1aIRfJLxeOw5jEF+PQSV3ONLthK2KunrWtcpQIpJkGhlTTiB crScnBTVBEIzrgRJEkOokfJhgb5eUc9XJNkoUfx2ik5VsxzfwBv08ajSAkqiQaeaeNgA YCeBQ2dPCiAyotPiXfETE9in6D/1QQNQTIPCWFFqOJONbBNqq18GCO/xB0uvWDZQE0i8 psZqd4SMQfz91nfEYlN/GgaimyrPmkXcwu9Zag5/nD7pjV1aniH5dqnIEWkdErD9rSO2 O/9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1768480458; x=1769085258; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=QNPf7AyEdd66U2b9EqbJGKRCPDJ9Ni2qNv6cJ5EgabI=; b=lYgCYdaMq9SSNjh4JMoDVkSAesuE1RY8OC6ERExaxspNKQvC39Mef2NqbNeDlHWIgJ SapOOM/0EyoCmBbSvcUZiWDnDgos28tw9D3Jxq6RDAdmW7SsJOsJ5Yhgr7UKLcXj8x3C x+jlK6b2pm2DaNfhGbs/uTt9R3L7er11UYB2fwaLxVUs2b50B0JdZU3gqJSy+EpU9GVb 3fH3aiYVfqAt8mochz20GwXzNWPIk7UIs4bXUx4XhG6Gmn7I0z91qpn+j/cEtMM2k8Kx bj4Se7gP/9uRHSjjbdyPSH3rX3BrMkB5pdgViiAeLe56m0q5NDNr+3auMMVF/V5yfrVn Xwkg== X-Forwarded-Encrypted: i=1; AJvYcCU5Wbx11lWT/m/hq9D6RH/U7jpEHXeAKSI73VL+9fx87cyMSW0uw1dWlULPmddZy3Dz6BCsFtxcQuoIBWU=@vger.kernel.org X-Gm-Message-State: AOJu0YyJ59qnVQMwbiM9JYDia7NXtsk6qklCjqZvgCdgcvxWQhFky68Q iqmXYAtoloqdIlrQIuUEMUQ1U1hMZ6nAM/iuhHIyAOebFoGkuMn82e8wkXQxOdYhp/MEV2QN5O1 3qlJXvGpkWMqMYodMjbebzPmTJjgni4UvCEgp2SkZ7J6QX6MyuYspqOrMkrNmU53ZdQ== X-Gm-Gg: AY/fxX4b82dEuw/vhYGHeehutAVy/9uvFafk8BUMo/N2MqFuDvAf9Aucn4g0hPLtK76 /C2UnT9WqfhKhPFYba/zAeeon6FAQ4SuI55vW/iwm6hfC1sTjX1nBl580iH+Fj90Jibsbmeqbyi iNGSBGHGQSe9JsFw6PwUKIFRKYjKkkFpOmCrlZ6w2osP9j7myo6A8j9ZSOgWzpbcnKJ6wao2zrD mnLY7n5rAPYTbHTJHzQTAO7kurbdMhjcOk9TkF3N3k8ApFhyruWWueKsBC5L27LxIsBfxEdHo7j pLN9KqdhLj5arLmZ78binBaYpB7sHK9euZP2bLInvSKoEag9tJ+RqHBV28y0DDvQg9FUt2wwkqz rzye5S9/L99jOHuD9WXzG+qz0I4EF2W6HYzMHs/qyus0T X-Received: by 2002:a05:622a:22aa:b0:4ff:c295:3c3e with SMTP id d75a77b69052e-501481e4073mr69469271cf.10.1768480458309; Thu, 15 Jan 2026 04:34:18 -0800 (PST) X-Received: by 2002:a05:622a:22aa:b0:4ff:c295:3c3e with SMTP id d75a77b69052e-501481e4073mr69468971cf.10.1768480457851; Thu, 15 Jan 2026 04:34:17 -0800 (PST) Received: from redhat.com (c-73-183-52-120.hsd1.pa.comcast.net. [73.183.52.120]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-892668a2419sm68145076d6.30.2026.01.15.04.34.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 15 Jan 2026 04:34:17 -0800 (PST) Date: Thu, 15 Jan 2026 07:34:16 -0500 From: Brian Masney To: Haoxiang Li Cc: mturquette@baylibre.com, sboyd@kernel.org, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] clk: st: clkgen-pll: Add cleaup in clkgen_c32_pll_setup() Message-ID: References: <20260115044439.632676-1-lihaoxiang@isrc.iscas.ac.cn> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260115044439.632676-1-lihaoxiang@isrc.iscas.ac.cn> User-Agent: Mutt/2.2.14 (2025-02-20) Hi Haoxiang, Thanks for the patch! On Thu, Jan 15, 2026 at 12:44:39PM +0800, Haoxiang Li wrote: > In clkgen_c32_pll_setup(), there exists several leaks if errors ouccers. > Add iounmap() to free the memory allocated by clkgen_get_register_base(). > Add clk_unregister() and kfree to do the cleaup for clkgen_pll_register(). > Add a while to do the cleaup for clkgen_odf_register(). > Use distinct variable names for two register calls' return values. > > Signed-off-by: Haoxiang Li There's a lot going on in this patch, and it's hard for someone else to review. Can you break this up into several patches? For example: - The kfree(pll_name) should be in it's own patch, with it's own explanation. - It would help if the rename of 'clk' to 'pll_clk' was in it's own patch. Along with the rename of 'clk' to 'odf_clk' in a separate patch. You can say in these two commit messages that the renames are in preparation for cleaning up some memory leaks. I'll pick up below with more suggestions. > --- > Changes in v2: > - Add several cleanups. Thanks, Brian! > - Modify the changelog. > --- > drivers/clk/st/clkgen-pll.c | 44 +++++++++++++++++++++++++------------ > 1 file changed, 30 insertions(+), 14 deletions(-) > > diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c > index c258ff87a171..100172f9fdf8 100644 > --- a/drivers/clk/st/clkgen-pll.c > +++ b/drivers/clk/st/clkgen-pll.c > @@ -752,11 +752,10 @@ static struct clk * __init clkgen_odf_register(const char *parent_name, > return clk; > } > > - > static void __init clkgen_c32_pll_setup(struct device_node *np, > struct clkgen_pll_data_clks *datac) > { > - struct clk *clk; > + struct clk *pll_clk; > const char *parent_name, *pll_name; > void __iomem *pll_base; > int num_odfs, odf; > @@ -774,18 +773,18 @@ static void __init clkgen_c32_pll_setup(struct device_node *np, > > of_clk_detect_critical(np, 0, &pll_flags); > > - clk = clkgen_pll_register(parent_name, datac->data, pll_base, pll_flags, > + pll_clk = clkgen_pll_register(parent_name, datac->data, pll_base, pll_flags, > np->name, datac->data->lock); > - if (IS_ERR(clk)) > - return; > + if (IS_ERR(pll_clk)) > + goto err_unmap; > > - pll_name = __clk_get_name(clk); > + pll_name = __clk_get_name(pll_clk); > > num_odfs = datac->data->num_odfs; > > clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL); > if (!clk_data) > - return; > + goto err_pll_unregister; This could also be it's own cleanup patch. > > clk_data->clk_num = num_odfs; > clk_data->clks = kcalloc(clk_data->clk_num, sizeof(struct clk *), > @@ -795,7 +794,7 @@ static void __init clkgen_c32_pll_setup(struct device_node *np, > goto err; > > for (odf = 0; odf < num_odfs; odf++) { > - struct clk *clk; > + struct clk *odf_clk; > const char *clk_name; > unsigned long odf_flags = 0; > > @@ -806,28 +805,45 @@ static void __init clkgen_c32_pll_setup(struct device_node *np, > if (of_property_read_string_index(np, > "clock-output-names", > odf, &clk_name)) > - return; > + goto err; > > of_clk_detect_critical(np, odf, &odf_flags); > } > > - clk = clkgen_odf_register(pll_name, pll_base, datac->data, > + odf_clk = clkgen_odf_register(pll_name, pll_base, datac->data, > odf_flags, odf, &clkgena_c32_odf_lock, > clk_name); > - if (IS_ERR(clk)) > - goto err; > + if (IS_ERR(odf_clk)) > + goto err_odf_unregister; > > - clk_data->clks[odf] = clk; > + clk_data->clks[odf] = odf_clk; > } > > of_clk_add_provider(np, of_clk_src_onecell_get, clk_data); > return; > > +err_odf_unregister: > + while (odf--) { odf is not initialized at the top of the function. It would be good to default it to 0. I know it is in the for loop, but just to be safe. > + struct clk_gate *gate = to_clk_gate(__clk_get_hw(clk_data->clks[odf])); > + struct clk_divider *div = to_clk_divider(__clk_get_hw(clk_data->clks[odf])); > + > + clk_unregister_composite(clk_data->clks[odf]); > + kfree(div); > + kfree(gate); > + } > err: > - kfree(pll_name); > kfree(clk_data->clks); > kfree(clk_data); > +err_pll_unregister: > + struct clkgen_pll *pll = to_clkgen_pll(__clk_get_hw(pll_clk)); Generally declarations go at the top of the function, or the top of an if / while. Since you are making changes to the top of the function, it would be good to add another patch to put the variables in reverse Christmas tree order (longest to shortest by name). > + > + clk_unregister(pll_clk); The clk_unregister() can also be it's own cleanup patch. > + kfree(pll); > +err_unmap: > + if (pll_base) > + iounmap(pll_base); The iounmap() can also be in it's own patch like you had in v1. You may end up with 6-8 patches, but each one will be small, boring, and it will make it easier for others review. Thanks! Brian