Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753909AbXJWDHY (ORCPT ); Mon, 22 Oct 2007 23:07:24 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752203AbXJWDHK (ORCPT ); Mon, 22 Oct 2007 23:07:10 -0400 Received: from mx1.redhat.com ([66.187.233.31]:39610 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752199AbXJWDHI (ORCPT ); Mon, 22 Oct 2007 23:07:08 -0400 Date: Mon, 22 Oct 2007 23:07:02 -0400 From: Dave Jones To: David Miller Cc: accusys.sw5@gmail.com, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: Add ACCUSYS RAID driver for Linux i386/x86-64 Message-ID: <20071023030701.GC7793@redhat.com> Mail-Followup-To: Dave Jones , David Miller , accusys.sw5@gmail.com, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org References: <20071022.190253.95509114.davem@davemloft.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20071022.190253.95509114.davem@davemloft.net> User-Agent: Mutt/1.5.14 (2007-02-12) Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3249 Lines: 108 On Mon, Oct 22, 2007 at 07:02:53PM -0700, David Miller wrote: > From: "Peter Chan" > Date: Tue, 23 Oct 2007 09:45:48 +0800 > > > Add linux-scsi and linux-kernel in mail group. > > Please do not post your driver as a "RAR" attachment, > not only are most Linux folks not familiar with this > archive format, it is also an attachment type rejected > by just about every large email service provider out > there. Before resubmitting this in a different format, this looks like it will need quite a bit of work before it's in mergable state. Just from a quick skim.. * Why are there separate drivers for i386 and x86-64 ? (With i386 and x86-64 now being one arch/ this makes even less sense) Typically in Linux we have one driver capable of driving the hardware, regardless of which architecture it's running on. The differences between the two seem to be locking related, which makes this look even more odd. * lots of #ifdef LINUX_VERSION_CODE > 2.5.0 and similar. These should just be removed. * None of this should be necessary.. #include #if defined(CONFIG_MODVERSIONS) && !defined(MODVERSIONS) #define MODVERSIONS #endif /* modversions.h should be before module.h */ #if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 0) #if defined(MODVERSIONS) #include #endif #endif * Don't use absolute paths in includes #include "/usr/src/linux/drivers/scsi/scsi.h" #include "/usr/src/linux/drivers/scsi/hosts.h" #include "/usr/src/linux/drivers/scsi/constants.h" #include "/usr/src/linux/drivers/scsi/sd.h" There's no guarantee (or need) for kernel source to be there. * Instead of reinventing a linked list implementation, use the one from * Use instead of reinventing your own. * Remove pointless wrappers like.. #define iowrite32 writel #define ioread32 readl and just use the functions directly. * This raises some eyebrows.. #include "/usr/src/linux/drivers/scsi/scsi_module.c" Asides from the absolute path problem, no new drivers should be using this file. There's even a helpful comment inside that file telling you this. * This isn't nice.. #define AllocRequestIndex(ResIndex)\ {\ /*DISABLE_IRQ();*/\ AllocRequest++;\ if (RequestHead == NULL) PANIC("Request FULL");\ ResIndex = RequestHead;\ ResIndex->InUsed = TRUE;\ RequestHead = RequestHead->Next;\ /*ENABLE_IRQ();*/\ } Drivers should never panic the box unless something seriously critical is happening. An allocation failure doesn't sound particularly catastrophic. * You don't need to redefine the SCSI opcodes, they are already defined for you in I stopped reading at this point. There's probably more lurking under that, and scripts/checkpatch.pl will probably pick up another bunch of nits. Dave -- http://www.codemonkey.org.uk - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/