RE: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in reset handler

RE: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in reset handler

[Ju, Seokmann]

Hi,

I've seen the patch (megaraid_mmmbox_fix_a_bug_in_reset_handler.patch) available on 2.6.17-rc1-mm3 under "SCSI warning fix" section.
What should I do to remove "warning" tag on the patch.
I've attached another patch in previous email that has 'udelay()' in the loop to remove NMI concern, and waiting for confirmation on it. Will this change remove the "warning"?

I'll submit the patch officially by end of today.

Any comment would be appreciated.

Thank you,


> -----Original Message-----
> From: Ju, Seokmann 
> Sent: Monday, April 17, 2006 9:13 AM
> To: 'Andre Hedrick'; Andrew Morton
> Cc: James.Bottomley@SteelEye.com; 
> linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org
> Subject: RE: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in 
> reset handler
> 
> Hi,
> 
> Thank you all for comment on the issue.
> From the comment, it looks having 'ndelay/udelay' would be 
> right way to address the issue.
> I've attached a patch for just review purpose.
> Once it confirmed, I'll post the patch officially.
> 
> Thank you,
> 
> > -----Original Message-----
> > From: Andre Hedrick [mailto:andre@linux-ide.org] 
> > Sent: Saturday, April 15, 2006 3:10 AM
> > To: Andrew Morton
> > Cc: Ju, Seokmann; Ju, Seokmann; James.Bottomley@SteelEye.com; 
> > linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org
> > Subject: Re: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in 
> > reset handler
> > 
> > 
> > Andrew,
> > 
> > This is real, and is a known bug which is 100% reproducable (sp).
> > There are other harry issues too, but this is as much as I can say.
> > 
> > cpu_relax() will not work, already tried some time ago.
> > 
> > 
> > Andre Hedrick
> > LAD Storage Consulting Group
> > 
> > On Wed, 12 Apr 2006, Andrew Morton wrote:
> > 
> > > "Ju, Seokmann" <Seokmann.Ju@lsil.com> wrote:
> > > >
> > > > This patch has fix for a bug in the 'megaraid_reset_handler()'.
> > > > 
> > > >  When abort failed, the driver gets reset handleer 
> > called. In the reset
> > > >  handler, driver calls 'scsi_done()' callback for same 
> > SCSI command
> > > >  packet (struct scsi_cmnd) multiple times if there are 
> > multiple SCSI
> > > >  command packet in the pend_list. More over, if there are 
> > entry in the
> > > >  pend_lsit with IOCTL packet associated, the driver 
> > returns it to wrong
> > > >  free_list so that, in turn, the driver could end up with 
> > 'NULL pointer
> > > >  dereference..' during I/O command building with 
> > incorrect resource.
> > > > 
> > > >  Also, the patch contains several minor/cosmetic changes 
> > besides this.
> > > >
> > > > ..
> > > >
> > > > @@ -2655,32 +2655,48 @@
> > > >  	// Also, reset all the commands currently owned 
> by the driver
> > > >  	spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
> > > >  	list_for_each_entry_safe(scb, tmp, 
> &adapter->pend_list, list) {
> > > > -
> > > >  		list_del_init(&scb->list);	// from 
> pending list
> > > >  
> > > > -		con_log(CL_ANN, (KERN_WARNING
> > > > -			"megaraid: %ld:%d[%d:%d], reset from 
> > pending list\n",
> > > > -				scp->serial_number, scb->sno,
> > > > -				scb->dev_channel, 
> scb->dev_target));
> > > > +		if (scb->sno >= MBOX_MAX_SCSI_CMDS) {
> > > > +			con_log(CL_ANN, (KERN_WARNING 
> > > > +			"megaraid: IOCTL packet with %d[%d:%d] 
> > being reset\n",
> > > > +			scb->sno, scb->dev_channel, 
> scb->dev_target));
> > > >  
> > > > -		scp->result = (DID_RESET << 16);
> > > > -		scp->scsi_done(scp);
> > > > +			scb->status = -EFAULT;
> > > 
> > > What is the significance of -EFAULT here?  Seems inappropriate?
> > > 
> > > > @@ -2918,12 +2933,12 @@
> > > >  	wmb();
> > > >  	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
> > > >  
> > > > -	for (i = 0; i < 0xFFFFF; i++) {
> > > > +	for (i = 0; i < 0xFFFFFF; i++) {
> > > >  		if (mbox->numstatus != 0xFF) break;
> > > >  		rmb();
> > > >  	}
> > > 
> > > Oh my.  That's an awfully long interrupts-off spin.  1.7e7 
> > operations with
> > > an NMI watchdog timeout of five seconds - I'm surprised it 
> > doesn't trigger.
> > > 
> > > Is that reading from a PCI register there?   Or main memory?
> > > 
> > > I'm somewhat surprised that the compiler never "optimises" 
> > this into a
> > > lockup, actually.  That's what `volatile' is for.
> > > 
> > > Is it not possible to do this with an interrupt?
> > > 
> > > A `cpu_relax()' in that loop would help cool things down a bit.
> > > 
> > > 
> > > -
> > > 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 1/1] megaraid_{mm,mbox}: fix a bug in reset handler

[Andre Hedrick]

Seokmann,

In the case where the card or the host issues a pci master abort that
wedges the adapter and marks adapter->hw_error = 1 and then never recovers
again, where is the code to bring the card back online?  I know first hand
this is a problem because I am working with the firmware folk to address
this issue.

Also please explain how changing the number of times one loops from 5 F's
to 6 F's allows the FW to continue?  I am betting it I take the new driver
and the firmware posted on LSI's websight it will still crash in the same
conditions.  Anybody up for lunch paid by the non-winning party?  I have
not tested it yet but everything I see does not address the fundamental
issues.

Cheers,

Andre Hedrick
LAD Storage Consulting Group

On Tue, 18 Apr 2006, Ju, Seokmann wrote:

> Hi,
> 
> I've seen the patch (megaraid_mmmbox_fix_a_bug_in_reset_handler.patch) available on 2.6.17-rc1-mm3 under "SCSI warning fix" section.
> What should I do to remove "warning" tag on the patch.
> I've attached another patch in previous email that has 'udelay()' in the loop to remove NMI concern, and waiting for confirmation on it. Will this change remove the "warning"?
> 
> I'll submit the patch officially by end of today.
> 
> Any comment would be appreciated.
> 
> Thank you,
> 
> 
> > -----Original Message-----
> > From: Ju, Seokmann 
> > Sent: Monday, April 17, 2006 9:13 AM
> > To: 'Andre Hedrick'; Andrew Morton
> > Cc: James.Bottomley@SteelEye.com; 
> > linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org
> > Subject: RE: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in 
> > reset handler
> > 
> > Hi,
> > 
> > Thank you all for comment on the issue.
> > From the comment, it looks having 'ndelay/udelay' would be 
> > right way to address the issue.
> > I've attached a patch for just review purpose.
> > Once it confirmed, I'll post the patch officially.
> > 
> > Thank you,
> > 
> > > -----Original Message-----
> > > From: Andre Hedrick [mailto:andre@linux-ide.org] 
> > > Sent: Saturday, April 15, 2006 3:10 AM
> > > To: Andrew Morton
> > > Cc: Ju, Seokmann; Ju, Seokmann; James.Bottomley@SteelEye.com; 
> > > linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org
> > > Subject: Re: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in 
> > > reset handler
> > > 
> > > 
> > > Andrew,
> > > 
> > > This is real, and is a known bug which is 100% reproducable (sp).
> > > There are other harry issues too, but this is as much as I can say.
> > > 
> > > cpu_relax() will not work, already tried some time ago.
> > > 
> > > 
> > > Andre Hedrick
> > > LAD Storage Consulting Group
> > > 
> > > On Wed, 12 Apr 2006, Andrew Morton wrote:
> > > 
> > > > "Ju, Seokmann" <Seokmann.Ju@lsil.com> wrote:
> > > > >
> > > > > This patch has fix for a bug in the 'megaraid_reset_handler()'.
> > > > > 
> > > > >  When abort failed, the driver gets reset handleer 
> > > called. In the reset
> > > > >  handler, driver calls 'scsi_done()' callback for same 
> > > SCSI command
> > > > >  packet (struct scsi_cmnd) multiple times if there are 
> > > multiple SCSI
> > > > >  command packet in the pend_list. More over, if there are 
> > > entry in the
> > > > >  pend_lsit with IOCTL packet associated, the driver 
> > > returns it to wrong
> > > > >  free_list so that, in turn, the driver could end up with 
> > > 'NULL pointer
> > > > >  dereference..' during I/O command building with 
> > > incorrect resource.
> > > > > 
> > > > >  Also, the patch contains several minor/cosmetic changes 
> > > besides this.
> > > > >
> > > > > ..
> > > > >
> > > > > @@ -2655,32 +2655,48 @@
> > > > >  	// Also, reset all the commands currently owned 
> > by the driver
> > > > >  	spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
> > > > >  	list_for_each_entry_safe(scb, tmp, 
> > &adapter->pend_list, list) {
> > > > > -
> > > > >  		list_del_init(&scb->list);	// from 
> > pending list
> > > > >  
> > > > > -		con_log(CL_ANN, (KERN_WARNING
> > > > > -			"megaraid: %ld:%d[%d:%d], reset from 
> > > pending list\n",
> > > > > -				scp->serial_number, scb->sno,
> > > > > -				scb->dev_channel, 
> > scb->dev_target));
> > > > > +		if (scb->sno >= MBOX_MAX_SCSI_CMDS) {
> > > > > +			con_log(CL_ANN, (KERN_WARNING 
> > > > > +			"megaraid: IOCTL packet with %d[%d:%d] 
> > > being reset\n",
> > > > > +			scb->sno, scb->dev_channel, 
> > scb->dev_target));
> > > > >  
> > > > > -		scp->result = (DID_RESET << 16);
> > > > > -		scp->scsi_done(scp);
> > > > > +			scb->status = -EFAULT;
> > > > 
> > > > What is the significance of -EFAULT here?  Seems inappropriate?
> > > > 
> > > > > @@ -2918,12 +2933,12 @@
> > > > >  	wmb();
> > > > >  	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
> > > > >  
> > > > > -	for (i = 0; i < 0xFFFFF; i++) {
> > > > > +	for (i = 0; i < 0xFFFFFF; i++) {
> > > > >  		if (mbox->numstatus != 0xFF) break;
> > > > >  		rmb();
> > > > >  	}
> > > > 
> > > > Oh my.  That's an awfully long interrupts-off spin.  1.7e7 
> > > operations with
> > > > an NMI watchdog timeout of five seconds - I'm surprised it 
> > > doesn't trigger.
> > > > 
> > > > Is that reading from a PCI register there?   Or main memory?
> > > > 
> > > > I'm somewhat surprised that the compiler never "optimises" 
> > > this into a
> > > > lockup, actually.  That's what `volatile' is for.
> > > > 
> > > > Is it not possible to do this with an interrupt?
> > > > 
> > > > A `cpu_relax()' in that loop would help cool things down a bit.
> > > > 
> > > > 
> > > > -
> > > > 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 1/1] megaraid_{mm,mbox}: fix a bug in reset handler

[Andrew Morton]

"Ju, Seokmann" <Seokmann.Ju@lsil.com> wrote:
>
> I've seen the patch (megaraid_mmmbox_fix_a_bug_in_reset_handler.patch) available on 2.6.17-rc1-mm3 under "SCSI warning fix" section.
>  What should I do to remove "warning" tag on the patch.
>  I've attached another patch in previous email that has 'udelay()' in the loop to remove NMI concern, and waiting for confirmation on it. Will this change remove the "warning"?
> 
>  I'll submit the patch officially by end of today.

There are four megaraid-specific patches in -mm:

megaraid-unused-variable.patch
drivers-scsi-megaraidc-add-a-dummy-mega_create_proc_entry-for-proc_fs=y.patch
scsi-megaraid-megaraid_mmc-fix-a-null-pointer-dereference.patch
megaraid_mmmbox-fix-a-bug-in-reset-handler.patch

The final one is your latest patch.

I'll periodically send such patches to the subsystem maintainer until
something happens.  

RE: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in reset handler

[Ju, Seokmann]

Hi Andre,

> In the case where the card or the host issues a pci master abort that
> wedges the adapter and marks adapter->hw_error = 1 and then 
> never recovers
> again, where is the code to bring the card back online?  I 
> know first hand
> this is a problem because I am working with the firmware folk 
> to address
> this issue.
That is because driver made decision to announce the controller as dead after waiting for the F/W honored timeout value which is 300 seconds.
As long as F/W comes back before the timeout, hw_error flag never set to 1.
Once it pass the timeout, it means that the F/W will not come back.

> Also please explain how changing the number of times one 
> loops from 5 F's
> to 6 F's allows the FW to continue?  I am betting it I take 
> the new driver
> and the firmware posted on LSI's websight it will still crash 
> in the same
> conditions.  Anybody up for lunch paid by the non-winning 
> party?  I have
> not tested it yet but everything I see does not address the 
> fundamental
> issues.
This is one of request made by developer and you can track down further from the link specified in the patch ( change history 2 in ChangeLog.megaraid file).
Basically, under certain situation which the customer has, the F/W took longer than usual so that driver could exits from the loop before the command (for clustering support) returns. To address this with minimal changes in the driver, I've accepted the request.
Also, as Andrew pointed out, I've modified this section of the code and added 'udelay()' so that there is NO possible NMI concern. I'm not sure what make you to think the system will crash. Can you please explain to me?

Thank you,

> -----Original Message-----
> From: Andre Hedrick [mailto:andre@linux-ide.org] 
> Sent: Tuesday, April 18, 2006 12:33 PM
> To: Ju, Seokmann
> Cc: Ju, Seokmann; Andrew Morton; 
> James.Bottomley@SteelEye.com; linux-kernel@vger.kernel.org; 
> linux-scsi@vger.kernel.org
> Subject: RE: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in 
> reset handler
> 
> 
> Seokmann,
> 
> In the case where the card or the host issues a pci master abort that
> wedges the adapter and marks adapter->hw_error = 1 and then 
> never recovers
> again, where is the code to bring the card back online?  I 
> know first hand
> this is a problem because I am working with the firmware folk 
> to address
> this issue.
> 
> Also please explain how changing the number of times one 
> loops from 5 F's
> to 6 F's allows the FW to continue?  I am betting it I take 
> the new driver
> and the firmware posted on LSI's websight it will still crash 
> in the same
> conditions.  Anybody up for lunch paid by the non-winning 
> party?  I have
> not tested it yet but everything I see does not address the 
> fundamental
> issues.
> 
> Cheers,
> 
> Andre Hedrick
> LAD Storage Consulting Group
> 
> On Tue, 18 Apr 2006, Ju, Seokmann wrote:
> 
> > Hi,
> > 
> > I've seen the patch 
> (megaraid_mmmbox_fix_a_bug_in_reset_handler.patch) available 
> on 2.6.17-rc1-mm3 under "SCSI warning fix" section.
> > What should I do to remove "warning" tag on the patch.
> > I've attached another patch in previous email that has 
> 'udelay()' in the loop to remove NMI concern, and waiting for 
> confirmation on it. Will this change remove the "warning"?
> > 
> > I'll submit the patch officially by end of today.
> > 
> > Any comment would be appreciated.
> > 
> > Thank you,
> > 
> > 
> > > -----Original Message-----
> > > From: Ju, Seokmann 
> > > Sent: Monday, April 17, 2006 9:13 AM
> > > To: 'Andre Hedrick'; Andrew Morton
> > > Cc: James.Bottomley@SteelEye.com; 
> > > linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org
> > > Subject: RE: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in 
> > > reset handler
> > > 
> > > Hi,
> > > 
> > > Thank you all for comment on the issue.
> > > From the comment, it looks having 'ndelay/udelay' would be 
> > > right way to address the issue.
> > > I've attached a patch for just review purpose.
> > > Once it confirmed, I'll post the patch officially.
> > > 
> > > Thank you,
> > > 
> > > > -----Original Message-----
> > > > From: Andre Hedrick [mailto:andre@linux-ide.org] 
> > > > Sent: Saturday, April 15, 2006 3:10 AM
> > > > To: Andrew Morton
> > > > Cc: Ju, Seokmann; Ju, Seokmann; James.Bottomley@SteelEye.com; 
> > > > linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org
> > > > Subject: Re: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in 
> > > > reset handler
> > > > 
> > > > 
> > > > Andrew,
> > > > 
> > > > This is real, and is a known bug which is 100% 
> reproducable (sp).
> > > > There are other harry issues too, but this is as much 
> as I can say.
> > > > 
> > > > cpu_relax() will not work, already tried some time ago.
> > > > 
> > > > 
> > > > Andre Hedrick
> > > > LAD Storage Consulting Group
> > > > 
> > > > On Wed, 12 Apr 2006, Andrew Morton wrote:
> > > > 
> > > > > "Ju, Seokmann" <Seokmann.Ju@lsil.com> wrote:
> > > > > >
> > > > > > This patch has fix for a bug in the 
> 'megaraid_reset_handler()'.
> > > > > > 
> > > > > >  When abort failed, the driver gets reset handleer 
> > > > called. In the reset
> > > > > >  handler, driver calls 'scsi_done()' callback for same 
> > > > SCSI command
> > > > > >  packet (struct scsi_cmnd) multiple times if there are 
> > > > multiple SCSI
> > > > > >  command packet in the pend_list. More over, if there are 
> > > > entry in the
> > > > > >  pend_lsit with IOCTL packet associated, the driver 
> > > > returns it to wrong
> > > > > >  free_list so that, in turn, the driver could end up with 
> > > > 'NULL pointer
> > > > > >  dereference..' during I/O command building with 
> > > > incorrect resource.
> > > > > > 
> > > > > >  Also, the patch contains several minor/cosmetic changes 
> > > > besides this.
> > > > > >
> > > > > > ..
> > > > > >
> > > > > > @@ -2655,32 +2655,48 @@
> > > > > >  	// Also, reset all the commands currently owned 
> > > by the driver
> > > > > >  	spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
> > > > > >  	list_for_each_entry_safe(scb, tmp, 
> > > &adapter->pend_list, list) {
> > > > > > -
> > > > > >  		list_del_init(&scb->list);	// from 
> > > pending list
> > > > > >  
> > > > > > -		con_log(CL_ANN, (KERN_WARNING
> > > > > > -			"megaraid: %ld:%d[%d:%d], reset from 
> > > > pending list\n",
> > > > > > -				scp->serial_number, scb->sno,
> > > > > > -				scb->dev_channel, 
> > > scb->dev_target));
> > > > > > +		if (scb->sno >= MBOX_MAX_SCSI_CMDS) {
> > > > > > +			con_log(CL_ANN, (KERN_WARNING 
> > > > > > +			"megaraid: IOCTL packet with %d[%d:%d] 
> > > > being reset\n",
> > > > > > +			scb->sno, scb->dev_channel, 
> > > scb->dev_target));
> > > > > >  
> > > > > > -		scp->result = (DID_RESET << 16);
> > > > > > -		scp->scsi_done(scp);
> > > > > > +			scb->status = -EFAULT;
> > > > > 
> > > > > What is the significance of -EFAULT here?  Seems 
> inappropriate?
> > > > > 
> > > > > > @@ -2918,12 +2933,12 @@
> > > > > >  	wmb();
> > > > > >  	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
> > > > > >  
> > > > > > -	for (i = 0; i < 0xFFFFF; i++) {
> > > > > > +	for (i = 0; i < 0xFFFFFF; i++) {
> > > > > >  		if (mbox->numstatus != 0xFF) break;
> > > > > >  		rmb();
> > > > > >  	}
> > > > > 
> > > > > Oh my.  That's an awfully long interrupts-off spin.  1.7e7 
> > > > operations with
> > > > > an NMI watchdog timeout of five seconds - I'm surprised it 
> > > > doesn't trigger.
> > > > > 
> > > > > Is that reading from a PCI register there?   Or main memory?
> > > > > 
> > > > > I'm somewhat surprised that the compiler never "optimises" 
> > > > this into a
> > > > > lockup, actually.  That's what `volatile' is for.
> > > > > 
> > > > > Is it not possible to do this with an interrupt?
> > > > > 
> > > > > A `cpu_relax()' in that loop would help cool things 
> down a bit.
> > > > > 
> > > > > 
> > > > > -
> > > > > 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 1/1] megaraid_{mm,mbox}: fix a bug in reset handler

[Ju, Seokmann]

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

Hi,

Thank you all for comment on the issue.
>From the comment, it looks having 'ndelay/udelay' would be right way to address the issue.
I've attached a patch for just review purpose.
Once it confirmed, I'll post the patch officially.

Thank you,

> -----Original Message-----
> From: Andre Hedrick [mailto:andre@linux-ide.org] 
> Sent: Saturday, April 15, 2006 3:10 AM
> To: Andrew Morton
> Cc: Ju, Seokmann; Ju, Seokmann; James.Bottomley@SteelEye.com; 
> linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in 
> reset handler
> 
> 
> Andrew,
> 
> This is real, and is a known bug which is 100% reproducable (sp).
> There are other harry issues too, but this is as much as I can say.
> 
> cpu_relax() will not work, already tried some time ago.
> 
> 
> Andre Hedrick
> LAD Storage Consulting Group
> 
> On Wed, 12 Apr 2006, Andrew Morton wrote:
> 
> > "Ju, Seokmann" <Seokmann.Ju@lsil.com> wrote:
> > >
> > > This patch has fix for a bug in the 'megaraid_reset_handler()'.
> > > 
> > >  When abort failed, the driver gets reset handleer 
> called. In the reset
> > >  handler, driver calls 'scsi_done()' callback for same 
> SCSI command
> > >  packet (struct scsi_cmnd) multiple times if there are 
> multiple SCSI
> > >  command packet in the pend_list. More over, if there are 
> entry in the
> > >  pend_lsit with IOCTL packet associated, the driver 
> returns it to wrong
> > >  free_list so that, in turn, the driver could end up with 
> 'NULL pointer
> > >  dereference..' during I/O command building with 
> incorrect resource.
> > > 
> > >  Also, the patch contains several minor/cosmetic changes 
> besides this.
> > >
> > > ..
> > >
> > > @@ -2655,32 +2655,48 @@
> > >  	// Also, reset all the commands currently owned by the driver
> > >  	spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
> > >  	list_for_each_entry_safe(scb, tmp, &adapter->pend_list, list) {
> > > -
> > >  		list_del_init(&scb->list);	// from pending list
> > >  
> > > -		con_log(CL_ANN, (KERN_WARNING
> > > -			"megaraid: %ld:%d[%d:%d], reset from 
> pending list\n",
> > > -				scp->serial_number, scb->sno,
> > > -				scb->dev_channel, scb->dev_target));
> > > +		if (scb->sno >= MBOX_MAX_SCSI_CMDS) {
> > > +			con_log(CL_ANN, (KERN_WARNING 
> > > +			"megaraid: IOCTL packet with %d[%d:%d] 
> being reset\n",
> > > +			scb->sno, scb->dev_channel, scb->dev_target));
> > >  
> > > -		scp->result = (DID_RESET << 16);
> > > -		scp->scsi_done(scp);
> > > +			scb->status = -EFAULT;
> > 
> > What is the significance of -EFAULT here?  Seems inappropriate?
> > 
> > > @@ -2918,12 +2933,12 @@
> > >  	wmb();
> > >  	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
> > >  
> > > -	for (i = 0; i < 0xFFFFF; i++) {
> > > +	for (i = 0; i < 0xFFFFFF; i++) {
> > >  		if (mbox->numstatus != 0xFF) break;
> > >  		rmb();
> > >  	}
> > 
> > Oh my.  That's an awfully long interrupts-off spin.  1.7e7 
> operations with
> > an NMI watchdog timeout of five seconds - I'm surprised it 
> doesn't trigger.
> > 
> > Is that reading from a PCI register there?   Or main memory?
> > 
> > I'm somewhat surprised that the compiler never "optimises" 
> this into a
> > lockup, actually.  That's what `volatile' is for.
> > 
> > Is it not possible to do this with an interrupt?
> > 
> > A `cpu_relax()' in that loop would help cool things down a bit.
> > 
> > 
> > -
> > 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
> > 
> 
> 

[-- Attachment #2: kernel.patch --]
[-- Type: application/octet-stream, Size: 7023 bytes --]

diff -Naur old/Documentation/scsi/ChangeLog.megaraid new/Documentation/scsi/ChangeLog.megaraid
--- old/Documentation/scsi/ChangeLog.megaraid	2006-04-10 18:04:04.000000000 -0400
+++ new/Documentation/scsi/ChangeLog.megaraid	2006-04-12 09:35:20.000000000 -0400
@@ -1,3 +1,28 @@
+Release Date	: Mon Apr 11 12:27:22 EST 2006 - Seokmann Ju <sju@lsil.com>
+Current Version : 2.20.4.8 (scsi module), 2.20.2.6 (cmm module)
+Older Version	: 2.20.4.7 (scsi module), 2.20.2.6 (cmm module)
+
+1.	Fixed a bug in megaraid_reset_handler().
+	Customer reported "Unable to handle kernel NULL pointer dereference
+	at virtual address 00000000" when system goes to reset condition
+	for some reason. It happened randomly.
+	Root Cause: in the megaraid_reset_handler(), there is possibility not 
+	returning pending packets in the pend_list if there are multiple
+	pending packets. 
+	Fix: Made the change in the driver so that it will return all packets 
+	in the pend_list.
+
+2.	Added change request.
+	As found in the following URL, rmb() only didn't help the
+	problem. I had to increase the loop counter to 0xFFFFFF. (6 F's)
+	http://marc.theaimsgroup.com/?l=linux-scsi&m=110971060502497&w=2
+ 
+	I attached a patch for your reference, too.
+	Could you check and get this fix in your driver?
+ 
+	Best Regards,
+	Jun'ichi Nomura
+
 Release Date	: Fri Nov 11 12:27:22 EST 2005 - Seokmann Ju <sju@lsil.com>
 Current Version : 2.20.4.7 (scsi module), 2.20.2.6 (cmm module)
 Older Version	: 2.20.4.6 (scsi module), 2.20.2.6 (cmm module)
diff -Naur old/drivers/scsi/megaraid/megaraid_mbox.c new/drivers/scsi/megaraid/megaraid_mbox.c
--- old/drivers/scsi/megaraid/megaraid_mbox.c	2006-04-10 17:14:22.000000000 -0400
+++ new/drivers/scsi/megaraid/megaraid_mbox.c	2006-04-13 16:46:53.677687368 -0400
@@ -10,7 +10,7 @@
  *	   2 of the License, or (at your option) any later version.
  *
  * FILE		: megaraid_mbox.c
- * Version	: v2.20.4.7 (Nov 14 2005)
+ * Version	: v2.20.4.8 (Apr 11 2006)
  *
  * Authors:
  * 	Atul Mukker		<Atul.Mukker@lsil.com>
@@ -2278,6 +2278,7 @@
 	unsigned long		flags;
 	uint8_t			c;
 	int			status;
+	uioc_t			*kioc;
 
 
 	if (!adapter) return;
@@ -2320,6 +2321,9 @@
 			// remove from local clist
 			list_del_init(&scb->list);
 
+			kioc			= (uioc_t *)scb->gp;
+			kioc->status		= 0;
+
 			megaraid_mbox_mm_done(adapter, scb);
 
 			continue;
@@ -2636,6 +2640,7 @@
 	int		recovery_window;
 	int		recovering;
 	int		i;
+	uioc_t		*kioc;
 
 	adapter		= SCP2ADAPTER(scp);
 	raid_dev	= ADAP2RAIDDEV(adapter);
@@ -2655,32 +2660,51 @@
 	// Also, reset all the commands currently owned by the driver
 	spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
 	list_for_each_entry_safe(scb, tmp, &adapter->pend_list, list) {
-
 		list_del_init(&scb->list);	// from pending list
 
-		con_log(CL_ANN, (KERN_WARNING
-			"megaraid: %ld:%d[%d:%d], reset from pending list\n",
-				scp->serial_number, scb->sno,
-				scb->dev_channel, scb->dev_target));
+		if (scb->sno >= MBOX_MAX_SCSI_CMDS) {
+			con_log(CL_ANN, (KERN_WARNING 
+			"megaraid: IOCTL packet with %d[%d:%d] being reset\n",
+			scb->sno, scb->dev_channel, scb->dev_target));
 
-		scp->result = (DID_RESET << 16);
-		scp->scsi_done(scp);
+			scb->status = -1;
 
-		megaraid_dealloc_scb(adapter, scb);
+			kioc			= (uioc_t *)scb->gp;
+			kioc->status		= -EFAULT;
+
+			megaraid_mbox_mm_done(adapter, scb);
+		} else {
+			if (scb->scp == scp) {	// Found command
+				con_log(CL_ANN, (KERN_WARNING
+					"megaraid: %ld:%d[%d:%d], reset from pending list\n",
+					scp->serial_number, scb->sno,
+					scb->dev_channel, scb->dev_target));
+			} else {
+				con_log(CL_ANN, (KERN_WARNING 
+				"megaraid: IO packet with %d[%d:%d] being reset\n",
+				scb->sno, scb->dev_channel, scb->dev_target));
+			}
+
+			scb->scp->result = (DID_RESET << 16);
+			scb->scp->scsi_done(scb->scp);
+
+			megaraid_dealloc_scb(adapter, scb);
+		}
 	}
 	spin_unlock_irqrestore(PENDING_LIST_LOCK(adapter), flags);
 
 	if (adapter->outstanding_cmds) {
 		con_log(CL_ANN, (KERN_NOTICE
 			"megaraid: %d outstanding commands. Max wait %d sec\n",
-			adapter->outstanding_cmds, MBOX_RESET_WAIT));
+			adapter->outstanding_cmds, 
+			(MBOX_RESET_WAIT + MBOX_RESET_EXT_WAIT)));
 	}
 
 	recovery_window = MBOX_RESET_WAIT + MBOX_RESET_EXT_WAIT;
 
 	recovering = adapter->outstanding_cmds;
 
-	for (i = 0; i < recovery_window && adapter->outstanding_cmds; i++) {
+	for (i = 0; i < recovery_window; i++) {
 
 		megaraid_ack_sequence(adapter);
 
@@ -2689,12 +2713,11 @@
 			con_log(CL_ANN, (
 			"megaraid mbox: Wait for %d commands to complete:%d\n",
 				adapter->outstanding_cmds,
-				MBOX_RESET_WAIT - i));
+				(MBOX_RESET_WAIT + MBOX_RESET_EXT_WAIT) - i));
 		}
 
 		// bailout if no recovery happended in reset time
-		if ((i == MBOX_RESET_WAIT) &&
-			(recovering == adapter->outstanding_cmds)) {
+		if (adapter->outstanding_cmds == 0) {
 			break;
 		}
 
@@ -2918,12 +2941,13 @@
 	wmb();
 	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
 
-	for (i = 0; i < 0xFFFFF; i++) {
+	for (i = 0; i < MBOX_SYNC_WAIT_CNT; i++) {
 		if (mbox->numstatus != 0xFF) break;
 		rmb();
+		udelay(MBOX_SYNC_DELAY_200);
 	}
 
-	if (i == 0xFFFFF) {
+	if (i == MBOX_SYNC_WAIT_CNT) {
 		// We may need to re-calibrate the counter
 		con_log(CL_ANN, (KERN_CRIT
 			"megaraid: fast sync command timed out\n"));
@@ -3475,7 +3499,7 @@
 	adp.drvr_data		= (unsigned long)adapter;
 	adp.pdev		= adapter->pdev;
 	adp.issue_uioc		= megaraid_mbox_mm_handler;
-	adp.timeout		= 300;
+	adp.timeout		= MBOX_RESET_WAIT + MBOX_RESET_EXT_WAIT;
 	adp.max_kioc		= MBOX_MAX_USER_CMDS;
 
 	if ((rval = mraid_mm_register_adp(&adp)) != 0) {
@@ -3702,7 +3726,6 @@
 	unsigned long		flags;
 
 	kioc			= (uioc_t *)scb->gp;
-	kioc->status		= 0;
 	mbox64			= (mbox64_t *)(unsigned long)kioc->cmdbuf;
 	mbox64->mbox32.status	= scb->status;
 	raw_mbox		= (uint8_t *)&mbox64->mbox32;
diff -Naur old/drivers/scsi/megaraid/megaraid_mbox.h new/drivers/scsi/megaraid/megaraid_mbox.h
--- old/drivers/scsi/megaraid/megaraid_mbox.h	2006-04-10 17:14:22.000000000 -0400
+++ new/drivers/scsi/megaraid/megaraid_mbox.h	2006-04-13 16:05:30.313216152 -0400
@@ -21,8 +21,8 @@
 #include "megaraid_ioctl.h"
 
 
-#define MEGARAID_VERSION	"2.20.4.7"
-#define MEGARAID_EXT_VERSION	"(Release Date: Mon Nov 14 12:27:22 EST 2005)"
+#define MEGARAID_VERSION	"2.20.4.8"
+#define MEGARAID_EXT_VERSION	"(Release Date: Mon Apr 11 12:27:22 EST 2006)"
 
 
 /*
@@ -100,6 +100,9 @@
 #define MBOX_BUSY_WAIT		10	// max usec to wait for busy mailbox
 #define MBOX_RESET_WAIT		180	// wait these many seconds in reset
 #define MBOX_RESET_EXT_WAIT	120	// extended wait reset
+#define MBOX_SYNC_WAIT_CNT	0xFFFF	// wait loop index for synchronous mode
+
+#define MBOX_SYNC_DELAY_200	200	// 200 micro-seconds
 
 /*
  * maximum transfer that can happen through the firmware commands issued

[PATCH 1/1] megaraid_{mm,mbox}: fix a bug in reset handler

[Ju, Seokmann]

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

Hi,

This patch has fix for a bug in the 'megaraid_reset_handler()'.

When abort failed, the driver gets reset handleer called. In the reset
handler, driver calls 'scsi_done()' callback for same SCSI command
packet (struct scsi_cmnd) multiple times if there are multiple SCSI
command packet in the pend_list. More over, if there are entry in the
pend_lsit with IOCTL packet associated, the driver returns it to wrong
free_list so that, in turn, the driver could end up with 'NULL pointer
dereference..' during I/O command building with incorrect resource.

Also, the patch contains several minor/cosmetic changes besides this.

Thank you,

Seokmann

Signed-Off By: Seokmann Ju <seokmann.ju@lsil.com>


---
diff -Naur old/Documentation/scsi/ChangeLog.megaraid
new/Documentation/scsi/ChangeLog.megaraid
--- old/Documentation/scsi/ChangeLog.megaraid	2006-04-10
18:04:04.000000000 -0400
+++ new/Documentation/scsi/ChangeLog.megaraid	2006-04-12
09:35:20.939778336 -0400
@@ -1,3 +1,28 @@
+Release Date	: Mon Apr 11 12:27:22 EST 2006 - Seokmann Ju
<sju@lsil.com>
+Current Version : 2.20.4.8 (scsi module), 2.20.2.6 (cmm module)
+Older Version	: 2.20.4.7 (scsi module), 2.20.2.6 (cmm module)
+
+1.	Fixed a bug in megaraid_reset_handler().
+	Customer reported "Unable to handle kernel NULL pointer
dereference
+	at virtual address 00000000" when system goes to reset condition
+	for some reason. It happened randomly.
+	Root Cause: in the megaraid_reset_handler(), there is
possibility not 
+	returning pending packets in the pend_list if there are multiple
+	pending packets. 
+	Fix: Made the change in the driver so that it will return all
packets 
+	in the pend_list.
+
+2.	Added change request.
+	As found in the following URL, rmb() only didn't help the
+	problem. I had to increase the loop counter to 0xFFFFFF. (6 F's)
+	http://marc.theaimsgroup.com/?l=linux-scsi&m=110971060502497&w=2
+ 
+	I attached a patch for your reference, too.
+	Could you check and get this fix in your driver?
+ 
+	Best Regards,
+	Jun'ichi Nomura
+
 Release Date	: Fri Nov 11 12:27:22 EST 2005 - Seokmann Ju
<sju@lsil.com>
 Current Version : 2.20.4.7 (scsi module), 2.20.2.6 (cmm module)
 Older Version	: 2.20.4.6 (scsi module), 2.20.2.6 (cmm module)
diff -Naur old/drivers/scsi/megaraid/megaraid_mbox.c
new/drivers/scsi/megaraid/megaraid_mbox.c
--- old/drivers/scsi/megaraid/megaraid_mbox.c	2006-04-10
17:14:22.000000000 -0400
+++ new/drivers/scsi/megaraid/megaraid_mbox.c	2006-04-11
17:32:12.000000000 -0400
@@ -10,7 +10,7 @@
  *	   2 of the License, or (at your option) any later version.
  *
  * FILE		: megaraid_mbox.c
- * Version	: v2.20.4.7 (Nov 14 2005)
+ * Version	: v2.20.4.8 (Apr 11 2006)
  *
  * Authors:
  * 	Atul Mukker		<Atul.Mukker@lsil.com>
@@ -2655,32 +2655,48 @@
 	// Also, reset all the commands currently owned by the driver
 	spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
 	list_for_each_entry_safe(scb, tmp, &adapter->pend_list, list) {
-
 		list_del_init(&scb->list);	// from pending list
 
-		con_log(CL_ANN, (KERN_WARNING
-			"megaraid: %ld:%d[%d:%d], reset from pending
list\n",
-				scp->serial_number, scb->sno,
-				scb->dev_channel, scb->dev_target));
+		if (scb->sno >= MBOX_MAX_SCSI_CMDS) {
+			con_log(CL_ANN, (KERN_WARNING 
+			"megaraid: IOCTL packet with %d[%d:%d] being
reset\n",
+			scb->sno, scb->dev_channel, scb->dev_target));
 
-		scp->result = (DID_RESET << 16);
-		scp->scsi_done(scp);
+			scb->status = -EFAULT;
 
-		megaraid_dealloc_scb(adapter, scb);
+			megaraid_mbox_mm_done(adapter, scb);
+		} else {
+			if (scb->scp == scp) {	// Found command
+				con_log(CL_ANN, (KERN_WARNING
+					"megaraid: %ld:%d[%d:%d], reset
from pending list\n",
+					scp->serial_number, scb->sno,
+					scb->dev_channel,
scb->dev_target));
+			} else {
+				con_log(CL_ANN, (KERN_WARNING 
+				"megaraid: IO packet with %d[%d:%d]
being reset\n",
+				scb->sno, scb->dev_channel,
scb->dev_target));
+			}
+
+			scb->scp->result = (DID_RESET << 16);
+			scb->scp->scsi_done(scb->scp);
+
+			megaraid_dealloc_scb(adapter, scb);
+		}
 	}
 	spin_unlock_irqrestore(PENDING_LIST_LOCK(adapter), flags);
 
 	if (adapter->outstanding_cmds) {
 		con_log(CL_ANN, (KERN_NOTICE
 			"megaraid: %d outstanding commands. Max wait %d
sec\n",
-			adapter->outstanding_cmds, MBOX_RESET_WAIT));
+			adapter->outstanding_cmds, 
+			(MBOX_RESET_WAIT + MBOX_RESET_EXT_WAIT)));
 	}
 
 	recovery_window = MBOX_RESET_WAIT + MBOX_RESET_EXT_WAIT;
 
 	recovering = adapter->outstanding_cmds;
 
-	for (i = 0; i < recovery_window && adapter->outstanding_cmds;
i++) {
+	for (i = 0; i < recovery_window; i++) {
 
 		megaraid_ack_sequence(adapter);
 
@@ -2689,12 +2705,11 @@
 			con_log(CL_ANN, (
 			"megaraid mbox: Wait for %d commands to
complete:%d\n",
 				adapter->outstanding_cmds,
-				MBOX_RESET_WAIT - i));
+				(MBOX_RESET_WAIT + MBOX_RESET_EXT_WAIT)
- i));
 		}
 
 		// bailout if no recovery happended in reset time
-		if ((i == MBOX_RESET_WAIT) &&
-			(recovering == adapter->outstanding_cmds)) {
+		if (adapter->outstanding_cmds == 0) {
 			break;
 		}
 
@@ -2918,12 +2933,12 @@
 	wmb();
 	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
 
-	for (i = 0; i < 0xFFFFF; i++) {
+	for (i = 0; i < 0xFFFFFF; i++) {
 		if (mbox->numstatus != 0xFF) break;
 		rmb();
 	}
 
-	if (i == 0xFFFFF) {
+	if (i == 0xFFFFFF) {
 		// We may need to re-calibrate the counter
 		con_log(CL_ANN, (KERN_CRIT
 			"megaraid: fast sync command timed out\n"));
@@ -3475,7 +3490,7 @@
 	adp.drvr_data		= (unsigned long)adapter;
 	adp.pdev		= adapter->pdev;
 	adp.issue_uioc		= megaraid_mbox_mm_handler;
-	adp.timeout		= 300;
+	adp.timeout		= MBOX_RESET_WAIT + MBOX_RESET_EXT_WAIT;
 	adp.max_kioc		= MBOX_MAX_USER_CMDS;
 
 	if ((rval = mraid_mm_register_adp(&adp)) != 0) {
diff -Naur old/drivers/scsi/megaraid/megaraid_mbox.h
new/drivers/scsi/megaraid/megaraid_mbox.h
--- old/drivers/scsi/megaraid/megaraid_mbox.h	2006-04-10
17:14:22.000000000 -0400
+++ new/drivers/scsi/megaraid/megaraid_mbox.h	2006-04-11
13:45:21.000000000 -0400
@@ -21,8 +21,8 @@
 #include "megaraid_ioctl.h"
 
 
-#define MEGARAID_VERSION	"2.20.4.7"
-#define MEGARAID_EXT_VERSION	"(Release Date: Mon Nov 14 12:27:22 EST
2005)"
+#define MEGARAID_VERSION	"2.20.4.8"
+#define MEGARAID_EXT_VERSION	"(Release Date: Mon Apr 11 12:27:22 EST
2006)"
 
 
 /*
---

[-- Attachment #2: megaraid_mm_mbox.patch --]
[-- Type: application/octet-stream, Size: 5573 bytes --]

diff -Naur old/Documentation/scsi/ChangeLog.megaraid new/Documentation/scsi/ChangeLog.megaraid
--- old/Documentation/scsi/ChangeLog.megaraid	2006-04-10 18:04:04.000000000 -0400
+++ new/Documentation/scsi/ChangeLog.megaraid	2006-04-12 09:35:20.939778336 -0400
@@ -1,3 +1,28 @@
+Release Date	: Mon Apr 11 12:27:22 EST 2006 - Seokmann Ju <sju@lsil.com>
+Current Version : 2.20.4.8 (scsi module), 2.20.2.6 (cmm module)
+Older Version	: 2.20.4.7 (scsi module), 2.20.2.6 (cmm module)
+
+1.	Fixed a bug in megaraid_reset_handler().
+	Customer reported "Unable to handle kernel NULL pointer dereference
+	at virtual address 00000000" when system goes to reset condition
+	for some reason. It happened randomly.
+	Root Cause: in the megaraid_reset_handler(), there is possibility not 
+	returning pending packets in the pend_list if there are multiple
+	pending packets. 
+	Fix: Made the change in the driver so that it will return all packets 
+	in the pend_list.
+
+2.	Added change request.
+	As found in the following URL, rmb() only didn't help the
+	problem. I had to increase the loop counter to 0xFFFFFF. (6 F's)
+	http://marc.theaimsgroup.com/?l=linux-scsi&m=110971060502497&w=2
+ 
+	I attached a patch for your reference, too.
+	Could you check and get this fix in your driver?
+ 
+	Best Regards,
+	Jun'ichi Nomura
+
 Release Date	: Fri Nov 11 12:27:22 EST 2005 - Seokmann Ju <sju@lsil.com>
 Current Version : 2.20.4.7 (scsi module), 2.20.2.6 (cmm module)
 Older Version	: 2.20.4.6 (scsi module), 2.20.2.6 (cmm module)
diff -Naur old/drivers/scsi/megaraid/megaraid_mbox.c new/drivers/scsi/megaraid/megaraid_mbox.c
--- old/drivers/scsi/megaraid/megaraid_mbox.c	2006-04-10 17:14:22.000000000 -0400
+++ new/drivers/scsi/megaraid/megaraid_mbox.c	2006-04-11 17:32:12.000000000 -0400
@@ -10,7 +10,7 @@
  *	   2 of the License, or (at your option) any later version.
  *
  * FILE		: megaraid_mbox.c
- * Version	: v2.20.4.7 (Nov 14 2005)
+ * Version	: v2.20.4.8 (Apr 11 2006)
  *
  * Authors:
  * 	Atul Mukker		<Atul.Mukker@lsil.com>
@@ -2655,32 +2655,48 @@
 	// Also, reset all the commands currently owned by the driver
 	spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
 	list_for_each_entry_safe(scb, tmp, &adapter->pend_list, list) {
-
 		list_del_init(&scb->list);	// from pending list
 
-		con_log(CL_ANN, (KERN_WARNING
-			"megaraid: %ld:%d[%d:%d], reset from pending list\n",
-				scp->serial_number, scb->sno,
-				scb->dev_channel, scb->dev_target));
+		if (scb->sno >= MBOX_MAX_SCSI_CMDS) {
+			con_log(CL_ANN, (KERN_WARNING 
+			"megaraid: IOCTL packet with %d[%d:%d] being reset\n",
+			scb->sno, scb->dev_channel, scb->dev_target));
 
-		scp->result = (DID_RESET << 16);
-		scp->scsi_done(scp);
+			scb->status = -EFAULT;
 
-		megaraid_dealloc_scb(adapter, scb);
+			megaraid_mbox_mm_done(adapter, scb);
+		} else {
+			if (scb->scp == scp) {	// Found command
+				con_log(CL_ANN, (KERN_WARNING
+					"megaraid: %ld:%d[%d:%d], reset from pending list\n",
+					scp->serial_number, scb->sno,
+					scb->dev_channel, scb->dev_target));
+			} else {
+				con_log(CL_ANN, (KERN_WARNING 
+				"megaraid: IO packet with %d[%d:%d] being reset\n",
+				scb->sno, scb->dev_channel, scb->dev_target));
+			}
+
+			scb->scp->result = (DID_RESET << 16);
+			scb->scp->scsi_done(scb->scp);
+
+			megaraid_dealloc_scb(adapter, scb);
+		}
 	}
 	spin_unlock_irqrestore(PENDING_LIST_LOCK(adapter), flags);
 
 	if (adapter->outstanding_cmds) {
 		con_log(CL_ANN, (KERN_NOTICE
 			"megaraid: %d outstanding commands. Max wait %d sec\n",
-			adapter->outstanding_cmds, MBOX_RESET_WAIT));
+			adapter->outstanding_cmds, 
+			(MBOX_RESET_WAIT + MBOX_RESET_EXT_WAIT)));
 	}
 
 	recovery_window = MBOX_RESET_WAIT + MBOX_RESET_EXT_WAIT;
 
 	recovering = adapter->outstanding_cmds;
 
-	for (i = 0; i < recovery_window && adapter->outstanding_cmds; i++) {
+	for (i = 0; i < recovery_window; i++) {
 
 		megaraid_ack_sequence(adapter);
 
@@ -2689,12 +2705,11 @@
 			con_log(CL_ANN, (
 			"megaraid mbox: Wait for %d commands to complete:%d\n",
 				adapter->outstanding_cmds,
-				MBOX_RESET_WAIT - i));
+				(MBOX_RESET_WAIT + MBOX_RESET_EXT_WAIT) - i));
 		}
 
 		// bailout if no recovery happended in reset time
-		if ((i == MBOX_RESET_WAIT) &&
-			(recovering == adapter->outstanding_cmds)) {
+		if (adapter->outstanding_cmds == 0) {
 			break;
 		}
 
@@ -2918,12 +2933,12 @@
 	wmb();
 	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
 
-	for (i = 0; i < 0xFFFFF; i++) {
+	for (i = 0; i < 0xFFFFFF; i++) {
 		if (mbox->numstatus != 0xFF) break;
 		rmb();
 	}
 
-	if (i == 0xFFFFF) {
+	if (i == 0xFFFFFF) {
 		// We may need to re-calibrate the counter
 		con_log(CL_ANN, (KERN_CRIT
 			"megaraid: fast sync command timed out\n"));
@@ -3475,7 +3490,7 @@
 	adp.drvr_data		= (unsigned long)adapter;
 	adp.pdev		= adapter->pdev;
 	adp.issue_uioc		= megaraid_mbox_mm_handler;
-	adp.timeout		= 300;
+	adp.timeout		= MBOX_RESET_WAIT + MBOX_RESET_EXT_WAIT;
 	adp.max_kioc		= MBOX_MAX_USER_CMDS;
 
 	if ((rval = mraid_mm_register_adp(&adp)) != 0) {
diff -Naur old/drivers/scsi/megaraid/megaraid_mbox.h new/drivers/scsi/megaraid/megaraid_mbox.h
--- old/drivers/scsi/megaraid/megaraid_mbox.h	2006-04-10 17:14:22.000000000 -0400
+++ new/drivers/scsi/megaraid/megaraid_mbox.h	2006-04-11 13:45:21.000000000 -0400
@@ -21,8 +21,8 @@
 #include "megaraid_ioctl.h"
 
 
-#define MEGARAID_VERSION	"2.20.4.7"
-#define MEGARAID_EXT_VERSION	"(Release Date: Mon Nov 14 12:27:22 EST 2005)"
+#define MEGARAID_VERSION	"2.20.4.8"
+#define MEGARAID_EXT_VERSION	"(Release Date: Mon Apr 11 12:27:22 EST 2006)"
 
 
 /*

Re: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in reset handler

[Andrew Morton]

"Ju, Seokmann" <Seokmann.Ju@lsil.com> wrote:
>
> This patch has fix for a bug in the 'megaraid_reset_handler()'.
> 
>  When abort failed, the driver gets reset handleer called. In the reset
>  handler, driver calls 'scsi_done()' callback for same SCSI command
>  packet (struct scsi_cmnd) multiple times if there are multiple SCSI
>  command packet in the pend_list. More over, if there are entry in the
>  pend_lsit with IOCTL packet associated, the driver returns it to wrong
>  free_list so that, in turn, the driver could end up with 'NULL pointer
>  dereference..' during I/O command building with incorrect resource.
> 
>  Also, the patch contains several minor/cosmetic changes besides this.
>
> ..
>
> @@ -2655,32 +2655,48 @@
>  	// Also, reset all the commands currently owned by the driver
>  	spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
>  	list_for_each_entry_safe(scb, tmp, &adapter->pend_list, list) {
> -
>  		list_del_init(&scb->list);	// from pending list
>  
> -		con_log(CL_ANN, (KERN_WARNING
> -			"megaraid: %ld:%d[%d:%d], reset from pending list\n",
> -				scp->serial_number, scb->sno,
> -				scb->dev_channel, scb->dev_target));
> +		if (scb->sno >= MBOX_MAX_SCSI_CMDS) {
> +			con_log(CL_ANN, (KERN_WARNING 
> +			"megaraid: IOCTL packet with %d[%d:%d] being reset\n",
> +			scb->sno, scb->dev_channel, scb->dev_target));
>  
> -		scp->result = (DID_RESET << 16);
> -		scp->scsi_done(scp);
> +			scb->status = -EFAULT;

What is the significance of -EFAULT here?  Seems inappropriate?

> @@ -2918,12 +2933,12 @@
>  	wmb();
>  	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
>  
> -	for (i = 0; i < 0xFFFFF; i++) {
> +	for (i = 0; i < 0xFFFFFF; i++) {
>  		if (mbox->numstatus != 0xFF) break;
>  		rmb();
>  	}

Oh my.  That's an awfully long interrupts-off spin.  1.7e7 operations with
an NMI watchdog timeout of five seconds - I'm surprised it doesn't trigger.

Is that reading from a PCI register there?   Or main memory?

I'm somewhat surprised that the compiler never "optimises" this into a
lockup, actually.  That's what `volatile' is for.

Is it not possible to do this with an interrupt?

A `cpu_relax()' in that loop would help cool things down a bit.

Re: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in reset handler

[Andrew Morton]

Andrew Morton <akpm@osdl.org> wrote:
>
>  > @@ -2918,12 +2933,12 @@
>  >  	wmb();
>  >  	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
>  >  
>  > -	for (i = 0; i < 0xFFFFF; i++) {
>  > +	for (i = 0; i < 0xFFFFFF; i++) {
>  >  		if (mbox->numstatus != 0xFF) break;
>  >  		rmb();
>  >  	}
> 
>  Oh my.  That's an awfully long interrupts-off spin.

And if that mbox is in main memory, the duration of this spin will vary by
a factor of many tens across all the different machines on which this
driver must operate.

Careful use of ndelay() or udelay() would fix that.

Re: [PATCH 1/1] megaraid_{mm,mbox}: fix a bug in reset handler

[Andre Hedrick]

Andrew,

This is real, and is a known bug which is 100% reproducable (sp).
There are other harry issues too, but this is as much as I can say.

cpu_relax() will not work, already tried some time ago.


Andre Hedrick
LAD Storage Consulting Group

On Wed, 12 Apr 2006, Andrew Morton wrote:

> "Ju, Seokmann" <Seokmann.Ju@lsil.com> wrote:
> >
> > This patch has fix for a bug in the 'megaraid_reset_handler()'.
> > 
> >  When abort failed, the driver gets reset handleer called. In the reset
> >  handler, driver calls 'scsi_done()' callback for same SCSI command
> >  packet (struct scsi_cmnd) multiple times if there are multiple SCSI
> >  command packet in the pend_list. More over, if there are entry in the
> >  pend_lsit with IOCTL packet associated, the driver returns it to wrong
> >  free_list so that, in turn, the driver could end up with 'NULL pointer
> >  dereference..' during I/O command building with incorrect resource.
> > 
> >  Also, the patch contains several minor/cosmetic changes besides this.
> >
> > ..
> >
> > @@ -2655,32 +2655,48 @@
> >  	// Also, reset all the commands currently owned by the driver
> >  	spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
> >  	list_for_each_entry_safe(scb, tmp, &adapter->pend_list, list) {
> > -
> >  		list_del_init(&scb->list);	// from pending list
> >  
> > -		con_log(CL_ANN, (KERN_WARNING
> > -			"megaraid: %ld:%d[%d:%d], reset from pending list\n",
> > -				scp->serial_number, scb->sno,
> > -				scb->dev_channel, scb->dev_target));
> > +		if (scb->sno >= MBOX_MAX_SCSI_CMDS) {
> > +			con_log(CL_ANN, (KERN_WARNING 
> > +			"megaraid: IOCTL packet with %d[%d:%d] being reset\n",
> > +			scb->sno, scb->dev_channel, scb->dev_target));
> >  
> > -		scp->result = (DID_RESET << 16);
> > -		scp->scsi_done(scp);
> > +			scb->status = -EFAULT;
> 
> What is the significance of -EFAULT here?  Seems inappropriate?
> 
> > @@ -2918,12 +2933,12 @@
> >  	wmb();
> >  	WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
> >  
> > -	for (i = 0; i < 0xFFFFF; i++) {
> > +	for (i = 0; i < 0xFFFFFF; i++) {
> >  		if (mbox->numstatus != 0xFF) break;
> >  		rmb();
> >  	}
> 
> Oh my.  That's an awfully long interrupts-off spin.  1.7e7 operations with
> an NMI watchdog timeout of five seconds - I'm surprised it doesn't trigger.
> 
> Is that reading from a PCI register there?   Or main memory?
> 
> I'm somewhat surprised that the compiler never "optimises" this into a
> lockup, actually.  That's what `volatile' is for.
> 
> Is it not possible to do this with an interrupt?
> 
> A `cpu_relax()' in that loop would help cool things down a bit.
> 
> 
> -
> 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 1/1] megaraid_{mm,mbox}: fix a bug in reset handler

[James Bottomley]

On Wed, 2006-04-12 at 22:00 -0700, Andrew Morton wrote:
> Oh my.  That's an awfully long interrupts-off spin.  1.7e7 operations with
> an NMI watchdog timeout of five seconds - I'm surprised it doesn't trigger.
> 
> Is that reading from a PCI register there?   Or main memory?

It's a "mailbox" region, which is a piece of main memory shared between
the driver and the card (allocated using dma_alloc_coherent).

> I'm somewhat surprised that the compiler never "optimises" this into a
> lockup, actually.  That's what `volatile' is for.

The rmb(); below ensures the compiler can't optimise.  However, I do
agree; tagging the mailbox as volatile would show the compiler better
what the intent is.

> Is it not possible to do this with an interrupt?

I'd guess not.  A lot of these types of driver have what's called a
doorbell/mailbox interface which means that as long as there's a command
slot you get access to the device (or wait for an interrupt to tell you
one's free) but you have to post the command to the device, so you wait
at the mailbox to see that it's taken (usually because the device has to
assign things like tracking numbers or indexes).  The intent is for
there to be a fairly instantaneous response however firmware doesn't
always see it that way ...

> A `cpu_relax()' in that loop would help cool things down a bit.

Actually, I think a simple udelay(25) might help in a lot of these
loops.

James