gdth oops

DISCUSSION James Bottomley 2 participants 3 messages May 4, 2008 – May 5, 2008 1 message not in the archive SCSI

lore

Share & tools

lore

James Bottomley

author maintainer

2008-05-04 17:32 UTC #1 lore raw

In reply to a message not in the archive

Quoted 51 lines · On Sun, 2008-05-04 at 00:52 +0200, [email protected] wrote:

hi,
thanks again for the pointer to kerneloops search.
i have probably found another - and for this i can`t find anything on lkml or elsewhere.
while doing some module load/unload tests, i noticed that "modprobe gdth;modprobe -r gdth" seems to leak something - i`m getting
Error: Driver 'gdth' is already registered, aborting...
on next modprobe .
i checked, if module is unloaded correctly before, and lsmod doesn`t list it - so this looks like an issue to me.
furthermore, i can reproduce the oops below very easily - just load/unload gdth in a loop and load/unload some other modules - this reproduceably oopses with the message below:
this is on a system WITHOUT such controller, so not a big issue - but maybe worth reporting, though !?
regards
roland
GDT-HA: Storage RAID Controller Driver. Version: 3.05
Error: Driver 'gdth' is already registered, aborting...
GDT-HA: Storage RAID Controller Driver. Version: 3.05
Error: Driver 'gdth' is already registered, aborting...
GDT-HA: Storage RAID Controller Driver. Version: 3.05
BUG: unable to handle kernel paging request at 839845ab
IP: [<c01d9dc1>] kobject_put+0xa/0x41
*pde = 00000000
Oops: 0000 [#1] SMP
Modules linked in: gdth(+) kafs af_rxrpc ipv6 button battery ac loop dm_mod e100 uhci_hcd ehci_hcd mii ide_pci_generic i2c_i801 intel_agp usbcore ide_core i2c_core agpgart rng_core parport_pc lp parport reiserfs edd fan thermal processor sg megaraid_mbox megaraid_mm sd_mod scsi_mod [last unloaded: bfs]
Pid: 4732, comm: modprobe Not tainted (2.6.25-default #1)
EIP: 0060:[<c01d9dc1>] EFLAGS: 00010286 CPU: 0
EIP is at kobject_put+0xa/0x41
EAX: 8398458b EBX: 8398458b ECX: f8e3eb55 EDX: f8c5f150
ESI: f8e41150 EDI: f8e41150 EBP: f6575de4 ESP: f6575de0
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process modprobe (pid: 4732, ti=f6574000 task=f713cd90 task.ti=f6574000)
Stack: f8e41120 f6575dec c023dc44 f6575e08 c023dc99 f8e41280 f8e41280 f8e41120
       f8e41280 f8e41150 f6575e1c c01e6da7 f8e41280 f8e41280 f8e3ea14 f6575e4c
       f8bee251 00000013 f7105b20 00000001 f8e41280 0000001e f8e32878 f6575e4c
Call Trace:
 [<c023dc44>] ? put_driver+0xb/0xd
 [<c023dc99>] ? driver_register+0x53/0xcd
 [<c01e6da7>] ? __pci_register_driver+0x35/0x62
 [<f8bee251>] ? gdth_init+0x5f7/0x68a [gdth]
 [<c0142f6f>] ? sys_init_module+0x17e2/0x193c
 [<c015403c>] ? generic_file_aio_read+0x489/0x4c1
 [<f885259c>] ? scsi_get_host_dev+0x0/0x78 [scsi_mod]

Quoted 8 lines

 [<c01c595d>] ? security_file_permission+0xf/0x11
 [<c016f156>] ? do_sync_read+0x0/0xe9
 [<c0104891>] ? sysenter_past_esp+0x6a/0x99
 [<c02d0000>] ? wait_for_common+0xb1/0x13e
 =======================
Code: 00 55 89 e5 53 31 db e8 0d 28 f9 ff 85 c0 74 0c ba 34 9e 3c c0 89 c3 e8 81 ff ff ff 89 d8 5b 5d c3 55 85 c0 89 e5 53 89 c3 74 32 <f6> 40 20 01 75 1f 50 ff 30 68 10 c5 37 c0 e8 77 be f4 ff ba 47
EIP: [<c01d9dc1>] kobject_put+0xa/0x41 SS:ESP 0068:f6575de0
---[ end trace 2bee9a2892ac67fa ]---

Yes, the problem is that the driver doesn't unregister it's PCI driver if it fails in gdth_init, leaving a registered PCI driver in free'd memory.

This is probably the best fix: a modern driver should never die in init unless there's some failure because soft binding or hotplug can come along later with the actual device.

I could make the remain in place conditional on CONFIG_PCI, but that would produce a strange driver that would behave differently depending on how you compile it.

So, try this as the fix.

James

---

drivers/scsi/gdth.c +34 ~~-17~~

index 8e2e964..46771d4 100644
@@ -550,7 +550,6 @@ static int __init gdth_search_isa(ulong32 bios_adr)
 #endif /* CONFIG_ISA */
 
 #ifdef CONFIG_PCI
-static bool gdth_pci_registered;
 
 static bool gdth_search_vortex(ushort device)
 {

@@ -3724,6 +3723,8 @@ static void gdth_log_event(gdth_evt_data *dvr, char *buffer)
 }
 
 #ifdef GDTH_STATISTICS
+static unchar	gdth_timer_running;
+
 static void gdth_timeout(ulong data)
 {
     ulong32 i;

@@ -3731,7 +3732,10 @@ static void gdth_timeout(ulong data)
     gdth_ha_str *ha;
     ulong flags;
 
-    BUG_ON(list_empty(&gdth_instances));
+    if(unlikely(list_empty(&gdth_instances))) {
+	    gdth_timer_running = 0;
+	    return;
+    }
 
     ha = list_first_entry(&gdth_instances, gdth_ha_str, list);
     spin_lock_irqsave(&ha->smp_lock, flags);

@@ -3751,6 +3755,22 @@ static void gdth_timeout(ulong data)
     add_timer(&gdth_timer);
     spin_unlock_irqrestore(&ha->smp_lock, flags);
 }
+
+static void gdth_timer_init(void)
+{
+	if (gdth_timer_running)
+		return;
+	gdth_timer_running = 1;
+	TRACE2(("gdth_detect(): Initializing timer !\n"));
+	gdth_timer.expires = jiffies + HZ;
+	gdth_timer.data = 0L;
+	gdth_timer.function = gdth_timeout;
+	add_timer(&gdth_timer);
+}
+#else
+static inline void gdth_timer_init(void)
+{
+}
 #endif
 
 static void __init internal_setup(char *str,int *ints)

@@ -4735,6 +4755,7 @@ static int __init gdth_isa_probe_one(ulong32 isa_bios)
 	if (error)
 		goto out_free_coal_stat;
 	list_add_tail(&ha->list, &gdth_instances);
+	gdth_timer_init();
 
 	scsi_scan_host(shp);

@@ -4865,6 +4886,7 @@ static int __init gdth_eisa_probe_one(ushort eisa_slot)
 	if (error)
 		goto out_free_coal_stat;
 	list_add_tail(&ha->list, &gdth_instances);
+	gdth_timer_init();
 
 	scsi_scan_host(shp);

@@ -5011,6 +5033,7 @@ static int gdth_pci_probe_one(gdth_pci_str *pcistr,
 	list_add_tail(&ha->list, &gdth_instances);
 
 	pci_set_drvdata(ha->pdev, ha);
+	gdth_timer_init();
 
 	scsi_scan_host(shp);

@@ -5110,6 +5133,7 @@ static int __init gdth_init(void)
 	/* initializations */
 	gdth_polling = TRUE;
 	gdth_clear_events();
+	init_timer(&gdth_timer);
 
 	/* As default we do not probe for EISA or ISA controllers */
 	if (probe_eisa_isa) {

@@ -5132,23 +5156,17 @@ static int __init gdth_init(void)
 
 #ifdef CONFIG_PCI
 	/* scanning for PCI controllers */
-	if (pci_register_driver(&gdth_pci_driver) == 0)
-		gdth_pci_registered = true;
+	if (pci_register_driver(&gdth_pci_driver)) {
+		gdth_ha_str *ha;
+
+		list_for_each_entry(ha, &gdth_instances, list)
+			gdth_remove_one(ha);
+		return -ENODEV;
+	}
 #endif /* CONFIG_PCI */
 
 	TRACE2(("gdth_detect() %d controller detected\n", gdth_ctr_count));
 
-	if (list_empty(&gdth_instances))
-		return -ENODEV;
-
-#ifdef GDTH_STATISTICS
-	TRACE2(("gdth_detect(): Initializing timer !\n"));
-	init_timer(&gdth_timer);
-	gdth_timer.expires = jiffies + HZ;
-	gdth_timer.data = 0L;
-	gdth_timer.function = gdth_timeout;
-	add_timer(&gdth_timer);
-#endif
 	major = register_chrdev(0,"gdth", &gdth_fops);
 	register_reboot_notifier(&gdth_notifier);
 	gdth_polling = FALSE;

@@ -5167,8 +5185,7 @@ static void __exit gdth_exit(void)
 #endif
 
 #ifdef CONFIG_PCI
-	if (gdth_pci_registered)
-		pci_unregister_driver(&gdth_pci_driver);
+	pci_unregister_driver(&gdth_pci_driver);
 #endif
 
 	list_for_each_entry(ha, &gdth_instances, list)

Roland

2008-05-04 23:37 UTC #2 lore raw

Hi James,

Quoted 3 lines

So, try this as the fix.
James

thanks!

i`d like to report that your patch resolves the reported issues.

regards
roland

----- Original Message ----- From: "James Bottomley" <[email protected]>
To: <[email protected]>
Cc: <[email protected]>; "linux-scsi" <[email protected]>
Sent: Sunday, May 04, 2008 7:32 PM
Subject: Re: gdth oops

Quoted 215 lines · On Sun, 2008-05-04 at 00:52 +0200, [email protected] wrote:

Quoted 53 lines

hi,
thanks again for the pointer to kerneloops search.
i have probably found another - and for this i can`t find anything on lkml or elsewhere.
while doing some module load/unload tests, i noticed that "modprobe gdth;modprobe -r gdth" seems to leak something - i`m getting
Error: Driver 'gdth' is already registered, aborting...
on next modprobe .
i checked, if module is unloaded correctly before, and lsmod doesn`t list it - so this looks like an issue to me.
furthermore, i can reproduce the oops below very easily - just load/unload gdth in a loop and load/unload some other modules - this reproduceably oopses with the message below:
this is on a system WITHOUT such controller, so not a big issue - but maybe worth reporting, though !?
regards
roland
GDT-HA: Storage RAID Controller Driver. Version: 3.05
Error: Driver 'gdth' is already registered, aborting...
GDT-HA: Storage RAID Controller Driver. Version: 3.05
Error: Driver 'gdth' is already registered, aborting...
GDT-HA: Storage RAID Controller Driver. Version: 3.05
BUG: unable to handle kernel paging request at 839845ab
IP: [<c01d9dc1>] kobject_put+0xa/0x41
*pde = 00000000
Oops: 0000 [#1] SMP
Modules linked in: gdth(+) kafs af_rxrpc ipv6 button battery ac loop dm_mod e100 uhci_hcd ehci_hcd mii ide_pci_generic i2c_i801 intel_agp usbcore ide_core i2c_core agpgart rng_core parport_pc lp parport reiserfs edd fan thermal processor sg megaraid_mbox megaraid_mm sd_mod scsi_mod [last unloaded: bfs]
Pid: 4732, comm: modprobe Not tainted (2.6.25-default #1)
EIP: 0060:[<c01d9dc1>] EFLAGS: 00010286 CPU: 0
EIP is at kobject_put+0xa/0x41
EAX: 8398458b EBX: 8398458b ECX: f8e3eb55 EDX: f8c5f150
ESI: f8e41150 EDI: f8e41150 EBP: f6575de4 ESP: f6575de0
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process modprobe (pid: 4732, ti=f6574000 task=f713cd90 task.ti=f6574000)
Stack: f8e41120 f6575dec c023dc44 f6575e08 c023dc99 f8e41280 f8e41280 f8e41120
       f8e41280 f8e41150 f6575e1c c01e6da7 f8e41280 f8e41280 f8e3ea14 
f6575e4c
       f8bee251 00000013 f7105b20 00000001 f8e41280 0000001e f8e32878 
f6575e4c
Call Trace:
 [<c023dc44>] ? put_driver+0xb/0xd
 [<c023dc99>] ? driver_register+0x53/0xcd
 [<c01e6da7>] ? __pci_register_driver+0x35/0x62
 [<f8bee251>] ? gdth_init+0x5f7/0x68a [gdth]
 [<c0142f6f>] ? sys_init_module+0x17e2/0x193c
 [<c015403c>] ? generic_file_aio_read+0x489/0x4c1
 [<f885259c>] ? scsi_get_host_dev+0x0/0x78 [scsi_mod]

Quoted 8 lines

 [<c01c595d>] ? security_file_permission+0xf/0x11
 [<c016f156>] ? do_sync_read+0x0/0xe9
 [<c0104891>] ? sysenter_past_esp+0x6a/0x99
 [<c02d0000>] ? wait_for_common+0xb1/0x13e
 =======================
Code: 00 55 89 e5 53 31 db e8 0d 28 f9 ff 85 c0 74 0c ba 34 9e 3c c0 89 c3 e8 81 ff ff ff 89 d8 5b 5d c3 55 85 c0 89 e5 53 89 c3 74 32 <f6> 40 20 01 75 1f 50 ff 30 68 10 c5 37 c0 e8 77 be f4 ff ba 47
EIP: [<c01d9dc1>] kobject_put+0xa/0x41 SS:ESP 0068:f6575de0
---[ end trace 2bee9a2892ac67fa ]---

Yes, the problem is that the driver doesn't unregister it's PCI driver if it fails in gdth_init, leaving a registered PCI driver in free'd memory.

This is probably the best fix: a modern driver should never die in init unless there's some failure because soft binding or hotplug can come along later with the actual device.

I could make the remain in place conditional on CONFIG_PCI, but that would produce a strange driver that would behave differently depending on how you compile it.

So, try this as the fix.

James

---

drivers/scsi/gdth.c +2 -1

index 8e2e964..46771d4 100644
@@ -550,7 +550,6 @@ static int __init gdth_search_isa(ulong32 bios_adr)
 #endif /* CONFIG_ISA */
 
 #ifdef CONFIG_PCI
-static bool gdth_pci_registered;
 
 static bool gdth_search_vortex(ushort device)
 {

@@ -3724,6 +3723,8 @@ static void gdth_log_event(gdth_evt_data *dvr, char 
 *buffer)
 }
 
 #ifdef GDTH_STATISTICS
+static unchar gdth_timer_running;
+
 static void gdth_timeout(ulong data)
 {

    ulong32 i;

drivers/scsi/gdth.c +32 ~~-16~~

@@ -3731,7 +3732,10 @@ static void gdth_timeout(ulong data)
    gdth_ha_str *ha;
    ulong flags;
 
-    BUG_ON(list_empty(&gdth_instances));
+    if(unlikely(list_empty(&gdth_instances))) {
+     gdth_timer_running = 0;
+     return;
+    }
 
    ha = list_first_entry(&gdth_instances, gdth_ha_str, list);
    spin_lock_irqsave(&ha->smp_lock, flags);

@@ -3751,6 +3755,22 @@ static void gdth_timeout(ulong data)
    add_timer(&gdth_timer);
    spin_unlock_irqrestore(&ha->smp_lock, flags);
 }
+
+static void gdth_timer_init(void)
+{
+ if (gdth_timer_running)
+ return;
+ gdth_timer_running = 1;
+ TRACE2(("gdth_detect(): Initializing timer !\n"));
+ gdth_timer.expires = jiffies + HZ;
+ gdth_timer.data = 0L;
+ gdth_timer.function = gdth_timeout;
+ add_timer(&gdth_timer);
+}
+#else
+static inline void gdth_timer_init(void)
+{
+}
 #endif
 
 static void __init internal_setup(char *str,int *ints)

@@ -4735,6 +4755,7 @@ static int __init gdth_isa_probe_one(ulong32 
 isa_bios)
 if (error)
 goto out_free_coal_stat;
 list_add_tail(&ha->list, &gdth_instances);
+ gdth_timer_init();
 
 scsi_scan_host(shp);

@@ -4865,6 +4886,7 @@ static int __init gdth_eisa_probe_one(ushort 
 eisa_slot)
 if (error)
 goto out_free_coal_stat;
 list_add_tail(&ha->list, &gdth_instances);
+ gdth_timer_init();
 
 scsi_scan_host(shp);

@@ -5011,6 +5033,7 @@ static int gdth_pci_probe_one(gdth_pci_str *pcistr,
 list_add_tail(&ha->list, &gdth_instances);
 
 pci_set_drvdata(ha->pdev, ha);
+ gdth_timer_init();
 
 scsi_scan_host(shp);

@@ -5110,6 +5133,7 @@ static int __init gdth_init(void)
 /* initializations */
 gdth_polling = TRUE;
 gdth_clear_events();
+ init_timer(&gdth_timer);
 
 /* As default we do not probe for EISA or ISA controllers */
 if (probe_eisa_isa) {

@@ -5132,23 +5156,17 @@ static int __init gdth_init(void)
 
 #ifdef CONFIG_PCI
 /* scanning for PCI controllers */
- if (pci_register_driver(&gdth_pci_driver) == 0)
- gdth_pci_registered = true;
+ if (pci_register_driver(&gdth_pci_driver)) {
+ gdth_ha_str *ha;
+
+ list_for_each_entry(ha, &gdth_instances, list)
+ gdth_remove_one(ha);
+ return -ENODEV;
+ }
 #endif /* CONFIG_PCI */
 
 TRACE2(("gdth_detect() %d controller detected\n", gdth_ctr_count));
 
- if (list_empty(&gdth_instances))
- return -ENODEV;
-
-#ifdef GDTH_STATISTICS
- TRACE2(("gdth_detect(): Initializing timer !\n"));
- init_timer(&gdth_timer);
- gdth_timer.expires = jiffies + HZ;
- gdth_timer.data = 0L;
- gdth_timer.function = gdth_timeout;
- add_timer(&gdth_timer);
-#endif
 major = register_chrdev(0,"gdth", &gdth_fops);
 register_reboot_notifier(&gdth_notifier);
 gdth_polling = FALSE;

@@ -5167,8 +5185,7 @@ static void __exit gdth_exit(void)
 #endif
 
 #ifdef CONFIG_PCI
- if (gdth_pci_registered)
- pci_unregister_driver(&gdth_pci_driver);
+ pci_unregister_driver(&gdth_pci_driver);
 #endif
 
 list_for_each_entry(ha, &gdth_instances, list)

James Bottomley

author maintainer

2008-05-05 03:33 UTC #3 lore raw

Quoted 9 lines · Roland

Hi James,
Quoted 3 lines
So, try this as the fix.
James
thanks!
i`d like to report that your patch resolves the reported issues.

Great, thanks. I'll send it as two patches, since fixing the PCI driver problem uncovered another one with the timers.

James