[Announce] Emulex LightPulse Device Driver

[Announce] Emulex LightPulse Device Driver

[Smart, James]

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.

Thanks,

-- James Smart
   Emulex Corporation

Re: [Announce] Emulex LightPulse Device Driver

[Stefan Smietanowski]

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.

I wish to just tell you that I think you're doing the Right Thing(TM).

There are people that don't buy hardware for which the source isn't
either available or included in the standard kernel, even if there
are more patches or newer driver versions external to the main tree.

Good work and good luck!

// Stefan

Re: [Announce] Emulex LightPulse Device Driver

[vda]

On Wednesday 10 March 2004 02:09, Stefan Smietanowski wrote:
> 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.
>
> I wish to just tell you that I think you're doing the Right Thing(TM).
>
> There are people that don't buy hardware for which the source isn't
> either available or included in the standard kernel, even if there
> are more patches or newer driver versions external to the main tree.

Being bitten by "buggy firmware from hell", I, too, will abstain from
using hardware from manufactures who do not open driver source
for impossible-to-understand reasons.
--
vda

Re: [Announce] Emulex LightPulse Device Driver

[Jeff Garzik]

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

Re: [Announce] Emulex LightPulse Device Driver

[James Bottomley]

On Wed, 2004-03-10 at 03:35, Jeff Garzik wrote:
> Embark you shall, and let me join in the chorus of kudos for making the 
> leap to open source.  :)

Yes, I'll add my welcome to that.

[...]
> That should get you started ;-)

Actually, it would be my interpretation of the FAQ that most of this
work is already intended (although Jeff gave specific instances than the
generalities in the FAQ).

There were many more places than this in the driver that caused me to go
"good grief no".  However, it probably makes more sense if you work down
your todo list and come back for a review when you're nearing the end of
it.  That way you don't get boat loads of comments about things you were
planning to fix anyway.

James

Re: [Announce] Emulex LightPulse Device Driver

[Pete Zaitcev]

On Wed, 10 Mar 2004 03:35:09 -0500
Jeff Garzik <jgarzik@pobox.com> wrote:

> 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."

Jeff, I agree completely that Emulex code is infested with wrappers
so much that it's harmful. However, the particular example you selected
you interpret wrong.

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

Flag problem on sparc is fixed by Keith Wesolowsky for 2.6.3-rcX,
and it never existed on sparc64, which keeps CWP in a separate register.

Why it took years to resolve is that the expirience showed that
there is no legitimate reason to pass flags as arguments. Every damn
time it was done, the author was being stupid. Keith resolved it
primarily because it was an unorthogonality in sparc implementation.

> 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.

Yes yes yes. This is the way elx_sli_lock is harmful, not because
of its passing flags.

-- Pete

Re: [Announce] Emulex LightPulse Device Driver

[Jeff Garzik]

Pete Zaitcev wrote:
> Jeff Garzik <jgarzik@pobox.com> wrote:
> Flag problem on sparc is fixed by Keith Wesolowsky for 2.6.3-rcX,
> and it never existed on sparc64, which keeps CWP in a separate register.
> 
> Why it took years to resolve is that the expirience showed that
> there is no legitimate reason to pass flags as arguments. Every damn
> time it was done, the author was being stupid. Keith resolved it
> primarily because it was an unorthogonality in sparc implementation.

You would never know there were so many sparc people, until I post 
something incorrect about it.  <grin>

I stand corrected.  As someone mentioned in private, it's actually a 
shame that was fixed, since that's one less argument that can be used 
against such wrappers ;-)


>>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.
> 
> 
> Yes yes yes. This is the way elx_sli_lock is harmful, not because
> of its passing flags.

Agreed.

	Jeff

RE: [Announce] Emulex LightPulse Device Driver

[Smart, James]

First, thanks to those that have actually taken a look at the FAQ and source
already. Do not believe your time was in vain - we will use every comment we
receive.

We know there are a lot of issues we need to address. We echo many of the
same sentiments. We had hoped the FAQ would explain where we are and where
we are heading, so that people can judiciously choose when to evaluate the
code base. That said, we will welcome any comments, at any time, at any
detail level. I would hope that, even while the code base is changing, that
we receive comments. There are constructs in the driver that are likely not
going to change, such as the logging facility. How contentious is this ?
What about the IP interfaces? and so on.  Anything we receive, especially on
the larger concepts in the driver, only helps us understand what's ahead. 

Our plans are to complete most of the work list on the FAQ by early April.
We'll try to make weekly drops on SourceForge, with each snapshot containing
a log of the changes.  Once the code base matures, we will ping the lists
again, asking for feedback.

Thanks.

-- James Smart


> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@SteelEye.com]
> Sent: Wednesday, March 10, 2004 10:21 AM
> To: Jeff Garzik
> Cc: Smart, James; 'linux-scsi@vger.kernel.org';
> 'linux-kernel@vger.kernel.org'
> Subject: Re: [Announce] Emulex LightPulse Device Driver
> 
> 
> On Wed, 2004-03-10 at 03:35, Jeff Garzik wrote:
> > Embark you shall, and let me join in the chorus of kudos 
> for making the 
> > leap to open source.  :)
> 
> Yes, I'll add my welcome to that.
> 
> [...]
> > That should get you started ;-)
> 
> Actually, it would be my interpretation of the FAQ that most of this
> work is already intended (although Jeff gave specific 
> instances than the
> generalities in the FAQ).
> 
> There were many more places than this in the driver that 
> caused me to go
> "good grief no".  However, it probably makes more sense if 
> you work down
> your todo list and come back for a review when you're nearing 
> the end of
> it.  That way you don't get boat loads of comments about 
> things you were
> planning to fix anyway.
> 
> James
> 
> 

RE: [Announce] Emulex LightPulse Device Driver

[James Bottomley]

On Wed, 2004-03-10 at 17:47, Smart, James wrote:
> we receive comments. There are constructs in the driver that are likely not
> going to change, such as the logging facility. How contentious is this ?
> What about the IP interfaces? and so on.  Anything we receive, especially on
> the larger concepts in the driver, only helps us understand what's ahead. 

Well, what your logging facility tries to do (discriminated messages to
the console based on a mask) is extremely standard for drivers.  The way
you implement it is slightly, erm, less than desirable.  Having your own
version of sprintf in your driver is a definite no-no (why do you do
this?  You never seem actually to use the added formatting characters
like %E?).  But at least it doesn't do anything silly like try to ouput
to the serial port.

As far as the IP portion goes: layering and separation should really be
the order of the day---perhaps even to the point where you have a core
module with a scsi and an IP one that sit on top of it.

> Our plans are to complete most of the work list on the FAQ by early April.
> We'll try to make weekly drops on SourceForge, with each snapshot containing
> a log of the changes.  Once the code base matures, we will ping the lists
> again, asking for feedback.

OK, I'll look forward to it.

James