linux-kernel - RE: EMCCTD kernel driver Patch for review

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:	Mon, 11 Jan 2016 03:05:48 +0000
From:	"Seymour, Shane M" <shane.seymour@....com>
To:	"Singhal, Maneesh" <Maneesh.Singhal@....com>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"JBottomley@...n.com" <JBottomley@...n.com>,
	"martin.petersen@...cle.com" <martin.petersen@...cle.com>
Subject: RE: EMCCTD kernel driver Patch for review

Can I make some suggestions:

1) Include linux-api in the review or your new driver - you're creating new files in /proc and there's a good chance you'll get told that isn't going to be allowed. 
2) With this comment in the source:

+ *        DO NOT MODIFY ANY DATA STRUCTURES IN THIS FILE WITHOUT CONSULTING THE
+ *        GUEST OS GROUP!

How would someone do that? If someone really wanted to change a structure and others (outside of EMC) and had really good reasons for doing so I don't think you can really stop someone from doing it (as much as you may not like it). You might try something less direct like "Before modifying any data structures in this file please discuss the change with the EMC Guest OS group" and say if the maintainers email address should be used or something else. 

3) The patch doesn't patch the MAINTAINERS file to indicate who is the maintainer for the driver
4) Have you run it through checkpatch.pl? It should be producing no warnings or errors (that cannot be justified). I would have expected it to have triggered the following warning multiple times at the very least (since there's an overabundance of LINUX_VERSION_CODE in the patch):

# avoid LINUX_VERSION_CODE
		if ($line =~ /\bLINUX_VERSION_CODE\b/) {
			WARN("LINUX_VERSION_CODE",
			     "LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged\n" . $herecurr);
		}

I know I stopped looking at anything after seeing lots of #ifdefs containing LINUX_VERSION_CODE.

5) What about any relevant documentation for the driver under the Documentation directory (if you switch to sysfs you will need to create documentation for every new file you create in sysfs describing what it is).
6) Read all of: https://www.kernel.org/doc/Documentation/SubmittingPatches - running checkpatch.pl is covered as is number 6 - no attachments (you are probably lucky if anyone apart from me opened it). Your email should have started with [PATCH] in the subject - without that it may get lost of the mailing list and nobody may read it.

Consider addressing all of these. I doubt that you'll get much interest in terms of reviews until you do so.

Thanks
Shane