Re: [Announce] Emulex LightPulse Device Driver

[Jeff Garzik <jgarzik@pobox.com>]

Smart, James wrote:
> All,
> 
> Emulex is embarking on an effort to open source the driver for its
> LightPulse Fibre Channel Adapter family. This effort will migrate Emulex's
> current code base to a driver centric to the Linux 2.6 kernel, with the goal
> to eventually gain inclusion in the base Linux kernel.
> 
> A new project has been created on SourceForge to host this effort - see
> http://sourceforge.net/projects/lpfcxxxx/ . Further information, such as the
> lastest FAQ, can be found on the project site.
> 
> We realize that this will be a significant effort for Emulex. We welcome any
> feedback that the community can provide us.

Embark you shall, and let me join in the chorus of kudos for making the 
leap to open source.  :)

I'm only part way through a review of the driver, but I felt there is a 
rather large and important issue that needs addressing...  "wrappers." 
These are a common tool for many hardware vendors, which allow one to 
more easily port a kernel driver across operating systems. 
Unfortunately, these sorts of abstractions continually lead to bugs.  In 
particular, the areas of locking, memory management, and PCI bus 
interaction are often most negatively affected.

In particular, here is an example of such a bug:

void
elx_sli_lock(elxHBA_t * phba, unsigned long *iflag)
{

         unsigned long flag;
         LINUX_HBA_t *lhba;

         flag = 0;
         lhba = (LINUX_HBA_t *) phba->pHbaOSEnv;
         spin_lock_irqsave(&lhba->slilock.elx_lock, flag);
         *iflag = flag;
         return;
}

It is not portable for code to return the value stored in the 'flags' 
argument of spin_lock_irqsave.  The usage _must_ be inlined.  This fails 
on, e.g., sparc64's register windows.

But this bug is only an example that serves to highlight the importance 
of directly using Linux API functions throughout your code.  It may 
sound redundant, but "Linux code should look like Linux code."  This 
emphasis on style may sound trivial, but it's important for 
review-ability, long term maintenance, and as we see here, bug prevention.

It may not be immediately apparent, but elimination of these wrappers 
also increases performance.  Many of the Linux API functions are inlined 
in key areas, intentionally, to improve performance.  By further 
wrapping these functions in non-inline functions of your own, you 
eliminate several compiler optimization opportunties.  In the case of 
spinlocks (above), you violate the API.

So I would like to see a slow unwinding, and elimination, of several of 
the wrappers in prod_linux.c.

1) elx_kmem_alloc, elx_kmem_free:  directly use kmalloc(size, 
GFP_KERNEL/ATOMIC) in the driver code.

2) eliminate all *_init_lock, *_lock, and *_unlock wrappers, and 
directly call the Linux spinlock primitives throughout your code.

3) strongly consider eliminating elx_read_pci_cmd, elx_read_pci, and 
simply calling the Linux PCI API directly from the lpfc driver code.

4) eliminate elx_sli_write_pci_cmd hook, elx_write_pci_cmd wrapper, and 
directly call the Linux PCI API in the code.

5) eliminate elx_remap_pci_mem, elx_unmap_pci_mem

6) fix unacceptably long delay in elx_sli_brdreset().  udelay() and 
mdelay() functions are meant for very small delays, since they do not 
reschedule.  Delays such as
         if (skip_post) {
                 mdelay(100);
         } else {
                 mdelay(2000);
         }
should be converted to timers or (if in kernel thread context) 
schedule_timeout().

7) eliminate elx_sli_pcimem_bcopy(,,sizeof(uint32_t)) in favor of "u32 
foo = readl()"

8) replace code such as
        ((SLI2_SLIM_t *) phba->slim2p.virt)->un.slim.pcb.hgpAddrHigh =
   (uint32_t) (psli->sliinit.elx_sli_read_pci) (phba, PCI_BAR_1_REGISTER);
   Laddr = (psli->sliinit.elx_sli_read_pci) (phba, PCI_BAR_0_REGISTER);
   Laddr &= ~0x4;
with calls to pci_resource_start() and/or pci_resource_len()

9) call pci_set_master() when you wish to enable PCI busmastering.  This 
will set the busmaster bit in PCI_COMMAND for you, as well as set up the 
PCI latency timer.

10) call pci_dma_sync functions rather than elx_pci_dma_sync()

That should get you started ;-)

	Jeff