[PATCH] aacraid: Initialize rx/rkt function pointers before calling them

[PATCH] aacraid: Initialize rx/rkt function pointers before calling them

[Darrick J. Wong]

Commit 8418852d11f0bbaeebeedd4243560d8fdc85410d to scsi-misc resulted in
the substitution of calls to rx_sync_cmd with a function pointer
abstraction.  aac_rx_restart_adapter requires a pointer to a sync_cmd
function, which is not set up before its first invocation.  That causes
the driver to crash at startup.  Move the initializers (we need both
rx_sync_cmd and enable_int pointers) further up to proceed the
restart_adapter call.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 drivers/scsi/aacraid/rx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/aacraid/rx.c b/drivers/scsi/aacraid/rx.c
index 0c71315..b7810d6 100644
--- a/drivers/scsi/aacraid/rx.c
+++ b/drivers/scsi/aacraid/rx.c
@@ -537,6 +537,8 @@ int _aac_rx_init(struct aac_dev *dev)
 		printk(KERN_WARNING "%s: unable to map adapter.\n", name);
 		goto error_iounmap;
 	}
+	dev->a_ops.adapter_sync_cmd = rx_sync_cmd;
+	aac_adapter_comm(dev, AAC_COMM_PRODUCER);
 
 	/* Failure to reset here is an option ... */
 	dev->OIMR = status = rx_readb (dev, MUnit.OIMR);
@@ -598,7 +600,6 @@ int _aac_rx_init(struct aac_dev *dev)
 	dev->a_ops.adapter_interrupt = aac_rx_interrupt_adapter;
 	dev->a_ops.adapter_disable_int = aac_rx_disable_interrupt;
 	dev->a_ops.adapter_notify = aac_rx_notify_adapter;
-	dev->a_ops.adapter_sync_cmd = rx_sync_cmd;
 	dev->a_ops.adapter_check_health = aac_rx_check_health;
 	dev->a_ops.adapter_restart = aac_rx_restart_adapter;
 
@@ -606,7 +607,6 @@ int _aac_rx_init(struct aac_dev *dev)
 	 *	First clear out all interrupts.  Then enable the one's that we
 	 *	can handle.
 	 */
-	aac_adapter_comm(dev, AAC_COMM_PRODUCER);
 	aac_adapter_disable_int(dev);
 	rx_writel(dev, MUnit.ODR, 0xffffffff);
 	aac_adapter_enable_int(dev);

RE: [PATCH] aacraid: Initialize rx/rkt function pointers before calling them

[Salyzyn, Mark]

Reject, this is already covered in aacraid_kexec_5.patch and again
separately in aacraid_kexec_fix.patch.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Darrick J. Wong [mailto:djwong@us.ibm.com] 
> Sent: Thursday, April 26, 2007 6:58 PM
> To: linux-scsi@vger.kernel.org
> Cc: Salyzyn, Mark; Alexis Bruemmer
> Subject: [PATCH] aacraid: Initialize rx/rkt function pointers 
> before calling them
> 
> 
> Commit 8418852d11f0bbaeebeedd4243560d8fdc85410d to scsi-misc 
> resulted in
> the substitution of calls to rx_sync_cmd with a function pointer
> abstraction.  aac_rx_restart_adapter requires a pointer to a sync_cmd
> function, which is not set up before its first invocation.  
> That causes
> the driver to crash at startup.  Move the initializers (we need both
> rx_sync_cmd and enable_int pointers) further up to proceed the
> restart_adapter call.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> ---
> 
>  drivers/scsi/aacraid/rx.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/rx.c b/drivers/scsi/aacraid/rx.c
> index 0c71315..b7810d6 100644
> --- a/drivers/scsi/aacraid/rx.c
> +++ b/drivers/scsi/aacraid/rx.c
> @@ -537,6 +537,8 @@ int _aac_rx_init(struct aac_dev *dev)
>  		printk(KERN_WARNING "%s: unable to map 
> adapter.\n", name);
>  		goto error_iounmap;
>  	}
> +	dev->a_ops.adapter_sync_cmd = rx_sync_cmd;
> +	aac_adapter_comm(dev, AAC_COMM_PRODUCER);
>  
>  	/* Failure to reset here is an option ... */
>  	dev->OIMR = status = rx_readb (dev, MUnit.OIMR);
> @@ -598,7 +600,6 @@ int _aac_rx_init(struct aac_dev *dev)
>  	dev->a_ops.adapter_interrupt = aac_rx_interrupt_adapter;
>  	dev->a_ops.adapter_disable_int = aac_rx_disable_interrupt;
>  	dev->a_ops.adapter_notify = aac_rx_notify_adapter;
> -	dev->a_ops.adapter_sync_cmd = rx_sync_cmd;
>  	dev->a_ops.adapter_check_health = aac_rx_check_health;
>  	dev->a_ops.adapter_restart = aac_rx_restart_adapter;
>  
> @@ -606,7 +607,6 @@ int _aac_rx_init(struct aac_dev *dev)
>  	 *	First clear out all interrupts.  Then enable 
> the one's that we
>  	 *	can handle.
>  	 */
> -	aac_adapter_comm(dev, AAC_COMM_PRODUCER);
>  	aac_adapter_disable_int(dev);
>  	rx_writel(dev, MUnit.ODR, 0xffffffff);
>  	aac_adapter_enable_int(dev);
> 

RE: [PATCH] aacraid: Initialize rx/rkt function pointers before calling them

[Salyzyn, Mark]

Just wait a second ...

We are not supposed to be calling aac_rx_restart_adapter unless the
Adapter has it's interrupts enabled (from a previous driver load in the
same warm session such as a kexec) or if the kernel reset_devices flag
is set (which is a mandatory kernel flag during kdump or kexec). This
restart of the Adapter is really for kexec and kdump issues and should
not be triggered elsewhere.

In my unit tests of aacraid_kexec_5.patch, restart was not called for
normal operations. If you are just doing a normal boot, what conditions
are causing restart to be called in your case? Is it a warm restart?
Some kind of operation that leaves the Adapter in an initialized state,
or a bug in the driver making sure that interrupts are disabled when
shut down. Inquiring minds want to know!

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Salyzyn, Mark
> Sent: Friday, April 27, 2007 8:47 AM
> To: Darrick J. Wong; linux-scsi@vger.kernel.org
> Cc: Alexis Bruemmer
> Subject: RE: [PATCH] aacraid: Initialize rx/rkt function 
> pointers before calling them
> 
> 
> Reject, this is already covered in aacraid_kexec_5.patch and again
> separately in aacraid_kexec_fix.patch.
> 
> Sincerely -- Mark Salyzyn
> 
> > -----Original Message-----
> > From: Darrick J. Wong [mailto:djwong@us.ibm.com] 
> > Sent: Thursday, April 26, 2007 6:58 PM
> > To: linux-scsi@vger.kernel.org
> > Cc: Salyzyn, Mark; Alexis Bruemmer
> > Subject: [PATCH] aacraid: Initialize rx/rkt function pointers 
> > before calling them
> > 
> > 
> > Commit 8418852d11f0bbaeebeedd4243560d8fdc85410d to scsi-misc 
> > resulted in
> > the substitution of calls to rx_sync_cmd with a function pointer
> > abstraction.  aac_rx_restart_adapter requires a pointer to 
> a sync_cmd
> > function, which is not set up before its first invocation.  
> > That causes
> > the driver to crash at startup.  Move the initializers (we need both
> > rx_sync_cmd and enable_int pointers) further up to proceed the
> > restart_adapter call.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> > ---
> > 
> >  drivers/scsi/aacraid/rx.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/scsi/aacraid/rx.c b/drivers/scsi/aacraid/rx.c
> > index 0c71315..b7810d6 100644
> > --- a/drivers/scsi/aacraid/rx.c
> > +++ b/drivers/scsi/aacraid/rx.c
> > @@ -537,6 +537,8 @@ int _aac_rx_init(struct aac_dev *dev)
> >  		printk(KERN_WARNING "%s: unable to map 
> > adapter.\n", name);
> >  		goto error_iounmap;
> >  	}
> > +	dev->a_ops.adapter_sync_cmd = rx_sync_cmd;
> > +	aac_adapter_comm(dev, AAC_COMM_PRODUCER);
> >  
> >  	/* Failure to reset here is an option ... */
> >  	dev->OIMR = status = rx_readb (dev, MUnit.OIMR);
> > @@ -598,7 +600,6 @@ int _aac_rx_init(struct aac_dev *dev)
> >  	dev->a_ops.adapter_interrupt = aac_rx_interrupt_adapter;
> >  	dev->a_ops.adapter_disable_int = aac_rx_disable_interrupt;
> >  	dev->a_ops.adapter_notify = aac_rx_notify_adapter;
> > -	dev->a_ops.adapter_sync_cmd = rx_sync_cmd;
> >  	dev->a_ops.adapter_check_health = aac_rx_check_health;
> >  	dev->a_ops.adapter_restart = aac_rx_restart_adapter;
> >  
> > @@ -606,7 +607,6 @@ int _aac_rx_init(struct aac_dev *dev)
> >  	 *	First clear out all interrupts.  Then enable 
> > the one's that we
> >  	 *	can handle.
> >  	 */
> > -	aac_adapter_comm(dev, AAC_COMM_PRODUCER);
> >  	aac_adapter_disable_int(dev);
> >  	rx_writel(dev, MUnit.ODR, 0xffffffff);
> >  	aac_adapter_enable_int(dev);
> > 
> -
> 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] aacraid: Initialize rx/rkt function pointers before calling them

[Darrick J. Wong]

Salyzyn, Mark wrote:

> In my unit tests of aacraid_kexec_5.patch, restart was not called for
> normal operations. If you are just doing a normal boot, what conditions
> are causing restart to be called in your case? Is it a warm restart?
> Some kind of operation that leaves the Adapter in an initialized state,
> or a bug in the driver making sure that interrupts are disabled when
> shut down. Inquiring minds want to know!

This is a normal boot of a "Serveraid 8k-l" on an IBM x3550.  One
wrinkle in the configuration is that the system is booted off the
network, though I don't see how that would affect the aacraid's state.
It looks like the MUnit.OIMR test just after the "Failure to reset here
is an option..." comment is succeeding.  The crash seems to happen
regardless of whether we had just done a warm or cold boot.  The option
ROM had run during POST, if that makes any difference.  No kexec/kdump
have been configured.  For that matter, neither kexec nor kdump have
ever been run in the lifetime of the machine.

Also observed on an IBM x3650.

--D

RE: [PATCH] aacraid: Initialize rx/rkt function pointers before calling them

[Salyzyn, Mark]

Maybe we should consider something like:

         dev->a_ops.adapter_sync_cmd = rx_sync_cmd;
         dev->a_ops.adapter_enable_int = aac_rx_disable_interrupt;
-        dev->OIMR = status = rx_readb (dev, MUnit.OIMR);
-        if ((((status & 0xff) != 0xff) || reset_devices) &&
+        if (reset_devices &&
          !aac_rx_restart_adapter(dev, 0))
                ++restart;
        /*
         *      Check to see if the board panic'd while booting.

As an option for a patch (later), what was the actual value of the
Munit.OIMR register (on the x3550 and the x3650 please, just in case)?
The BIOS should not be enabling any interrupts, I will be checking if
they have needed to in some variants(IBM?) for customization or
chip(Aurora interface?) reasons...

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Darrick J. Wong [mailto:djwong@us.ibm.com] 
> Sent: Friday, April 27, 2007 2:11 PM
> To: Salyzyn, Mark
> Cc: linux-scsi@vger.kernel.org; Alexis Bruemmer; Vivek Goyal; 
> Judith Lebzelter
> Subject: Re: [PATCH] aacraid: Initialize rx/rkt function 
> pointers before calling them
> 
> 
> Salyzyn, Mark wrote:
> 
> > In my unit tests of aacraid_kexec_5.patch, restart was not 
> called for
> > normal operations. If you are just doing a normal boot, 
> what conditions
> > are causing restart to be called in your case? Is it a warm restart?
> > Some kind of operation that leaves the Adapter in an 
> initialized state,
> > or a bug in the driver making sure that interrupts are disabled when
> > shut down. Inquiring minds want to know!
> 
> This is a normal boot of a "Serveraid 8k-l" on an IBM x3550.  One
> wrinkle in the configuration is that the system is booted off the
> network, though I don't see how that would affect the aacraid's state.
> It looks like the MUnit.OIMR test just after the "Failure to 
> reset here
> is an option..." comment is succeeding.  The crash seems to happen
> regardless of whether we had just done a warm or cold boot.  
> The option
> ROM had run during POST, if that makes any difference.  No kexec/kdump
> have been configured.  For that matter, neither kexec nor kdump have
> ever been run in the lifetime of the machine.
> 
> Also observed on an IBM x3650.
> 
> --D
> 

Re: [PATCH] aacraid: Initialize rx/rkt function pointers before calling them

[Darrick J. Wong]

Salyzyn, Mark wrote:
> As an option for a patch (later), what was the actual value of the
> Munit.OIMR register (on the x3550 and the x3650 please, just in case)?

0xF.

--D

[PATCH] aacraid: superfluous adapter reset for IBM 8 series ServeRAID controllers

[Salyzyn, Mark]

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

The kexec patch introduced a superfluous (and otherwise inert) reset of
some adapters. The register can have a hardware default value that has
zeros for the undefined interrupts. This patch refines the test of the
interrupt enable register to focus on only the interrupts that affect
the driver in order to detect if an incomplete shutdown of the Adapter
had occurred (kdump).

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
handling of patches. Dynamic fast paced public patches create confusion.

This attached patch is against current scsi-misc-2.6 *after* either the
aacraid_kexec_5 or the aacraid_kexec_fix patches have propagated. The
reason for supplying this patch in this manner is to recognize the lower
priority of this patch, the dependence and to survive cleanly the
sequencing and the overlap of patch application. James, say the word and
I can provide an aacraid_kexec_6 with this rolled in, but my goal is NOT
to delay aacraid_kexec_5 (or aacraid_kexec_fix as applied to
aacraid_kexec_2 which currently is in scsi-misc-2.6 and 2.6.21-rc6-mm1+)
from propagating otherwise.

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

---

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Darrick J. Wong [mailto:djwong@us.ibm.com] 
> Sent: Friday, April 27, 2007 5:49 PM
> To: Salyzyn, Mark
> Cc: linux-scsi@vger.kernel.org; Alexis Bruemmer; Vivek Goyal; 
> Judith Lebzelter
> Subject: Re: [PATCH] aacraid: Initialize rx/rkt function pointers
before calling them
> 
> Salyzyn, Mark wrote:
> > As an option for a patch (later), what was the actual value of the
> > Munit.OIMR register (on the x3550 and the x3650 please, 
> just in case)?
> 
> 0xF.
> 
> --D

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

diff -ru a/drivers/scsi/aacraid/rx.c b/drivers/scsi/aacraid/rx.c
--- a/drivers/scsi/aacraid/rx.c	2007-05-01 11:17:46.668321260 -0400
+++ b/drivers/scsi/aacraid/rx.c	2007-05-01 11:18:24.384707394 -0400
@@ -542,7 +542,7 @@
 	dev->a_ops.adapter_sync_cmd = rx_sync_cmd;
 	dev->a_ops.adapter_enable_int = aac_rx_disable_interrupt;
 	dev->OIMR = status = rx_readb (dev, MUnit.OIMR);
-	if ((((status & 0xff) != 0xff) || reset_devices) &&
+	if ((((status & 0x0c) != 0x0c) || reset_devices) &&
 	  !aac_rx_restart_adapter(dev, 0))
 		++restart;
 	/*

Re: [PATCH] aacraid: superfluous adapter reset for IBM 8 series ServeRAID controllers

[Darrick J. Wong]

Salyzyn, Mark wrote:
> The kexec patch introduced a superfluous (and otherwise inert) reset of
> some adapters. The register can have a hardware default value that has
> zeros for the undefined interrupts. This patch refines the test of the
> interrupt enable register to focus on only the interrupts that affect
> the driver in order to detect if an incomplete shutdown of the Adapter
> had occurred (kdump).

Tests out ok on the affected machines, so:

Acked-by: Darrick J. Wong <djwong@us.ibm.com>

--D

Re: [PATCH] aacraid: superfluous adapter reset for IBM 8 series ServeRAID controllers

[Darrick J. Wong]

Darrick J. Wong wrote:
> Salyzyn, Mark wrote:
>> The kexec patch introduced a superfluous (and otherwise inert) reset of
>> some adapters. The register can have a hardware default value that has
>> zeros for the undefined interrupts. This patch refines the test of the
>> interrupt enable register to focus on only the interrupts that affect
>> the driver in order to detect if an incomplete shutdown of the Adapter
>> had occurred (kdump).
> 
> Tests out ok on the affected machines, so:

/me shoves foot in mouth.  Crashes on a 2410SA aacraid card; OIMR =
0xF7.  De-ack.

--D