linux-kernel - Re: [PATCH v2 21/22] arm64: cpuinfo: Expose MIDR_EL1 and REVIDR

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]

Date:	Tue, 6 Oct 2015 20:16:41 +0100
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Steve Capper <steve.capper@...aro.org>
Cc:	"Suzuki K. Poulose" <suzuki.poulose@....com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Mark Rutland <mark.rutland@....com>, Vladimir.Murzin@....com,
	Ard Biesheuvel <ard.biesheuvel@...aro.org>,
	Marc Zyngier <marc.zyngier@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Edward Nevill <edward.nevill@...aro.org>, aph@...hat.com,
	james.morse@....com, Andre Przywara <andre.przywara@....com>,
	dave.martin@....com
Subject: Re: [PATCH v2 21/22] arm64: cpuinfo: Expose MIDR_EL1 and REVIDR_EL1
 to sysfs

On Tue, Oct 06, 2015 at 11:18:42AM +0100, Steve Capper wrote:
> On 6 October 2015 at 10:09, Russell King - ARM Linux
> <linux@....linux.org.uk> wrote:
> > On Mon, Oct 05, 2015 at 06:02:10PM +0100, Suzuki K. Poulose wrote:
> >> +static int __init cpuinfo_regs_init(void)
> >> +{
> >> +     int cpu, ret;
> >> +
> >> +     for_each_present_cpu(cpu) {
> >> +             struct device *dev = get_cpu_device(cpu);
> >> +
> >> +             if (!dev)
> >> +                     return -1;
> >
> > NAK.  Go figure out why, I'm too lazy to tell you.
> 
> I will correct the return code to be -ENODEV.
> Was that the reasoning behind the NAK?

The reason behind the NAK was indeed returning -1 from a function where
a negative return code is a negative errno value.  I'm fed up with
telling people about this, and I put it down to laziness.  So, I've
decided I'm going to NAK such patches and let the author have the hard
work of working out why - hopefully learning something by doing so,
and hopefully remembering for next time. :)

-1 corresponds to -EPERM, which means "operation not permitted"
(and, if this gets returned to userspace, the user program executing
the syscall will most probably tell the user "operation not permitted"
as the reason for failure - which is rather daft when you think about
it.

I have my mail client setup to highlight with a red background on a
"\+.*return -1" regexp, which nicely lets me catch every one of those
in any patch I skim through.

This is about quality of implementation issue - and, tbh, if someone
can't spend the time to check what happens to the return value, and
then can't spend the time to look up a proper errno value, then what's
the chance the rest of the patch (or patch set) has had sufficient care
and attention paid to it?

I'm getting to the point where if I see a "return -1" I just stop
reviewing the rest of the patch set, especially if it's a large patch
set.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/