linux-kernel - Re: [PATCH v1 06/16] clk: starfive: Add JH7100 clock generator driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANBLGcxBjrhp6sfLqsKu=0hgYh+oxmozriWVCwdSKFYWtYnPmQ@mail.gmail.com>
Date:   Tue, 12 Oct 2021 23:26:27 +0200
From:   Emil Renner Berthing <kernel@...il.dk>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     linux-riscv <linux-riscv@...ts.infradead.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Rob Herring <robh+dt@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Jiri Slaby <jirislaby@...nel.org>,
        Maximilian Luz <luzmaximilian@...il.com>,
        Sagar Kadam <sagar.kadam@...ive.com>,
        Drew Fustini <drew@...gleboard.org>,
        Anup Patel <anup.patel@....com>,
        Atish Patra <atish.patra@....com>,
        Matteo Croce <mcroce@...rosoft.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 06/16] clk: starfive: Add JH7100 clock generator driver

On Tue, 12 Oct 2021 at 23:21, Andy Shevchenko <andy.shevchenko@...il.com> wrote:
>
> On Tue, Oct 12, 2021 at 11:08 PM Emil Renner Berthing <kernel@...il.dk> wrote:
> > On Tue, 12 Oct 2021 at 17:40, Andy Shevchenko <andy.shevchenko@...il.com> wrote:
> > > On Tue, Oct 12, 2021 at 4:42 PM Emil Renner Berthing <kernel@...il.dk> wrote:
>
> ...
>
> > > > +       value |= readl_relaxed(reg) & ~mask;
> > >
> > > value is not masked, is it okay?
> > >
> > > Usual pattern for this kind of operations is
> > >
> > > value = (current & ~mask) | (value & mask);
> >
> > This function is only ever called with constants, already masked
> > values or the parent number from the clk framework, so it should be
> > ok.
>
> Up to you, but I think it's better to have a usual pattern.
>
> > > > +       writel_relaxed(value, reg);
>
> ...
>
> > > > +       rate = parent / div;
> > > > +       if (rate < req->min_rate && div > 1) {
> > > > +               div -= 1;
> > > > +               rate = parent / div;
> > > > +       }
> > >
> > > Seems like homegrown DIV_ROUND_UP() or so. Who will guarantee that
> > > decreasing div by 1 will satisfy the conditional again?
> >
> > Maths unless I'm mistaken: div = DIV_ROUND_UP(parent, target), so in
> > rational numbers
> >   div - 1 < parent / target
> > But the target is clamped by min_rate and max_rate, so
> >   min_rate <= target < parent / (div - 1) = rate
> >
> > Sorry, re-using the rate varable for both the target and result is
> > confusing. I'll fix that.
>
> Also needs a comment, I believe.

Will add.

> ...
>
> > > > +#ifdef CONFIG_DEBUG_FS
> > >
> > > Perhaps __maybe_unused?
> >
> > I can definitely use __maybe_unused for the function declaration, but
> > then I'll need a conditional every time clk_ops.debug_init needs to be
> > initialized to either the function or NULL depending on
> > CONFIG_DEBUG_FS below. Is that better?
>
> Actually, why can't you always initialize the field? Shouldn't CLK
> core take care about this conditional?

It could, but I see other drivers avoiding the code bloat when debugfs
is not enabled, so I thought I'd copy that.

> > > > +#else
> > > > +#define jh7100_clk_debug_init NULL
> > > > +#endif
>
> ...
>
> > > > +       while (idx > 0)
> > > > +               clk_hw_unregister(&priv->reg[--idx].hw);
> > >
> > > The
> > >
> > >        while (idx--)
> > >                clk_hw_unregister(&priv->reg[idx].hw);
> > >
> > > is slightly better to read.
> >
> > It's not something I'll insist hard on, but I must admit I disagree.
> > To me the above looks like cartoon characters running off a cliff and
> > back. As a middle ground could we maybe do this?
> >
> >   while (idx)
> >     clk_hw_unregister(&priv->reg[--idx].hw);
>
> My point is exactly in having the common pattern for error paths, i.e.
>
>   while (counter--)
>     ...bla-bla-bla...
>
> Your second approach is better, but I think that proposed by me is even better.
>
> ...
>
> > > > +subsys_initcall(clk_starfive_jh7100_init);
> > >
> > > Any  explanation why subsys_initcall() is in use?
> >
> > TBH I just inherited that from Geert's first mock driver and never
> > thought to question it. What would be a better alternative to try?
>
> At least add a comment to explain the choice.
>
> --
> With Best Regards,
> Andy Shevchenko