[REPOST][PATCH 1/6] mpt fusion - fibre channel target discovery prematurely terminates

[REPOST][PATCH 1/6] mpt fusion - fibre channel target discovery prematurely terminates

[Michael Reed]

[-- Attachment #1: Type: text/plain, Size: 514 bytes --]

Reset processing sets ioc->active to zero.  This causes target discovery
to be unable to read configuration pages which can result in fewer
than the total number of targets being discovered.  This patch skips
target discovery if ioc->active is zero.  The driver reschedules
target discovery once ioc->active becomes non-zero.

This situation occurs when the lsiutil program is being used to reset
the board.

(Logic change from previously posted 01-mptfc_eagain.patch.)

Signed-off-by: Michael Reed <mdr@sgi.com>


[-- Attachment #2: 01-mptfc_ioc_active.patch --]
[-- Type: text/x-patch, Size: 1207 bytes --]

Reset processing sets ioc->active to zero.  This causes target discovery
to be unable to read configuration pages which can result in fewer
than the total number of targets being discovered.  This patch skips
target discovery if ioc->active is zero.  The driver reschedules
target discovery once ioc->active becomes non-zero.

This situation occurs when the lsiutil program is being used to reset
the board.

Signed-off-by: Michael Reed <mdr@sgi.com>


--- rc4u/drivers/message/fusion/mptfc.c	2006-05-18 16:43:54.968309949 -0500
+++ rc4/drivers/message/fusion/mptfc.c	2006-05-24 14:08:58.999830952 -0500
@@ -639,6 +639,14 @@
 	struct mptfc_rport_info *ri;
 
 	do {
+		/*
+		 * if the ioc isn't active, cannot fetch config pages
+		 * if it becomes active, work count will increment and
+		 * another pass will occur or work will be rescheduled
+		 */
+		if (ioc->active == 0)
+			goto check_work_remaining;
+
 		/* start by tagging all ports as missing */
 		list_for_each_entry(ri, &ioc->fc_rports, list) {
 			if (ri->flags & MPT_RPORT_INFO_FLAGS_REGISTERED) {
@@ -676,6 +684,7 @@
 			}
 		}
 
+ check_work_remaining:
 		/*
 		 * allow multiple passes as target state
 		 * might have changed during scan

Re: [REPOST][PATCH 1/6] mpt fusion - fibre channel target discovery prematurely terminates

[James Bottomley]

On Wed, 2006-05-24 at 15:06 -0500, Michael Reed wrote:
> Reset processing sets ioc->active to zero.  This causes target discovery
> to be unable to read configuration pages which can result in fewer
> than the total number of targets being discovered.  This patch skips
> target discovery if ioc->active is zero.  The driver reschedules
> target discovery once ioc->active becomes non-zero.

This doesn't quite solve all cases, though, does it?  mpt_config()
returns -EAGAIN for both ioc->active set to zero *and* if we run out of
frames.

> This situation occurs when the lsiutil program is being used to reset
> the board.
> 
> (Logic change from previously posted 01-mptfc_eagain.patch.)

I was still thinking of something that made mpt_config() never return
-EAGAIN.  How does the following work out?  It's my first pass at
sorting out the frame and active logic in mptbase:

James

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index a300840..585f42f 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -121,6 +121,11 @@ static int	mpt_base_index = -1;
 static int	last_drv_idx = -1;
 
 static DECLARE_WAIT_QUEUE_HEAD(mpt_waitq);
+/* This waitq is used for mpt_get_msg_frame failures which may be
+ * caused either by the ioc->active being zero or by the adapter being
+ * out of frames.  Receiving this event doesn't guarantee that a frame
+ * is available, so you must check */
+static DECLARE_WAIT_QUEUE_HEAD(mpt_config_waitq);
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /*
@@ -722,6 +727,13 @@ mpt_device_driver_deregister(int cb_idx)
 	MptDeviceDriverHandlers[cb_idx] = NULL;
 }
 
+static void
+mpt_ioc_activate(MPT_ADAPTER *ioc)
+{
+	ioc->active = 1;
+	wake_up(&mpt_config_waitq);
+}
+
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /**
@@ -835,6 +847,7 @@ #endif
 	mf_dma_addr = (ioc->req_frames_low_dma + req_offset) | ioc->RequestNB[req_idx];
 	dsgprintk((MYIOC_s_INFO_FMT "mf_dma_addr=%x req_idx=%d RequestNB=%x\n", ioc->name, mf_dma_addr, req_idx, ioc->RequestNB[req_idx]));
 	CHIPREG_WRITE32(&ioc->chip->RequestFifo, mf_dma_addr);
+	wake_up(&mpt_config_waitq);
 }
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
@@ -1582,7 +1595,7 @@ mpt_resume(struct pci_dev *pdev)
 
 	/* enable interrupts */
 	CHIPREG_WRITE32(&ioc->chip->IntMask, MPI_HIM_DIM);
-	ioc->active = 1;
+	mpt_ioc_activate(ioc);
 
 	printk(MYIOC_s_INFO_FMT
 		"pci-resume: ioc-state=0x%x,doorbell=0x%x\n",
@@ -1682,7 +1695,7 @@ mpt_do_ioc_recovery(MPT_ADAPTER *ioc, u3
 				dprintk((KERN_INFO MYNAM ": alt-%s reply irq re-enabled\n",
 						ioc->alt_ioc->name));
 				CHIPREG_WRITE32(&ioc->alt_ioc->chip->IntMask, MPI_HIM_DIM);
-				ioc->alt_ioc->active = 1;
+				mpt_ioc_activate(ioc->alt_ioc);
 			}
 
 		} else {
@@ -1798,7 +1811,7 @@ mpt_do_ioc_recovery(MPT_ADAPTER *ioc, u3
 	if (ret == 0) {
 		/* Enable! (reply interrupt) */
 		CHIPREG_WRITE32(&ioc->chip->IntMask, MPI_HIM_DIM);
-		ioc->active = 1;
+		mpt_ioc_activate(ioc);
 	}
 
 	if (reset_alt_ioc_active && ioc->alt_ioc) {
@@ -1806,7 +1819,7 @@ mpt_do_ioc_recovery(MPT_ADAPTER *ioc, u3
 		dinitprintk((KERN_INFO MYNAM ": alt-%s reply irq re-enabled\n",
 				ioc->alt_ioc->name));
 		CHIPREG_WRITE32(&ioc->alt_ioc->chip->IntMask, MPI_HIM_DIM);
-		ioc->alt_ioc->active = 1;
+		mpt_ioc_activate(ioc->alt_ioc);
 	}
 
 	/*  Enable MPT base driver management of EventNotification
@@ -4069,7 +4082,6 @@ #endif
  *	Return: 0 for success
  *	-ENOMEM if no memory available
  *		-EPERM if not allowed due to ISR context
- *		-EAGAIN if no msg frames currently available
  *		-EFAULT for non-successful reply or no reply (timeout)
  */
 static int
@@ -4181,7 +4193,6 @@ GetLanConfigPages(MPT_ADAPTER *ioc)
  *	Return: 0 for success
  *	-ENOMEM if no memory available
  *		-EPERM if not allowed due to ISR context
- *		-EAGAIN if no msg frames currently available
  *		-EFAULT for non-successful reply or no reply (timeout)
  */
 int
@@ -4493,7 +4504,6 @@ mptbase_raid_process_event_data(MPT_ADAP
  *	Returns: 0 for success
  *	-ENOMEM if no memory available
  *		-EPERM if not allowed due to ISR context
- *		-EAGAIN if no msg frames currently available
  *		-EFAULT for non-successful reply or no reply (timeout)
  */
 static int
@@ -5156,7 +5166,6 @@ SendEventAck(MPT_ADAPTER *ioc, EventNoti
  *
  *	Returns 0 for success
  *	-EPERM if not allowed due to ISR context
- *	-EAGAIN if no msg frames currently available
  *	-EFAULT for non-successful reply or no reply (timeout)
  */
 int
@@ -5182,10 +5191,10 @@ mpt_config(MPT_ADAPTER *ioc, CONFIGPARMS
 
 	/* Get and Populate a free Frame
 	 */
-	if ((mf = mpt_get_msg_frame(mpt_base_index, ioc)) == NULL) {
+	while ((mf = mpt_get_msg_frame(mpt_base_index, ioc)) == NULL) {
 		dcprintk((MYIOC_s_WARN_FMT "mpt_config: no msg frames!\n",
 				ioc->name));
-		return -EAGAIN;
+		wait_event(mpt_config_waitq, pCfg->wait_done);
 	}
 	pReq = (Config_t *)mf;
 	pReq->Action = pCfg->action;

Re: [REPOST][PATCH 1/6] mpt fusion - fibre channel target discovery prematurely terminates

[Michael Reed]

James Bottomley wrote:
> 
> I was still thinking of something that made mpt_config() never return
> -EAGAIN.  How does the following work out?  It's my first pass at
> sorting out the frame and active logic in mptbase:


pCfg->wait_done needs to be initialized to zero before sleeping.

It takes seven to ten seconds for the ioc->active value to become
non-zero and the wake up to occur.

It appears that the order of execution during "lsiutil 99" testing
has changed.  Once the driver marks the fibre targets missing, they
never return.  Hmmm....  Until this is resolved, this patch cannot
be applied.  I'll see if I can sort out how the execution order
has changed.

Another comment, for every i/o, this patch introduces overhead.
spinlock and checking a list which is empty 99.99[...]% of the time.
I'll have to perform an iop measurement to see if there is a
measurable impact on system performance.


Mike

> 
> James
> 
> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> index a300840..585f42f 100644
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c
> @@ -121,6 +121,11 @@ static int	mpt_base_index = -1;
>  static int	last_drv_idx = -1;
>  
>  static DECLARE_WAIT_QUEUE_HEAD(mpt_waitq);
> +/* This waitq is used for mpt_get_msg_frame failures which may be
> + * caused either by the ioc->active being zero or by the adapter being
> + * out of frames.  Receiving this event doesn't guarantee that a frame
> + * is available, so you must check */
> +static DECLARE_WAIT_QUEUE_HEAD(mpt_config_waitq);
>  
>  /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
>  /*
> @@ -722,6 +727,13 @@ mpt_device_driver_deregister(int cb_idx)
>  	MptDeviceDriverHandlers[cb_idx] = NULL;
>  }
>  
> +static void
> +mpt_ioc_activate(MPT_ADAPTER *ioc)
> +{
> +	ioc->active = 1;
> +	wake_up(&mpt_config_waitq);
> +}
> +
>  
>  /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
>  /**
> @@ -835,6 +847,7 @@ #endif
>  	mf_dma_addr = (ioc->req_frames_low_dma + req_offset) | ioc->RequestNB[req_idx];
>  	dsgprintk((MYIOC_s_INFO_FMT "mf_dma_addr=%x req_idx=%d RequestNB=%x\n", ioc->name, mf_dma_addr, req_idx, ioc->RequestNB[req_idx]));
>  	CHIPREG_WRITE32(&ioc->chip->RequestFifo, mf_dma_addr);
> +	wake_up(&mpt_config_waitq);
>  }
>  
>  /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
> @@ -1582,7 +1595,7 @@ mpt_resume(struct pci_dev *pdev)
>  
>  	/* enable interrupts */
>  	CHIPREG_WRITE32(&ioc->chip->IntMask, MPI_HIM_DIM);
> -	ioc->active = 1;
> +	mpt_ioc_activate(ioc);
>  
>  	printk(MYIOC_s_INFO_FMT
>  		"pci-resume: ioc-state=0x%x,doorbell=0x%x\n",
> @@ -1682,7 +1695,7 @@ mpt_do_ioc_recovery(MPT_ADAPTER *ioc, u3
>  				dprintk((KERN_INFO MYNAM ": alt-%s reply irq re-enabled\n",
>  						ioc->alt_ioc->name));
>  				CHIPREG_WRITE32(&ioc->alt_ioc->chip->IntMask, MPI_HIM_DIM);
> -				ioc->alt_ioc->active = 1;
> +				mpt_ioc_activate(ioc->alt_ioc);
>  			}
>  
>  		} else {
> @@ -1798,7 +1811,7 @@ mpt_do_ioc_recovery(MPT_ADAPTER *ioc, u3
>  	if (ret == 0) {
>  		/* Enable! (reply interrupt) */
>  		CHIPREG_WRITE32(&ioc->chip->IntMask, MPI_HIM_DIM);
> -		ioc->active = 1;
> +		mpt_ioc_activate(ioc);
>  	}
>  
>  	if (reset_alt_ioc_active && ioc->alt_ioc) {
> @@ -1806,7 +1819,7 @@ mpt_do_ioc_recovery(MPT_ADAPTER *ioc, u3
>  		dinitprintk((KERN_INFO MYNAM ": alt-%s reply irq re-enabled\n",
>  				ioc->alt_ioc->name));
>  		CHIPREG_WRITE32(&ioc->alt_ioc->chip->IntMask, MPI_HIM_DIM);
> -		ioc->alt_ioc->active = 1;
> +		mpt_ioc_activate(ioc->alt_ioc);
>  	}
>  
>  	/*  Enable MPT base driver management of EventNotification
> @@ -4069,7 +4082,6 @@ #endif
>   *	Return: 0 for success
>   *	-ENOMEM if no memory available
>   *		-EPERM if not allowed due to ISR context
> - *		-EAGAIN if no msg frames currently available
>   *		-EFAULT for non-successful reply or no reply (timeout)
>   */
>  static int
> @@ -4181,7 +4193,6 @@ GetLanConfigPages(MPT_ADAPTER *ioc)
>   *	Return: 0 for success
>   *	-ENOMEM if no memory available
>   *		-EPERM if not allowed due to ISR context
> - *		-EAGAIN if no msg frames currently available
>   *		-EFAULT for non-successful reply or no reply (timeout)
>   */
>  int
> @@ -4493,7 +4504,6 @@ mptbase_raid_process_event_data(MPT_ADAP
>   *	Returns: 0 for success
>   *	-ENOMEM if no memory available
>   *		-EPERM if not allowed due to ISR context
> - *		-EAGAIN if no msg frames currently available
>   *		-EFAULT for non-successful reply or no reply (timeout)
>   */
>  static int
> @@ -5156,7 +5166,6 @@ SendEventAck(MPT_ADAPTER *ioc, EventNoti
>   *
>   *	Returns 0 for success
>   *	-EPERM if not allowed due to ISR context
> - *	-EAGAIN if no msg frames currently available
>   *	-EFAULT for non-successful reply or no reply (timeout)
>   */
>  int
> @@ -5182,10 +5191,10 @@ mpt_config(MPT_ADAPTER *ioc, CONFIGPARMS
>  
>  	/* Get and Populate a free Frame
>  	 */
> -	if ((mf = mpt_get_msg_frame(mpt_base_index, ioc)) == NULL) {
> +	while ((mf = mpt_get_msg_frame(mpt_base_index, ioc)) == NULL) {
>  		dcprintk((MYIOC_s_WARN_FMT "mpt_config: no msg frames!\n",
>  				ioc->name));
> -		return -EAGAIN;
> +		wait_event(mpt_config_waitq, pCfg->wait_done);
>  	}
>  	pReq = (Config_t *)mf;
>  	pReq->Action = pCfg->action;
> 
> 

Re: [REPOST][PATCH 1/6] mpt fusion - fibre channel target discovery prematurely terminates

[Michael Reed]

Michael Reed wrote:
> James Bottomley wrote:
...snip...
> 
> It appears that the order of execution during "lsiutil 99" testing
> has changed.  Once the driver marks the fibre targets missing, they
> never return.  Hmmm....  Until this is resolved, this patch cannot
> be applied.  I'll see if I can sort out how the execution order
> has changed.
> 

I found the problem with the order of execution and it's my own doing.
When I added the setup_reset_work, it can get queued while the
rescan work is running, and THEN the rescan work count can be bumped.
As the rescan work loops on the work count, this changes the exec
order.  Doh.  The second rescan needs to execute after the setup reset
work.

Mike

> 
> 
> Mike
> 
>> James
>>
>> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
>> index a300840..585f42f 100644
>> --- a/drivers/message/fusion/mptbase.c
>> +++ b/drivers/message/fusion/mptbase.c
>> @@ -121,6 +121,11 @@ static int	mpt_base_index = -1;
>>  static int	last_drv_idx = -1;
>>  
>>  static DECLARE_WAIT_QUEUE_HEAD(mpt_waitq);
>> +/* This waitq is used for mpt_get_msg_frame failures which may be
>> + * caused either by the ioc->active being zero or by the adapter being
>> + * out of frames.  Receiving this event doesn't guarantee that a frame
>> + * is available, so you must check */
>> +static DECLARE_WAIT_QUEUE_HEAD(mpt_config_waitq);
>>  
>>  /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
>>  /*
>> @@ -722,6 +727,13 @@ mpt_device_driver_deregister(int cb_idx)
>>  	MptDeviceDriverHandlers[cb_idx] = NULL;
>>  }
>>  
>> +static void
>> +mpt_ioc_activate(MPT_ADAPTER *ioc)
>> +{
>> +	ioc->active = 1;
>> +	wake_up(&mpt_config_waitq);
>> +}
>> +
>>  
>>  /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
>>  /**
>> @@ -835,6 +847,7 @@ #endif
>>  	mf_dma_addr = (ioc->req_frames_low_dma + req_offset) | ioc->RequestNB[req_idx];
>>  	dsgprintk((MYIOC_s_INFO_FMT "mf_dma_addr=%x req_idx=%d RequestNB=%x\n", ioc->name, mf_dma_addr, req_idx, ioc->RequestNB[req_idx]));
>>  	CHIPREG_WRITE32(&ioc->chip->RequestFifo, mf_dma_addr);
>> +	wake_up(&mpt_config_waitq);
>>  }
>>  
>>  /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
>> @@ -1582,7 +1595,7 @@ mpt_resume(struct pci_dev *pdev)
>>  
>>  	/* enable interrupts */
>>  	CHIPREG_WRITE32(&ioc->chip->IntMask, MPI_HIM_DIM);
>> -	ioc->active = 1;
>> +	mpt_ioc_activate(ioc);
>>  
>>  	printk(MYIOC_s_INFO_FMT
>>  		"pci-resume: ioc-state=0x%x,doorbell=0x%x\n",
>> @@ -1682,7 +1695,7 @@ mpt_do_ioc_recovery(MPT_ADAPTER *ioc, u3
>>  				dprintk((KERN_INFO MYNAM ": alt-%s reply irq re-enabled\n",
>>  						ioc->alt_ioc->name));
>>  				CHIPREG_WRITE32(&ioc->alt_ioc->chip->IntMask, MPI_HIM_DIM);
>> -				ioc->alt_ioc->active = 1;
>> +				mpt_ioc_activate(ioc->alt_ioc);
>>  			}
>>  
>>  		} else {
>> @@ -1798,7 +1811,7 @@ mpt_do_ioc_recovery(MPT_ADAPTER *ioc, u3
>>  	if (ret == 0) {
>>  		/* Enable! (reply interrupt) */
>>  		CHIPREG_WRITE32(&ioc->chip->IntMask, MPI_HIM_DIM);
>> -		ioc->active = 1;
>> +		mpt_ioc_activate(ioc);
>>  	}
>>  
>>  	if (reset_alt_ioc_active && ioc->alt_ioc) {
>> @@ -1806,7 +1819,7 @@ mpt_do_ioc_recovery(MPT_ADAPTER *ioc, u3
>>  		dinitprintk((KERN_INFO MYNAM ": alt-%s reply irq re-enabled\n",
>>  				ioc->alt_ioc->name));
>>  		CHIPREG_WRITE32(&ioc->alt_ioc->chip->IntMask, MPI_HIM_DIM);
>> -		ioc->alt_ioc->active = 1;
>> +		mpt_ioc_activate(ioc->alt_ioc);
>>  	}
>>  
>>  	/*  Enable MPT base driver management of EventNotification
>> @@ -4069,7 +4082,6 @@ #endif
>>   *	Return: 0 for success
>>   *	-ENOMEM if no memory available
>>   *		-EPERM if not allowed due to ISR context
>> - *		-EAGAIN if no msg frames currently available
>>   *		-EFAULT for non-successful reply or no reply (timeout)
>>   */
>>  static int
>> @@ -4181,7 +4193,6 @@ GetLanConfigPages(MPT_ADAPTER *ioc)
>>   *	Return: 0 for success
>>   *	-ENOMEM if no memory available
>>   *		-EPERM if not allowed due to ISR context
>> - *		-EAGAIN if no msg frames currently available
>>   *		-EFAULT for non-successful reply or no reply (timeout)
>>   */
>>  int
>> @@ -4493,7 +4504,6 @@ mptbase_raid_process_event_data(MPT_ADAP
>>   *	Returns: 0 for success
>>   *	-ENOMEM if no memory available
>>   *		-EPERM if not allowed due to ISR context
>> - *		-EAGAIN if no msg frames currently available
>>   *		-EFAULT for non-successful reply or no reply (timeout)
>>   */
>>  static int
>> @@ -5156,7 +5166,6 @@ SendEventAck(MPT_ADAPTER *ioc, EventNoti
>>   *
>>   *	Returns 0 for success
>>   *	-EPERM if not allowed due to ISR context
>> - *	-EAGAIN if no msg frames currently available
>>   *	-EFAULT for non-successful reply or no reply (timeout)
>>   */
>>  int
>> @@ -5182,10 +5191,10 @@ mpt_config(MPT_ADAPTER *ioc, CONFIGPARMS
>>  
>>  	/* Get and Populate a free Frame
>>  	 */
>> -	if ((mf = mpt_get_msg_frame(mpt_base_index, ioc)) == NULL) {
>> +	while ((mf = mpt_get_msg_frame(mpt_base_index, ioc)) == NULL) {
>>  		dcprintk((MYIOC_s_WARN_FMT "mpt_config: no msg frames!\n",
>>  				ioc->name));
>> -		return -EAGAIN;
>> +		wait_event(mpt_config_waitq, pCfg->wait_done);
>>  	}
>>  	pReq = (Config_t *)mf;
>>  	pReq->Action = pCfg->action;
>>
>>
> -
> 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
> 
> 

RE: [REPOST][PATCH 1/6] mpt fusion - fibre channel target discovery prematurely terminates

[Moore, Eric]

On Wednesday, June 14, 2006 1:00 PM, Mike Reed wrote: 
> 
> James Bottomley wrote:
> > 
> > I was still thinking of something that made mpt_config() 
> never return
> > -EAGAIN.  How does the following work out?  It's my first pass at
> > sorting out the frame and active logic in mptbase:
> 
> 
> pCfg->wait_done needs to be initialized to zero before sleeping.
> 

agreed

> It takes seven to ten seconds for the ioc->active value to become
> non-zero and the wake up to occur.
> 
> It appears that the order of execution during "lsiutil 99" testing
> has changed.  Once the driver marks the fibre targets missing, they
> never return.  Hmmm....  Until this is resolved, this patch cannot
> be applied.  I'll see if I can sort out how the execution order
> has changed.

not clear what you mean by "they never return"

> 
> Another comment, for every i/o, this patch introduces overhead.
> spinlock and checking a list which is empty 99.99[...]% of the time.
> I'll have to perform an iop measurement to see if there is a
> measurable impact on system performance.
> 

What overhead?  We don't read config pages on every I/O.  James
patch only effects the reading of config pages when there is
a host reset.

One concern on this patch.  A possible deadlock situation could
occur if we run out of message frames.  Meaning there is another way
that  mpt_get_msg_frame() could return NULL, and that is becuase
the ioc->FreeQ list is empty.  This patch only addresses the case
when there is a host reset.   Utimatley we need to address both
cases when NULL is returned.  Id rather try to come  up with a
solution where the fix is isolated inside of mpt_get_msg_frame().

Eric Moore

Re: [REPOST][PATCH 1/6] mpt fusion - fibre channel target discovery prematurely terminates

[Michael Reed]

Moore, Eric wrote:
> On Wednesday, June 14, 2006 1:00 PM, Mike Reed wrote: 
>> James Bottomley wrote:
>>> I was still thinking of something that made mpt_config() 
>> never return
>>> -EAGAIN.  How does the following work out?  It's my first pass at
>>> sorting out the frame and active logic in mptbase:
> 
...snip...
> 
>> Another comment, for every i/o, this patch introduces overhead.
>> spinlock and checking a list which is empty 99.99[...]% of the time.
>> I'll have to perform an iop measurement to see if there is a
>> measurable impact on system performance.
>>
> 
> What overhead?  We don't read config pages on every I/O.  James
> patch only effects the reading of config pages when there is
> a host reset.

I think you're mistaken about James' patch.

Doesn't every i/o use a message frame?  James' patch to sleep on frame
exhaustion requires a wakeup every time a msg frame is released,
which is performed at the end of mpt_put_msg_frame().  Am I missing
something?


> 
> One concern on this patch.  A possible deadlock situation could
> occur if we run out of message frames.  Meaning there is another way
> that  mpt_get_msg_frame() could return NULL, and that is becuase
> the ioc->FreeQ list is empty.  This patch only addresses the case
> when there is a host reset.   Utimatley we need to address both
> cases when NULL is returned.  Id rather try to come  up with a
> solution where the fix is isolated inside of mpt_get_msg_frame().

James' code handles both frame exhaustion and ioc->active == 0.
But, the only caller to mpt_get_msg_frame() that now sleeps is
mpt_config().

Mike


> 
> Eric Moore
> 

RE: [REPOST][PATCH 1/6] mpt fusion - fibre channel target discovery prematurely terminates

[Moore, Eric]

On Thursday, June 15, 2006 8:51 AM, Michael Reed wrote: 

> > What overhead?  We don't read config pages on every I/O.  James
> > patch only effects the reading of config pages when there is
> > a host reset.
> 
> I think you're mistaken about James' patch.
> 
> Doesn't every i/o use a message frame?  James' patch to sleep on frame
> exhaustion requires a wakeup every time a msg frame is released,
> which is performed at the end of mpt_put_msg_frame().  Am I missing
> something?
> 
> 

Ok, my mistake.  I overlooked the wake_up() at the end of
mpt_put_msg_frame().

BTW, this is incorrect.  The wake_up() should been in
mpt_free_msg_frame(), 
instead of mpt_put_msg_frame().   The _put_ function is sending the mf
to 
the firmware.   The mf is not freed untill the command is completd by
firmware,
and the _free_function is called.  Meaning this issue can be solved by
moving 
wake_up() to mpt_free_msg_frame(), and only calling wake_up() when the
list_empty(&ioc->FreeQ) was empty going into this call.

Eric Moore

Re: [REPOST][PATCH 1/6] mpt fusion - fibre channel target discovery prematurely terminates

[Michael Reed]

Thanks for finding the problem.  I'll rework James' patch to take
into account your comments and give it a try.

Mike


Moore, Eric wrote:
> On Thursday, June 15, 2006 8:51 AM, Michael Reed wrote: 
> 
>>> What overhead?  We don't read config pages on every I/O.  James
>>> patch only effects the reading of config pages when there is
>>> a host reset.
>> I think you're mistaken about James' patch.
>>
>> Doesn't every i/o use a message frame?  James' patch to sleep on frame
>> exhaustion requires a wakeup every time a msg frame is released,
>> which is performed at the end of mpt_put_msg_frame().  Am I missing
>> something?
>>
>>
> 
> Ok, my mistake.  I overlooked the wake_up() at the end of
> mpt_put_msg_frame().
> 
> BTW, this is incorrect.  The wake_up() should been in
> mpt_free_msg_frame(), 
> instead of mpt_put_msg_frame().   The _put_ function is sending the mf
> to 
> the firmware.   The mf is not freed untill the command is completd by
> firmware,
> and the _free_function is called.  Meaning this issue can be solved by
> moving 
> wake_up() to mpt_free_msg_frame(), and only calling wake_up() when the
> list_empty(&ioc->FreeQ) was empty going into this call.
> 
> Eric Moore
> 

Re: [REPOST][PATCH 1/6] mpt fusion - fibre channel target discovery prematurely terminates

[Michael Reed]

There's another problem.  The routines doing the wakeup don't have
access to the sleep variable so the thread sleeps forever.

I'll summarize the problems in the original patch so they don't get
lost.
	1) sleep variable must be initialized to zero before sleep
	2) sleep variable must be set non-zero before wakeup
	3) wakeup should be in mpt_free_msg_frame, conditioned
	   on whether frames are exhausted, instead of in
	   mpt_put_msg_frame().  (keep mpt_ioc_activate())

That's probably all I can do on this today.  I'll be OOTO tomorrow
and next week.

Mike


Michael Reed wrote:
> Thanks for finding the problem.  I'll rework James' patch to take
> into account your comments and give it a try.
> 
> Mike
> 
> 
> Moore, Eric wrote:
>> On Thursday, June 15, 2006 8:51 AM, Michael Reed wrote: 
>>
>>>> What overhead?  We don't read config pages on every I/O.  James
>>>> patch only effects the reading of config pages when there is
>>>> a host reset.
>>> I think you're mistaken about James' patch.
>>>
>>> Doesn't every i/o use a message frame?  James' patch to sleep on frame
>>> exhaustion requires a wakeup every time a msg frame is released,
>>> which is performed at the end of mpt_put_msg_frame().  Am I missing
>>> something?
>>>
>>>
>> Ok, my mistake.  I overlooked the wake_up() at the end of
>> mpt_put_msg_frame().
>>
>> BTW, this is incorrect.  The wake_up() should been in
>> mpt_free_msg_frame(), 
>> instead of mpt_put_msg_frame().   The _put_ function is sending the mf
>> to 
>> the firmware.   The mf is not freed untill the command is completd by
>> firmware,
>> and the _free_function is called.  Meaning this issue can be solved by
>> moving 
>> wake_up() to mpt_free_msg_frame(), and only calling wake_up() when the
>> list_empty(&ioc->FreeQ) was empty going into this call.
>>
>> Eric Moore
>>
> -
> 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
> 
>