[PATCH 6/9] mpt fusion: error recovery improvements, and synchronizing internal commands

[PATCH 6/9] mpt fusion: error recovery improvements, and synchronizing internal commands

[Eric Moore]

1) cleanup ioc_reset callback handlers, introducing wrappers for synronizing error recovery (mpt_set_taskmgmt_in_progress_flag, mpt_clear_taskmgmt_in_progress_flag), as the fusion firmware only handles one task management request at a time.
2) set vtarget->deleted flag when devices have been removed.
3) mdr@sgi.com - fix's provided from SGI after testing with single threaded internal commands


Signed-off-by: Eric Moore <Eric.Moore@lsi.com>

--- b/drivers/message/fusion/mptfc.c	2007-09-17 16:35:48.000000000 -0600
+++ a/drivers/message/fusion/mptfc.c	2007-09-17 15:05:33.000000000 -0600
@@ -476,6 +476,7 @@ mptfc_register_dev(MPT_ADAPTER *ioc, int
 				if (vtarget) {
 					vtarget->id = pg0->CurrentTargetID;
 					vtarget->channel = pg0->CurrentBus;
+					vtarget->deleted = 0;
 				}
 			}
 			*((struct mptfc_rport_info **)rport->dd_data) = ri;
@@ -614,7 +615,6 @@ mptfc_slave_alloc(struct scsi_device *sd
 
 	hd = shost_priv(sdev->host);
 	ioc = hd->ioc;
-
 	vdevice = kzalloc(sizeof(VirtDevice), GFP_KERNEL);
 	if (!vdevice) {
 		printk(MYIOC_s_ERR_FMT "slave_alloc kmalloc(%zd) FAILED!\n",
@@ -944,11 +944,12 @@ start_over:
 	return rc;
 }
 
-static void
+static int
 mptfc_SetFcPortPage1_defaults(MPT_ADAPTER *ioc)
 {
 	int		ii;
 	FCPortPage1_t	*pp1;
+	int		rc;
 
 	#define MPTFC_FW_DEVICE_TIMEOUT	(1)
 	#define MPTFC_FW_IO_PEND_TIMEOUT (1)
@@ -956,8 +957,8 @@ mptfc_SetFcPortPage1_defaults(MPT_ADAPTE
 	#define OFF_FLAGS (MPI_FCPORTPAGE1_FLAGS_VERBOSE_RESCAN_EVENTS)
 
 	for (ii=0; ii<ioc->facts.NumberOfPorts; ii++) {
-		if (mptfc_GetFcPortPage1(ioc, ii) != 0)
-			continue;
+		if ((rc = mptfc_GetFcPortPage1(ioc, ii)) < 0)
+			return rc;
 		pp1 = ioc->fc_data.fc_port_page1[ii].data;
 		if ((pp1->InitiatorDeviceTimeout == MPTFC_FW_DEVICE_TIMEOUT)
 		 && (pp1->InitiatorIoPendTimeout == MPTFC_FW_IO_PEND_TIMEOUT)
@@ -968,8 +969,10 @@ mptfc_SetFcPortPage1_defaults(MPT_ADAPTE
 		pp1->InitiatorIoPendTimeout = MPTFC_FW_IO_PEND_TIMEOUT;
 		pp1->Flags &= ~OFF_FLAGS;
 		pp1->Flags |= ON_FLAGS;
-		mptfc_WriteFcPortPage1(ioc, ii);
+		if ((rc = mptfc_WriteFcPortPage1(ioc, ii)) < 0)
+			return rc;
 	}
+	return 0;
 }
 
 
@@ -1086,6 +1089,8 @@ mptfc_setup_reset(struct work_struct *wo
 		container_of(work, MPT_ADAPTER, fc_setup_reset_work);
 	u64			pn;
 	struct mptfc_rport_info *ri;
+	struct scsi_target      *starget;
+	VirtTarget              *vtarget;
 
 	/* reset about to happen, delete (block) all rports */
 	list_for_each_entry(ri, &ioc->fc_rports, list) {
@@ -1093,6 +1098,12 @@ mptfc_setup_reset(struct work_struct *wo
 			ri->flags &= ~MPT_RPORT_INFO_FLAGS_REGISTERED;
 			fc_remote_port_delete(ri->rport);	/* won't sleep */
 			ri->rport = NULL;
+			starget = ri->starget;
+			if (starget) {
+				vtarget = starget->hostdata;
+				if (vtarget)
+					vtarget->deleted = 1;
+			}
 
 			pn = (u64)ri->pg0.WWPN.High << 32 |
 			     (u64)ri->pg0.WWPN.Low;
@@ -1111,8 +1122,22 @@ mptfc_rescan_devices(struct work_struct 
 	MPT_ADAPTER		*ioc =
 		container_of(work, MPT_ADAPTER, fc_rescan_work);
 	int			ii;
+	int			rc;
 	u64			pn;
 	struct mptfc_rport_info *ri;
+	struct scsi_target      *starget;
+	VirtTarget              *vtarget;
+
+	/*
+	 * if cannot set defaults, something's really wrong, bail out
+	 */
+
+	if ((rc = mptfc_SetFcPortPage1_defaults(ioc)) < 0) {
+		dfcprintk (ioc, printk(MYIOC_s_DEBUG_FMT
+		    "mptfc_rescan.%d: unable to set PP1 defaults, rc %d.\n",
+		    ioc->name, ioc->sh->host_no, rc));
+		return;
+	}
 
 	/* start by tagging all ports as missing */
 	list_for_each_entry(ri, &ioc->fc_rports, list) {
@@ -1140,6 +1165,12 @@ mptfc_rescan_devices(struct work_struct 
 				       MPT_RPORT_INFO_FLAGS_MISSING);
 			fc_remote_port_delete(ri->rport);	/* won't sleep */
 			ri->rport = NULL;
+			starget = ri->starget;
+			if (starget) {
+				vtarget = starget->hostdata;
+				if (vtarget)
+					vtarget->deleted = 1;
+			}
 
 			pn = (u64)ri->pg0.WWPN.High << 32 |
 			     (u64)ri->pg0.WWPN.Low;
@@ -1292,30 +1323,6 @@ mptfc_probe(struct pci_dev *pdev, const 
 	dprintk(ioc, printk(MYIOC_s_DEBUG_FMT "ScsiLookup @ %p\n",
 		 ioc->name, ioc->ScsiLookup));
 
-	/* Clear the TM flags
-	 */
-	hd->tmPending = 0;
-	hd->tmState = TM_STATE_NONE;
-	hd->resetPending = 0;
-	hd->abortSCpnt = NULL;
-
-	/* Clear the pointer used to store
-	 * single-threaded commands, i.e., those
-	 * issued during a bus scan, dv and
-	 * configuration pages.
-	 */
-	hd->cmdPtr = NULL;
-
-	/* Initialize this SCSI Hosts' timers
-	 * To use, set the timer expires field
-	 * and add_timer
-	 */
-	init_timer(&hd->timer);
-	hd->timer.data = (unsigned long) hd;
-	hd->timer.function = mptscsih_timer_expired;
-
-	init_waitqueue_head(&hd->scandv_waitq);
-	hd->scandv_wait_done = 0;
 	hd->last_queue_full = 0;
 
 	sh->transportt = mptfc_transport_template;
@@ -1342,7 +1349,6 @@ mptfc_probe(struct pci_dev *pdev, const 
 	for (ii=0; ii < ioc->facts.NumberOfPorts; ii++) {
 		(void) mptfc_GetFcPortPage0(ioc, ii);
 	}
-	mptfc_SetFcPortPage1_defaults(ioc);
 
 	/*
 	 * scan for rports -
@@ -1380,9 +1386,6 @@ mptfc_event_process(MPT_ADAPTER *ioc, Ev
 	unsigned long flags;
 	int rc=1;
 
-	devtverboseprintk(ioc, printk(MYIOC_s_DEBUG_FMT "MPT event (=%02Xh) routed to SCSI host driver!\n",
-			ioc->name, event));
-
 	if (ioc->sh == NULL ||
 		((hd = shost_priv(ioc->sh)) == NULL))
 		return 1;
@@ -1418,35 +1421,36 @@ mptfc_ioc_reset(MPT_ADAPTER *ioc, int re
 	unsigned long	flags;
 
 	rc = mptscsih_ioc_reset(ioc,reset_phase);
-	if (rc == 0)
+	if ((ioc->bus_type != FC) || (!rc))
 		return rc;
 
-
-	dtmprintk(ioc, printk(MYIOC_s_DEBUG_FMT
-		": IOC %s_reset routed to FC host driver!\n",ioc->name,
-		reset_phase==MPT_IOC_SETUP_RESET ? "setup" : (
-		reset_phase==MPT_IOC_PRE_RESET ? "pre" : "post")));
-
-	if (reset_phase == MPT_IOC_SETUP_RESET) {
+	switch(reset_phase) {
+	case MPT_IOC_SETUP_RESET:
+		dtmprintk(ioc, printk(MYIOC_s_DEBUG_FMT
+		    "%s: MPT_IOC_SETUP_RESET\n", ioc->name, __FUNCTION__));
 		spin_lock_irqsave(&ioc->fc_rescan_work_lock, flags);
 		if (ioc->fc_rescan_work_q) {
 			queue_work(ioc->fc_rescan_work_q,
 				   &ioc->fc_setup_reset_work);
 		}
 		spin_unlock_irqrestore(&ioc->fc_rescan_work_lock, flags);
-	}
-
-	else if (reset_phase == MPT_IOC_PRE_RESET) {
-	}
-
-	else {	/* MPT_IOC_POST_RESET */
-		mptfc_SetFcPortPage1_defaults(ioc);
+		break;
+	case MPT_IOC_PRE_RESET:
+		dtmprintk(ioc, printk(MYIOC_s_DEBUG_FMT
+		    "%s: MPT_IOC_PRE_RESET\n", ioc->name, __FUNCTION__));
+		break;
+	case MPT_IOC_POST_RESET:
+		dtmprintk(ioc, printk(MYIOC_s_DEBUG_FMT
+		    "%s: MPT_IOC_POST_RESET\n",  ioc->name, __FUNCTION__));
 		spin_lock_irqsave(&ioc->fc_rescan_work_lock, flags);
 		if (ioc->fc_rescan_work_q) {
 			queue_work(ioc->fc_rescan_work_q,
 				   &ioc->fc_rescan_work);
 		}
 		spin_unlock_irqrestore(&ioc->fc_rescan_work_lock, flags);
+		break;
+	default:
+		break;
 	}
 	return 1;
 }

Re: [PATCH 6/9] mpt fusion: error recovery improvements, and synchronizing internal commands

[James Bottomley]

On Mon, 2007-09-17 at 19:59 -0600, Eric Moore wrote:
> 3) mdr@sgi.com - fix's provided from SGI after testing with single threaded internal commands

What fixes?

The object of a change log is to preserve the history of a particular
change (as per the Developer Certificate of Origin).  This means if you
get a patch from someone, you should also collect their signoff with it,
then you pass it on to me complete with your signoff added (and a note
if you had to modify it to fit it into the patch series or otherwise
alter it).

James

RE: [PATCH 6/9] mpt fusion: error recovery improvements, andsynchronizing internal commands

[Moore, Eric]

On Saturday, September 22, 2007 10:02 AM, James Bottomley  wrote: 

> What fixes?
> 
> The object of a change log is to preserve the history of a particular
> change (as per the Developer Certificate of Origin).  This 
> means if you
> get a patch from someone, you should also collect their 
> signoff with it,
> then you pass it on to me complete with your signoff added (and a note
> if you had to modify it to fit it into the patch series or otherwise
> alter it).
> 
> James
> 
> 
> 

James, Mike sent me an email back on June 13, contain 11 suggested
changes associated with his test efforts with my new code having rewrote
internal command error handling, and adding new polling function for
controller fault handling.   The changes he quoted associated to the
ones in mptfc seem vague.  I have the email I could forward, but at that
time, It probably doesnt matter.

Eric

RE: [PATCH 6/9] mpt fusion: error recovery improvements, andsynchronizing internal commands

[James Bottomley]

On Mon, 2007-09-24 at 19:35 -0600, Moore, Eric wrote:
> On Saturday, September 22, 2007 10:02 AM, James Bottomley  wrote: 
> 
> > What fixes?
> > 
> > The object of a change log is to preserve the history of a particular
> > change (as per the Developer Certificate of Origin).  This 
> > means if you
> > get a patch from someone, you should also collect their 
> > signoff with it,
> > then you pass it on to me complete with your signoff added (and a note
> > if you had to modify it to fit it into the patch series or otherwise
> > alter it).
> > 
> > James
> > 
> > 
> > 
> 
> James, Mike sent me an email back on June 13, contain 11 suggested
> changes associated with his test efforts with my new code having rewrote
> internal command error handling, and adding new polling function for
> controller fault handling.   The changes he quoted associated to the
> ones in mptfc seem vague.  I have the email I could forward, but at that
> time, It probably doesnt matter.

I just need a description that says what the patch is doing.  Fixes
provided by SGI isn't descriptive enough.

James

Re: [PATCH 6/9] mpt fusion: error recovery improvements, and synchronizing internal commands

[James Bottomley]

On Mon, 2007-09-17 at 19:59 -0600, Eric Moore wrote:
> 1) cleanup ioc_reset callback handlers, introducing wrappers for synronizing error recovery (mpt_set_taskmgmt_in_progress_flag, mpt_clear_taskmgmt_in_progress_flag), as the fusion firmware only handles one task management request at a time.
> 2) set vtarget->deleted flag when devices have been removed.
> 3) mdr@sgi.com - fix's provided from SGI after testing with single threaded internal commands

OK, I thought I'd wait here for the breakout, but in the meantime I
tried to compile the first five patches, but they don't:

  CC [M]  drivers/message/fusion/mptscsih.o
drivers/message/fusion/mptscsih.c: In function 'mptscsih_qcmd':
drivers/message/fusion/mptscsih.c:1357: error: 'MPT_SCSI_HOST' has no
member named 'resetPending'
drivers/message/fusion/mptscsih.c: In function 'mptscsih_TMHandler':
drivers/message/fusion/mptscsih.c:1554: error: 'MPT_ADAPTER' has no
member named 'diagLock'
...

The series can't be applied in this form, because if git bisect steps
into the middle of this, the compile will break (and hence the
bisection) and a lot of people will say a lot of nasty things.

A patch series really needs to be one patch per logical change (with
other peoples' patches broken out) in a form that is separately
compilable for each patch.  Additionally, with a separate change log and
summary (i.e. not four patches all saying "mpt fusion: error recovery
improvements, and synchronizing internal commands").

I'll back all of this out; can you resend the series conforming to the
above request?

Thanks,

James

RE: [PATCH 6/9] mpt fusion: error recovery improvements, andsynchronizing internal commands

[Moore, Eric]

On Saturday, September 22, 2007 10:23 AM,  James Bottomley wrote:

> 
> OK, I thought I'd wait here for the breakout, but in the meantime I
> tried to compile the first five patches, but they don't:
> 
>   CC [M]  drivers/message/fusion/mptscsih.o
> drivers/message/fusion/mptscsih.c: In function 'mptscsih_qcmd':
> drivers/message/fusion/mptscsih.c:1357: error: 'MPT_SCSI_HOST' has no
> member named 'resetPending'
> drivers/message/fusion/mptscsih.c: In function 'mptscsih_TMHandler':
> drivers/message/fusion/mptscsih.c:1554: error: 'MPT_ADAPTER' has no
> member named 'diagLock'
> ...

Its not going to compile if you didn't pull down all 6 patchs associated
to "error recovery improvements".   Those six patchs were divided by
sources.  Meaning patch 4 was mptbase.[h,c] changes, then patch5 was
mptctl.[c,h] changes, and so on.  So the changes assoicated for
mptscsich.c are in patch 8.   Since you didn't apply the 8th patch,
offcourse your going to get the errrors in mptscish.c.    All patchs 4-9
need to go togeather, as mentioned in the first patch 0.

> 
> The series can't be applied in this form, because if git bisect steps
> into the middle of this, the compile will break (and hence the
> bisection) and a lot of people will say a lot of nasty things.

> 
> A patch series really needs to be one patch per logical change (with
> other peoples' patches broken out) in a form that is separately
> compilable for each patch.  Additionally, with a separate 
> change log and
> summary (i.e. not four patches all saying "mpt fusion: error recovery
> improvements, and synchronizing internal commands").
>

I understand, and thats been going on for the past couple months,
starting with Sathya's patchs early August.   The changes associated
with the rewrite of internal commands, and errror recovery is rather
difficult to seperate due to many depandancies and new structure
changes, and my concerned risk of what small steps could introduce more
issues, so IMO, the least risk was jump starting you to the customer and
factory tested code. 
 
> I'll back all of this out; can you resend the series conforming to the
> above request?
> 

The first three patchs did conform that, which are:

1) adding usage of shost_priv and removing all the typecasting
2) Fixing sparse warnings
3) locking down ScsiLookup


Youve rejected the error recovery patchs, which is fair enough.  

RE: [PATCH 6/9] mpt fusion: error recovery improvements, andsynchronizing internal commands

[James Bottomley]

On Mon, 2007-09-24 at 19:26 -0600, Moore, Eric wrote:
> On Saturday, September 22, 2007 10:23 AM,  James Bottomley wrote:
> 
> > 
> > OK, I thought I'd wait here for the breakout, but in the meantime I
> > tried to compile the first five patches, but they don't:
> > 
> >   CC [M]  drivers/message/fusion/mptscsih.o
> > drivers/message/fusion/mptscsih.c: In function 'mptscsih_qcmd':
> > drivers/message/fusion/mptscsih.c:1357: error: 'MPT_SCSI_HOST' has no
> > member named 'resetPending'
> > drivers/message/fusion/mptscsih.c: In function 'mptscsih_TMHandler':
> > drivers/message/fusion/mptscsih.c:1554: error: 'MPT_ADAPTER' has no
> > member named 'diagLock'
> > ...
> 
> Its not going to compile if you didn't pull down all 6 patchs associated
> to "error recovery improvements".   Those six patchs were divided by
> sources.  Meaning patch 4 was mptbase.[h,c] changes, then patch5 was
> mptctl.[c,h] changes, and so on.  So the changes assoicated for
> mptscsich.c are in patch 8.   Since you didn't apply the 8th patch,
> offcourse your going to get the errrors in mptscish.c.    All patchs 4-9
> need to go togeather, as mentioned in the first patch 0.

If that's the case, and they can't be split up into logically separate
sub patches (each of which individually compiles), then they'll have to
go as a single patch.

> > 
> > The series can't be applied in this form, because if git bisect steps
> > into the middle of this, the compile will break (and hence the
> > bisection) and a lot of people will say a lot of nasty things.
> 
> > 
> > A patch series really needs to be one patch per logical change (with
> > other peoples' patches broken out) in a form that is separately
> > compilable for each patch.  Additionally, with a separate 
> > change log and
> > summary (i.e. not four patches all saying "mpt fusion: error recovery
> > improvements, and synchronizing internal commands").
> >
> 
> I understand, and thats been going on for the past couple months,
> starting with Sathya's patchs early August.   The changes associated
> with the rewrite of internal commands, and errror recovery is rather
> difficult to seperate due to many depandancies and new structure
> changes, and my concerned risk of what small steps could introduce more
> issues, so IMO, the least risk was jump starting you to the customer and
> factory tested code. 
>  
> > I'll back all of this out; can you resend the series conforming to the
> > above request?
> > 
> 
> The first three patchs did conform that, which are:
> 
> 1) adding usage of shost_priv and removing all the typecasting
> 2) Fixing sparse warnings
> 3) locking down ScsiLookup

OK ... I'll look at applying those.

> Youve rejected the error recovery patchs, which is fair enough.  

Just the separation ... the actual patch looks OK.

James

RE: [PATCH 6/9] mpt fusion: error recovery improvements,andsynchronizing internal commands

[Moore, Eric]

On Tuesday, September 25, 2007 11:32 AM,  James Bottomley  wrote:
 
> > Youve rejected the error recovery patchs, which is fair enough.  
> 
> Just the separation ... the actual patch looks OK.
> 

I'll will continue working to separate the "error recovery
improvements:" into smaller feature add, but will take some time.   I
want some constructive feedback, and too big of patch is deterring some
people from looking at it.

Thanks,
Eric 

Re: [PATCH 6/9] mpt fusion: error recovery improvements,andsynchronizing internal commands

[Jeff Garzik]

Moore, Eric wrote:
> On Tuesday, September 25, 2007 11:32 AM,  James Bottomley  wrote:
>  
>>> Youve rejected the error recovery patchs, which is fair enough.  
>> Just the separation ... the actual patch looks OK.
>>
> 
> I'll will continue working to separate the "error recovery
> improvements:" into smaller feature add, but will take some time.   I
> want some constructive feedback, and too big of patch is deterring some
> people from looking at it.

I think it's fair to break it up, as long as its clearly noted that the 
patch is for review only.

	Jeff

Re: [PATCH 6/9] mpt fusion: error recovery improvements,andsynchronizing internal commands

[Michael Reed]

Jeff Garzik wrote:
> Moore, Eric wrote:
>> On Tuesday, September 25, 2007 11:32 AM,  James Bottomley  wrote:
>>  
>>>> Youve rejected the error recovery patchs, which is fair enough.  
>>> Just the separation ... the actual patch looks OK.
>>>
>>
>> I'll will continue working to separate the "error recovery
>> improvements:" into smaller feature add, but will take some time.   I
>> want some constructive feedback, and too big of patch is deterring some
>> people from looking at it.
> 
> I think it's fair to break it up, as long as its clearly noted that the
> patch is for review only.

Any chance the changes, properly documented, could be accepted as
one big patch?  Breaking this code into smaller patches, when it's
really intended as a driver update, could prove problematic.

Mike


> 
>     Jeff
> 
> 
> 
> -
> 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: [PATCH 6/9] mpt fusion: error recovery improvements,andsynchronizing internal commands

[James Bottomley]

On Tue, 2007-09-25 at 17:38 -0500, Michael Reed wrote:
> Jeff Garzik wrote:
> > Moore, Eric wrote:
> >> On Tuesday, September 25, 2007 11:32 AM,  James Bottomley  wrote:
> >>  
> >>>> Youve rejected the error recovery patchs, which is fair enough.  
> >>> Just the separation ... the actual patch looks OK.
> >>>
> >>
> >> I'll will continue working to separate the "error recovery
> >> improvements:" into smaller feature add, but will take some time.   I
> >> want some constructive feedback, and too big of patch is deterring some
> >> people from looking at it.
> > 
> > I think it's fair to break it up, as long as its clearly noted that the
> > patch is for review only.
> 
> Any chance the changes, properly documented, could be accepted as
> one big patch?  Breaking this code into smaller patches, when it's
> really intended as a driver update, could prove problematic.

Yes, that's OK ... separate patches with signoffs are not absolute
requirements as long as the description and the heritage is in the
change log (although if you can do it, I prefer the separate signoffs
and distinct patches).

James

Re: [PATCH 6/9] mpt fusion: error recovery improvements,andsynchronizing internal commands

[Jeff Garzik]

Michael Reed wrote:
> Jeff Garzik wrote:
>> Moore, Eric wrote:
>>> On Tuesday, September 25, 2007 11:32 AM,  James Bottomley  wrote:
>>>  
>>>>> Youve rejected the error recovery patchs, which is fair enough.  
>>>> Just the separation ... the actual patch looks OK.
>>>>
>>> I'll will continue working to separate the "error recovery
>>> improvements:" into smaller feature add, but will take some time.   I
>>> want some constructive feedback, and too big of patch is deterring some
>>> people from looking at it.
>> I think it's fair to break it up, as long as its clearly noted that the
>> patch is for review only.
> 
> Any chance the changes, properly documented, could be accepted as
> one big patch?  Breaking this code into smaller patches, when it's
> really intended as a driver update, could prove problematic.

I'll let James provide an authoritative answer, but in general, that's a 
bad way to do Linux engineering.  The worst case would be appearing 
every month or two, posted a single, huge "driver update" patch.  That 
set of changes is bi-sectable, but it does not fully collect the changes 
within the driver update.

Some consolidation of changes is just plain natural, so you strike a 
balance.  Nothing but big driver-update patches?  Bad.  Four bazillion 
individual patches, each for a single whitespace change, cleanup, etc.? 
  Also unwise.  The true balance is somewhere in the middle.

The more important or intrusive the change, within the overall driver 
update, the more important it is to separate out that logical change.

	Jeff