lkml.org 
[lkml]   [1998]   [Aug]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: AMI MegaRAID beta release
Hello,

> this patch will add support for the MegaRAID 428 and 438 controllers, and
> most likely
> will work with others but has not yet been tested. If anyone out there has
> any
> MegaRAID controller and wants to test this driver, please send me feedback
> on bugs,
> performance, suggestions, etc.

I don't have any MegaRAIDs here to test the driver, but I've read the driver
and there are several comments:

> +#include <linux/init.h>

Do you use the __init macros anywhere?

> +#if LINUX_VERSION_CODE <= 0x20034
> +#include <linux/bios32.h>
> +#endif

It should be #if LINUX_VERSION_CODE < 0x20100.

> +#define ENQUEUE(obj,type,list,next) \
> +{ type **node; long cpuflag; \
> + save_flags(cpuflag); cli(); \
> + for(node=&(list); *node; node=(type **)&(*node)->##next); \
> + (*node) = obj; \
> + (*node)->##next = NULL; \
> + restore_flags(cpuflag); \
> +};
> +
> +#define DEQUEUE(obj,type,list,next) \
> +{ long cpuflag; \
> + save_flags(cpuflag); cli(); \
> + if ((obj=list) != NULL) {\
> + list = (type *)(list)->##next; \
> + } \
> + restore_flags(cpuflag); \
> +};

Do you really need a global cli() here? IMHO a IRQ-safe spinlock would be
sufficient and it would give better performance on SMP systems.

> +#define SERDEBUG 0
> +#ifdef SERDEBUG

?? #if SERDEBUG instead?

> +static void freeSCB(mega_scb *scb)
> +{
> + long flags;
> +
> + save_flags(flags);
> + cli();
> + scb->flag = 0;
> + scb->idx = -1;
> + scb->next = NULL;
> + scb->SCpnt = NULL;
> + restore_flags(flags);
> +}

Maybe the atomic read/write operations in <asm/atomic.h> would be strong
enough and again give better performance.

> + save_flags(flags);
> + cli();

Global cli() in interrupt handler is going to deadlock SMP systems
and it's probably not needed anyway.

> + /* Read the base port and IRQ from PCI */
> + pcibios_read_config_dword(pciBus, pciDevFun,
> + PCI_BASE_ADDRESS_0,
> + (u_int *)&megaBase);
> + pcibios_read_config_byte(pciBus, pciDevFun,
>
> + PCI_INTERRUPT_LINE,
> + &megaIrq);

On 2.1, you have to fetch the base address and IRQ from the pci_dev structure
instead of reading it from the configuration space since it doesn't fit there
on several architectures and the kernel does its own mapping of both addresses
and IRQ's.

> + else {
> + megaBase += 0xF;
> + }

megaBase &= PCI_BASE_ADDRESS_MEM_MASK is the correct way to do this for memory
regions, &= PCI_BASE_ADDRESS_IO_MASK for I/O regions.

> + count += findCard(pHostTmpl, 0x101E, 0x9010, 0);
> + count += findCard(pHostTmpl, 0x101E, 0x9060, 0);
> + count += findCard(pHostTmpl, 0x8086, 0x1960, BOARD_QUARTZ);

Please send these device IDs to Jens Maurer <linux-pcisupport@cck.uni-kl.de>
who maintains the Linux PCI ID list, so that pciutils will be able to recognize
these cards.

> + for (i=0;i<0xfffff;i++);

udelay() should be used instead.

> + printk(""); /* coax kernel into running tasks... whole
> */

Call schedule() instead.

> + cylinders = disk->capacity / (heads * sectors);

I guess it should be rounded up, not down.

> + cylinders = disk->capacity / (heads * sectors);

Again.

> +#define MEGARAID \
> + { NULL, /* Next */\
> + NULL, /* Module */\

In 2.1 kernels, SCSI drivers use GCC extensions for naming structure fields
during initialization, so that the structure can be modified without touching
the drivers (and it's of course much more readable):

proc_info: megaraid_info,
detect: megaraid_detect,
... and so on

> +#pragma pack(1)

This pragma is not supported by GCC. If your structures don't have natural
alignment (which doesn't seem to be your case), add __attribute__((packed))
to the declaration, but then you of course cannot access the structure members
directly since most CPUs don't allow unaligned accesses.

Have a nice fortnight
--
Martin `MJ' Mares <mj@ucw.cz> http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
"Light-year? One-third less calories than a regular year."

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.altern.org/andrebalsa/doc/lkml-faq.html

\
 
 \ /
  Last update: 2005-03-22 13:44    [from the cache]
©2003-2011 Jasper Spaans