Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230521143202.155399-1-tmaimon77@gmail.com> <20230521143202.155399-2-tmaimon77@gmail.com>
 <0e72c35f-5d9d-45a7-5f85-3971b8029106@wanadoo.fr> <CAP6Zq1jOSKpzFvto1LMs=JftLK0fxrrg+73Sh34GunuLfcAfEA@mail.gmail.com>
 <fa7a8bc4-d1a1-3b1a-8b9e-618681d281dd@wanadoo.fr> <CAP6Zq1gb8zJc=2QwNbes-AXP25thKsEVJ11pQfX24eWN62zDMQ@mail.gmail.com>
In-Reply-To: <CAP6Zq1gb8zJc=2QwNbes-AXP25thKsEVJ11pQfX24eWN62zDMQ@mail.gmail.com>
From:   Tomer Maimon <tmaimon77@gmail.com>
Date:   Thu, 1 Jun 2023 12:27:33 +0300
Message-ID: <CAP6Zq1g00SVfPjfQLsgz3V+vU4VHq_MYFcoy0Um46NDZZ9iY_w@mail.gmail.com>
Subject: Re: [PATCH v16 1/1] clk: npcm8xx: add clock controller
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc:     mturquette@baylibre.com, sboyd@kernel.org, avifishman70@gmail.com,
        tali.perry1@gmail.com, joel@jms.id.au, venture@google.com,
        yuenn@google.com, benjaminfair@google.com,
        openbmc@lists.ozlabs.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Stephen,

Kind reminder regarding the patch, appreciate your comments.

Thanks,

Tomer

On Mon, 29 May 2023 at 18:52, Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> On Mon, 22 May 2023 at 20:36, Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
> >
> > Le 22/05/2023 =C3=A0 14:56, Tomer Maimon a =C3=A9crit :
> > > 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_da=
ta *parent,
> > >>> +                      unsigned long flags)
> > >>> +{
> > >>> +     struct npcm8xx_clk_pll *pll;
> > >>> +     struct clk_init_data init =3D {};
> > >>> +     int ret;
> > >>> +
> > >>> +     pll =3D 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/20220621131=
424.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)
> O.K. Thanks for your clarification.
> >
> > CJ
>
> Best regards,
>
> Tomer