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    [W:0.034 / U:0.680 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site