Re: Add ACCUSYS RAID driver for Linux i386/x86-64
On Mon, 22 Oct 2007 18:17:49 +0800 Peter Chan [EMAIL PROTECTED] wrote:
Dear Morton
Thanks for your doing.
We modified source code as your requested. If you have any comment please
let me know.
Do you need RAID HBA to test at this stage? If yes, Which address can i
ship RAID HBA for you?
Please, you really will need to become a bit more familiar with the way we
work. As far as I know, nobody in the linux world uses RAR format - that's
a windows thing. I doubt if anyone except I has actually gone to the
effort to decrypt that attachment.
Start with
Documentation/SubmittingPatches
Documentation/SubmittingDrivers
Documentation/SubmitChecklist
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
http://linux.yyz.us/patch-format.html
There are a number of remaining stylistic things which we can look at more
closely when we have patches which are in a usable form.
- Linux doesn't use capitalisation in variable names. Use tail, not Tail
- Linux uses underscored to separate words. Use reply_frame, not
replyframe or ReplyFrame.
- We don't like to see code which has any dependency on
LINUX_VERSION_CODE or KENREL_VERSION: the code in Linux is suppsoed to
work correctly in the version of the kernel which it s found and that's
it.
- I don't know what this:
+#if defined(CONFIG_MODVERSIONS) !defined(MODVERSIONS)
+ #define MODVERSIONS
+#endif
is doing, but it's probably wrong.
- Use request_node, not RequestNode, etc.
- Don't parenthesise the argument to `return'.
- I see at least one U32 in there. Please use u32. (Does U32 even work?)
- This:
+static int acs_ame_get_log(
+ struct Acs_Adapter *acs_adt,
+ struct EventLog *event_log)
isn't preferred style. Use
static int acs_ame_get_log(struct Acs_Adapter *acs_adt,
struct EventLog *event_log)
or, if you particularly dislike that, blow the 80-col rule and do
static int acs_ame_get_log(struct Acs_Adapter *acs_adt, struct EventLog
*event_log)
- This
+ writel((replyframe), base_addr+AME_REPLY_MSG_PORT);
is overparenthesised.
- Beware that the scatter/gather APIs just got significantly changed.
You code might need adjustment to work against the latest mainline tree.
- What does CHAR_DEV do? Probably it should be a Kconfig CONFIG_* option.
- All the code around acs_ame_schedule_command() (which is incorrectly
identified as arcmsr_schedule_command in its comment block) is indented a
tab stop. That's really weird. Please make it normal.
- acs_ame_schedule_command() has an up-to-sixty-second busywait. Bad.
Can we get a sleep+wakeup in there?
- This:
+ struct
+ {
+ unsigned int vendor_id;
+ unsigned int device_id;
+ } const acs_ame_devices[] = {
+ { 0x14D6, DEVICEID_ACS_61000_XX }
+ , { 0x14D6, DEVICEID_ACS_62000_08 }
+ , { 0x1AB6, DEVICEID_ACS_61000_XX }
+ , { 0x1AB6, DEVICEID_ACS_62000_08 }
+ };
should be
static const struct {
unsigned int vendor_id;
unsigned int device_id;
} acs_ame_devices[] = {
{ 0x14D6, DEVICEID_ACS_61000_XX },
{ 0x14D6, DEVICEID_ACS_62000_08 },
{ 0x1AB6, DEVICEID_ACS_61000_XX },
{ 0x1AB6, DEVICEID_ACS_62000_08 },
};
which has many changes from the original.
I'd have thought that the kernel already has a data type for this, but
I can't find it. Most drivers just rely upon the normal PCI device ID
tables. It is suspicious that this one doesn't.
Anyway, that's just from a quick scan. There are a huge number of similar
issues in there. Please take some time to study some well-maintained Linux
driver code and the interfaces which scsi and PCI drivers use and try to
make this driver a lot more Linux-like, thanks.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Add ACCUSYS RAID driver for Linux i386/x86-64
On Mon, Oct 22, 2007 at 07:02:53PM -0700, David Miller wrote:
From: Peter Chan [EMAIL PROTECTED]
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 linux/version.h
#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 config/modversions.h
#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 linux/list.h
* Use linux/types.h 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 scsi/scsi.h
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-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Add ACCUSYS RAID driver for Linux i386/x86-64
On Thu, 30 Aug 2007 13:45:06 +0800 Peter Chan 詹家昌 [EMAIL PROTECTED] wrote: Dear Morton ACCUSYS Inc dedicated to design PCI-e RAID HBA, hence we would like to provide our driver bundle in kernel 2.6 for user. Thank you! Could you kindly take a look attached file if it looks like Linux Driver? It looks a bit like a Linux driver ;) More on this below. If you need the RAID card to verify please let me know. I personally am unlikely to make the time to test a specific HBA driver. But perhaps James or one of the other scsi developers (or any developer at all) might like to volunteer to help out with testing, in which case yes, please do send out a card or two. First up, please become familar with the way in which we handle code patches. I'd suggest that you subscribe to the linux-kernel or linux-scsi mailing lists for a while, watch how other people prepare and present their patches. Download, install and become familiar with quilt (http://savannah.nongnu.org/projects/quilt) and use that to prepare patches. quilt can also be used to email patches which might prove useful: new kernel developers frequently have trouble with wordwrapping, tab-to-space conversion and other ways in which mail clients corrupt patches. Once you have quilt up and running you should convert this driver into a single patch against Linus's latest kernel. At this time, that is 2.6.23-rc4. That patch should include the appropriate changes to drivers/scsi/Kconfig and drivers/scsi/Makefile to wire this driver up to the kernel configuration and build system. (You may choose to create a separate directory drivers/scsi/acs_ame/ for this driver). Once we reach this stage, we have a patch which others can apply, test and review. Review will be the first step. Your driver isn't ready for review yet. There are a number of minor but often-repeated problems which will require extensive changes to correct. They're silly little things and they won't change the operation of the driver at all, but it is best to get the driver looking and feeling like a typical driver before we ask reviewers to start taking a serious look at the code. - No c++-style comments, please. Replace all // with /* .. */ - there's some commented-out and obviously dead code. Things like //#include asm/semaphore.h //void sema_init //DECLARE_MUTEX(name) //DECLARE_MUTEX_LOCKED(name) //#include linux/rwsem.h// read /write semaphore // Why lock_kernel //#include linux/smp_lock.h //lock_kernel(); //#if LINUX_VERSION_CODE KERNEL_VERSION(2,5,60) daemonize(); //#else //daemonize(arcmsr kernel thread); //#endif // unlock_kernel(); //Use unsigned long as a pointer // cause of size(unsigend long) always equal to pointer size under all linux compatible platform just delete all that, please. - CHAR_DEV and EVENTSWITCH_ON should probably become CONFIG variables (set in drivers/scsi/Kconfig) - try to keep the code fitting nicely in an 80-column display - Identifiers like RequestNode and RemapPCIMem should be renamed request_node and remap_pci_mem if at all possible. We use underscores to separate words within an identifier, not capital letters. - Once you have a cleaned up patch, please pass that patch through scripts/checkpatch.pl. I just turned acs_ame.c, acs_ame.h and ame.h into a patch and fed it through checkpatch.pl. Please see the result at http://userweb.kernel.org/~akpm/x.txt. (For those who are curious, the patch is at http://userweb.kernel.org/~akpm/a.patch) We have about 4,000 warnings there ;) Actually, a huge number of those warnings are incorrect (trailing whitespace). checkpatch is still under development a bit. But still, there are a lot of things in the checkpatch output which can and should be fixed up. All together I expect that after one or maybe two days work, you will have a driver patch which is ready to be applied to the kernel and is ready to be reviewed by kernel and scsi experts. At that stage we will be able to get into more substantial details such as the use of the scsi APIs, the DMA APIs, locking, commenting, design, etc. I'd expect that further changes will be made to the driver as a result of that discussion, but they will improve the code. When sending new versions of the patch, please send them to Andrew Morton [EMAIL PROTECTED] James Bottomley [EMAIL PROTECTED] linux-scsi@vger.kernel.org [EMAIL PROTECTED] The way this driver will enter the 2.6 kernel is from you, into James's scsi-misc git tree and then from James's scsi-misc tree into Linus's git tree. It is possible that I'll take a copy into my -mm tree for some early testing but it is unlikely that I would send a scsi patch directly into Linus - James does that. Thanks. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at
