Skip to contentSkip to conversation

aacraid: Initialize rx/rkt function pointers before calling them

PATCH In review Darrick J. Wong 2 participants 9 messages Apr 26, 2007 – May 2, 2007

Subsystems (2)
SCSI Aacraid SCSI RAID driver
lore
Share & tools
lore

Darrick J. Wong

author
#0 lore raw

PATCH 1/1 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 <[email protected]>

---

 drivers/scsi/aacraid/rx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/aacraid/rx.c +2 -2
index 0c71315..b7810d6 100644
@@ -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);
#1 lore raw

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

Sincerely -- Mark Salyzyn

Quoted 57 lines

-----Original Message-----
From: Darrick J. Wong [mailto:[email protected]]
Sent: Thursday, April 26, 2007 6:58 PM
To: [email protected]
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 <[email protected]>

---

 drivers/scsi/aacraid/rx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/aacraid/rx.c +2 -0
index 0c71315..b7810d6 100644
@@ -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 ... */
drivers/scsi/aacraid/rx.c +0 -1
 	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

drivers/scsi/aacraid/rx.c +0 -1
 	 *	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);
#2 lore raw

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

Quoted 79 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Salyzyn, Mark
Sent: Friday, April 27, 2007 8:47 AM
To: Darrick J. Wong; [email protected]
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

Quoted 9 lines

-----Original Message-----
From: Darrick J. Wong [mailto:[email protected]]
Sent: Thursday, April 26, 2007 6:58 PM
To: [email protected]
Cc: Salyzyn, Mark; Alexis Bruemmer
Subject: [PATCH] aacraid: Initialize rx/rkt function pointers before calling them

Quoted 4 lines

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

Quoted 38 lines

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 <[email protected]>

---

 drivers/scsi/aacraid/rx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/aacraid/rx.c +2 -0
index 0c71315..b7810d6 100644
@@ -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 ... */
drivers/scsi/aacraid/rx.c +0 -1
 	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

drivers/scsi/aacraid/rx.c int _aac_rx_init(struct aac_dev *dev) · line 612

Quoted 6 lines
drivers/scsi/aacraid/rx.c +0 -1
 	 *	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#3 lore raw
Quoted 6 lines · Mark

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

#4 lore raw

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

Quoted 38 lines

-----Original Message-----
From: Darrick J. Wong [mailto:[email protected]]
Sent: Friday, April 27, 2007 2:11 PM
To: Salyzyn, Mark
Cc: [email protected]; Alexis Bruemmer; Vivek Goyal; Judith Lebzelter
Subject: Re: [PATCH] aacraid: Initialize rx/rkt function pointers before calling them

Quoted 1 line · Mark

In my unit tests of aacraid_kexec_5.patch, restart was not

called for

Quoted 1 line

normal operations. If you are just doing a normal boot,

what conditions

Quoted 2 lines

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,

Quoted 2 lines

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

author
#5 lore raw
Quoted 2 lines · Mark

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

#6 lore raw

PATCH 1/1 aacraid: superfluous adapter reset for IBM 8 series ServeRAID controllers

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 <[email protected]>

---

Sincerely -- Mark Salyzyn

Quoted 7 lines

-----Original Message-----
From: Darrick J. Wong [mailto:[email protected]]
Sent: Friday, April 27, 2007 5:49 PM
To: Salyzyn, Mark
Cc: [email protected]; Alexis Bruemmer; Vivek Goyal; Judith Lebzelter
Subject: Re: [PATCH] aacraid: Initialize rx/rkt function pointers

before calling them

Quoted 9 lines
Quoted 2 lines · Mark

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

drivers/scsi/aacraid/rx.c +1 -1
diff -ru a/drivers/scsi/aacraid/rx.c b/drivers/scsi/aacraid/rx.c
@@ -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;
 	/*
author Acked-by on v1 1/1
#7 lore raw

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

Quoted 6 lines · Mark

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 <[email protected]>

--D

author
#8 lore raw

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

Quoted 9 lines · Darrick J. Wong
Quoted 6 lines · Mark

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