GDTH driver not working after upgrade to 2.6.24

Hi,

so i have upgraded a system to kernel 2.6.24. After that, it failed to boot with the usual message telling, that the rootfs on device /dev/sda1
cannot be mounted (a raid1 run by the controller below).

With 2.6.23.12, everything is working fine.

# lspci -v:

03:01.0 RAID bus controller: Intel Corporation RAID Controller
Subsystem: Intel Corporation Unknown device 01db
Flags: bus master, 66MHz, slow devsel, latency 64, IRQ 17 Memory at ddffc000 (32-bit, prefetchable) [size=16K] [virtual] Expansion ROM at deef0000 [disabled] [size=32K]
Capabilities: [80] Power Management version 2

# GDT-related dmesg output (2.6.23.12):

GDT-HA: Storage RAID Controller Driver. Version: 3.05
ACPI: PCI Interrupt 0000:03:01.0[A] -> GSI 24 (level, low) -> IRQ 17
GDT-HA: Found 1 PCI Storage RAID Controllers
Configuring GDT-PCI HA at 3/1 IRQ 17
GDT-HA 0: Name: SRCU42L
scsi0 : SRCU42L

scsi 0:0:0:0: Direct-Access     Intel    Host Drive  #00       PQ: 0 ANSI: 2
scsi 0:2:6:0: Processor         ESG-SHV  SCA HSBP M29     1.06 PQ: 0 ANSI: 2

sd 0:0:0:0: [sda] 143299800 512-byte hardware sectors (73369 MB) sd 0:0:0:0: [sda] Assuming Write Enabled
sd 0:0:0:0: [sda] Assuming drive cache: write through sd 0:0:0:0: [sda] 143299800 512-byte hardware sectors (73369 MB) sd 0:0:0:0: [sda] Assuming Write Enabled
sd 0:0:0:0: [sda] Assuming drive cache: write through
sda: sda1 sda2 < sda5 >
sd 0:0:0:0: [sda] Attached SCSI disk

# cat /boot/config-2.6.24 |grep GDT

CONFIG_SCSI_GDTH=y

Any ideas?

http://www.kernel.org/diff/diffview.cgi?file=%2Fpub%2Flinux%2Fkernel%2Fv2.6%2Fpatch-2.6.24.bz2
show huge drivers/scsi/gdth* related changes.

Can't test at the moment. System went production.

Regards,
Sven

Yes the gdth driver passed an open hart surgery in kernel 2.6.24. The bad thing about it is that all three of the Coders that did that did not have any HW to work on. One of them is me. We did cry for tester for a long time but no one came forward.

Could you test patches for us? first thing would be to enable debug output patch below.

If you absolutely need a 2.6.24 kernel, + gdth in a production system you could checkout the 2.6.23 driver and compile. The old driver will work the same in 2.6.24. It will not however even compile in 2.6.25-rcx.

If any one wants to send me a card that uses the gdth driver, I will be very happy to debug this card, and return it once I'm done.

Quoted 3 lines

Yes the gdth driver passed an open hart surgery in kernel 2.6.24. The bad thing about it is that all three of the Coders that did that did not have any HW to work on. One of them is me. We did cry for tester for a long time but no one came forward.

All i'd like to ask is WHY!?
WHY such a big open heart surgery?
OK, you have your reasons.

Quoted 1 line

Could you test patches for us? first thing would be to enable debug output patch below.

The machine is still production. It will be replaced by a 64bit system with an AACRAID card some time in the future. Then, i could maybe test patches on the old machine. But unfortunatly, it's not my machine. It's just administrated by me. And i don't know, what's exactly is the future of it.

Quoted 3 lines

If you absolutely need a 2.6.24 kernel, + gdth in a production system you could checkout the 2.6.23 driver and compile. The old driver will work the same in 2.6.24. It will not however even compile in 2.6.25-rcx.

I see. Thanks for the hint. Would be an alternative. Actually, i don't need 2.6.24 - but if something security related is fixed, then i'm not able to move on to 2.6.24. And i'm using Gentoo. There's not really a kernel maintained by the distribution.

Quoted 2 lines

If any one wants to send me a card that uses the gdth driver, I will be very happy to debug this card, and return it once I'm done.

I don't think, it's a seperate card, that i could send you. It's some 19" rack server with such a card on-board, i think.

So it don't see much opportunity for me to test patched and stuff :-(

Thanks, Perhaps someone else then.
Anyone with gdth HW that can test patches?

Your lspci said: "Intel Corporation RAID Controller" Matthew is there a gdth card lying around in an Intel lab near you?

James do we need to mark gdth BROKEN for 2.6.24 and higher?

Hi,

Quoted 2 lines · Boaz Harrosh

Thanks, Perhaps someone else then.
Anyone with gdth HW that can test patches?

Is bisecting down the existing chain and finding the bad commit sufficient? (I also take new patches.)

Quoted 5 lines

Your lspci said: "Intel Corporation RAID Controller" Matthew is there a gdth card lying around in an Intel lab near you?

James do we need to mark gdth BROKEN for 2.6.24 and higher?

I say revert it for the time being.

Quoted 48 lines · Sven Köhler

Hi,

so i have upgraded a system to kernel 2.6.24. After that, it failed to boot with the usual message telling, that the rootfs on device /dev/sda1
cannot be mounted (a raid1 run by the controller below).

With 2.6.23.12, everything is working fine.

# lspci -v:

03:01.0 RAID bus controller: Intel Corporation RAID Controller
Subsystem: Intel Corporation Unknown device 01db
Flags: bus master, 66MHz, slow devsel, latency 64, IRQ 17 Memory at ddffc000 (32-bit, prefetchable) [size=16K] [virtual] Expansion ROM at deef0000 [disabled] [size=32K]
Capabilities: [80] Power Management version 2

# GDT-related dmesg output (2.6.23.12):

GDT-HA: Storage RAID Controller Driver. Version: 3.05
ACPI: PCI Interrupt 0000:03:01.0[A] -> GSI 24 (level, low) -> IRQ 17
GDT-HA: Found 1 PCI Storage RAID Controllers
Configuring GDT-PCI HA at 3/1 IRQ 17
GDT-HA 0: Name: SRCU42L
scsi0 : SRCU42L

scsi 0:0:0:0: Direct-Access     Intel    Host Drive  #00       PQ: 0 ANSI: 2
scsi 0:2:6:0: Processor         ESG-SHV  SCA HSBP M29     1.06 PQ: 0 ANSI: 2

sd 0:0:0:0: [sda] 143299800 512-byte hardware sectors (73369 MB) sd 0:0:0:0: [sda] Assuming Write Enabled
sd 0:0:0:0: [sda] Assuming drive cache: write through sd 0:0:0:0: [sda] 143299800 512-byte hardware sectors (73369 MB) sd 0:0:0:0: [sda] Assuming Write Enabled
sd 0:0:0:0: [sda] Assuming drive cache: write through
sda: sda1 sda2 < sda5 >
sd 0:0:0:0: [sda] Attached SCSI disk

# cat /boot/config-2.6.24 |grep GDT

CONFIG_SCSI_GDTH=y

Any ideas?

http://www.kernel.org/diff/diffview.cgi?file=%2Fpub%2Flinux%2Fkernel%2Fv2.6%2Fpatch-2.6.24.bz2
show huge drivers/scsi/gdth* related changes.

Quoted 6 lines

Can't test at the moment. System went production.

Regards,
Sven

Hi Sven!

The patch: "gdth: switch to modern scsi host registration"

missed one simple fact when moving a way from scsi_module.c. That is to call scsi_scan_host() on the probed host. With this the gdth driver from 2.6.24 is again able to see drives and boot.

gdth _exit would first remove all cards then stop the timer and would not sync with the timer function. This caused a crash in gdth_timer() when module was unloaded.

del_timer_sync the timer before we delete the cards.

NOT YET TESTED

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/scsi/gdth.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

drivers/scsi/gdth.c +8 -7

index 8eb78be..103280e 100644
@@ -3793,6 +3793,8 @@ static void gdth_timeout(ulong data)
     gdth_ha_str *ha;
     ulong flags;
 
+    BUG_ON(list_empty(&gdth_instances));
+
     ha = list_first_entry(&gdth_instances, gdth_ha_str, list);
     spin_lock_irqsave(&ha->smp_lock, flags);
 

@@ -5146,8 +5148,6 @@ static void gdth_remove_one(gdth_ha_str *ha)
 		ha->sdev = NULL;
 	}
 
-	gdth_flush(ha);
-
 	if (shp->irq)
 		free_irq(shp->irq,ha);
 

@@ -5245,14 +5245,15 @@ static void __exit gdth_exit(void)
 {
 	gdth_ha_str *ha;
 
-	list_for_each_entry(ha, &gdth_instances, list)
-		gdth_remove_one(ha);
+	unregister_chrdev(major,"gdth");
+	unregister_reboot_notifier(&gdth_notifier);
 
 #ifdef GDTH_STATISTICS
-	del_timer(&gdth_timer);
+	del_timer_sync(&gdth_timer);
 #endif
-	unregister_chrdev(major,"gdth");
-	unregister_reboot_notifier(&gdth_notifier);
+
+	list_for_each_entry(ha, &gdth_instances, list)
+		gdth_remove_one(ha);
 }
 
 module_init(gdth_init);

Hello!

I've tested this patch now - and it works fine. Now rmmod, halt and reboot also works.

Stefan Priebe

Quoted 6 lines

I've tested this patch now - and it works fine. Now rmmod, halt and reboot also works.

Stefan Priebe

This is grate news Stefan. Thank you very much for all your time and effort, with out we could not have fixed all this.

Quoted 7 lines · Boaz Harrosh

Quoted 5 lines

I've tested this patch now - and it works fine. Now rmmod, halt and reboot also works.

Stefan Priebe

This is grate news Stefan. Thank you very much for all your time and effort, with out we could not have fixed all this.

Do you have a git tree with the latest pieces?

gdth _exit would first remove all cards then stop the timer and would not sync with the timer function. This caused a crash in gdth_timer() when module was unloaded.

del_timer_sync the timer before we delete the cards.

NOT YET TESTED

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/scsi/gdth.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

drivers/scsi/gdth.c +4 -5

index 8eb78be..103280e 100644
@@ -3793,6 +3793,8 @@ static void gdth_timeout(ulong data)
     gdth_ha_str *ha;
     ulong flags;
 
+    BUG_ON(list_empty(&gdth_instances));
+
     ha = list_first_entry(&gdth_instances, gdth_ha_str, list);
     spin_lock_irqsave(&ha->smp_lock, flags);
 

@@ -5146,8 +5148,6 @@ static void gdth_remove_one(gdth_ha_str *ha)
 		ha->sdev = NULL;
 	}
 
-	gdth_flush(ha);
-
 	if (shp->irq)
 		free_irq(shp->irq,ha);
 

@@ -5245,14 +5245,15 @@ static void __exit gdth_exit(void)
 {
 	gdth_ha_str *ha;
 
-	list_for_each_entry(ha, &gdth_instances, list)
-		gdth_remove_one(ha);
+	unregister_chrdev(major,"gdth");
+	unregister_reboot_notifier(&gdth_notifier);
 
 #ifdef GDTH_STATISTICS
-	del_timer(&gdth_timer);

drivers/scsi/gdth.c static void gdth_remove_one(gdth_ha_str *ha) · line 5150

Quoted 2 lines · Boaz Harrosh

drivers/scsi/gdth.c +0 -2

-	gdth_flush(ha);
-

This piece doesn't look right. gdth_flush() forces the internal cache to disk backing. If you remove it, you're taking the chance that the machine will be powered off without a writeback which can cause data corruption.

James

Quoted 10 lines · James Bottomley

drivers/scsi/gdth.c static void gdth_remove_one(gdth_ha_str *ha) · line 5150

Quoted 2 lines · Boaz Harrosh

drivers/scsi/gdth.c +0 -2

-	gdth_flush(ha);
-

This piece doesn't look right. gdth_flush() forces the internal cache to disk backing. If you remove it, you're taking the chance that the machine will be powered off without a writeback which can cause data corruption.

James

Yes.
I have more problems reported, with exit, and am just sending one more patch that puts this back in. Which was tested.

So I will resend this one plus one new one.

Boaz

Quoted 11 lines · Boaz Harrosh

Quoted 10 lines · James Bottomley

drivers/scsi/gdth.c static void gdth_remove_one(gdth_ha_str *ha) · line 5150

Quoted 2 lines · Boaz Harrosh

drivers/scsi/gdth.c +0 -2

-	gdth_flush(ha);
-

This piece doesn't look right. gdth_flush() forces the internal cache to disk backing. If you remove it, you're taking the chance that the machine will be powered off without a writeback which can cause data corruption.

James

Quoted 8 lines

Yes.
I have more problems reported, with exit, and am just sending one more patch that puts this back in. Which was tested.

So I will resend this one plus one new one.

Boaz

The gdth driver would do a register_reboot_notifier(&gdth_notifier); to a gdth_halt() function, which would then redo half of what gdth_exit does, and wrongly so, and crash.

Are we guaranteed in todays kernel that modules .exit function be called on an halt or reboot? If so then there is no need for duplications and the gdth_halt() should go.

Quoted 24 lines · Boaz Harrosh

Quoted 11 lines · Boaz Harrosh

Quoted 10 lines · James Bottomley

drivers/scsi/gdth.c static void gdth_remove_one(gdth_ha_str *ha) · line 5150

Quoted 2 lines · Boaz Harrosh

drivers/scsi/gdth.c +0 -2

-	gdth_flush(ha);
-

This piece doesn't look right. gdth_flush() forces the internal cache to disk backing. If you remove it, you're taking the chance that the machine will be powered off without a writeback which can cause data corruption.

James

Quoted 8 lines

Yes.
I have more problems reported, with exit, and am just sending one more patch that puts this back in. Which was tested.

So I will resend this one plus one new one.

Boaz

The gdth driver would do a register_reboot_notifier(&gdth_notifier); to a gdth_halt() function, which would then redo half of what gdth_exit does, and wrongly so, and crash.

Quoted 3 lines

Are we guaranteed in todays kernel that modules .exit function be called on an halt or reboot? If so then there is no need for duplications and the gdth_halt() should go.

No. The __exit section is actually discardable if you promise never to remove the module.

James

Quoted 29 lines · James Bottomley

Quoted 27 lines · Boaz Harrosh

Quoted 11 lines · Boaz Harrosh

Quoted 10 lines · James Bottomley

drivers/scsi/gdth.c static void gdth_remove_one(gdth_ha_str *ha) · line 5150

Quoted 2 lines · Boaz Harrosh

drivers/scsi/gdth.c +0 -2

-	gdth_flush(ha);
-

This piece doesn't look right. gdth_flush() forces the internal cache to disk backing. If you remove it, you're taking the chance that the machine will be powered off without a writeback which can cause data corruption.

James

Quoted 8 lines

Yes.
I have more problems reported, with exit, and am just sending one more patch that puts this back in. Which was tested.

So I will resend this one plus one new one.

Boaz

The gdth driver would do a register_reboot_notifier(&gdth_notifier); to a gdth_halt() function, which would then redo half of what gdth_exit does, and wrongly so, and crash.

Are we guaranteed in todays kernel that modules .exit function be called on an halt or reboot? If so then there is no need for duplications and the gdth_halt() should go.

Quoted 3 lines

No. The __exit section is actually discardable if you promise never to remove the module.

I don't understand please explain.
What does a driver need to do if it needs a consistent shutdown retine? module or built in? unload or shutdown?

Quoted 29 lines · Boaz Harrosh

Quoted 28 lines · James Bottomley

Quoted 27 lines · Boaz Harrosh

Quoted 11 lines · Boaz Harrosh

Quoted 10 lines · James Bottomley

Review

Quoted 2 lines · Boaz Harrosh

(quoted diff) +0 -2

-	gdth_flush(ha);
-

This piece doesn't look right. gdth_flush() forces the internal cache to disk backing. If you remove it, you're taking the chance that the machine will be powered off without a writeback which can cause data corruption.

James

Quoted 8 lines

Yes.
I have more problems reported, with exit, and am just sending one more patch that puts this back in. Which was tested.

So I will resend this one plus one new one.

Boaz

The gdth driver would do a register_reboot_notifier(&gdth_notifier); to a gdth_halt() function, which would then redo half of what gdth_exit does, and wrongly so, and crash.

Are we guaranteed in todays kernel that modules .exit function be called on an halt or reboot? If so then there is no need for duplications and the gdth_halt() should go.

Quoted 6 lines

Quoted 3 lines

No. The __exit section is actually discardable if you promise never to remove the module.

I don't understand please explain.
What does a driver need to do if it needs a consistent shutdown retine? module or built in? unload or shutdown?

It needs to register a reboot notifier, which gdth does.

However, the notifier is only called on reboot, so it also needs to clean up correctly on module exit as well.

The alternative for GDTH would be to process the SCSI SYNCHRONIZE CACHE command. That's done by a shutdown notifier from sd, so the correct thing would always get done; however it does mean the driver has to be in a condition to process the last sync cache command.

For the quick fix, just keep the current infrastructure and put back the gdth_flush() command where it can be effective.

James

Quoted 47 lines · James Bottomley

Quoted 29 lines · Boaz Harrosh

Quoted 28 lines · James Bottomley

Quoted 27 lines · Boaz Harrosh

Quoted 11 lines · Boaz Harrosh

Quoted 10 lines · James Bottomley

Review

Quoted 2 lines · Boaz Harrosh

(quoted diff) +0 -2

-	gdth_flush(ha);
-

This piece doesn't look right. gdth_flush() forces the internal cache to disk backing. If you remove it, you're taking the chance that the machine will be powered off without a writeback which can cause data corruption.

James

Quoted 8 lines

Yes.
I have more problems reported, with exit, and am just sending one more patch that puts this back in. Which was tested.

So I will resend this one plus one new one.

Boaz

The gdth driver would do a register_reboot_notifier(&gdth_notifier); to a gdth_halt() function, which would then redo half of what gdth_exit does, and wrongly so, and crash.

Are we guaranteed in todays kernel that modules .exit function be called on an halt or reboot? If so then there is no need for duplications and the gdth_halt() should go.

Quoted 6 lines

Quoted 3 lines

No. The __exit section is actually discardable if you promise never to remove the module.

I don't understand please explain.
What does a driver need to do if it needs a consistent shutdown retine? module or built in? unload or shutdown?

It needs to register a reboot notifier, which gdth does.

However, the notifier is only called on reboot, so it also needs to clean up correctly on module exit as well.

The alternative for GDTH would be to process the SCSI SYNCHRONIZE CACHE command. That's done by a shutdown notifier from sd, so the correct thing would always get done; however it does mean the driver has to be in a condition to process the last sync cache command.

Quoted 6 lines

For the quick fix, just keep the current infrastructure and put back the gdth_flush() command where it can be effective.

James

Totally untested.

---
From: Boaz Harrosh <[email protected]>
Subject: [PATCH] gdth: bugfix for the at-exit problems

gdth_exit would first remove all cards then stop the timer and would not sync with the timer function. This caused a crash in gdth_timer() when module was unloaded.
So del_timer_sync the timer before we delete the cards.

also the reboot notifier function would crash. So unify the exit and halt functions with a gdth_shutdown() that's called by both.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/scsi/gdth.c |   99 ++++++++++++++++++++------------------------------
 1 files changed, 40 insertions(+), 59 deletions(-)

drivers/scsi/gdth.c +10 -48

index 8eb78be..7bb9b45 100644
@@ -183,7 +183,6 @@ static int gdth_ioctl(struct inode *inode, struct file *filep,
                       unsigned int cmd, unsigned long arg);
 
 static void gdth_flush(gdth_ha_str *ha);
-static int gdth_halt(struct notifier_block *nb, ulong event, void *buf);
 static int gdth_queuecommand(Scsi_Cmnd *scp,void (*done)(Scsi_Cmnd *));
 static int __gdth_queuecommand(gdth_ha_str *ha, struct scsi_cmnd *scp,
 				struct gdth_cmndinfo *cmndinfo);

@@ -418,12 +417,6 @@ static inline void gdth_set_sglist(struct scsi_cmnd *cmd,
 #include "gdth_proc.h"
 #include "gdth_proc.c"
 
-/* notifier block to get a notify on system shutdown/halt/reboot */
-static struct notifier_block gdth_notifier = {
-    gdth_halt, NULL, 0
-};
-static int notifier_disabled = 0;
-
 static gdth_ha_str *gdth_find_ha(int hanum)
 {
 	gdth_ha_str *ha;

@@ -3793,6 +3786,8 @@ static void gdth_timeout(ulong data)
     gdth_ha_str *ha;
     ulong flags;
 
+    BUG_ON(list_empty(&gdth_instances));
+
     ha = list_first_entry(&gdth_instances, gdth_ha_str, list);
     spin_lock_irqsave(&ha->smp_lock, flags);
 

@@ -4668,45 +4663,6 @@ static void gdth_flush(gdth_ha_str *ha)
     }
 }
 
-/* shutdown routine */
-static int gdth_halt(struct notifier_block *nb, ulong event, void *buf)
-{
-    gdth_ha_str *ha;
-#ifndef __alpha__
-    gdth_cmd_str    gdtcmd;
-    char            cmnd[MAX_COMMAND_SIZE];   
-#endif
-
-    if (notifier_disabled)
-        return NOTIFY_OK;
-
-    TRACE2(("gdth_halt() event %d\n",(int)event));
-    if (event != SYS_RESTART && event != SYS_HALT && event != SYS_POWER_OFF)
-        return NOTIFY_DONE;
-
-    notifier_disabled = 1;
-    printk("GDT-HA: Flushing all host drives .. ");
-    list_for_each_entry(ha, &gdth_instances, list) {
-        gdth_flush(ha);
-
-#ifndef __alpha__
-        /* controller reset */
-        memset(cmnd, 0xff, MAX_COMMAND_SIZE);
-        gdtcmd.BoardNode = LOCALBOARD;
-        gdtcmd.Service = CACHESERVICE;
-        gdtcmd.OpCode = GDT_RESET;
-        TRACE2(("gdth_halt(): reset controller %d\n", ha->hanum));
-        gdth_execute(ha->shost, &gdtcmd, cmnd, 10, NULL);
-#endif
-    }
-    printk("Done.\n");
-
-#ifdef GDTH_STATISTICS
-    del_timer(&gdth_timer);
-#endif
-    return NOTIFY_OK;
-}
-
 /* configure lun */
 static int gdth_slave_configure(struct scsi_device *sdev)
 {

@@ -5141,13 +5097,13 @@ static void gdth_remove_one(gdth_ha_str *ha)
 
 	scsi_remove_host(shp);
 
+	gdth_flush(ha);
+
 	if (ha->sdev) {
 		scsi_free_host_dev(ha->sdev);
 		ha->sdev = NULL;
 	}
 
-	gdth_flush(ha);
-
 	if (shp->irq)
 		free_irq(shp->irq,ha);
 

@@ -5173,6 +5129,40 @@ static void gdth_remove_one(gdth_ha_str *ha)
 	scsi_host_put(shp);
 }
 
+static void gdth_shutdown(void);
+static int gdth_halt(struct notifier_block *nb, ulong event, void *buf)
+{
+	TRACE2(("gdth_halt() event %d\n",(int)event));
+	if (event != SYS_RESTART && event != SYS_HALT && event != SYS_POWER_OFF)
+		return NOTIFY_DONE;

Quoted 15 lines

---
From: Boaz Harrosh <[email protected]>
Subject: [PATCH] gdth: bugfix for the at-exit problems

gdth_exit would first remove all cards then stop the timer and would not sync with the timer function. This caused a crash in gdth_timer() when module was unloaded.
So del_timer_sync the timer before we delete the cards.

also the reboot notifier function would crash. So unify the exit and halt functions with a gdth_shutdown() that's called by both.

Signed-off-by Boaz Harrosh <[email protected]>

---

drivers/scsi/gdth.c static void gdth_remove_one(gdth_ha_str *ha) · line 5147

Quoted 5 lines

drivers/scsi/gdth.c +5 -0

+static struct notifier_block gdth_notifier = {
+    gdth_halt, NULL, 0
+};
+
+bool gdth_shutdown_done;

Static police alert! Just make it static and move it into gdth_shutdown()

drivers/scsi/gdth.c static void gdth_remove_one(gdth_ha_str *ha) · line 5156

Quoted 9 lines

drivers/scsi/gdth.c +9 -0

+static void gdth_shutdown()
+{
+	gdth_ha_str *ha;
+	if (gdth_shutdown_done)
+		return;
+
+	gdth_shutdown_done = true;
+	unregister_chrdev(major,"gdth");
+	unregister_reboot_notifier(&gdth_notifier);

I'm not sure you can do this, aren't reboot notifiers called with the rwsem held? In which case the unregister which also takes the rwsem will hang the system.

James

This is a bugfix for the 2.6.24.x stable releases.

gdth_exit would first remove all cards then stop the timer and would not sync with the timer function. This caused a crash in gdth_timer() when module was unloaded.
So del_timer_sync the timer before we delete the cards.

also the reboot notifier function would crash. So unify the exit and halt functions with a gdth_shutdown() that's called by both.

Quoted 7 lines · Boaz Harrosh

This is a bugfix for the 2.6.24.x stable releases.

gdth_exit would first remove all cards then stop the timer and would not sync with the timer function. This caused a crash in gdth_timer() when module was unloaded.
So del_timer_sync the timer before we delete the cards.

Quoted 3 lines

also the reboot notifier function would crash. So unify the exit and halt functions with a gdth_shutdown() that's called by both.

The patch looks fine now, thanks. Can we actually get a tester just to make sure there's nothing I missed.

James

Quoted 29 lines · Boaz Harrosh

Quoted 28 lines · James Bottomley

Quoted 27 lines · Boaz Harrosh

Quoted 11 lines · Boaz Harrosh

Quoted 10 lines · James Bottomley

Review

Quoted 2 lines · Boaz Harrosh

(quoted diff) +0 -2

-	gdth_flush(ha);
-

This piece doesn't look right. gdth_flush() forces the internal cache to disk backing. If you remove it, you're taking the chance that the machine will be powered off without a writeback which can cause data corruption.

James

Quoted 8 lines

Yes.
I have more problems reported, with exit, and am just sending one more patch that puts this back in. Which was tested.

So I will resend this one plus one new one.

Boaz

The gdth driver would do a register_reboot_notifier(&gdth_notifier); to a gdth_halt() function, which would then redo half of what gdth_exit does, and wrongly so, and crash.

Are we guaranteed in todays kernel that modules .exit function be called on an halt or reboot? If so then there is no need for duplications and the gdth_halt() should go.

Quoted 6 lines

Quoted 3 lines

No. The __exit section is actually discardable if you promise never to remove the module.

I don't understand please explain.
What does a driver need to do if it needs a consistent shutdown retine? module or built in? unload or shutdown?

It needs to register a reboot notifier, which gdth does.

However, the notifier is only called on reboot, so it also needs to clean up correctly on module exit as well.

The alternative for GDTH would be to process the SCSI SYNCHRONIZE CACHE

command. That's done by a shutdown notifier from sd, so the correct thing would always get done; however it does mean the driver has to be in a condition to process the last sync cache command.

For the quick fix, just keep the current infrastructure and put back the gdth_flush() command where it can be effective.

James

Quoted 6 lines · James Bottomley

It needs to register a reboot notifier, which gdth does.

However, the notifier is only called on reboot, so it also needs to clean up correctly on module exit as well.

The alternative for GDTH would be to process the SCSI SYNCHRONIZE CACHE

Why would we think that the controller does not support this command is it not in the mandatory section of the standard?

Quoted 3 lines

command. That's done by a shutdown notifier from sd, so the correct thing would always get done; however it does mean the driver has to be in a condition to process the last sync cache command.

Why would it not be ready? what do other drivers do?
The drivers is ready until the very last module's .exit. Is that good enough?

Quoted 4 lines

For the quick fix, just keep the current infrastructure and put back the gdth_flush() command where it can be effective.

Just did. But if needed I would prefer to emulate the SCSI SYNCHRONIZE CACHE command and not that boot notifier thing. Please advise.

Quoted 3 lines

I don't understand please explain.
What does a driver need to do if it needs a consistent shutdown retine? module or built in? unload or shutdown?

It needs to register a reboot notifier, which gdth does.