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: <aF6_jVJYCXeKZfXo@smile.fi.intel.com>
Date: Fri, 27 Jun 2025 18:58:05 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Rahul Pathak <rpathak@...tanamicro.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

On Fri, Jun 27, 2025 at 08:36:41PM +0530, Rahul Pathak wrote:

...

> > > > > +     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.

Because it's simply bad code, look at the simplified model:

	int foo, bar;
	int ret;

	func_1(..., &foo, &bar);
	ret = func_2(&foo);
	if (ret || bar)
		...do something...

When one reads this code the immediate reaction will be like mine.
This is also (without deeper understanding) tempting to someone
who even thinks that the code can be simplified (w/o knowing that it
may not) to change it as

	func_1(..., &foo, &bar);
	if (bar)
		...do something...

	ret = func_2(&foo);
	if (ret)
		...do something...

Using msg is the right thing to do. In that way there is no questions asked
and everything is clear. Also why layering violation? Because the conditional
requires to know the guts of rx in the code which doesn't use rx that way
(or rather using it as semi-opaque object).

-- 
With Best Regards,
Andy Shevchenko



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ