linux-kernel - Re: [PATCH 1/3] Add Numachip APIC support

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: <20111027074449.GF15923@elte.hu>
Date:	Thu, 27 Oct 2011 09:44:49 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Daniel J Blueman <daniel@...ascale-asia.com>
Cc:	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, H Peter Anvin <hpa@...or.com>,
	Steffen Persvold <sp@...ascale.com>,
	linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH 1/3] Add Numachip APIC support

* Daniel J Blueman <daniel@...ascale-asia.com> wrote:

> +	numachip_write_local_csr(NUMACHIP_CSR_G3_EXT_INTERRUPT_GEN,
> +				 int_gen.v);

> +	numachip_write_local_csr(NUMACHIP_CSR_G3_EXT_INTERRUPT_GEN,
> +				 int_gen.v);

> +	numachip_write_local_csr(NUMACHIP_CSR_G3_EXT_INTERRUPT_GEN,
> +				 int_gen.v);

Please don't break the line in an ugly way like that just to placate 
checkpatch.pl.

checkpatch.pl is right to complain about the too long line here - but 
your fix is wrong: the proper approach is to make the code *shorter*.

So for example instead of numachip_write_local_csr() you could 
abbreviate it to something obvious such as:

	write_lcsr()

it's not like these will be used outside of the Numachip code, so 
there's no confusion from the missing numachip_ prefix.

write_gcsr() can be used for the global registers.

Likewise, NUMACHIP_CSR_G3_EXT_INTERRUPT_GEN is way too long as well - 
you can easily rename it to CSR_G3_EXT_IRQ_GEN or such, without 
losing expressive value. Then it could be written in the much shorter 
form of:

	write_lcsr(CSR_G3_EXT_IRQ_GEN, irq_gen.v);

... and magically checkpatch.pl won't complain anymore.

Likewise there's ton's of other way too long names in the patch as 
well - please try to abbreviate them wherever that can be done 
without losing expressive value.

Note, we don't want to over-do it: we obviously don't want to shorten 
names to CSRG3EIG kind of gibberish, so *some* length is of course 
acceptable.

When a line becomes too long then leave it too long instead of 
breaking it - line length up to say 100 cols in width are still OK, 
as long as the surrounding code does not have obvious deficiencies 
like too much complexity.

Note, while the merge window is in progress already, i think we can 
still add these patches slightly outside the merge window as well, as 
they add new hardware support and don't risk regressions elsewhere - 
if all the structural and fine-detail problems i pointed out during 
review are fixed.

Thanks,

	Ingo
--
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/