linux-kernel - Re: [PATCH v6 09/23] clk: Add clock driver for the RISC-V RPMI clock service group

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CA+Oz1=bAsykB=qAk3r8FV8K8cnPEVT4Ow7L4SWBvrc_3DsyaWw@mail.gmail.com>
Date: Fri, 27 Jun 2025 20:36:41 +0530
From: Rahul Pathak <rpathak@...tanamicro.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Anup Patel <apatel@...tanamicro.com>, Michael Turquette <mturquette@...libre.com>, 
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jassi Brar <jassisinghbrar@...il.com>, Thomas Gleixner <tglx@...utronix.de>, 
	"Rafael J . Wysocki" <rafael@...nel.org>, Mika Westerberg <mika.westerberg@...ux.intel.com>, 
	Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>, 
	Uwe Kleine-König <ukleinek@...nel.org>, 
	Palmer Dabbelt <palmer@...belt.com>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Len Brown <lenb@...nel.org>, Sunil V L <sunilvl@...tanamicro.com>, 
	Leyfoon Tan <leyfoon.tan@...rfivetech.com>, Atish Patra <atish.patra@...ux.dev>, 
	Andrew Jones <ajones@...tanamicro.com>, Samuel Holland <samuel.holland@...ive.com>, 
	Anup Patel <anup@...infault.org>, linux-clk@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 09/23] clk: Add clock driver for the RISC-V RPMI clock
 service group

> > > > +union rpmi_clk_rates {
> > > > +     u64 discrete[RPMI_CLK_DISCRETE_MAX_NUM_RATES];
> > > > +     struct {
> > > > +             u64 min;
> > > > +             u64 max;
> > > > +             u64 step;
> > > > +     } linear;
> > >
> > > Have you looked at the linear_range.h? Why can it not be (re-)used here?
> >
> > I did the first time only when you commented. And i dont see any
> > benefit in that.
> > linear_range has slightly different way to access any value using `sel`.
> > Here this union represents how RPMI protocol represents the rates and
> > reusing linear_range will only introduce conversion to and fro.
>
> Summarize this in the comment on top of the struct definition so one will not
> attempt to convert the driver in the future.
>

Sure, I will update.

> >
> > >
> > > > +     if (ret || rx.status)
> > > > +             return 0;
> > >
> > > Why rx.status can't be checked before calling to a sending message?
> > > Sounds like the rpmi_mbox_init_send_with_response() links rx to msg somehow.
> > > If this is the case, use msg here, otherwise move the check to be in the
> > > correct place.
> >
> > Yes, the rpmi_mbox_init_send_with_response is a helper function which links
> > the rx to msg. It's a very simple function which only performs assignments.
> >
> > Using msg instead of rx directly will require additional typecasting
> > which will only clutter
> > I can add a comment if that helps wherever the rpmi_mbox_init_send_with_response
> > is used.
>
> This is besides harder-to-read code is kinda of layering violation.
> If you afraid of a casting, add a helper to check for the status error.
> Comment won't help much as making code better to begin with.

Why using rx is the issue in the first place when it's the same layer
which links the rx with msg using the helper function and then
uses it directly?  Infact using rx directly avoids unnecessary code
which is only increasing redundant code which ultimately results in
same thing. Even if I add a helper function that will require additional
pointer passing with NULL checking which all is currently avoided.
And, we are not just talking about rx.status but a lot of other fields.

>
> > > > +     struct rpmi_clk_context *context = rpmi_clk->context;
> > > > +     struct rpmi_clk_rate_discrete *rate_discrete;
> > > > +     struct rpmi_clk_rate_linear *rate_linear;
>
> > > > +     struct rpmi_get_supp_rates_rx *rx __free(kfree) = NULL;
>
> This should be assigned where it's used. NULL assignment is not encouraged.

Sure, I will update.

>
> > > > +     struct rpmi_get_supp_rates_tx tx;
> > > > +     struct rpmi_mbox_message msg;
> > >
> > > > +     size_t clk_rate_idx = 0;
> > >
> > > This kind of assignments is hard to maintain and it's mistake prone in case
> > > some additional code is injected in the future that might reuse it.
> > >
> > I dont understand what is the problem with this assignment. If any
> > code added in the future reuse it then it has to make sure that
> > clk_rate_idx has the correct initial value before any further references.
>
> Yes, it will add an additional churn and require more brain activity to
> maintain such a code. It's a general recommendation to assign when it's
> actually needed (there are, of course, exceptions for some small functions,
> but this one is not).

Sure, I will update.



-- 

Thanks
Rahul Pathak