gdth new set of patches for 2.6.24 stable

PATCH In review Boaz Harrosh 7 participants 19 messages Feb 14, 2008 – Feb 19, 2008

Subsystems (2)

x86 SCSI

lore

Share & tools

lore

Boaz Harrosh

author

2008-02-14 18:48 UTC #0 lore raw

Submitted are a new set of patches, that fix lots of problems with the gdth driver.

It fixes the following problems:
- scan for drives on hosts. (Already in mainline)
- truly fixes the exit/reboot problems but does call flush() before reboot.
- fix crash when accessing array with icpcon management application.
- fix crash when doing $ cat /proc/sys/gdth/0.
This one still has the below WARN_ON in messages (see <gdth_info> below) So there is one more thing hiding in there.
- use pci_get_device
One of the testers requested if we can also put the move to pci_get_device patch with removal of dependency on PCI_LEGACY, to the stable release.

The patches are for and based on Linux-2.6.24. here is the list of patches:
[PATCH 1/5] gdth: update deprecated pci_find_device
[PATCH 2/5] gdth: scan for scsi devices
[PATCH 3/5] gdth: bugfix for the at-exit problems
[PATCH 4/5] gdth: fix to internal commands execution
[PATCH 5/5] gdth: remove gdth cooked up command accessors

Please all test and report your findings.

Thanks in advance
Boaz

---
<gdth_info>

  WARNING: at arch/x86/kernel/pci-dma_32.c:66 dma_free_coherent()
  Pid: 5501, comm: cat Not tainted 2.6.24 #43
   [<c0107137>] dma_free_coherent+0x93/0x95
   [<c025ef73>] gdth_ioctl_free+0x4c/0x69
   [<c0264a36>] gdth_proc_info+0x165f/0x182c
   [<c0111f7a>] update_curr+0xeb/0xf2
   [<c01132aa>] task_rq_lock+0x29/0x50
   [<c0113706>] try_to_wake_up+0x42/0x342
   [<c0113706>] try_to_wake_up+0x42/0x342
   [<c0111a9f>] __wake_up_common+0x46/0x6d
   [<c0113569>] __wake_up+0x32/0x42
   [<c022dad9>] n_tty_receive_buf+0x2e8/0xe97
   [<c022dad9>] n_tty_receive_buf+0x2e8/0xe97
   [<c0111f0a>] update_curr+0x7b/0xf2
   [<c0112625>] enqueue_task_fair+0x27/0x30
   [<c0111783>] enqueue_task+0xa/0x14
   [<c025e351>] proc_scsi_read+0x29/0x3d
   [<c025e328>] proc_scsi_read+0x0/0x3d
   [<c0189704>] proc_file_read+0x1c6/0x279
   [<c018953e>] proc_file_read+0x0/0x279
   [<c0185eca>] proc_reg_read+0x53/0x71
   [<c0185e77>] proc_reg_read+0x0/0x71
   [<c0159968>] vfs_read+0x85/0x11b
   [<c0159d9d>] sys_read+0x41/0x6a
   [<c0102822>] sysenter_past_esp+0x5f/0x85
 </gdth_info>

Boaz Harrosh

author

2008-02-14 18:57 UTC #1 lore raw

PATCH 1/5 gdth: update deprecated pci_find_device

From: Sergio Luis <[email protected]>

Fix compilation warning in gdth.c, which was using the deprecated pci_find_device.

drivers/scsi/gdth.c:645: warning: 'pci_find_device' is deprecated (declared at include/linux/pci.h:495)

Changing it to use pci_get_device, instead.

Signed-off-by Sergio Luis <[email protected]>

Signed-off-by James Bottomley <[email protected]>

---
 drivers/scsi/Kconfig |    2 +-
 drivers/scsi/gdth.c  |    7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

drivers/scsi/Kconfig +1 -1

index 184c7ae..46fcb82 100644
@@ -725,7 +725,7 @@ config SCSI_FD_MCS
 
 config SCSI_GDTH
 	tristate "Intel/ICP (former GDT SCSI Disk Array) RAID Controller support"
-	depends on (ISA || EISA || PCI) && SCSI && ISA_DMA_API && PCI_LEGACY
+	depends on (ISA || EISA || PCI) && SCSI && ISA_DMA_API
 	---help---
 	  Formerly called GDT SCSI Disk Array Controller Support.

drivers/scsi/gdth.c +5 -2

index b253b8c..5f0e054 100644
@@ -643,12 +643,15 @@ static void __init gdth_search_dev(gdth_pci_str *pcistr, ushort *cnt,
           *cnt, vendor, device));
 
     pdev = NULL;
-    while ((pdev = pci_find_device(vendor, device, pdev)) 
+    while ((pdev = pci_get_device(vendor, device, pdev))
            != NULL) {
         if (pci_enable_device(pdev))
             continue;
-        if (*cnt >= MAXHA)
+        if (*cnt >= MAXHA) {
+            pci_dev_put(pdev);
             return;
+        }
+
         /* GDT PCI controller found, resources are already in pdev */
         pcistr[*cnt].pdev = pdev;
         pcistr[*cnt].irq = pdev->irq;

Signature · 1 line

1.5.3.3

Jeff Garzik

2008-02-14 20:14 UTC #2 lore raw

Re: [PATCH 1/5] gdth: update deprecated pci_find_device

Quoted 37 lines · Boaz Harrosh

From: Sergio Luis <[email protected]>
Fix compilation warning in gdth.c, which was using the deprecated pci_find_device.
drivers/scsi/gdth.c:645: warning: 'pci_find_device' is deprecated (declared at include/linux/pci.h:495)
Changing it to use pci_get_device, instead.
Signed-off-by Sergio Luis <[email protected]>
Signed-off-by James Bottomley <[email protected]>
---
 drivers/scsi/Kconfig |    2 +-
 drivers/scsi/gdth.c  |    7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)
drivers/scsi/Kconfig +1 -1
index 184c7ae..46fcb82 100644
@@ -725,7 +725,7 @@ config SCSI_FD_MCS
 
 config SCSI_GDTH
 	tristate "Intel/ICP (former GDT SCSI Disk Array) RAID Controller support"
-	depends on (ISA || EISA || PCI) && SCSI && ISA_DMA_API && PCI_LEGACY
+	depends on (ISA || EISA || PCI) && SCSI && ISA_DMA_API
 	---help---
 	  Formerly called GDT SCSI Disk Array Controller Support.
 
drivers/scsi/gdth.c +0 -0
index b253b8c..5f0e054 100644
@@ -643,12 +643,15 @@ static void __init gdth_search_dev(gdth_pci_str *pcistr, ushort *cnt,
           *cnt, vendor, device));
 
     pdev = NULL;

drivers/scsi/gdth.c static void __init gdth_search_dev(gdth_pci_str *pcistr, ushort *cnt, · line 653

Quoted 10 lines

drivers/scsi/gdth.c +4 -2

-    while ((pdev = pci_find_device(vendor, device, pdev)) 
+    while ((pdev = pci_get_device(vendor, device, pdev))
            != NULL) {
         if (pci_enable_device(pdev))
             continue;
-        if (*cnt >= MAXHA)
+        if (*cnt >= MAXHA) {
+            pci_dev_put(pdev);
             return;
+        }

This patch is already upstream... (unfortunately)

Jeff

James Bottomley

maintainer

2008-02-14 20:45 UTC #3 lore raw

Re: [PATCH 1/5] gdth: update deprecated pci_find_device

Quoted 5 lines · Jeff Garzik

Quoted 4 lines · Boaz Harrosh
From: Sergio Luis <[email protected]>
Fix compilation warning in gdth.c, which was using the deprecated pci_find_device.

[...]

Quoted 1 line

This patch is already upstream... (unfortunately)

I think, in spite of the cover name (patches for stable), these are for the set of testers who're based on 2.6.24.

James

Jeff Garzik

2008-02-14 20:56 UTC #4 lore raw

Re: [PATCH 1/5] gdth: update deprecated pci_find_device

Quoted 11 lines · James Bottomley

Quoted 5 lines · Jeff Garzik
Quoted 4 lines · Boaz Harrosh
From: Sergio Luis <[email protected]>
Fix compilation warning in gdth.c, which was using the deprecated pci_find_device.
[...]
Quoted 1 line
This patch is already upstream... (unfortunately)
I think, in spite of the cover name (patches for stable), these are for the set of testers who're based on 2.6.24.

Oops! My mistake, then.

Jeff

Boaz Harrosh

author

2008-02-14 18:53 UTC #5 lore raw

PATCH 2/5 gdth: scan for scsi devices

In reply to Boaz Harrosh

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.

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

Tested-by Joerg Dorchain: <[email protected]>

Tested-by Stefan Priebe <[email protected]>

Tested-by Jon Chelton <[email protected]>

---
 drivers/scsi/gdth.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

drivers/scsi/gdth.c +9 -0

index 5f0e054..4959037 100644
@@ -4841,6 +4841,9 @@ static int __init gdth_isa_probe_one(ulong32 isa_bios)
 	if (error)
 		goto out_free_coal_stat;
 	list_add_tail(&ha->list, &gdth_instances);
+
+	scsi_scan_host(shp);
+
 	return 0;
 
  out_free_coal_stat:

@@ -4968,6 +4971,9 @@ static int __init gdth_eisa_probe_one(ushort eisa_slot)
 	if (error)
 		goto out_free_coal_stat;
 	list_add_tail(&ha->list, &gdth_instances);
+
+	scsi_scan_host(shp);
+
 	return 0;
 
  out_free_ccb_phys:

@@ -5105,6 +5111,9 @@ static int __init gdth_pci_probe_one(gdth_pci_str *pcistr, int ctr)
 	if (error)
 		goto out_free_coal_stat;
 	list_add_tail(&ha->list, &gdth_instances);
+
+	scsi_scan_host(shp);
+
 	return 0;
 
  out_free_coal_stat:

Signature · 1 line

1.5.3.3

Boaz Harrosh

author

2008-02-14 18:56 UTC #6 lore raw

PATCH 3/5 gdth: bugfix for the at-exit problems

In reply to 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.

also the reboot notifier function would crash. So clean that up and fix the crashes.

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

---
 drivers/scsi/gdth.c |   82 +++++++++++++++++---------------------------------
 1 files changed, 28 insertions(+), 54 deletions(-)

drivers/scsi/gdth.c +28 ~~-54~~

index 4959037..0979553 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;

@@ -3796,6 +3789,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);

@@ -4671,45 +4666,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)
 {

@@ -5144,13 +5100,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);

@@ -5176,6 +5132,24 @@ static void gdth_remove_one(gdth_ha_str *ha)
 	scsi_host_put(shp);
 }
 
+static int gdth_halt(struct notifier_block *nb, ulong event, void *buf)
+{
+	gdth_ha_str *ha;
+
+	TRACE2(("gdth_halt() event %d\n", (int)event));
+	if (event != SYS_RESTART && event != SYS_HALT && event != SYS_POWER_OFF)
+		return NOTIFY_DONE;
+
+	list_for_each_entry(ha, &gdth_instances, list)
+		gdth_flush(ha);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block gdth_notifier = {
+    gdth_halt, NULL, 0
+};
+
 static int __init gdth_init(void)
 {
 	if (disable) {

@@ -5238,7 +5212,6 @@ static int __init gdth_init(void)
 	add_timer(&gdth_timer);
 #endif
 	major = register_chrdev(0,"gdth", &gdth_fops);
-	notifier_disabled = 0;
 	register_reboot_notifier(&gdth_notifier);
 	gdth_polling = FALSE;
 	return 0;

@@ -5248,14 +5221,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);

Signature · 1 line

1.5.3.3

Boaz Harrosh

author

2008-02-14 18:57 UTC #7 lore raw

PATCH 4/5 gdth: fix to internal commands execution

In reply to Boaz Harrosh

The recent patch named:
[SCSI] gdth: !use_sg cleanup and use of scsi accessors

has done a bad job in handling internal commands issued by gdth_execute().

Internal commands are issued with device gdth_cmd_str ready made directly to the card, without any mapping or translations of scsi commands. So here I added a gdth_cmd_str pointer to the gdth_cmndinfo private structure which is then copied directly to host.

following this patch is a cleanup that removes the home cooked accessors and reverts them to regular scsi_cmnd accessors. Since they are not used anymore. After review maybe the 2 patches should be squashed together.

FIXME: There is still a problem with gdth_get_info(). as reported there
is a WARN_ON trigerd in dma_free_coherent() when doing:

   $ cat /proc/sys/gdth/0

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

---
 drivers/scsi/gdth.c |   30 ++++++++++++------------------
 drivers/scsi/gdth.h |    1 +
 2 files changed, 13 insertions(+), 18 deletions(-)

drivers/scsi/gdth.c +12 ~~-18~~

index 0979553..077f972 100644
@@ -160,7 +160,7 @@ static void gdth_readapp_event(gdth_ha_str *ha, unchar application,
 static void gdth_clear_events(void);
 
 static void gdth_copy_internal_data(gdth_ha_str *ha, Scsi_Cmnd *scp,
-                                    char *buffer, ushort count, int to_buffer);
+                                    char *buffer, ushort count);
 static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp);
 static int gdth_fill_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, ushort hdrive);

@@ -439,8 +439,8 @@ static struct gdth_cmndinfo *gdth_get_cmndinfo(gdth_ha_str *ha)
 	for (i=0; i<GDTH_MAXCMDS; ++i) {
 		if (ha->cmndinfo[i].index == 0) {
 			priv = &ha->cmndinfo[i];
-			priv->index = i+1;
 			memset(priv, 0, sizeof(*priv));
+			priv->index = i+1;
 			break;
 		}
 	}

@@ -487,7 +487,6 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     gdth_ha_str *ha = shost_priv(sdev->host);
     Scsi_Cmnd *scp;
     struct gdth_cmndinfo cmndinfo;
-    struct scatterlist one_sg;
     DECLARE_COMPLETION_ONSTACK(wait);
     int rval;

@@ -501,13 +500,10 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     /* use request field to save the ptr. to completion struct. */
     scp->request = (struct request *)&wait;
     scp->timeout_per_command = timeout*HZ;
-    sg_init_one(&one_sg, gdtcmd, sizeof(*gdtcmd));
-    gdth_set_sglist(scp, &one_sg);
-    gdth_set_sg_count(scp, 1);
-    gdth_set_bufflen(scp, sizeof(*gdtcmd));
     scp->cmd_len = 12;
     memcpy(scp->cmnd, cmnd, 12);
     cmndinfo.priority = IOCTL_PRI;
+    cmndinfo.internal_cmd_str = gdtcmd;
     cmndinfo.internal_command = 1;
 
     TRACE(("__gdth_execute() cmd 0x%x\n", scp->cmnd[0]));

@@ -2351,7 +2347,7 @@ static void gdth_next(gdth_ha_str *ha)
  * buffers, kmap_atomic() as needed.
  */
 static void gdth_copy_internal_data(gdth_ha_str *ha, Scsi_Cmnd *scp,
-                                    char *buffer, ushort count, int to_buffer)
+                                    char *buffer, ushort count)
 {
     ushort cpcount,i, max_sg = gdth_sg_count(scp);
     ushort cpsum,cpnow;

@@ -2377,10 +2373,7 @@ static void gdth_copy_internal_data(gdth_ha_str *ha, Scsi_Cmnd *scp,
             }
             local_irq_save(flags);
             address = kmap_atomic(sg_page(sl), KM_BIO_SRC_IRQ) + sl->offset;
-            if (to_buffer)
-                memcpy(buffer, address, cpnow);
-            else
-                memcpy(address, buffer, cpnow);
+            memcpy(address, buffer, cpnow);
             flush_dcache_page(sg_page(sl));
             kunmap_atomic(address, KM_BIO_SRC_IRQ);
             local_irq_restore(flags);

@@ -2434,7 +2427,7 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
         strcpy(inq.vendor,ha->oem_name);
         sprintf(inq.product,"Host Drive  #%02d",t);
         strcpy(inq.revision,"   ");
-        gdth_copy_internal_data(ha, scp, (char*)&inq, sizeof(gdth_inq_data), 0);
+        gdth_copy_internal_data(ha, scp, (char*)&inq, sizeof(gdth_inq_data));
         break;
 
       case REQUEST_SENSE:

@@ -2444,7 +2437,7 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
         sd.key       = NO_SENSE;
         sd.info      = 0;
         sd.add_length= 0;
-        gdth_copy_internal_data(ha, scp, (char*)&sd, sizeof(gdth_sense_data), 0);
+        gdth_copy_internal_data(ha, scp, (char*)&sd, sizeof(gdth_sense_data));
         break;
 
       case MODE_SENSE:

@@ -2456,7 +2449,7 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
         mpd.bd.block_length[0] = (SECTOR_SIZE & 0x00ff0000) >> 16;
         mpd.bd.block_length[1] = (SECTOR_SIZE & 0x0000ff00) >> 8;
         mpd.bd.block_length[2] = (SECTOR_SIZE & 0x000000ff);
-        gdth_copy_internal_data(ha, scp, (char*)&mpd, sizeof(gdth_modep_data), 0);
+        gdth_copy_internal_data(ha, scp, (char*)&mpd, sizeof(gdth_modep_data));
         break;
 
       case READ_CAPACITY:

@@ -2466,7 +2459,7 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
         else
             rdc.last_block_no = cpu_to_be32(ha->hdr[t].size-1);
         rdc.block_length  = cpu_to_be32(SECTOR_SIZE);
-        gdth_copy_internal_data(ha, scp, (char*)&rdc, sizeof(gdth_rdcap_data), 0);
+        gdth_copy_internal_data(ha, scp, (char*)&rdc, sizeof(gdth_rdcap_data));
         break;
 
       case SERVICE_ACTION_IN:

@@ -2478,7 +2471,7 @@ static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
             rdc16.last_block_no = cpu_to_be64(ha->hdr[t].size-1);
             rdc16.block_length  = cpu_to_be32(SECTOR_SIZE);
             gdth_copy_internal_data(ha, scp, (char*)&rdc16,
-                                                 sizeof(gdth_rdcap16_data), 0);
+                                                 sizeof(gdth_rdcap16_data));
         } else { 
             scp->result = DID_ABORT << 16;
         }

@@ -2848,6 +2841,7 @@ static int gdth_fill_raw_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, unchar b)
 static int gdth_special_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
 {
     register gdth_cmd_str *cmdp;
+    struct gdth_cmndinfo *cmndinfo = gdth_cmnd_priv(scp);
     int cmd_index;
 
     cmdp= ha->pccb;

@@ -2856,7 +2850,7 @@ static int gdth_special_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp)
     if (ha->type==GDT_EISA && ha->cmd_cnt>0) 
         return 0;
 
-    gdth_copy_internal_data(ha, scp, (char *)cmdp, sizeof(gdth_cmd_str), 1);
+    *cmdp = *cmndinfo->internal_cmd_str;
     cmdp->RequestBuffer = scp;
 
     /* search free command index */

drivers/scsi/gdth.h +1 -0

index 1434c6b..26e4e92 100644
@@ -915,6 +915,7 @@ typedef struct {
     struct gdth_cmndinfo {                      /* per-command private info */
         int index;
         int internal_command;                   /* don't call scsi_done */
+        gdth_cmd_str *internal_cmd_str;         /* crier for internal messages*/
         dma_addr_t sense_paddr;                 /* sense dma-addr */
         unchar priority;
         int timeout;

Signature · 1 line

1.5.3.3

Boaz Harrosh

author

2008-02-14 18:57 UTC #8 lore raw

PATCH 5/5 gdth: remove gdth cooked up command accessors

In reply to Boaz Harrosh

Last patch rendered these, to stupid status. So bye bye

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

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

drivers/scsi/gdth.c +14 ~~-59~~

index 077f972..cfce0fe 100644
@@ -85,10 +85,10 @@
 
 /* The meaning of the Scsi_Pointer members in this driver is as follows:
  * ptr:                     Chaining
- * this_residual:           gdth_bufflen
- * buffer:                  gdth_sglist
+ * this_residual:           unused
+ * buffer:                  unused
  * dma_handle:              unused
- * buffers_residual:        gdth_sg_count
+ * buffers_residual:        unused
  * Status:                  unused
  * Message:                 unused
  * have_data_in:            unused

@@ -373,47 +373,6 @@ static const struct file_operations gdth_fops = {
     .release = gdth_close,
 };
 
-/*
- * gdth scsi_command access wrappers.
- *   below 6 functions are used throughout the driver to access scsi_command's
- *   io parameters. The reason we do not use the regular accessors from
- *   scsi_cmnd.h is because of gdth_execute(). Since it is unrecommended for
- *   llds to directly set scsi_cmnd's IO members. This driver will use SCp
- *   members for IO parameters, and will copy scsi_cmnd's members to Scp
- *   members in queuecommand. For internal commands through gdth_execute()
- *   SCp's members will be set directly.
- */
-static inline unsigned gdth_bufflen(struct scsi_cmnd *cmd)
-{
-	return (unsigned)cmd->SCp.this_residual;
-}
-
-static inline void gdth_set_bufflen(struct scsi_cmnd *cmd, unsigned bufflen)
-{
-	cmd->SCp.this_residual = bufflen;
-}
-
-static inline unsigned gdth_sg_count(struct scsi_cmnd *cmd)
-{
-	return (unsigned)cmd->SCp.buffers_residual;
-}
-
-static inline void gdth_set_sg_count(struct scsi_cmnd *cmd, unsigned sg_count)
-{
-	cmd->SCp.buffers_residual = sg_count;
-}
-
-static inline struct scatterlist *gdth_sglist(struct scsi_cmnd *cmd)
-{
-	return cmd->SCp.buffer;
-}
-
-static inline void gdth_set_sglist(struct scsi_cmnd *cmd,
-                                   struct scatterlist *sglist)
-{
-	cmd->SCp.buffer = sglist;
-}
-
 #include "gdth_proc.h"
 #include "gdth_proc.c"

@@ -2349,12 +2308,12 @@ static void gdth_next(gdth_ha_str *ha)
 static void gdth_copy_internal_data(gdth_ha_str *ha, Scsi_Cmnd *scp,
                                     char *buffer, ushort count)
 {
-    ushort cpcount,i, max_sg = gdth_sg_count(scp);
+    ushort cpcount,i, max_sg = scsi_sg_count(scp);
     ushort cpsum,cpnow;
     struct scatterlist *sl;
     char *address;
 
-    cpcount = min_t(ushort, count, gdth_bufflen(scp));
+    cpcount = min_t(ushort, count, scsi_bufflen(scp));
 
     if (cpcount) {
         cpsum=0;

@@ -2362,7 +2321,7 @@ static void gdth_copy_internal_data(gdth_ha_str *ha, Scsi_Cmnd *scp,
             unsigned long flags;
             cpnow = (ushort)sl->length;
             TRACE(("copy_internal() now %d sum %d count %d %d\n",
-                          cpnow, cpsum, cpcount, gdth_bufflen(scp)));
+                          cpnow, cpsum, cpcount, scsi_bufflen(scp)));
             if (cpsum+cpnow > cpcount) 
                 cpnow = cpcount - cpsum;
             cpsum += cpnow;

@@ -2585,10 +2544,10 @@ static int gdth_fill_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, ushort hdrive)
             cmdp->u.cache.BlockCnt = blockcnt;
         }
 
-        if (gdth_bufflen(scp)) {
+        if (scsi_bufflen(scp)) {
             cmndinfo->dma_dir = (read_write == 1 ?
                 PCI_DMA_TODEVICE : PCI_DMA_FROMDEVICE);   
-            sgcnt = pci_map_sg(ha->pdev, gdth_sglist(scp), gdth_sg_count(scp),
+            sgcnt = pci_map_sg(ha->pdev, scsi_sglist(scp), scsi_sg_count(scp),
                                cmndinfo->dma_dir);
             if (mode64) {
                 struct scatterlist *sl;

@@ -2735,7 +2694,7 @@ static int gdth_fill_raw_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, unchar b)
             cmdp->u.raw64.lun        = l;
             cmdp->u.raw64.bus        = b;
             cmdp->u.raw64.priority   = 0;
-            cmdp->u.raw64.sdlen      = gdth_bufflen(scp);
+            cmdp->u.raw64.sdlen      = scsi_bufflen(scp);
             cmdp->u.raw64.sense_len  = 16;
             cmdp->u.raw64.sense_data = sense_paddr;
             cmdp->u.raw64.direction  =

@@ -2752,7 +2711,7 @@ static int gdth_fill_raw_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, unchar b)
             cmdp->u.raw.bus        = b;
             cmdp->u.raw.priority   = 0;
             cmdp->u.raw.link_p     = 0;
-            cmdp->u.raw.sdlen      = gdth_bufflen(scp);
+            cmdp->u.raw.sdlen      = scsi_bufflen(scp);
             cmdp->u.raw.sense_len  = 16;
             cmdp->u.raw.sense_data = sense_paddr;
             cmdp->u.raw.direction  =

@@ -2761,9 +2720,9 @@ static int gdth_fill_raw_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, unchar b)
             cmdp->u.raw.sg_ranz    = 0;
         }
 
-        if (gdth_bufflen(scp)) {
+        if (scsi_bufflen(scp)) {
             cmndinfo->dma_dir = PCI_DMA_BIDIRECTIONAL;
-            sgcnt = pci_map_sg(ha->pdev, gdth_sglist(scp), gdth_sg_count(scp),
+            sgcnt = pci_map_sg(ha->pdev, scsi_sglist(scp), scsi_sg_count(scp),
                                cmndinfo->dma_dir);
             if (mode64) {
                 struct scatterlist *sl;

@@ -3383,8 +3342,8 @@ static int gdth_sync_event(gdth_ha_str *ha, int service, unchar index,
             /* retry */
             return 2;
         }
-        if (gdth_bufflen(scp))
-            pci_unmap_sg(ha->pdev, gdth_sglist(scp), gdth_sg_count(scp),
+        if (scsi_bufflen(scp))
+            pci_unmap_sg(ha->pdev, scsi_sglist(scp), scsi_sg_count(scp),
                          cmndinfo->dma_dir);
 
         if (cmndinfo->sense_paddr)

@@ -4026,10 +3985,6 @@ static int gdth_queuecommand(struct scsi_cmnd *scp,
     gdth_update_timeout(scp, scp->timeout_per_command * 6);
     cmndinfo->priority = DEFAULT_PRI;
 
-    gdth_set_bufflen(scp, scsi_bufflen(scp));
-    gdth_set_sg_count(scp, scsi_sg_count(scp));
-    gdth_set_sglist(scp, scsi_sglist(scp));
-
     return __gdth_queuecommand(ha, scp, cmndinfo);
 }

Signature · 1 line

1.5.3.3

Boaz Harrosh

author

2008-02-17 16:47 UTC #9 lore raw

In reply to Boaz Harrosh

Quoted 55 lines · Boaz Harrosh

Submitted are a new set of patches, that fix lots of problems with the gdth driver.
It fixes the following problems:
- scan for drives on hosts. (Already in mainline)
- truly fixes the exit/reboot problems but does call flush() before reboot.
- fix crash when accessing array with icpcon management application.
- fix crash when doing $ cat /proc/sys/gdth/0.
This one still has the below WARN_ON in messages (see <gdth_info> below) So there is one more thing hiding in there.
- use pci_get_device
One of the testers requested if we can also put the move to pci_get_device patch with removal of dependency on PCI_LEGACY, to the stable release.
The patches are for and based on Linux-2.6.24. here is the list of patches:
[PATCH 1/5] gdth: update deprecated pci_find_device
[PATCH 2/5] gdth: scan for scsi devices
[PATCH 3/5] gdth: bugfix for the at-exit problems
[PATCH 4/5] gdth: fix to internal commands execution
[PATCH 5/5] gdth: remove gdth cooked up command accessors
Please all test and report your findings.
Thanks in advance
Boaz
---
<gdth_info>
  WARNING: at arch/x86/kernel/pci-dma_32.c:66 dma_free_coherent()
  Pid: 5501, comm: cat Not tainted 2.6.24 #43
   [<c0107137>] dma_free_coherent+0x93/0x95
   [<c025ef73>] gdth_ioctl_free+0x4c/0x69
   [<c0264a36>] gdth_proc_info+0x165f/0x182c
   [<c0111f7a>] update_curr+0xeb/0xf2
   [<c01132aa>] task_rq_lock+0x29/0x50
   [<c0113706>] try_to_wake_up+0x42/0x342
   [<c0113706>] try_to_wake_up+0x42/0x342
   [<c0111a9f>] __wake_up_common+0x46/0x6d
   [<c0113569>] __wake_up+0x32/0x42
   [<c022dad9>] n_tty_receive_buf+0x2e8/0xe97
   [<c022dad9>] n_tty_receive_buf+0x2e8/0xe97
   [<c0111f0a>] update_curr+0x7b/0xf2
   [<c0112625>] enqueue_task_fair+0x27/0x30
   [<c0111783>] enqueue_task+0xa/0x14
   [<c025e351>] proc_scsi_read+0x29/0x3d
   [<c025e328>] proc_scsi_read+0x0/0x3d
   [<c0189704>] proc_file_read+0x1c6/0x279
   [<c018953e>] proc_file_read+0x0/0x279
   [<c0185eca>] proc_reg_read+0x53/0x71
   [<c0185e77>] proc_reg_read+0x0/0x71
   [<c0159968>] vfs_read+0x85/0x11b
   [<c0159d9d>] sys_read+0x41/0x6a
   [<c0102822>] sysenter_past_esp+0x5f/0x85
 </gdth_info>

Quoted 1 line

-

James hi.

All my testers have reported back that with these 5 patches applied they can now run with a 2.6.24 kernel the same way they ran before. However there is that reported issue, with the dma_free_coherent WARN_ON (above). The code was like that from day one and it is a very old issue, however it is a regression because 2.6.24 introduced that new WARN_ON.
(infamous commit aa24886e379d2b641c5117e178b15ce1d5d366ba)

Quoted 1 line

From posts on lkml and even recent one in linux-scsi about the arcmsr driver

it looks that all a driver can do is work around it with different kernel mechanisms and driver rewrites. I'm afraid I need your help here. I'm not sure I understand why does the gdth driver uses the pci_{alloc,free}_consistent() API's, and what is needed to replace it. Could you please have a look in gdth_proc.c and also in gdth.c for all the places that call gdth_ioctl_alloc/gdth_ioctl_free, and advise what can I do in it's place. Please bear in mind that we need it for 2.6.24, as a bugfix.

Apart from the above issue, please accept patches 3,4,5 above they have now been tested and are reported to bring broken system back to production. (Given that you approve off course). And mark them for inclusion to the 2.6.24 stable releases. (Or is there some thing that I should do)

---
Meanwhile on x86 systems I understand the WARN_ON is cosmetic, and does not pose any harm. Some people have reported stability with temporarily disabling it. For testers that want to try, here it is below. At your own risk.

---

Quoted 1 line

From 50d3657bf6a138ee63ad1ce00052380edc75ace7 Mon Sep 17 00:00:00 2001

From: Boaz Harrosh <[email protected]>
Date: Sun, 17 Feb 2008 12:49:35 +0200
Subject: [PATCH] gdth: Hack to remove WARN_ON in arch/x86/kernel/pci-dma_32.c

gdth uses dma_free_coherent() with interrupts disabled. Which is not portable, but is safe on the HW that supports gdth.

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

---
 arch/x86/kernel/pci-dma_32.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/x86/kernel/pci-dma_32.c +1 -1

index 5133032..350dcfd 100644
@@ -63,7 +63,7 @@ void dma_free_coherent(struct device *dev, size_t size,
 	struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
 	int order = get_order(size);
 
-	WARN_ON(irqs_disabled());	/* for portability */
+/*	WARN_ON(irqs_disabled());*/	/* for portability */
 	if (mem && vaddr >= mem->virt_base && vaddr < (mem->virt_base + (mem->size << PAGE_SHIFT))) {
 		int page = (vaddr - mem->virt_base) >> PAGE_SHIFT;

Signature · 1 line

1.5.3.3

James Bottomley

maintainer

2008-02-17 17:25 UTC #10 lore raw

Quoted 79 lines · Boaz Harrosh

Quoted 55 lines · Boaz Harrosh
Submitted are a new set of patches, that fix lots of problems with the gdth driver.
It fixes the following problems:
- scan for drives on hosts. (Already in mainline)
- truly fixes the exit/reboot problems but does call flush() before reboot.
- fix crash when accessing array with icpcon management application.
- fix crash when doing $ cat /proc/sys/gdth/0.
This one still has the below WARN_ON in messages (see <gdth_info> below) So there is one more thing hiding in there.
- use pci_get_device
One of the testers requested if we can also put the move to pci_get_device patch with removal of dependency on PCI_LEGACY, to the stable release.
The patches are for and based on Linux-2.6.24. here is the list of patches:
[PATCH 1/5] gdth: update deprecated pci_find_device
[PATCH 2/5] gdth: scan for scsi devices
[PATCH 3/5] gdth: bugfix for the at-exit problems
[PATCH 4/5] gdth: fix to internal commands execution
[PATCH 5/5] gdth: remove gdth cooked up command accessors
Please all test and report your findings.
Thanks in advance
Boaz
---
<gdth_info>
  WARNING: at arch/x86/kernel/pci-dma_32.c:66 dma_free_coherent()
  Pid: 5501, comm: cat Not tainted 2.6.24 #43
   [<c0107137>] dma_free_coherent+0x93/0x95
   [<c025ef73>] gdth_ioctl_free+0x4c/0x69
   [<c0264a36>] gdth_proc_info+0x165f/0x182c
   [<c0111f7a>] update_curr+0xeb/0xf2
   [<c01132aa>] task_rq_lock+0x29/0x50
   [<c0113706>] try_to_wake_up+0x42/0x342
   [<c0113706>] try_to_wake_up+0x42/0x342
   [<c0111a9f>] __wake_up_common+0x46/0x6d
   [<c0113569>] __wake_up+0x32/0x42
   [<c022dad9>] n_tty_receive_buf+0x2e8/0xe97
   [<c022dad9>] n_tty_receive_buf+0x2e8/0xe97
   [<c0111f0a>] update_curr+0x7b/0xf2
   [<c0112625>] enqueue_task_fair+0x27/0x30
   [<c0111783>] enqueue_task+0xa/0x14
   [<c025e351>] proc_scsi_read+0x29/0x3d
   [<c025e328>] proc_scsi_read+0x0/0x3d
   [<c0189704>] proc_file_read+0x1c6/0x279
   [<c018953e>] proc_file_read+0x0/0x279
   [<c0185eca>] proc_reg_read+0x53/0x71
   [<c0185e77>] proc_reg_read+0x0/0x71
   [<c0159968>] vfs_read+0x85/0x11b
   [<c0159d9d>] sys_read+0x41/0x6a
   [<c0102822>] sysenter_past_esp+0x5f/0x85
 </gdth_info>
Quoted 1 line
-
James hi.
All my testers have reported back that with these 5 patches applied they can now run with a 2.6.24 kernel the same way they ran before. However there is that reported issue, with the dma_free_coherent WARN_ON (above). The code was like that from day one and it is a very old issue, however it is a regression because 2.6.24 introduced that new WARN_ON.
(infamous commit aa24886e379d2b641c5117e178b15ce1d5d366ba)
Quoted 1 line
From posts on lkml and even recent one in linux-scsi about the arcmsr driver
it looks that all a driver can do is work around it with different kernel mechanisms and driver rewrites. I'm afraid I need your help here. I'm not sure I understand why does the gdth driver uses the pci_{alloc,free}_consistent() API's, and what is needed to replace it. Could you please have a look in gdth_proc.c and also in gdth.c for all the places that call gdth_ioctl_alloc/gdth_ioctl_free, and advise what can I do in it's place. Please bear in mind that we need it for 2.6.24, as a bugfix.
Apart from the above issue, please accept patches 3,4,5 above they have now been tested and are reported to bring broken system back to production. (Given that you approve off course). And mark them for inclusion to the 2.6.24 stable releases. (Or is there some thing that I should do)

Quoted 4 lines

---
Meanwhile on x86 systems I understand the WARN_ON is cosmetic, and does not pose any harm. Some people have reported stability with temporarily disabling it. For testers that want to try, here it is below. At your own risk.

Isn't this the correct fix? pscratch is a permanent address (it's allocated at boot time and never changes). All you need the smp lock for is mediating the scratch in use flag.

James

drivers/scsi/gdth_proc.c +2 -4

index de57734..ce0228e 100644
@@ -694,15 +694,13 @@ static void gdth_ioctl_free(gdth_ha_str *ha, int size, char *buf, ulong64 paddr)
 {
     ulong flags;
 
-    spin_lock_irqsave(&ha->smp_lock, flags);
-
     if (buf == ha->pscratch) {
+	spin_lock_irqsave(&ha->smp_lock, flags);
         ha->scratch_busy = FALSE;
+	spin_unlock_irqrestore(&ha->smp_lock, flags);
     } else {
         pci_free_consistent(ha->pdev, size, buf, paddr);
     }
-
-    spin_unlock_irqrestore(&ha->smp_lock, flags);
 }
 
 #ifdef GDTH_IOCTL_PROC

Boaz Harrosh

author

2008-02-18 09:27 UTC #11 lore raw

Quoted 106 lines · James Bottomley

Quoted 79 lines · Boaz Harrosh
Quoted 55 lines · Boaz Harrosh
Submitted are a new set of patches, that fix lots of problems with the gdth driver.
It fixes the following problems:
- scan for drives on hosts. (Already in mainline)
- truly fixes the exit/reboot problems but does call flush() before reboot.
- fix crash when accessing array with icpcon management application.
- fix crash when doing $ cat /proc/sys/gdth/0.
This one still has the below WARN_ON in messages (see <gdth_info> below) So there is one more thing hiding in there.
- use pci_get_device
One of the testers requested if we can also put the move to pci_get_device patch with removal of dependency on PCI_LEGACY, to the stable release.
The patches are for and based on Linux-2.6.24. here is the list of patches:
[PATCH 1/5] gdth: update deprecated pci_find_device
[PATCH 2/5] gdth: scan for scsi devices
[PATCH 3/5] gdth: bugfix for the at-exit problems
[PATCH 4/5] gdth: fix to internal commands execution
[PATCH 5/5] gdth: remove gdth cooked up command accessors
Please all test and report your findings.
Thanks in advance
Boaz
---
<gdth_info>
  WARNING: at arch/x86/kernel/pci-dma_32.c:66 dma_free_coherent()
  Pid: 5501, comm: cat Not tainted 2.6.24 #43
   [<c0107137>] dma_free_coherent+0x93/0x95
   [<c025ef73>] gdth_ioctl_free+0x4c/0x69
   [<c0264a36>] gdth_proc_info+0x165f/0x182c
   [<c0111f7a>] update_curr+0xeb/0xf2
   [<c01132aa>] task_rq_lock+0x29/0x50
   [<c0113706>] try_to_wake_up+0x42/0x342
   [<c0113706>] try_to_wake_up+0x42/0x342
   [<c0111a9f>] __wake_up_common+0x46/0x6d
   [<c0113569>] __wake_up+0x32/0x42
   [<c022dad9>] n_tty_receive_buf+0x2e8/0xe97
   [<c022dad9>] n_tty_receive_buf+0x2e8/0xe97
   [<c0111f0a>] update_curr+0x7b/0xf2
   [<c0112625>] enqueue_task_fair+0x27/0x30
   [<c0111783>] enqueue_task+0xa/0x14
   [<c025e351>] proc_scsi_read+0x29/0x3d
   [<c025e328>] proc_scsi_read+0x0/0x3d
   [<c0189704>] proc_file_read+0x1c6/0x279
   [<c018953e>] proc_file_read+0x0/0x279
   [<c0185eca>] proc_reg_read+0x53/0x71
   [<c0185e77>] proc_reg_read+0x0/0x71
   [<c0159968>] vfs_read+0x85/0x11b
   [<c0159d9d>] sys_read+0x41/0x6a
   [<c0102822>] sysenter_past_esp+0x5f/0x85
 </gdth_info>
Quoted 1 line
-
James hi.
All my testers have reported back that with these 5 patches applied they can now run with a 2.6.24 kernel the same way they ran before. However there is that reported issue, with the dma_free_coherent WARN_ON (above). The code was like that from day one and it is a very old issue, however it is a regression because 2.6.24 introduced that new WARN_ON.
(infamous commit aa24886e379d2b641c5117e178b15ce1d5d366ba)
Quoted 1 line
From posts on lkml and even recent one in linux-scsi about the arcmsr driver
it looks that all a driver can do is work around it with different kernel mechanisms and driver rewrites. I'm afraid I need your help here. I'm not sure I understand why does the gdth driver uses the pci_{alloc,free}_consistent() API's, and what is needed to replace it. Could you please have a look in gdth_proc.c and also in gdth.c for all the places that call gdth_ioctl_alloc/gdth_ioctl_free, and advise what can I do in it's place. Please bear in mind that we need it for 2.6.24, as a bugfix.
Apart from the above issue, please accept patches 3,4,5 above they have now been tested and are reported to bring broken system back to production. (Given that you approve off course). And mark them for inclusion to the 2.6.24 stable releases. (Or is there some thing that I should do)
Quoted 4 lines
---
Meanwhile on x86 systems I understand the WARN_ON is cosmetic, and does not pose any harm. Some people have reported stability with temporarily disabling it. For testers that want to try, here it is below. At your own risk.
Isn't this the correct fix? pscratch is a permanent address (it's allocated at boot time and never changes). All you need the smp lock for is mediating the scratch in use flag.
James
drivers/scsi/gdth_proc.c +2 -2
index de57734..ce0228e 100644
@@ -694,15 +694,13 @@ static void gdth_ioctl_free(gdth_ha_str *ha, int size, char *buf, ulong64 paddr)
 {
     ulong flags;
 
-    spin_lock_irqsave(&ha->smp_lock, flags);
-
     if (buf == ha->pscratch) {
+	spin_lock_irqsave(&ha->smp_lock, flags);
         ha->scratch_busy = FALSE;
+	spin_unlock_irqrestore(&ha->smp_lock, flags);
     } else {

drivers/scsi/gdth_proc.c static void gdth_ioctl_free(gdth_ha_str *ha, int size, char *buf, ulong64 paddr)

Quoted 10 lines

drivers/scsi/gdth_proc.c +0 -3

         pci_free_consistent(ha->pdev, size, buf, paddr);
     }
-
-    spin_unlock_irqrestore(&ha->smp_lock, flags);
 }
 
 #ifdef GDTH_IOCTL_PROC
 
 
-

James
You are bung on the money. It was tested and it works. So simple, I was thinking it was accessed by DMA and freed at interrupt. But no, just a simple lock like this.

So that's it then, all reported problems with gdth are now resolved. Please Submit above together with the other patches.

Do I need to do anything else to get it into 2.6.24.x stable releases?

Thanks for everything
Boaz

Andrew Morton

2008-02-18 13:05 UTC #12 lore raw

In reply to Boaz Harrosh

Quoted 47 lines · On Sun, 17 Feb 2008 18:46:03 +0200 Boaz Harrosh <[email protected]> wrote:

...
All my testers have reported back that with these 5 patches applied they can now run with a 2.6.24 kernel the same way they ran before. However there is that reported issue, with the dma_free_coherent WARN_ON (above). The code was like that from day one and it is a very old issue, however it is a regression because 2.6.24 introduced that new WARN_ON.
(infamous commit aa24886e379d2b641c5117e178b15ce1d5d366ba)
Quoted 1 line
From posts on lkml and even recent one in linux-scsi about the arcmsr driver
it looks that all a driver can do is work around it with different kernel mechanisms and driver rewrites. I'm afraid I need your help here. I'm not sure I understand why does the gdth driver uses the pci_{alloc,free}_consistent() API's, and what is needed to replace it. Could you please have a look in gdth_proc.c and also in gdth.c for all the places that call gdth_ioctl_alloc/gdth_ioctl_free, and advise what can I do in it's place. Please bear in mind that we need it for 2.6.24, as a bugfix.
Apart from the above issue, please accept patches 3,4,5 above they have now been tested and are reported to bring broken system back to production. (Given that you approve off course). And mark them for inclusion to the 2.6.24 stable releases. (Or is there some thing that I should do)
---
Meanwhile on x86 systems I understand the WARN_ON is cosmetic, and does not pose any harm. Some people have reported stability with temporarily disabling it. For testers that want to try, here it is below. At your own risk.
---
Quoted 1 line
From 50d3657bf6a138ee63ad1ce00052380edc75ace7 Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <[email protected]>
Date: Sun, 17 Feb 2008 12:49:35 +0200
Subject: [PATCH] gdth: Hack to remove WARN_ON in arch/x86/kernel/pci-dma_32.c
gdth uses dma_free_coherent() with interrupts disabled. Which is not portable, but is safe on the HW that supports gdth.
NOT Signed-off-by: Boaz Harrosh <[email protected]>
---
 arch/x86/kernel/pci-dma_32.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kernel/pci-dma_32.c +0 -0
index 5133032..350dcfd 100644

arch/x86/kernel/pci-dma_32.c void dma_free_coherent(struct device *dev, size_t size, · line 69

Quoted 8 lines

arch/x86/kernel/pci-dma_32.c +1 -1

@@ -63,7 +63,7 @@ void dma_free_coherent(struct device *dev, size_t size,
 	struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
 	int order = get_order(size);
 
-	WARN_ON(irqs_disabled());	/* for portability */
+/*	WARN_ON(irqs_disabled());*/	/* for portability */
 	if (mem && vaddr >= mem->virt_base && vaddr < (mem->virt_base + (mem->size << PAGE_SHIFT))) {
 		int page = (vaddr - mem->virt_base) >> PAGE_SHIFT;

Yes. Let's reprise aa24886e379d2b641c5117e178b15ce1d5d366ba:

: commit aa24886e379d2b641c5117e178b15ce1d5d366ba
: Author: David Brownell <[email protected]>
: Date: Fri Aug 10 13:10:27 2007 -0700
:
: dma_free_coherent() needs irqs enabled (sigh)
:

:     On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish
:     call context requirement: unlike its dma_alloc_coherent() sibling, it may
:     not be called with IRQs disabled.  (This was new behavior on ARM as of late
:     2005, caused by ARM SMP updates.) This little surprise can be annoyingly
:     driver-visible.
:     
:     Since it looks like that restriction won't be removed, this patch changes
:     the definition of the API to include that requirement.  Also, to help catch
:     nonportable drivers, it updates the x86 and swiotlb versions to include the
:     relevant warnings.  (I already observed that it trips on the
:     bus_reset_tasklet of the new firewire_ohci driver.)

In general, all Linux memory-freeing functions can be called from all contexts. (vfree is an irritating exception). This is good, and provides maximum usefulness to callees, as all utility functions should seek to do. It would be best to fix arm and mips.

But arm and mips require enabled local irqs because their dma_free_coherent() needs to do a cross-cpu IPI call. Presumably because of certain unusual TLB protocols.

I'm not sure what we should do about this. Presumably the gdth-on-arm usage base is, umm, zero, so we could lamely add CONFIG_DMA_FREE_COHERENT_WITH_LOCAL_IRQS_DISABLED_IS_OK and then use that to disable gdth (and similar) on arm amd mips. But ugh.

Russell, Ralf: is there something we can do here to relax this requirement?

I'm thinking that perhaps we can do some rcu/refcounting tricks: launch the IPI from within dma_free_coherent(), but don't wait for it to complete. When all CPUs have handled the IPI then (and only then) the virtual address becomes recyclable, or something like that?

<double-checks>

Actually I think David might have been wrong about mips. afaict its dma_free_coherent() is callable under local_irq_disable(), so ARM SMP is the sole exception?

Boaz Harrosh

author

2008-02-18 13:19 UTC #13 lore raw

Quoted 99 lines · Andrew Morton

Quoted 46 lines · On Sun, 17 Feb 2008 18:46:03 +0200 Boaz Harrosh <[email protected]> wrote:
...
All my testers have reported back that with these 5 patches applied they can now run with a 2.6.24 kernel the same way they ran before. However there is that reported issue, with the dma_free_coherent WARN_ON (above). The code was like that from day one and it is a very old issue, however it is a regression because 2.6.24 introduced that new WARN_ON.
(infamous commit aa24886e379d2b641c5117e178b15ce1d5d366ba)
Quoted 1 line
From posts on lkml and even recent one in linux-scsi about the arcmsr driver
it looks that all a driver can do is work around it with different kernel mechanisms and driver rewrites. I'm afraid I need your help here. I'm not sure I understand why does the gdth driver uses the pci_{alloc,free}_consistent() API's, and what is needed to replace it. Could you please have a look in gdth_proc.c and also in gdth.c for all the places that call gdth_ioctl_alloc/gdth_ioctl_free, and advise what can I do in it's place. Please bear in mind that we need it for 2.6.24, as a bugfix.
Apart from the above issue, please accept patches 3,4,5 above they have now been tested and are reported to bring broken system back to production. (Given that you approve off course). And mark them for inclusion to the 2.6.24 stable releases. (Or is there some thing that I should do)
---
Meanwhile on x86 systems I understand the WARN_ON is cosmetic, and does not pose any harm. Some people have reported stability with temporarily disabling it. For testers that want to try, here it is below. At your own risk.
---
Quoted 1 line
From 50d3657bf6a138ee63ad1ce00052380edc75ace7 Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <[email protected]>
Date: Sun, 17 Feb 2008 12:49:35 +0200
Subject: [PATCH] gdth: Hack to remove WARN_ON in arch/x86/kernel/pci-dma_32.c
gdth uses dma_free_coherent() with interrupts disabled. Which is not portable, but is safe on the HW that supports gdth.
NOT Signed-off-by: Boaz Harrosh <[email protected]>
---
 arch/x86/kernel/pci-dma_32.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kernel/pci-dma_32.c +0 -0
index 5133032..350dcfd 100644
arch/x86/kernel/pci-dma_32.c void dma_free_coherent(struct device *dev, size_t size, · line 69
Quoted 8 lines
arch/x86/kernel/pci-dma_32.c +1 -1
@@ -63,7 +63,7 @@ void dma_free_coherent(struct device *dev, size_t size,
 	struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
 	int order = get_order(size);
 
-	WARN_ON(irqs_disabled());	/* for portability */
+/*	WARN_ON(irqs_disabled());*/	/* for portability */
 	if (mem && vaddr >= mem->virt_base && vaddr < (mem->virt_base + (mem->size << PAGE_SHIFT))) {
 		int page = (vaddr - mem->virt_base) >> PAGE_SHIFT;
 
Yes. Let's reprise aa24886e379d2b641c5117e178b15ce1d5d366ba:
: commit aa24886e379d2b641c5117e178b15ce1d5d366ba
: Author: David Brownell <[email protected]>
: Date: Fri Aug 10 13:10:27 2007 -0700
:
: dma_free_coherent() needs irqs enabled (sigh)
:
:     On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish
:     call context requirement: unlike its dma_alloc_coherent() sibling, it may
:     not be called with IRQs disabled.  (This was new behavior on ARM as of late
:     2005, caused by ARM SMP updates.) This little surprise can be annoyingly
:     driver-visible.
:     
:     Since it looks like that restriction won't be removed, this patch changes
:     the definition of the API to include that requirement.  Also, to help catch
:     nonportable drivers, it updates the x86 and swiotlb versions to include the
:     relevant warnings.  (I already observed that it trips on the
:     bus_reset_tasklet of the new firewire_ohci driver.)
:
In general, all Linux memory-freeing functions can be called from all contexts. (vfree is an irritating exception). This is good, and provides maximum usefulness to callees, as all utility functions should seek to do. It would be best to fix arm and mips.
But arm and mips require enabled local irqs because their dma_free_coherent() needs to do a cross-cpu IPI call. Presumably because of certain unusual TLB protocols.
I'm not sure what we should do about this. Presumably the gdth-on-arm usage base is, umm, zero, so we could lamely add CONFIG_DMA_FREE_COHERENT_WITH_LOCAL_IRQS_DISABLED_IS_OK and then use that to disable gdth (and similar) on arm amd mips. But ugh.
Russell, Ralf: is there something we can do here to relax this requirement?
I'm thinking that perhaps we can do some rcu/refcounting tricks: launch the IPI from within dma_free_coherent(), but don't wait for it to complete. When all CPUs have handled the IPI then (and only then) the virtual address becomes recyclable, or something like that?

Quoted 7 lines

<double-checks>
Actually I think David might have been wrong about mips. afaict its dma_free_coherent() is callable under local_irq_disable(), so ARM SMP is the sole exception?

Thank you Andrew for your reply.

But actually James has resolved this issue for the gdth driver. And all testers that witnessed a problem have reported success with his simple fix, which is for sure a much more correct way then was done before.

So for gdth sake I is OK as it is, so far.

Boaz

Russell King

2008-02-18 14:09 UTC #14 lore raw

In reply to Andrew Morton

Quoted 3 lines · Andrew Morton

But arm and mips require enabled local irqs because their dma_free_coherent() needs to do a cross-cpu IPI call. Presumably because of certain unusual TLB protocols.

Consider that TLB flushing needs to call a function on another CPU. Now look at x86's implementation to do cross calls:

/**
* smp_call_function_mask(): Run a function on a set of other CPUs.
* @mask: The set of cpus to run on. Must not include the current cpu.
* @func: The function to run. This must be fast and non-blocking.
* @info: An arbitrary pointer to pass to the function.
* @wait: If true, wait (atomically) until function has completed on other CPUs. *
* Returns 0 on success, else a negative status code.
*
* If @wait is true, then returns once @func has returned; otherwise
* it returns just before the target cpu calls @func.
*
* You must not call this function with disabled interrupts or from a
* hardware interrupt handler or from a bottom half handler.
*/
static int
native_smp_call_function_mask(cpumask_t mask,

                              void (*func)(void *), void *info,
                              int wait)

You can not call functions on other CPUs without having IRQs enabled, otherwise this functionality deadlocks. That restriction is on all smp_call_function() implementations.

Unfortunately, to flush the TLB on other CPUs on ARM, we need to do a cross call, which means using smp_call_function(), which introduces the same sodding restrictions that smp_call_function() has on the functions which call it.

Quoted 1 line

Russell, Ralf: is there something we can do here to relax this requirement?

Do what x86 people have so far been unable to resolve and find some way to allow smp_call_function() to operate with IRQs disabled without deadlocking?

Another solution jejb suggested was to make dma_free_coherent() lazy, but (a) I'm unconvinced that this'll work with drivers which constantly alloc+free in IRQ context since there's generally only 2MB of VM space for such mappings, and it probably won't take long to eat through that limited space with such a scheme.

Also, I don't have the facility to really test out these issues...

Signature · 3 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

James Bottomley

maintainer

2008-02-18 14:51 UTC #15 lore raw

Quoted 6 lines · Russell King

Another solution jejb suggested was to make dma_free_coherent() lazy, but (a) I'm unconvinced that this'll work with drivers which constantly
alloc+free in IRQ context since there's generally only 2MB of VM space for such mappings, and it probably won't take long to eat through that limited space with such a scheme.

I didn't say make the alloc/free lazy, I said make the mapping setup teardown lazy. So on alloc, you first look for existing space, if none, you map some more. On free you *don't* teardown the mappings (which is what requires the IPI) but simply free to the existing space pool ready for reuse. You map and free in page size chunks, since the coherent memory users that want > PAGE_SIZE usually all do boot time allocation (and don't free until release time).

The reuse of the pages should keep even the drivers that do persistent alloc/free of dma coherent memory happy, and it should reach a steady state fairly quickly.

James

David Brownell

2008-02-18 21:00 UTC #16 lore raw

In reply to Andrew Morton

Quoted 6 lines · Andrew Morton

Russell, Ralf: is there something we can do here to relax this requirement?
I'm thinking that perhaps we can do some rcu/refcounting tricks: launch the IPI from within dma_free_coherent(), but don't wait for it to complete. When all CPUs have handled the IPI then (and only then) the virtual address becomes recyclable, or something like that?

Or the trick some drivers had to do: just defer the actual work of dma_free_coherent() into a tasklet. Better have one such tasklet in arch code than N of them in drivers.

To be clear: I never thought that API restriction was a good idea.

(Although this discussion now seems moot for the gdth driver.)

Quoted 5 lines

<double-checks>
Actually I think David might have been wrong about mips. afaict its dma_free_coherent() is callable under local_irq_disable(), so ARM SMP is the sole exception?

All I recall at this point was getting some arch-specific patches in that area; I thought it was MIPS, maybe it was PPC. The arch code may have changed since then too.

- Dave
-
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

Ralf Baechle

2008-02-19 14:38 UTC #17 lore raw

Quoted 7 lines · David Brownell

Quoted 3 lines
Actually I think David might have been wrong about mips. afaict its dma_free_coherent() is callable under local_irq_disable(), so ARM SMP is the sole exception?
All I recall at this point was getting some arch-specific patches in that area; I thought it was MIPS, maybe it was PPC. The arch code may have changed since then too.

Mostly to avoid code duplication on the MIPS side but the core of the implementation has always been similar to what it is currently.

Ralf
-
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

Ralf Baechle

2008-02-18 21:29 UTC #18 lore raw

In reply to Andrew Morton

Quoted 87 lines · Andrew Morton

Quoted 46 lines · On Sun, 17 Feb 2008 18:46:03 +0200 Boaz Harrosh <[email protected]> wrote:
...
All my testers have reported back that with these 5 patches applied they can now run with a 2.6.24 kernel the same way they ran before. However there is that reported issue, with the dma_free_coherent WARN_ON (above). The code was like that from day one and it is a very old issue, however it is a regression because 2.6.24 introduced that new WARN_ON.
(infamous commit aa24886e379d2b641c5117e178b15ce1d5d366ba)
Quoted 1 line
From posts on lkml and even recent one in linux-scsi about the arcmsr driver
it looks that all a driver can do is work around it with different kernel mechanisms and driver rewrites. I'm afraid I need your help here. I'm not sure I understand why does the gdth driver uses the pci_{alloc,free}_consistent() API's, and what is needed to replace it. Could you please have a look in gdth_proc.c and also in gdth.c for all the places that call gdth_ioctl_alloc/gdth_ioctl_free, and advise what can I do in it's place. Please bear in mind that we need it for 2.6.24, as a bugfix.
Apart from the above issue, please accept patches 3,4,5 above they have now been tested and are reported to bring broken system back to production. (Given that you approve off course). And mark them for inclusion to the 2.6.24 stable releases. (Or is there some thing that I should do)
---
Meanwhile on x86 systems I understand the WARN_ON is cosmetic, and does not pose any harm. Some people have reported stability with temporarily disabling it. For testers that want to try, here it is below. At your own risk.
---
Quoted 1 line
From 50d3657bf6a138ee63ad1ce00052380edc75ace7 Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <[email protected]>
Date: Sun, 17 Feb 2008 12:49:35 +0200
Subject: [PATCH] gdth: Hack to remove WARN_ON in arch/x86/kernel/pci-dma_32.c
gdth uses dma_free_coherent() with interrupts disabled. Which is not portable, but is safe on the HW that supports gdth.
NOT Signed-off-by: Boaz Harrosh <[email protected]>
---
 arch/x86/kernel/pci-dma_32.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kernel/pci-dma_32.c +0 -0
index 5133032..350dcfd 100644
arch/x86/kernel/pci-dma_32.c void dma_free_coherent(struct device *dev, size_t size, · line 69
Quoted 8 lines
arch/x86/kernel/pci-dma_32.c +1 -1
@@ -63,7 +63,7 @@ void dma_free_coherent(struct device *dev, size_t size,
 	struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
 	int order = get_order(size);
 
-	WARN_ON(irqs_disabled());	/* for portability */
+/*	WARN_ON(irqs_disabled());*/	/* for portability */
 	if (mem && vaddr >= mem->virt_base && vaddr < (mem->virt_base + (mem->size << PAGE_SHIFT))) {
 		int page = (vaddr - mem->virt_base) >> PAGE_SHIFT;
 
Yes. Let's reprise aa24886e379d2b641c5117e178b15ce1d5d366ba:
: commit aa24886e379d2b641c5117e178b15ce1d5d366ba
: Author: David Brownell <[email protected]>
: Date: Fri Aug 10 13:10:27 2007 -0700
:
: dma_free_coherent() needs irqs enabled (sigh)
:
:     On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish
:     call context requirement: unlike its dma_alloc_coherent() sibling, it may
:     not be called with IRQs disabled.  (This was new behavior on ARM as of late
:     2005, caused by ARM SMP updates.) This little surprise can be annoyingly
:     driver-visible.
:     
:     Since it looks like that restriction won't be removed, this patch changes
:     the definition of the API to include that requirement.  Also, to help catch
:     nonportable drivers, it updates the x86 and swiotlb versions to include the
:     relevant warnings.  (I already observed that it trips on the
:     bus_reset_tasklet of the new firewire_ohci driver.)
:
In general, all Linux memory-freeing functions can be called from all contexts. (vfree is an irritating exception). This is good, and provides maximum usefulness to callees, as all utility functions should seek to do. It would be best to fix arm and mips.
But arm and mips require enabled local irqs because their dma_free_coherent() needs to do a cross-cpu IPI call. Presumably because of certain unusual TLB protocols.

Quoted 6 lines

I'm not sure what we should do about this. Presumably the gdth-on-arm usage base is, umm, zero, so we could lamely add CONFIG_DMA_FREE_COHERENT_WITH_LOCAL_IRQS_DISABLED_IS_OK and then use that to disable gdth (and similar) on arm amd mips. But ugh.
Russell, Ralf: is there something we can do here to relax this requirement?

The current MIPS implementation of dma_alloc_coherent / dma_free_coherent is a glorified wrapper around __get_free_pages rsp. free_pages that is it doesn't depend on interrupts enabled.

This works because the current dma_alloc_coherent will only return lowmem which is accessible without TLB mappings through KSEG0/1.

The embedded world being as quirky as it is I expect support of highmem which will require the use of TLB entries to become eventually relevant on MIPS but I think the lazy teardown of the mappings can take care of that and worst case a caller of dma_alloc_coherent() has to be prepared to handle an error return anyway.

Ralf