[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fa7a8bc4-d1a1-3b1a-8b9e-618681d281dd@wanadoo.fr>
Date: Mon, 22 May 2023 19:36:47 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Tomer Maimon <tmaimon77@...il.com>
Cc: mturquette@...libre.com, sboyd@...nel.org, avifishman70@...il.com,
tali.perry1@...il.com, joel@....id.au, venture@...gle.com,
yuenn@...gle.com, benjaminfair@...gle.com,
openbmc@...ts.ozlabs.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v16 1/1] clk: npcm8xx: add clock controller
Le 22/05/2023 à 14:56, Tomer Maimon a écrit :
> Hi Christophe,
>
> Thanks for your comments
>
[...]
>>> +static struct clk_hw *
>>> +npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
>>> + const char *name, const struct clk_parent_data *parent,
>>> + unsigned long flags)
>>> +{
>>> + struct npcm8xx_clk_pll *pll;
>>> + struct clk_init_data init = {};
>>> + int ret;
>>> +
>>> + pll = kzalloc(sizeof(*pll), GFP_KERNEL);
>>
>> Everything looks devm_()'ed in this driver, except this kzalloc.
>> Except the one below, there is no kfree to free this memory, and no
>> .remove() function.
> Also clk_hw_register_divider_parent_data doesn't use devm_
> about free the pll, we use it, return at the end of the function.
> about adding remove, we had a dissection about it in V4, since the
> clock is a service driver it shouldn't be removed.
> https://patchwork.kernel.org/project/linux-watchdog/patch/20220621131424.162355-7-tmaimon77@gmail.com/
LoL.
At least, I'm consistent :).
Just to make it clear, what I mean about kfree() is not to add one here,
but either:
- to use devm_kzalloc() here, to avoid a leak, should loading the
driver fails OR
- have some kfree() where needed (at least in the error handling
path of the probe, if the remove function makes no point)
CJ
Powered by blists - more mailing lists