public inbox for linux-scsi@vger.kernel.org
 help / color / mirror / Atom feed
* RE: [Emulex] Ready for next round...
@ 2004-07-09 19:24 Smart, James
  2004-07-09 22:08 ` 'Christoph Hellwig'
  2004-07-09 22:10 ` 'Christoph Hellwig'
  0 siblings, 2 replies; 17+ messages in thread
From: Smart, James @ 2004-07-09 19:24 UTC (permalink / raw)
  To: 'Christoph Hellwig'; +Cc: Linux SCSI Reflector

Christoph,

Can you please take a look at the snapshot just posted on SourceForge.

The following things have been reworked:
- USE_HGP_HOST_SLIM: finally resolved this issue and implemented it
correctly.
- Logging: We killed lpfc_logmsg.c and converted over to a single define
using dev_printk. Still implements a subsystem filter in the macro.
- Attributes: They have been moved over to shost_attrs. We changed the basic
philosophy about per-instance attributes, as many simply aren't needed to be
set prior to attachment. This allowed them to go back integers. You will
find several removed as we removed some of the logic that insulated the
midlayer and above from transient conditions (delayed io error, etc - see
last paragraph). We killed lpfc.conf and have converted over to a separation
of attributes from their description arrays. One discrepancy - the static
configuration array for persistent bindings remains for compatibility - but
it's own file.
- Your big patch to lpfc_fcp.c - was incorporated.
- [PATCH] streamline lpfc error handling: This was incorporated. There were
some changes to clean up return codes and misplaced blocks.
- [PATCH] remove dead event counters from lpfc. Rather than remove this
code, as it will be used later, we have at least consolidated it and made
the events visible via sysfs.


Things we're still addressing - and should be in drop on 7/16:
- clk_data: We're removing the safety list, and converting from multiple
clock args to 1 so that it better maps to the kernel api.
- secondary data structures for target/lun data : We're attempting to
reduce/remove the notions of luns as, in general, the driver issues i/o at
the target level. Part of this is removing report luns use, etc. Note: We
will see if we can help the midlayer with addressing mode support.

We have one major issue, which has been partially discussed already
(http://marc.theaimsgroup.com/?l=linux-scsi&m=108275840501608&w=2). In
summary, there are discovery state machine events when we lose connectivity
with the target for a short period. If we do not suspend i/o to the target,
the upper layer retries can exhaust before the target comes back, triggering
other system error reports/problems/etc.  James Bottomly suggested adding a
scsi device SUSPEND function. We are actively addressing this and you should
see a conversation started shortly.


-- James S



^ permalink raw reply	[flat|nested] 17+ messages in thread
* RE: [Emulex] Ready for next round...
@ 2004-07-19 20:02 Ely, Paul
  0 siblings, 0 replies; 17+ messages in thread
From: Ely, Paul @ 2004-07-19 20:02 UTC (permalink / raw)
  To: 'Christoph Hellwig', Smart, James; +Cc: Linux SCSI Reflector

Christoph,

We have applied this patch to our source base and
tested it successfully.
 
Patch accepted for our 8.0.7 release.

Thanks,
 
Paul Ely
Emulex

> -----Original Message-----
> From: 'Christoph Hellwig' [mailto:hch@infradead.org]
> Sent: Saturday, July 17, 2004 4:27 PM
> To: Smart, James
> Cc: 'Christoph Hellwig'; Linux SCSI Reflector
> Subject: Re: [Emulex] Ready for next round...
> 
> 
> On Sat, Jul 17, 2004 at 09:17:35PM +0100, 'Christoph Hellwig' wrote:
> > On Sat, Jul 17, 2004 at 07:26:32AM -0400, Smart, James wrote:
> > > Christoph,
> > > 
> > > We've updated the driver on SourceForge again.  This 
> addresses the rework of
> > > the clk functions. Also rids the driver of Report Lun use.
> > > 
> > > I know this next week will probably be busy due to the 
> OLS, but we would
> > > appreciate comments any time that you can provide them.
> > 
> > Okay, this starts to look better.  There's lots of dead 
> code again, see the
> > patch below to remove it.  Also while the parameter rework 
> in the last drop
> > is already much better it still needs some more work I think.
> 
> Sorry, wrong patch.  The correct one is below.
> 
> 
> diff -uNr -X /home/hch/dontdiff lpfcdriver-2.6-8.0.6/lpfc.h 
> lpfcdriver-2.6-8.0.6-hch/lpfc.h
> --- lpfcdriver-2.6-8.0.6/lpfc.h	2004-07-16 
> 20:59:54.000000000 +0200
> +++ lpfcdriver-2.6-8.0.6-hch/lpfc.h	2004-07-17 
> 21:07:40.000000000 +0200
> @@ -411,9 +411,7 @@
>  	/* pci_mem_pools */
>  	struct pci_pool *lpfc_scsi_dma_ext_pool;
>  	struct pci_pool *lpfc_mbuf_pool;
> -	struct pci_pool *lpfc_page_pool;
>  	struct lpfc_dma_pool lpfc_mbuf_safety_pool;
> -	struct lpfc_dma_pool lpfc_page_safety_pool;
>  	mempool_t *scsibuf_mem_pool;
>  
>  	mempool_t *iocb_mem_pool;
> diff -uNr -X /home/hch/dontdiff 
> lpfcdriver-2.6-8.0.6/lpfc_crtn.h lpfcdriver-2.6-8.0.6-hch/lpfc_crtn.h
> --- lpfcdriver-2.6-8.0.6/lpfc_crtn.h	2004-07-16 
> 20:59:54.000000000 +0200
> +++ lpfcdriver-2.6-8.0.6-hch/lpfc_crtn.h	2004-07-17 
> 20:57:50.000000000 +0200
> @@ -76,13 +76,9 @@
>  void lpfc_disc_start(struct lpfc_hba *);
>  void lpfc_disc_flush_list(struct lpfc_hba *);
>  void lpfc_linkdown_timeout(unsigned long);
> -void lpfc_nodev_timeout(unsigned long);
>  void lpfc_establish_link_tmo(unsigned long);
>  void lpfc_disc_timeout(unsigned long);
>  struct lpfc_lun *lpfc_find_lun(struct lpfc_hba *, uint32_t, 
> uint64_t, int);
> -struct lpfc_scsi_buf *lpfc_build_scsi_cmd(struct lpfc_hba *,
> -					  struct lpfc_nodelist *,
> -					  uint32_t, uint64_t);
>  void lpfc_set_failmask(struct lpfc_hba *, struct 
> lpfc_nodelist *, uint32_t,
>  		       uint32_t);
>  
> @@ -223,7 +219,6 @@
>  			    uint16_t);
>  void lpfc_mbox_timeout(unsigned long);
>  
> -void lpfc_free_scsi_buf(struct lpfc_scsi_buf *);
>  void lpfc_map_fcp_cmnd_to_bpl(struct lpfc_hba *, struct 
> lpfc_scsi_buf *);
>  void lpfc_free_scsi_cmd(struct lpfc_scsi_buf *);
>  uint32_t lpfc_os_timeout_transform(struct lpfc_hba *, uint32_t);
> @@ -242,9 +237,6 @@
>  void lpfc_sli_wake_iocb_high_priority(struct lpfc_hba * phba,
>  				      struct lpfc_iocbq * queue1,
>  				      struct lpfc_iocbq * queue2);
> -void *lpfc_page_alloc(struct lpfc_hba *, int, dma_addr_t *);
> -void lpfc_page_free(struct lpfc_hba *, void *, dma_addr_t);
> -
>  void *lpfc_mbuf_alloc(struct lpfc_hba *, int, dma_addr_t *);
>  void lpfc_mbuf_free(struct lpfc_hba *, void *, dma_addr_t);
>  
> diff -uNr -X /home/hch/dontdiff 
> lpfcdriver-2.6-8.0.6/lpfc_hbadisc.c 
> lpfcdriver-2.6-8.0.6-hch/lpfc_hbadisc.c
> --- lpfcdriver-2.6-8.0.6/lpfc_hbadisc.c	2004-07-16 
> 20:59:54.000000000 +0200
> +++ lpfcdriver-2.6-8.0.6-hch/lpfc_hbadisc.c	2004-07-17 
> 20:57:30.000000000 +0200
> @@ -2450,7 +2450,7 @@
>  	return;
>  }
>  
> -void
> +static void
>  lpfc_nodev_timeout(unsigned long ptr)
>  {
>  	struct lpfc_hba *phba;
> diff -uNr -X /home/hch/dontdiff 
> lpfcdriver-2.6-8.0.6/lpfc_mem.c lpfcdriver-2.6-8.0.6-hch/lpfc_mem.c
> --- lpfcdriver-2.6-8.0.6/lpfc_mem.c	2004-07-16 
> 20:59:54.000000000 +0200
> +++ lpfcdriver-2.6-8.0.6-hch/lpfc_mem.c	2004-07-17 
> 21:08:10.000000000 +0200
> @@ -40,12 +40,8 @@
>  static void lpfc_mem_safety_pool_destroy(struct lpfc_hba *);
>  static void lpfc_mbuf_safety_pool_create(struct lpfc_hba *, 
> uint32_t );
>  static void lpfc_mbuf_safety_pool_destroy(struct lpfc_hba *);
> -static void lpfc_page_safety_pool_create(struct lpfc_hba *, 
> uint32_t );
> -static void lpfc_page_safety_pool_destroy(struct lpfc_hba *);
>  void *lpfc_mbuf_alloc(struct lpfc_hba *, int, dma_addr_t *);
>  void lpfc_mbuf_free(struct lpfc_hba *, void *, dma_addr_t);
> -void *lpfc_page_alloc(struct lpfc_hba *, int, dma_addr_t *);
> -void lpfc_page_free(struct lpfc_hba *, void *, dma_addr_t);
>  static void *lpfc_scsibuf_pool_alloc(int , void *);
>  static void  lpfc_scsibuf_pool_free(void *, void *);
>  
> @@ -66,8 +62,6 @@
>  
>  	phba->lpfc_scsi_dma_ext_pool = 0;
>  	phba->lpfc_mbuf_pool = 0;
> -	phba->lpfc_page_pool = 0;
> -
>  
>  	phba->lpfc_scsi_dma_ext_pool =
>  		pci_pool_create("lpfc_scsi_dma_ext_pool",
> @@ -83,21 +77,12 @@
>  				8,
>  				0);
>  
> -	phba->lpfc_page_pool =
> -		pci_pool_create("lpfc_page_pool",
> -				phba->pcidev,
> -				LPFC_SCSI_PAGE_BUF_SZ,
> -				8,
> -				0);
> -
> -	if ((!phba->lpfc_scsi_dma_ext_pool) || 
> (!phba->lpfc_mbuf_pool) ||
> -	   (!phba->lpfc_page_pool)) {
> +	if (!phba->lpfc_scsi_dma_ext_pool || !phba->lpfc_mbuf_pool) {
>  		lpfc_mem_free(phba);
>  		return (0);
>  	}
>  
>  	lpfc_mbuf_safety_pool_create(phba, LPFC_MBUF_POOL_SIZE);
> -	lpfc_page_safety_pool_create(phba, LPFC_PAGE_POOL_SIZE);
>  	lpfc_mem_safety_pool_create(phba, LPFC_MEM_POOL_SIZE);
>  
>  	return (1);
> @@ -126,7 +111,6 @@
>  	}
>  
>  	lpfc_mbuf_safety_pool_destroy(phba);
> -	lpfc_page_safety_pool_destroy(phba);
>  	lpfc_mem_safety_pool_destroy(phba);
>  
>  	if (phba->lpfc_scsi_dma_ext_pool)
> @@ -134,11 +118,7 @@
>  
>  	if (phba->lpfc_mbuf_pool)
>  		pci_pool_destroy(phba->lpfc_mbuf_pool);
> -
> -	if (phba->lpfc_page_pool)
> -		pci_pool_destroy(phba->lpfc_page_pool);
> -
> -	return (1);
> +	return 1;
>  }
>  
>  void
> @@ -232,51 +212,6 @@
>  	kfree(pool->elements);
>  }
>  
> -
> -static void
> -lpfc_page_safety_pool_create(struct lpfc_hba * phba, uint32_t count)
> -{
> -	int i;
> -
> -	struct lpfc_dma_pool *pool =
> -		&(phba->lpfc_page_safety_pool);
> -
> -	pool->elements = kmalloc(sizeof(struct lpfc_dmabuf) * count,
> -				 GFP_KERNEL);
> -	pool->max_count = 0;
> -	pool->current_count = 0;
> -	for ( i=0; i<count; i++) {
> -		pool->elements[i].virt =
> -			pci_pool_alloc(phba->lpfc_page_pool,
> -				       GFP_KERNEL,
> -				       &(pool->elements[i].phys));
> -
> -		if (!pool->elements[i].virt)
> -			break;
> -		pool->max_count++;
> -		pool->current_count++;
> -	}
> -}
> -
> -static void
> -lpfc_page_safety_pool_destroy(struct lpfc_hba * phba)
> -{
> -	struct lpfc_dma_pool *pool =
> -		&(phba->lpfc_page_safety_pool);
> -	int i;
> -
> -	if ( pool->max_count != pool->current_count)
> -		printk("Memory leaked in page safety pool \n");
> -
> -	for (i=0; i< pool->current_count; i++) {
> -		pci_pool_free(phba->lpfc_page_pool,
> -			      pool->elements[i].virt,
> -			      pool->elements[i].phys);
> -	}
> -
> -	kfree(pool->elements);
> -}
> -
>  void *
>  lpfc_mbuf_alloc(struct lpfc_hba * phba, int mem_flags, 
> dma_addr_t * handle)
>  {
> @@ -315,44 +250,6 @@
>  	return;
>  }
>  
> -void *
> -lpfc_page_alloc(struct lpfc_hba * phba, int mem_flags, 
> dma_addr_t * handle)
> -{
> -	void *ret;
> -	struct lpfc_dma_pool *pool =
> -		&(phba->lpfc_page_safety_pool);
> -
> -	ret = pci_pool_alloc(phba->lpfc_page_pool, GFP_ATOMIC, handle);
> -	/*
> -	   If we are low in memory and is priority memory allocation
> -	   use safety pool
> -	*/
> -	if ((!ret) && ( mem_flags & MEM_PRI)
> -	    && (pool->current_count)) {
> -		pool->current_count--;
> -		ret = pool->elements[pool->current_count].virt;
> -		*handle = pool->elements[pool->current_count].phys;
> -	}
> -	return ret;
> -}
> -
> -void
> -lpfc_page_free(struct lpfc_hba * phba, void *virt, dma_addr_t dma)
> -{
> -	struct lpfc_dma_pool *pool =
> -		&(phba->lpfc_page_safety_pool);
> -
> -	if ( pool->current_count < pool->max_count) {
> -		pool->elements[pool->current_count].virt = virt;
> -		pool->elements[pool->current_count].phys = dma;
> -		pool->current_count++;
> -		return;
> -	}
> -
> -	pci_pool_free(phba->lpfc_page_pool, virt, dma);
> -	return;
> -}
> -
>  static void *
>  lpfc_scsibuf_pool_alloc(int gfp_flags, void *data) {
>  	void * ret;
> diff -uNr -X /home/hch/dontdiff 
> lpfcdriver-2.6-8.0.6/lpfc_scsiport.c 
> lpfcdriver-2.6-8.0.6-hch/lpfc_scsiport.c
> --- lpfcdriver-2.6-8.0.6/lpfc_scsiport.c	2004-07-16 
> 20:59:54.000000000 +0200
> +++ lpfcdriver-2.6-8.0.6-hch/lpfc_scsiport.c	2004-07-17 
> 20:56:01.000000000 +0200
> @@ -185,7 +185,7 @@
>  }
>  
>  /* This routine frees a scsi buffer, both DMAable and 
> non-DMAable regions */
> -void
> +static void
>  lpfc_free_scsi_buf(struct lpfc_scsi_buf * psb)
>  {
>  	struct lpfc_hba *phba;
> @@ -1127,177 +1127,6 @@
>  	return ret;
>  }
>  
> -struct lpfc_scsi_buf *
> -lpfc_build_scsi_cmd(struct lpfc_hba * phba,
> -		    struct lpfc_nodelist * nlp, uint32_t 
> scsi_cmd, uint64_t lun)
> -{
> -	struct lpfc_sli *psli;
> -	struct lpfc_target *targetp;
> -	struct lpfc_lun *lunp;
> -	struct lpfc_dmabuf *mp;
> -	struct lpfc_scsi_buf *lpfc_cmd;
> -	struct lpfc_iocbq *piocbq;
> -	IOCB_t *piocb;
> -	struct fcp_cmnd *fcpCmnd;
> -	struct ulp_bde64 *bpl;
> -	uint32_t tgt, size;
> -	struct scsi_cmnd *lnx_cmd;
> -	struct scsi_device *scsi_dev;
> -
> -	tgt = nlp->nlp_sid;
> -	lunp = lpfc_find_lun(phba, tgt, lun, 1);
> -	lpfc_cmd = 0;
> -	/* First see if the SCSI ID has an allocated struct 
> lpfc_target */
> -	if (lunp && lunp->pTarget) {
> -		targetp = lunp->pTarget;
> -		psli = &phba->sli;
> -
> -		/* Get a buffer to hold SCSI data */
> -		mp = kmalloc(sizeof (struct lpfc_dmabuf), GFP_ATOMIC);
> -		if (mp == 0) {
> -			return (0);
> -		}
> -
> -		INIT_LIST_HEAD(&mp->list);
> -		/* Get resources to send a SCSI command */
> -		lpfc_cmd = lpfc_get_scsi_buf(phba, GFP_ATOMIC);
> -		if (lpfc_cmd == 0) {
> -			kfree(mp);
> -			return (0);
> -		}
> -		lpfc_cmd->pLun = lunp;
> -		/* Internal-generated cmd, so we need to create 
> a temporary
> -		 * linux scsi_cmnd and scsi_device.
> -		 * Also means when this internal cmd done, we 
> need to free
> -		 * this scsi_cmnd and scsi_device structures.
> -		 */
> -		lnx_cmd = kmalloc(sizeof(struct scsi_cmnd), GFP_ATOMIC);
> -		if (lnx_cmd == 0) {
> -			kfree(mp);
> -			lpfc_free_scsi_buf(lpfc_cmd);
> -			return (0);
> -		}
> -		scsi_dev = kmalloc(sizeof(struct scsi_device), 
> GFP_ATOMIC);
> -		if (scsi_dev == 0) {
> -			kfree(mp);
> -			kfree(lnx_cmd);
> -			lpfc_free_scsi_buf(lpfc_cmd);
> -			return (0);
> -		}
> -		scsi_dev->id = tgt;
> -		scsi_dev->lun = lun;
> -		scsi_dev->hostdata = lunp;
> -		lnx_cmd->device = scsi_dev;
> -		lpfc_cmd->pCmd = lnx_cmd;
> -		lpfc_cmd->timeout = 30 + phba->fcp_timeout_offset;
> -
> -		/* Finish building BPL with the I/O dma ptrs.
> -		 * setup FCP CMND, and setup IOCB.
> -		 */
> -
> -		fcpCmnd = lpfc_cmd->fcp_cmnd;
> -
> -		putLunHigh(fcpCmnd->fcpLunMsl, lun);	/* LUN */
> -		putLunLow(fcpCmnd->fcpLunLsl, lun);	/* LUN */
> -
> -		switch (scsi_cmd) {
> -		case REPORT_LUNS:
> -			size = LPFC_SCSI_PAGE_BUF_SZ;
> -			fcpCmnd->fcpCdb[0] = scsi_cmd;
> -			/* 0x1000 = LPFC_SCSI_PAGE_BUF_SZ */
> -			fcpCmnd->fcpCdb[8] = 0x10;
> -			fcpCmnd->fcpCdb[9] = 0x00;
> -			fcpCmnd->fcpCntl3 = READ_DATA;
> -			fcpCmnd->fcpDl = 
> be32_to_cpu(LPFC_SCSI_PAGE_BUF_SZ);
> -			/* Get a buffer to hold SCSI data */
> -			if ((mp->virt =
> -			     lpfc_page_alloc(phba, 0, 
> &(mp->phys))) == 0) {
> -				if (mp)
> -					kfree(mp);
> -				lpfc_free_scsi_buf(lpfc_cmd);
> -				return (0);
> -			}
> -			break;
> -		case INQUIRY:
> -			fcpCmnd->fcpCdb[0] = scsi_cmd;	/* SCSI Inquiry
> -							   Command */
> -			fcpCmnd->fcpCdb[4] = 0xff;	/* 
> allocation length */
> -			fcpCmnd->fcpCntl3 = READ_DATA;
> -			fcpCmnd->fcpDl = be32_to_cpu(LPFC_SCSI_BUF_SZ);
> -			/* drop thru to get a buffer */
> -		default:
> -			size = LPFC_SCSI_BUF_SZ;
> -			/* Get a buffer to hold SCSI data */
> -			if ((mp->virt =
> -			     lpfc_mbuf_alloc(phba, 0, 
> &(mp->phys))) == 0) {
> -				if (mp)
> -					kfree(mp);
> -				lpfc_free_scsi_buf(lpfc_cmd);
> -				return (0);
> -			}
> -			break;
> -		}
> -
> -		bpl = lpfc_cmd->fcp_bpl;
> -		bpl += 2;	/* Bump past FCP CMND and FCP RSP */
> -
> -		/* no scatter-gather list case */
> -		bpl->addrLow = le32_to_cpu(putPaddrLow(mp->phys));
> -		bpl->addrHigh = le32_to_cpu(putPaddrHigh(mp->phys));
> -		bpl->tus.f.bdeSize = size;
> -		bpl->tus.f.bdeFlags = BUFF_USE_RCV;
> -		bpl->tus.w = le32_to_cpu(bpl->tus.w);
> -		bpl++;
> -		bpl->addrHigh = 0;
> -		bpl->addrLow = 0;
> -		bpl->tus.w = 0;
> -
> -		piocbq = &lpfc_cmd->cur_iocbq;
> -		piocb = &piocbq->iocb;
> -		piocb->ulpCommand = CMD_FCP_IREAD64_CR;
> -		piocb->ulpPU = PARM_READ_CHECK;
> -		piocb->un.fcpi.fcpi_parm = size;
> -		piocb->un.fcpi64.bdl.bdeSize += sizeof (struct 
> ulp_bde64);
> -		piocb->ulpBdeCount = 1;
> -		piocb->ulpLe = 1;	/* Set the LE bit in the iocb */
> -
> -		/* Get an iotag and finish setup of IOCB  */
> -		piocb->ulpIoTag = lpfc_sli_next_iotag(phba,
> -					      
> &phba->sli.ring[psli->fcp_ring]);
> -		piocb->ulpContext = nlp->nlp_rpi;
> -		if (nlp->nlp_fcp_info & NLP_FCP_2_DEVICE) {
> -			piocb->ulpFCP2Rcvy = 1;
> -		}
> -		piocb->ulpClass = (nlp->nlp_fcp_info & 0x0f);
> -
> -		/* ulpTimeout is only one byte */
> -		if (lpfc_cmd->timeout > 0xff) {
> -			/*
> -			 * Do not timeout the command at the 
> firmware level.
> -			 * The driver will provide the timeout 
> mechanism.
> -			 */
> -			piocb->ulpTimeout = 0;
> -		} else {
> -			piocb->ulpTimeout = lpfc_cmd->timeout;
> -		}
> -
> -		/*
> -		 * Setup driver timeout, in case the command 
> does not complete
> -		 * Driver timeout should be greater than ulpTimeout
> -		 */
> -
> -		piocbq->drvrTimeout = lpfc_cmd->timeout + 
> LPFC_DRVR_TIMEOUT;
> -
> -		/* set up iocb return path by setting the context fields
> -		 * and the completion function.
> -		 */
> -		piocbq->context1 = lpfc_cmd;
> -		piocbq->context2 = mp;
> -
> -	}
> -	return (lpfc_cmd);
> -}
> -
>  void
>  lpfc_scsi_timeout_handler(unsigned long ptr)
>  {
> -
> To unsubscribe from this list: send the line "unsubscribe 
> linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread
* RE: [Emulex] Ready for next round...
@ 2004-07-17 11:26 Smart, James
  2004-07-17 20:17 ` 'Christoph Hellwig'
  0 siblings, 1 reply; 17+ messages in thread
From: Smart, James @ 2004-07-17 11:26 UTC (permalink / raw)
  To: Smart, James, 'Christoph Hellwig'; +Cc: Linux SCSI Reflector

Christoph,

We've updated the driver on SourceForge again.  This addresses the rework of
the clk functions. Also rids the driver of Report Lun use.

I know this next week will probably be busy due to the OLS, but we would
appreciate comments any time that you can provide them.

Thanks.

- James S


> -----Original Message-----
> From: Smart, James 
> Sent: Friday, July 09, 2004 3:24 PM
> To: 'Christoph Hellwig'
> Cc: Linux SCSI Reflector
> Subject: RE: [Emulex] Ready for next round...
> 
> 
> Christoph,
> 
> Can you please take a look at the snapshot just posted on SourceForge.
> 
> The following things have been reworked:
> - USE_HGP_HOST_SLIM: finally resolved this issue and implemented it
> correctly.
> - Logging: We killed lpfc_logmsg.c and converted over to a 
> single define
> using dev_printk. Still implements a subsystem filter in the macro.
> - Attributes: They have been moved over to shost_attrs. We 
> changed the basic
> philosophy about per-instance attributes, as many simply 
> aren't needed to be
> set prior to attachment. This allowed them to go back 
> integers. You will
> find several removed as we removed some of the logic that 
> insulated the
> midlayer and above from transient conditions (delayed io 
> error, etc - see
> last paragraph). We killed lpfc.conf and have converted over 
> to a separation
> of attributes from their description arrays. One discrepancy 
> - the static
> configuration array for persistent bindings remains for 
> compatibility - but
> it's own file.
> - Your big patch to lpfc_fcp.c - was incorporated.
> - [PATCH] streamline lpfc error handling: This was 
> incorporated. There were
> some changes to clean up return codes and misplaced blocks.
> - [PATCH] remove dead event counters from lpfc. Rather than 
> remove this
> code, as it will be used later, we have at least consolidated 
> it and made
> the events visible via sysfs.
> 
> 
> Things we're still addressing - and should be in drop on 7/16:
> - clk_data: We're removing the safety list, and converting 
> from multiple
> clock args to 1 so that it better maps to the kernel api.
> - secondary data structures for target/lun data : We're attempting to
> reduce/remove the notions of luns as, in general, the driver 
> issues i/o at
> the target level. Part of this is removing report luns use, 
> etc. Note: We
> will see if we can help the midlayer with addressing mode support.
> 
> We have one major issue, which has been partially discussed already
> (http://marc.theaimsgroup.com/?l=linux-scsi&m=108275840501608&w=2). In
> summary, there are discovery state machine events when we 
> lose connectivity
> with the target for a short period. If we do not suspend i/o 
> to the target,
> the upper layer retries can exhaust before the target comes 
> back, triggering
> other system error reports/problems/etc.  James Bottomly 
> suggested adding a
> scsi device SUSPEND function. We are actively addressing this 
> and you should
> see a conversation started shortly.
> 
> 
> -- James S
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe 
> linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread
* RE: [Emulex] Ready for next round...
@ 2004-06-18 19:56 Smart, James
  2004-06-18 20:58 ` 'Christoph Hellwig'
  0 siblings, 1 reply; 17+ messages in thread
From: Smart, James @ 2004-06-18 19:56 UTC (permalink / raw)
  To: 'Christoph Hellwig'; +Cc: Linux SCSI Reflector

Christoph,

per your comments:
 
> > - 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)
> 
> Your paramters are still extremly crappy.  lpfc.conf is 
> horrible, as is
> having the module paramters in a header.  What's especially 
> bad is that
> they are character paramters without any abvious reasons, please use
> the proper integer types.
> 
> lpfc_cfgparm.h is truely horrible.

More guidance needed than the terms used.... What are you looking for ?

lpfc.conf - should be better renamed to a .c file and it is compiled into
the driver. It provides the default values for the attributes for each
adapter. Our requirement is to have individual attributes per instance, with
the approach to setting them being - supply a global default, then
optionally allow per-instance overrides.

Why per-instance attributes ?  With large adapter counts (no real max,
expect > 32), the potential uses and configurations can vary widely on each.
This requirement has been confirmed by customer requests.

Why compile it in ? The view on how an attribute can be dynamically updated
is either via modules.conf or a utility that runs post module load. Given
the large adapter count and the number of attributes - it's too much for
modules.conf. Given that anything that runs post module load means the
driver has already performed adapter initialization and link bringup, it's
too late for some parameters. For these late parameters, we can reconfigure
to pick up the dynamic settings, but brief behavior with the unwanted
parameters may cause undesired effects for the user. The end result - we let
users update the values and recompile the driver. If there's a better way,
please point us in the right direction. I'd love to not require
recompilation.

Why the character parameters ? It becomes clear rapidly. Previously, we had
integer arrays, but as we didn't want to cap the number of adapters that can
be supported, nor require huge numbers of elements to be dealt with (a set
per adapter), we reduced it to 1 element per attribute - a character string
so that it can be dynamic in length. The string contains the global default,
and can be extended if there are 1 or more instances that needed specific
values. This significantly reduced the memory requirements, the number of
elements, and seemed more intuitive to specify by the user.  We also looked
at module_param_array, but this doesn't seem to work with the model of
wanting to set different defaults for different elements at compile time.

lpfc_cfgparm.h: for the most part, it's a structured way to manage the
attributes. Contains min/max so that we can verify the user didn't
inadvertantly screw up a setting, as well as knowing what to fall back to in
case they did. A couple of the elements, such as the strings, are largely
unnecessary, but could be useful for log messages.  One thing we will change
- this huge array is currently copied per instance right now. It will be
changes so that there is only 1 master array containing the boundaries, and
the current values will be the only thing kept per instance.

Can you provide some additional comments or options ?

> 
> > - 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..
> 
> Still not much better.  In fact the crappy datastructures and code are
> still there, just spread all over the place now instead of fixed up.

Ok - so the disagreement here really is the fact we had clock data
structures.

The data structures were for 2 purposes:
- to pass more than 1 argument to a callback.  
- to keep a "safety" list of all timers when we needed to take an adapter
offline.

Since this isn't acceptable, here's what our approach is:
- Were seeing if the need for >1 arg is really needed. If so, we'll axe this
part of the structure.
- We're removing the safety list. We shouldn't need it anyway as normal data
structure teardown should handle it.
If both happen, the structure will be gone.

> 
> > - 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?
> 
> You're still attaching all the attributes yourself.  Please use the
> scsi-provided host/device attributes instead.

Ok. The way I interpret this comment: the sysfs implementation (mechanics)
is ok. The per adapter attributes are ok, with the exception of where they
overlap with scsi attributes (such as lun skip and queue depths).  I agree -
we'll remove the overlaps. This was old history..

> 
> > - 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.
> 
> While you started using sdev->hostdata the whole code is 
> still more than
> fishy.  You have a complete set of secondary data structures 
> for target/lun
> data lookup yet.  Please allocate all per-lun data in 
> ->slave_alloc and
> if the target data doesn't exist yet that one too and hang it 
> off the lun
> data.
> 

Not fishy - but done differently based on the code's history.  In either
case, we'll rework to fit the desired approach.

-- James

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [Emulex] Ready for next round...
@ 2004-06-15 17:32 Smart, James
  2004-06-16  8:17 ` Christoph Hellwig
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Smart, James @ 2004-06-15 17:32 UTC (permalink / raw)
  To: Linux SCSI Reflector

Christoph, et al...

I'd like to request the next round of review comments on the driver.

We believe that tonights posting on SourceForge has addressed all of the
comments to date with only a couple of exceptions (see below).

-- James S



Open items:
------------
[From 5/9] Re: typedefs in sSuDly Caps and pfoo pointer naming.
We have addressed some, but not all of this. We will remove all of it.

[From 5/9] Re: explain USE_HGP_HOST_SLIM and why only ppc64 sets it
A very valid question (thank you). This was masking several issues. Some of
which we are still testing. For now, the code has been left as is. We will
be posting the actual change shortly.

[From 5/0] Re: don't mess with eh_timeout from a LLDD please
We haven't sync'd with the IBM patch as yet. Will be doing so.


Comments:
--------------
[From 5/9] Re: lpfc_msg* stuff is not acceptable.
We've inlined the print strings and filter values. Should be acceptable now.

[From 5/9] Re: you should probably redesign your driver to use the host_lock
instead of the drvlock everywhere.
For now, we haven't changed. However, we are actively looking at this.

[From 5/9] Re: what do you need lpfcDRVR.loadtime for?
Is used for diagnostics and trouble-shooting.

[From 5/18] Re: while we're at it reorder includes alphabetically
We do not agree with alphabetical ordering. We believe in letting the header
dependencies decide the ordering, and where ordering is not dictated, let
logical grouping take place.

[From 5/31] Re: remove dead EXPORT_SYMBOLS
Based on the feedback and recommendations - we've removed the external
symbols. At a later date, we'll propose patches to reintroduce the
functionality in a manner that will be more acceptable to the group.

[From 6/1]Re: Adam Blanchard's comment on page alloc fail on ppc64, thought
on using a mempool & Re: other queuecommand problems - too many allocations,
allocs under spinlock, etc.
We made several code reorgs to move locks, and change the allocations so
that everything is GFP_NOIO under no locks. However, we're subject to an
issue with blk_run_queue() which invokes us under softirq. At this point, we
have to move things back to GFP_ATOMIC to be safe. What you will see in
tonights drop is: a) we've removed a couple of "always allocate" cases by
embedding the structures into the base scsi_buf; b) we've implemented a
mempool to hopefully help (but not solve) the failure case.




^ permalink raw reply	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2004-07-19 20:03 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-07-09 19:24 [Emulex] Ready for next round Smart, James
2004-07-09 22:08 ` 'Christoph Hellwig'
2004-07-09 22:10 ` 'Christoph Hellwig'
  -- strict thread matches above, loose matches on Subject: below --
2004-07-19 20:02 Ely, Paul
2004-07-17 11:26 Smart, James
2004-07-17 20:17 ` 'Christoph Hellwig'
2004-07-17 20:26   ` 'Christoph Hellwig'
2004-06-18 19:56 Smart, James
2004-06-18 20:58 ` 'Christoph Hellwig'
2004-06-15 17:32 Smart, James
2004-06-16  8:17 ` Christoph Hellwig
2004-06-16 13:35   ` Jamie Wellnitz
2004-06-16 13:50     ` Christoph Hellwig
2004-06-16  8:20 ` Christoph Hellwig
2004-06-16  8:31 ` Christoph Hellwig
2004-06-20 13:35 ` Christoph Hellwig
2004-06-23 15:31 ` Anton Blanchard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox