linux-kernel - Re: [PATCH] cpufreq: mediatek: Use dev_err

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: <8602bf12-1b8d-4249-8814-52ecaa29d0ec@notapiano>
Date: Fri, 5 Jul 2024 11:13:09 -0400
From: Nícolas F. R. A. Prado <nfraprado@...labora.com>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Matthias Brugger <matthias.bgg@...il.com>, kernel@...labora.com,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH] cpufreq: mediatek: Use dev_err_probe in every error path
 in probe

On Tue, Jul 02, 2024 at 02:13:07PM +0530, Viresh Kumar wrote:
> On 02-07-24, 10:26, AngeloGioacchino Del Regno wrote:
> > Il 02/07/24 07:57, Viresh Kumar ha scritto:
> > > On 28-06-24, 15:48, Nícolas F. R. A. Prado wrote:
> > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> > > > @@ -629,11 +630,9 @@ static int mtk_cpufreq_probe(struct platform_device *pdev)
> > > >   	int cpu, ret;
> > > >   	data = dev_get_platdata(&pdev->dev);
> > > > -	if (!data) {
> > > > -		dev_err(&pdev->dev,
> > > > -			"failed to get mtk cpufreq platform data\n");
> > > > -		return -ENODEV;
> > > > -	}
> > > > +	if (!data)
> > > > +		return dev_err_probe(&pdev->dev, -ENODEV,
> > > 
> > > What's the point of calling dev_err_probe() when we know for sure that
> > > the error isn't EPROBE_DEFER ?
> > > 
> > 
> > Logging consistency, that's all; the alternative would be to rewrite the dev_err()
> > messages to be consistent with what dev_err_probe() says, so that'd be
> > dev_err("error %pe: (message)");
> 
> That would be better I guess. There is no point adding inefficient
> code.

Well, that would add duplication of the error format string, and make the error
message (the actual information here) less clear in the source code. Also,
dev_err doesn't return the error so it needs to be written in two lines, instead
of a single one.

Consider it's not just about consistency of the log messages, but consistency of
the source code as well: with so many drivers transitioning to the use of
`return dev_err_probe(...);` patterns, sticking to the pattern means it's
immediately clear what the code does. And it is a desirable pattern because it
removes all boilerplate and leaves just the real information (the error code and
the message).

Besides, the inneficient code amounts to an extra va_list usage and an int
comparison. That would only run once during boot and only in the error path...
I'm confident in saying this won't amount to any real performance gain. So the
usage of dev_err_probe() everywhere for log and source code standardization is
well worth it.

Thanks,
Nícolas