LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* RE: [(re)Announce] Emulex LightPulse Device Driver
@ 2004-05-10 16:47 Smart, James
  0 siblings, 0 replies; 10+ messages in thread
From: Smart, James @ 2004-05-10 16:47 UTC (permalink / raw)
  To: 'Matthew Wilcox'
  Cc: 'linux-scsi@vger.kernel.org',
	'linux-kernel@vger.kernel.org'

The driver module name (in the sources, etc)is "lpfc". Only the project on
SourceForge has the "lpfcxxxx" name.

-- james

> -----Original Message-----
> From: Matthew Wilcox [mailto:willy@debian.org]
> Sent: Monday, May 10, 2004 12:35 PM
> To: Smart, James
> Cc: 'linux-scsi@vger.kernel.org'; 'linux-kernel@vger.kernel.org'
> Subject: Re: [(re)Announce] Emulex LightPulse Device Driver
> 
> 
> On Sun, May 09, 2004 at 12:33:35AM -0400, Smart, James wrote:
> > The source for the driver can be downloaded from it's 
> project page on source
> > forge - http://sourceforge.net/projects/lpfcxxxx/ . Be sure 
> to download the
> > 20040507 snapshot - and the version for the 2.6 kernel.
> 
> You might want to consider changing the name of this driver.  aic7xxx
> trips a number of spamfilters, so maybe just lpfc would be a good name
> for it?
> 
> -- 
> "Next the statesmen will invent cheap lies, putting the blame upon 
> the nation that is attacked, and every man will be glad of those
> conscience-soothing falsities, and will diligently study 
> them, and refuse
> to examine any refutations of them; and thus he will by and 
> by convince 
> himself that the war is just, and will thank God for the better sleep 
> he enjoys after this process of grotesque self-deception." -- 
> Mark Twain
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread
* RE: [(re)Announce] Emulex LightPulse Device Driver
@ 2004-05-17 21:25 Smart, James
  0 siblings, 0 replies; 10+ messages in thread
From: Smart, James @ 2004-05-17 21:25 UTC (permalink / raw)
  To: 'Christoph Hellwig'
  Cc: 'linux-scsi@vger.kernel.org',
	'linux-kernel@vger.kernel.org'



> -----Original Message-----
> From: 'Christoph Hellwig' [mailto:hch@infradead.org]
> Sent: Friday, May 14, 2004 4:03 PM
>
> On Fri, May 14, 2004 at 03:51:14PM -0400, Smart, James wrote:
> >  - please explain USE_HGP_HOST_SLIM and why only ppc64 sets it,
> >    per-arch ifdefs in LLDDs are very suspect
> > 
> >      Our hardware allows for control structures to be 
> placed in adapter
> > 	 memory rather than host memory. In general, this is the most
> > 	 efficient mode of operation. USE_HGP_HOST_SLIM instructs us to
> > override
> > 	 this optimization and place them in host memory.
> > 	 For PPC64, to work around byte ordering issues 
> relative to how the
> > 	 hardware is accessing the control structures, we found 
> it easier to
> > 	 just use the unoptimized location.
> 
> Still not an explanation.  What exact issue is that?  Is is 
> just faster
> this way or doesn't it work another way?  Why is ppc64 so 
> different from
> other plattforms, e.g. why does ist this set when I boot my 
> (well, if I had
> one :)) G5 mac with a 64bit kernel and why not when I boot it 
> with a 32bit
> kernel?  As you mention byte ordering why is it not needed for other
> big endian system.

I'm tracking the details on this down. It's rather dated and prior to my
joining Emulex. It is possible that ppc64 was the only platform on Emulex's
support matrix that was subject to the issue. If this issue applies to other
architectures as well, we will obviously need to correct this (e.g. Emulex's
support matrix doesn't matter - we must do the right things for every
architecture).

 
> 
> > 
> >  - lpfc_clock.c should go away, just use add_timer/etc. diretly and
> >    embedd the timer into your structures.  Yes, I know that's not
> >    how SVR4-derivates work, but Linux does.  You also have an
> >    unchecked kmalloc and a list_head cast in there..
> > 
> >      It's not an SVR4 vs linux thing.  This wrapper 
> function does two
> > 	 things - start the timer, and allocate/link a small control
> > structure.
> > 	 The control structure allows us to just keep a record of every
> > 	 outstanding timer. This is a safety check, for driver
> > 	 unloads/kills/etc, to ensure that we have cancelled 
> all the timers
> > 	 that have been registered with the OS.
> 
> No.  The timer should be embedded into the object it's 
> associated with,
> and you must call del_timer_sync in the object destructor.  
> BTW, you have
> a lot of timers, more than the other fc drivers counted 
> together.  This
> looks slightly fishy.
> 

We could embed the timer into the associated object. This is mainly an
approach issue - it was just a simplistic choice to track the timers via the
hba object. Given the amount of code churn changing this would incur, I'm
inclined to ask why, other than for style and approach, must this be
changed?

As far as timer count goes... (shrug) we do manage discovery and port login
in the driver, which other driver's don't do, which adds quite a bit. We
could certainly delineate what we have and why...

> >  - lpfcDRVR_t should go away in favour of a bunch of global 
> variables
> > 
> >      Can you explain this comment further ?
> 
> You have a structure type lpfcDRVR_t but just a single global instance
> of it.  Aka the data structure is just obsfucation and each 
> member should
> be a driver-wide variable of it's own.

Ok - so I can see just killing the structure and making it's elements
globals. But, is this all you are asking ?  The elements are just a linked
list of the per-hba structures. You aren't recommending that we make arrays
of the per-hba structures are you ? 


> 
> > 
> >  - lpfc_linux_attach should be merged into lpfc_pci_detect, 
> which would
> >    better be named lpfc_{,pci_}probe{,one}
> > 
> > 	 We've kept this organization so it's more in line with the
> > 	 organization of the 2.4 driver. I don't see a real 
> need to change
> > 	 this.
> >      Note: we have renamed the probe functions...
> 
> It's really awkward to follow.  See qla1280.c to see how to 
> use 2.6-style
> probe/release calls in a 2.4ish enviroment.  Been there, done that.

Ok - so why is this a significant issue ?


> 
> > 
> >  - same for lpfc_linux_detach into lpfc_pci_release
> >      
> > 	 Same as last item
> 
> Dito.  Note that this one is especially awkward because of 
> the brainded
> pass index instead of object calling convention.

It is a little awkward, with the only difference whether the base structure
is found in the lpfc_pci_release routine or not. Still doesn't seem a
significant issue.

> 
> >  - you don't need a pci_pool per hba, just one per driver
> > 
> > 	 The pools are pre-dma mapped. As there are systems that require
> > 	 dma maps to be specific to the adapter using them, 
> we're maintaing
> > 	 a pool per hba.
> 
> Ok, braino.  Of course you're right, I was thinking of the mempool.
> 
> >  - four mempool per hba seems a little much, care to 
> explain why they're
> >    needed?
> > 
> >      These are the safety pools just mentioned. 1 pool is 
> for non-dma
> > 	 mapped structures - which are used during discovery. 
> The other 3
> > 	 pools are pre-dma mapped buffers - of different 
> allocation sizes
> > 	 (256 bytes, 1k, and 4k). It's just an optimization on 
> the amount
> > 	 of memory used by the pool. I'm sure there's other 
> methods, but...
> 
> Still doesn't say why they can't be global.  You'd need to uncomment
> mempool_resize in mm/mempool.c for that, though.

Also doesn't say they need to be global. Again - what's the significant
issue ? Seems to be just a style - whether things are done on a per-instance
basis or on a global basis. As the size requirements for this pool is based
on the number of instances - per-hba pools scales with far less effort when
more adapters are present. Growing/shrinking the pool each time an instance
attaches/detaches actually makes the global pool implementation more
complex. Coding for a global vs per-hba pool is roughly identical - the only
difference being whether the data structure is anchored globally or in the
per-hba structure.  And if you've already got per-hba pool code (the pre-dma
mapped pools), making the pool global seems to just add code.


^ permalink raw reply	[flat|nested] 10+ messages in thread
* RE: [(re)Announce] Emulex LightPulse Device Driver
@ 2004-05-14 19:51 Smart, James
  2004-05-14 20:03 ` 'Christoph Hellwig'
  0 siblings, 1 reply; 10+ messages in thread
From: Smart, James @ 2004-05-14 19:51 UTC (permalink / raw)
  To: 'Christoph Hellwig'
  Cc: 'linux-scsi@vger.kernel.org',
	'linux-kernel@vger.kernel.org'

Christoph,

Here's a reply/status relative to your comments.

-- James



The following items are completed and should be in our next
drop (slated for today/tomorrow):

 - the hba api crap must die
	  Note: We have removed the hbaapi header and associated
dependencies.
	    We will continue to work on a common linux HBAAPI
implementation.
 - lpfc_fcp.h duplicates the scsi opcode - use those from scsi/scsi.h
 - #define EXPORT_SYMTAB in lpfc_fcp.c is wrong, you never need that
   one in 2.6.
 - why do you need <linux/if_arp.h> and <linux/rtnetlink.h> in there?
 - please always include <linux/*> first, then <asm/*>, then <scsi/*>,
   then private headers.
 - OpenSource in the module description is useless - MODULE_LICENSE
   already tels us that.
 - formatting of lpfc_driver is broken
 - not need to memset lpfcDRVR, it's 0 if it's in .bss already
 - please kill all those silly one-line wrappers in lpfc_mem.c.
 - lpfc_sysfs_set_show/lpfc_sysfs_set_store violate the one value per
   attribute rule
 - dito for lpfc_sysfs_params_show/lpfc_sysfs_params_store
 - dito for lpfc_sysfs_info_show, in short all your sysfs work is completely
   wrong, also you seem to use driver attributes instead of scsi device/host
   attributes.  why?


Will Do shortly - hopefully by next week's drop:

 - lots of strange typedefs in sSuDly Caps that want to die.
 - Dito for the strange pfoo pointer naming.
 - do you really care for pre-2.6.5 compat?
      Note: For now, it will remain, as we have a couple of consumers
	    that aren't on the latest and greatest. It will be removed
before
	    we're done.
 - lpfc_msg* stuff is not acceptable.   simply put the string you
   want to print into the printks
      Note: I'm not sure how we will resolve this yet. We place all error
	    strings in a single location so we can use automated tools to 
		generate documentation for customers. However, as
developers,
		we share the same sentiment you do. So, we'll figure
something out.
 - your module_param usage is b0rked.  Instead of duplicating every
   option 31 times use module_param_array.  You also seem to miss
   the paramter descriptions (MODULE_PARAM_DESC)
      Note: Our implementation will actually move away from arrays, etc.
        We're redesigning the interface so that it's variable and does
	    not have a maximum instance size.
 - you're using list_for_each{,_safe} a lot where you'd want to
   use the _entry versions.
 - why do you disable clustering in the 2.6 driver?
 - you must attach the driver attributes always unless pci_module_init
   failed, alese hotpluggin does´t work.
 - lpfc_sleep_ms is broken, it'll happily sleep under spinlock, please
   use mdelay directly in contexts where you can't sleep and
schedule_timeout
   only where you can sleep.
 - lpfc_sleep needs to die, it's just a broken reimplementation of
   sleep_on{,_interruptible,}{_timeout,}
 - lpfc_tasklet would probably benefit from a list_splice_init early on
   so the proceasing can happen without retaking the spinlock all the time
 - why all the EXPORT_SYMBOLS?
      Note: We'll ensure that these actually need to be exported.
 - you shouldn't need the scsi_{bus,target,lun} members in lpfc_cmd but
   always use the scsi_cmnd->device linked from there.
 - the lpfc_find_target usage in queuecommand looks bogus.  You have
   scsi_device->hostdata to put per-lun data, from which you can trivially
   link per-target data directly.  All the checks for inquiry and valid
   luns are similarly b0rked - scsi probing works by calling ->slave_alloc
   first so you'll a) have a place where you know the midlayer is probing
   and b) always private data when quecommand is called.
 - in lpfc_scsi_cmd_start you use lpfc_find_lun_device which is copletely
   bogus again, use scsi_cmnd->device->hostdata
 - 2nd email on findstatic results - adding static declarations


Things we haven't gotten to yet:

 - what do you need lpfcDRVR.loadtime for?
 - lpfcDRVR.hba_list_head should be initializes at compile time
   LIST_HEAD(hba_list); if properly taken out of the struct.
 - in lpfc_queuecommand you don't need to spin_lock_irqsave your drvlock
   as the host_lock is already taken with irqs disabled.  you should
probably
   redesign your driver to use the host_lock instead of the drvlock
everywhere.
 - the error return if queuecommand lpfc_get_scsi_buf fails looks bogus,
   why do you set DID_BUS_BUSY?  also instead of 1 please return
   SCSI_MLQUEUE_HOST_BUSY


Things we need to discuss/clarify:

 - please explain USE_HGP_HOST_SLIM and why only ppc64 sets it,
   per-arch ifdefs in LLDDs are very suspect

     Our hardware allows for control structures to be placed in adapter
	 memory rather than host memory. In general, this is the most
	 efficient mode of operation. USE_HGP_HOST_SLIM instructs us to
override
	 this optimization and place them in host memory.
	 For PPC64, to work around byte ordering issues relative to how the
	 hardware is accessing the control structures, we found it easier to
	 just use the unoptimized location.

 - lpfc_clock.c should go away, just use add_timer/etc. diretly and
   embedd the timer into your structures.  Yes, I know that's not
   how SVR4-derivates work, but Linux does.  You also have an
   unchecked kmalloc and a list_head cast in there..

     It's not an SVR4 vs linux thing.  This wrapper function does two
	 things - start the timer, and allocate/link a small control
structure.
	 The control structure allows us to just keep a record of every
	 outstanding timer. This is a safety check, for driver
	 unloads/kills/etc, to ensure that we have cancelled all the timers
	 that have been registered with the OS.

 - lpfcDRVR_t should go away in favour of a bunch of global variables

     Can you explain this comment further ?  

 - lpfc_linux_attach should be merged into lpfc_pci_detect, which would
   better be named lpfc_{,pci_}probe{,one}

	 We've kept this organization so it's more in line with the
	 organization of the 2.4 driver. I don't see a real need to change
	 this.
     Note: we have renamed the probe functions...

 - same for lpfc_linux_detach into lpfc_pci_release
     
	 Same as last item

 - you don't need a pci_pool per hba, just one per driver

	 The pools are pre-dma mapped. As there are systems that require
	 dma maps to be specific to the adapter using them, we're maintaing
	 a pool per hba.

	 Also - these are for the most part safety pools for elements we
	 use during discovery/port validation. We use the base os functions,
	 and only if they fail do we use these pools. We resort to this as
	 the results of failing discovery in low-memory configurations can
	 be catastrophic, especially if we are connected to a disk that is
	 a root or swap disk. As there's nothing that says all adapters
can't
	 be plugged into the same switch, thus see the same discovery event,
	 the safety pool must be sized to support all adapters. So - there's
	 little gain in sharing a pool.

 - four mempool per hba seems a little much, care to explain why they're
   needed?

     These are the safety pools just mentioned. 1 pool is for non-dma
	 mapped structures - which are used during discovery. The other 3
	 pools are pre-dma mapped buffers - of different allocation sizes
	 (256 bytes, 1k, and 4k). It's just an optimization on the amount
	 of memory used by the pool. I'm sure there's other methods, but...

 - don't mess with eh_timeout from a LLDD please

     This exists until we fully sync up with the IBM patch that allows
	 the LLD to set the peripheral driver's basic read/write timeout
	 value - which just went into the last scsi patches.
	 See prior discussions on linux-scsi.

 

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [(re)Announce] Emulex LightPulse Device Driver
@ 2004-05-13 21:21 David.Egolf
  2004-05-13 21:26 ` Christoph Hellwig
  0 siblings, 1 reply; 10+ messages in thread
From: David.Egolf @ 2004-05-13 21:21 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: 'linux-kernel@vger.kernel.org',
	'linux-scsi@vger.kernel.org',
	linux-scsi-owner

Christoph Hellwig <hch@infradead.org>
Sent by: linux-scsi-owner@vger.kernel.org
05/09/2004 01:20 AM

>A bunch of comments from looking over the headers and itnerface to
>upper layers a little:  (next I'll try to understand what's going on
>in the I/O submission path - it's just to freakin complicated..):
>
> - the hba api crap must die
> o o o

The first bullet on your post is of interest to us.  We currently support 
customers with Emulex fc cards using a 2.4 kernel on IA64.  Our software 
employs the hba api supplied from Emulex in order determine the 
configuration of the SAN(s) connected to the cards.

Your comment is on the terse side.  Is your comment directed at this 
particular implementation of the hba api code, the current packaging 
situation, or do you have a general disregard for the hba api strategy? In 
short, do you believe that the hba api can and/or should be supported for 
the Emulex LightPulse Device Driver? 

-- David Egolf

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [(re)Announce] Emulex LightPulse Device Driver
@ 2004-05-09  4:33 Smart, James
  2004-05-09  8:20 ` Christoph Hellwig
  2004-05-10 16:34 ` Matthew Wilcox
  0 siblings, 2 replies; 10+ messages in thread
From: Smart, James @ 2004-05-09  4:33 UTC (permalink / raw)
  To: Smart, James, 'linux-scsi@vger.kernel.org',
	'linux-kernel@vger.kernel.org'

All,

Emulex has spent the last 8 wks significantly reorganizing its code base to
create a native 2.6 driver. Although there are still a couple of small items
remaining, we believe we're ready to have the community review the sources.

The source for the driver can be downloaded from it's project page on source
forge - http://sourceforge.net/projects/lpfcxxxx/ . Be sure to download the
20040507 snapshot - and the version for the 2.6 kernel.

If you have any questions, please contact me.  I look forward to your
comments.

Thanks...

-- James Smart


>  -----Original Message-----
> From: 	Smart, James  
> Sent:	Tuesday, March 09, 2004 5:45 PM
> To:	'linux-scsi@vger.kernel.org'; 'linux-kernel@vger.kernel.org'
> Subject:	[Announce] Emulex LightPulse Device Driver
> 
> 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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2004-05-17 21:26 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-05-10 16:47 [(re)Announce] Emulex LightPulse Device Driver Smart, James
  -- strict thread matches above, loose matches on Subject: below --
2004-05-17 21:25 Smart, James
2004-05-14 19:51 Smart, James
2004-05-14 20:03 ` 'Christoph Hellwig'
2004-05-13 21:21 David.Egolf
2004-05-13 21:26 ` Christoph Hellwig
2004-05-09  4:33 Smart, James
2004-05-09  8:20 ` Christoph Hellwig
2004-05-09  8:51   ` Christoph Hellwig
2004-05-10 16:34 ` Matthew Wilcox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).