Skip to contentSkip to conversation

2.6.6-rc3 ia64 smp_call_function() called with interrupts disabled

PATCH In review Andrew Morton 6 participants 10 messages May 3, 2004 – Jun 4, 2004

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

Begin forwarded message:

Date: Mon, 03 May 2004 12:39:44 +1000
From: Keith Owens <[email protected]>
To: [email protected]
Subject: 2.6.6-rc3 ia64 smp_call_function() called with interrupts disabled

2.6.6-rc3, modprobe sg calls vfree() with interrupts disabled. On ia64, vfree calls smp_flush_tlb_all() which calls smp_call_function(). Calling smp_call_function() with interrupts disabled can deadlock.

Badness in smp_call_function at arch/ia64/kernel/smp.c:312

Call Trace:
 [<a0000001000190a0>] show_stack+0x80/0xa0
                                sp=e00001307811fc40 bsp=e0000130781191c0
 [<a000000100058a50>] smp_call_function+0x3d0/0x3e0
                                sp=e00001307811fe10 bsp=e000013078119160
 [<a000000100057e60>] smp_flush_tlb_all+0x40/0x80
                                sp=e00001307811fe30 bsp=e000013078119140
 [<a000000100142850>] unmap_vm_area+0xf0/0x120
                                sp=e00001307811fe30 bsp=e000013078119108
 [<a000000100142ed0>] remove_vm_area+0x150/0x1e0
                                sp=e00001307811fe30 bsp=e0000130781190e0
 [<a000000100142fb0>] __vunmap+0x50/0x1e0
                                sp=e00001307811fe30 bsp=e0000130781190a0
 [<a0000002002ae4d0>] sg_add+0x2d0/0xbe0 [sg]
                                sp=e00001307811fe30 bsp=e000013078119038
 [<a00000010047da40>] class_interface_register+0x1e0/0x2a0
                                sp=e00001307811fe30 bsp=e000013078119000
 [<a0000001005371b0>] scsi_register_interface+0x30/0x60
                                sp=e00001307811fe30 bsp=e000013078118fe0
 [<a0000002002d0200>] init_sg+0x140/0x190 [sg]
                                sp=e00001307811fe30 bsp=e000013078118fb8
 [<a000000100104b40>] sys_init_module+0x3c0/0x660
                                sp=e00001307811fe30 bsp=e000013078118f48
 [<a000000100011be0>] ia64_ret_from_syscall+0x0/0x20
                                sp=e00001307811fe30 bsp=e000013078118f40

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#1 lore raw
Quoted 12 lines · Andrew Morton

Begin forwarded message:

Date: Mon, 03 May 2004 12:39:44 +1000
From: Keith Owens <[email protected]>
To: [email protected]
Subject: 2.6.6-rc3 ia64 smp_call_function() called with interrupts disabled

2.6.6-rc3, modprobe sg calls vfree() with interrupts disabled. On ia64, vfree calls smp_flush_tlb_all() which calls smp_call_function(). Calling smp_call_function() with interrupts disabled can deadlock.

Quoted 7 lines

Badness in smp_call_function at arch/ia64/kernel/smp.c:312

Call Trace:
 [<a000000100142fb0>] __vunmap+0x50/0x1e0
                                spà0001307811fe30 bspà000130781190a0
 [<a0000002002ae4d0>] sg_add+0x2d0/0xbe0 [sg]
                                spà0001307811fe30 bspà00013078119038

How about the following patch? Noet that vfree() handles a NULL argument, so it's not necessary to check the pointer.

Index: drivers/scsi/sg.c
=================================RCS file: /var/cvs/linux-2.6/drivers/scsi/sg.c,v retrieving revision 1.13

drivers/scsi/sg.c +4 -3
diff -u -p -r1.13 sg.c
@@ -1341,6 +1341,7 @@ sg_add(struct class_device *cl_dev)
 	Sg_device *sdp = NULL;
 	unsigned long iflags;
 	struct cdev * cdev = NULL;
+	void *old_sg_dev_arr = NULL;
 	int k, error;
 
 	disk = alloc_disk(1);
@@ -1368,7 +1369,7 @@ sg_add(struct class_device *cl_dev)
 		memset(tmp_da, 0, tmp_dev_max * sizeof (Sg_device *));
 		memcpy(tmp_da, sg_dev_arr,
 		       sg_dev_max * sizeof (Sg_device *));
-		vfree((char *) sg_dev_arr);
+		old_sg_dev_arr = sg_dev_arr;
 		sg_dev_arr = tmp_da;
 		sg_dev_max = tmp_dev_max;
 	}
@@ -1384,8 +1385,7 @@ find_empty_slot:
 		       " type=%d, minor number exceeds %d\n",
 		       scsidp->host->host_no, scsidp->channel, scsidp->id,
 		       scsidp->lun, scsidp->type, SG_MAX_DEVS - 1);
-		if (NULL != sdp)
-			vfree((char *) sdp);
+		vfree(sdp);
 		error = -ENODEV;
 		goto out;
 	}
@@ -1459,6 +1459,7 @@ find_empty_slot:
 	return 0;
 
 out:
+	vfree(old_sg_dev_arr);
 	put_disk(disk);
 	if (cdev)
 		cdev_del(cdev);
Signature · 6 lines
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain
#2 lore raw
Quoted 2 lines · Matthew Wilcox

How about the following patch? Noet that vfree() handles a NULL argument, so it's not necessary to check the pointer.

That patch is crap -- it only frees the memory on the error path, not the normal exit. Since I got confused by this function, it struck me as not unreasonable that somebody else might also get confused by it and split it into two parts.

I simplified some of the code. The old code took the lock, scanned through looking for a free slot, dropped the lock, allocated an sdp, grabbed the lock and checked the slot was still free, branching back if it had raced. This rewrite assumes that we will find a slot and allocates an sdp in advance.

Does anybody like this patch? It survived booting on my test box which only has one scsi device. More testing welcomed.

Index: drivers/scsi/sg.c
=================================RCS file: /var/cvs/linux-2.6/drivers/scsi/sg.c,v retrieving revision 1.13

drivers/scsi/sg.c +66 -58
diff -u -p -r1.13 sg.c
@@ -1333,85 +1333,44 @@ static struct class_simple * sg_sysfs_cl
 
 static int sg_sysfs_valid = 0;
 
-static int
-sg_add(struct class_device *cl_dev)
+static int sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 {
-	struct scsi_device *scsidp = to_scsi_device(cl_dev->dev);
-	struct gendisk *disk;
-	Sg_device *sdp = NULL;
+	Sg_device *sdp;
 	unsigned long iflags;
-	struct cdev * cdev = NULL;
+	void *old_sg_dev_arr = NULL;
 	int k, error;
 
-	disk = alloc_disk(1);
-	if (!disk)
+	sdp = vmalloc(sizeof(Sg_device));
+	if (!sdp)
 		return -ENOMEM;
 
-	cdev = cdev_alloc();
-	if (! cdev)
-		return -ENOMEM;
 	write_lock_irqsave(&sg_dev_arr_lock, iflags);
-	if (sg_nr_dev >= sg_dev_max) {	/* try to resize */
+	if (unlikely(sg_nr_dev >= sg_dev_max)) {	/* try to resize */
 		Sg_device **tmp_da;
 		int tmp_dev_max = sg_nr_dev + SG_DEV_ARR_LUMP;
-
 		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
-		tmp_da = (Sg_device **)vmalloc(
-				tmp_dev_max * sizeof(Sg_device *));
-		if (NULL = tmp_da) {
-			printk(KERN_ERR
-			       "sg_add: device array cannot be resized\n");
-			error = -ENOMEM;
-			goto out;
-		}
+
+		tmp_da = vmalloc(tmp_dev_max * sizeof(Sg_device *));
+		if (unlikely(!tmp_da))
+			goto expand_failed;
+
 		write_lock_irqsave(&sg_dev_arr_lock, iflags);
-		memset(tmp_da, 0, tmp_dev_max * sizeof (Sg_device *));
-		memcpy(tmp_da, sg_dev_arr,
-		       sg_dev_max * sizeof (Sg_device *));
-		vfree((char *) sg_dev_arr);
+		memset(tmp_da, 0, tmp_dev_max * sizeof(Sg_device *));
+		memcpy(tmp_da, sg_dev_arr, sg_dev_max * sizeof(Sg_device *));
+		old_sg_dev_arr = sg_dev_arr;
 		sg_dev_arr = tmp_da;
 		sg_dev_max = tmp_dev_max;
 	}
 
-find_empty_slot:
 	for (k = 0; k < sg_dev_max; k++)
 		if (!sg_dev_arr[k])
 			break;
-	if (k >= SG_MAX_DEVS) {
-		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
-		printk(KERN_WARNING
-		       "Unable to attach sg device <%d, %d, %d, %d>"
-		       " type=%d, minor number exceeds %d\n",
-		       scsidp->host->host_no, scsidp->channel, scsidp->id,
-		       scsidp->lun, scsidp->type, SG_MAX_DEVS - 1);
-		if (NULL != sdp)
-			vfree((char *) sdp);
-		error = -ENODEV;
-		goto out;
-	}
-	if (k < sg_dev_max) {
-		if (NULL = sdp) {
-			write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
-			sdp = (Sg_device *)vmalloc(sizeof(Sg_device));
-			write_lock_irqsave(&sg_dev_arr_lock, iflags);
-			if (!sg_dev_arr[k])
-				goto find_empty_slot;
-		}
-	} else
-		sdp = NULL;
-	if (NULL = sdp) {
-		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
-		printk(KERN_ERR "sg_add: Sg_device cannot be allocated\n");
-		error = -ENOMEM;
-		goto out;
-	}
+	if (unlikely(k >= SG_MAX_DEVS))
+		goto overflow;
 
-	SCSI_LOG_TIMEOUT(3, printk("sg_add: dev=%d \n", k));
 	memset(sdp, 0, sizeof(*sdp));
+	SCSI_LOG_TIMEOUT(3, printk("sg_add: dev=%d \n", k));
 	sprintf(disk->disk_name, "sg%d", k);
-	cdev->owner = THIS_MODULE;
-	cdev->ops = &sg_fops;
-	disk->major = SCSI_GENERIC_MAJOR;
 	disk->first_minor = k;
 	sdp->disk = disk;
 	sdp->device = scsidp;
@@ -1421,6 +1380,55 @@ find_empty_slot:
 	sg_nr_dev++;
 	sg_dev_arr[k] = sdp;
 	write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
+	error = k;
+
+ out:
+	if (error < 0)
+		vfree(sdp);
+	vfree(old_sg_dev_arr);
+	return error;
+
+ expand_failed:
+	printk(KERN_ERR "sg_add: device array cannot be resized\n");
+	error = -ENOMEM;
+	goto out;
+
+ overflow:
+	write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
+	printk(KERN_WARNING
+	       "Unable to attach sg device <%d, %d, %d, %d> type=%d, minor "
+	       "number exceeds %d\n", scsidp->host->host_no, scsidp->channel,
+	       scsidp->id, scsidp->lun, scsidp->type, SG_MAX_DEVS - 1);
+	error = -ENODEV;
+	goto out;
+}
+
+static int
+sg_add(struct class_device *cl_dev)
+{
+	struct scsi_device *scsidp = to_scsi_device(cl_dev->dev);
+	struct gendisk *disk;
+	Sg_device *sdp = NULL;
+	struct cdev * cdev = NULL;
+	int error, k;
+
+	disk = alloc_disk(1);
+	if (!disk)
+		return -ENOMEM;
+	disk->major = SCSI_GENERIC_MAJOR;
+
+	error = -ENOMEM;
+	cdev = cdev_alloc();
+	if (!cdev)
+		goto out;
+	cdev->owner = THIS_MODULE;
+	cdev->ops = &sg_fops;
+
+	error = sg_alloc(disk, scsidp);
+	if (error < 0)
+		goto out;
+	k = error;
+	sdp = sg_dev_arr[k];
 
 	devfs_mk_cdev(MKDEV(SCSI_GENERIC_MAJOR, k),
 			S_IFCHR | S_IRUSR | S_IWUSR | S_IRGRP,
Signature · 6 lines
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain
#3 lore raw
Quoted 5 lines · Matthew Wilcox

That patch is crap -- it only frees the memory on the error path, not the normal exit. Since I got confused by this function, it struck me as not unreasonable that somebody else might also get confused by it and split it into two parts.

Quoted 8 lines

I simplified some of the code. The old code took the lock, scanned through looking for a free slot, dropped the lock, allocated an sdp, grabbed the lock and checked the slot was still free, branching back if it had raced. This rewrite assumes that we will find a slot and allocates an sdp in advance.

Does anybody like this patch? It survived booting on my test box which only has one scsi device. More testing welcomed.

Better than what was there, but I still don't like it. A global array of devices is just utter crap. Every entry point from scsi already has struct scsi_device from which we can derive the sg-specific portion easily, and for anything else (from a quick look that seems to be only procfs stuff which should fade out anyway) a linear search on a linked list is okay.

btw, why are we vmalloc()ing Sg_device?

#4 lore raw

Christopoh -

Quoted 8 lines · Christoph Hellwig

Better than what was there, but I still don't like it. A global array of devices is just utter crap. Every entry point from scsi already has struct scsi_device from which we can derive the sg-specific portion easily, and for anything else (from a quick look that seems to be only procfs stuff which should fade out anyway) a linear search on a linked list is okay.

btw, why are we vmalloc()ing Sg_device?

With Doug's latest version of the patch, and changing the vmalloc of Sg_device to a kmalloc, I was able to get sg to attach to 16k devices. (I'm still debugging major/minor issues, hopefully just userspace stuff.)

I was trying to get rid of the sg_dev_arr, but there is no connection from a scsi_device to a Sg_device, there is only the pointer from Sg_device to scsi_device. The sg simple class class_data is set but never used (class_set_devdata is used but not class_get_devdata).

We have a scsi_device class_data that could store the Sg_device, that is a bit of a hack, since it is supposed hold scsi core data, and in theory we could have multiple scsi_device class interfaces (st and any other upper level character drivers would not use this).

There is no cdev private_data, similiar to the block_device gendisk private data, so we can't use that in sg_open. The other upper level char devices (st) could also use this, as well as other char drivers. I am told this is a 2.7 item.

Do you think we should do anything else (besides losing the one vmalloc) here for sg in 2.6?

Specifically -

Do you think we should try for a cdev private_data? With only this added, we could have a global list of all Sg_devices, and not have to do a linear search on open (I don't know how bad that would be for large numbers of devices). We would still need a linear search of the list on removal (not that bad).

Should we hack the scsi_device class_data to hold a Sg_device?

-- Patrick Mansfield

#5 lore raw

Re: Fw: 2.6.6-rc3 ia64 smp_call_function() called with interrupts

In reply to Matthew Wilcox
Quoted 11 lines · Matthew Wilcox
Quoted 2 lines · Matthew Wilcox

How about the following patch? Noet that vfree() handles a NULL argument, so it's not necessary to check the pointer.

That patch is crap -- it only frees the memory on the error path, not the normal exit. Since I got confused by this function, it struck me as not unreasonable that somebody else might also get confused by it and split it into two parts.

Quoted 8 lines

I simplified some of the code. The old code took the lock, scanned through looking for a free slot, dropped the lock, allocated an sdp, grabbed the lock and checked the slot was still free, branching back if it had raced. This rewrite assumes that we will find a slot and allocates an sdp in advance.

Does anybody like this patch? It survived booting on my test box which only has one scsi device. More testing welcomed.

Sorry, I missed this thread. The change looks good and survived about one hour of scsi_debug bashing (on i386). I also checked it against the previous version.

Attached is my version with only a superficial change to 2 printk()s plus:
- bump version number
- introduce MODULE_VERSION
- increase over allocation of sg_dev_arr from 6 to 32

Doug Gilbert

drivers/scsi/sg.c266mw2 renamed from drivers/scsi/sg.c +77 -76
@@ -7,7 +7,7 @@
  * Original driver (sg.c):
  *        Copyright (C) 1992 Lawrence Foard
  * Version 2 and 3 extensions to driver:
- *        Copyright (C) 1998 - 2002 Douglas Gilbert
+ *        Copyright (C) 1998 - 2004 Douglas Gilbert
  *
  *  Modified  19-JAN-1998  Richard Gooch <[email protected]>  Devfs support
  *
@@ -17,27 +17,18 @@
  * any later version.
  *
  */
-#include <linux/config.h>
-static int sg_version_num = 30530;	/* 2 digits for each component */
+
+static int sg_version_num = 30531;	/* 2 digits for each component */
+#define SG_VERSION_STR "3.5.31"
+
 /*
  *  D. P. Gilbert ([email protected], [email protected]), notes:
  *      - scsi logging is available via SCSI_LOG_TIMEOUT macros. First
  *        the kernel/module needs to be built with CONFIG_SCSI_LOGGING
  *        (otherwise the macros compile to empty statements).
- *        Then before running the program to be debugged enter:
- *          # echo "scsi log timeout 7" > /proc/scsi/scsi
- *        This will send copious output to the console and the log which
- *        is usually /var/log/messages. To turn off debugging enter:
- *          # echo "scsi log timeout 0" > /proc/scsi/scsi
- *        The 'timeout' token was chosen because it is relatively unused.
- *        The token 'hlcomplete' should be used but that triggers too
- *        much output from the sd device driver. To dump the current
- *        state of the SCSI mid level data structures enter:
- *          # echo "scsi dump 1" > /proc/scsi/scsi
- *        To dump the state of sg's data structures use:
- *          # cat /proc/scsi/sg/debug
  *
  */
+#include <linux/config.h>
 #include <linux/module.h>
 
 #include <linux/fs.h>
@@ -69,7 +60,7 @@
 
 #ifdef CONFIG_SCSI_PROC_FS
 #include <linux/proc_fs.h>
-static char *sg_version_str = "3.5.30 [20040124]";
+static char *sg_version_date = "20040513";
 
 static int sg_proc_init(void);
 static void sg_proc_cleanup(void);
@@ -110,7 +101,7 @@
 #define SG_SECTOR_SZ 512
 #define SG_SECTOR_MSK (SG_SECTOR_SZ - 1)
 
-#define SG_DEV_ARR_LUMP 6	/* amount to over allocate sg_dev_arr by */
+#define SG_DEV_ARR_LUMP 32	/* amount to over allocate sg_dev_arr by */
 
 static int sg_add(struct class_device *);
 static void sg_remove(struct class_device *);
@@ -1333,85 +1324,44 @@
 
 static int sg_sysfs_valid = 0;
 
-static int
-sg_add(struct class_device *cl_dev)
+static int sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 {
-	struct scsi_device *scsidp = to_scsi_device(cl_dev->dev);
-	struct gendisk *disk;
-	Sg_device *sdp = NULL;
+	Sg_device *sdp;
 	unsigned long iflags;
-	struct cdev * cdev = NULL;
+	void *old_sg_dev_arr = NULL;
 	int k, error;
 
-	disk = alloc_disk(1);
-	if (!disk)
+	sdp = vmalloc(sizeof(Sg_device));
+	if (!sdp)
 		return -ENOMEM;
 
-	cdev = cdev_alloc();
-	if (! cdev)
-		return -ENOMEM;
 	write_lock_irqsave(&sg_dev_arr_lock, iflags);
-	if (sg_nr_dev >= sg_dev_max) {	/* try to resize */
+	if (unlikely(sg_nr_dev >= sg_dev_max)) {	/* try to resize */
 		Sg_device **tmp_da;
 		int tmp_dev_max = sg_nr_dev + SG_DEV_ARR_LUMP;
-
 		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
-		tmp_da = (Sg_device **)vmalloc(
-				tmp_dev_max * sizeof(Sg_device *));
-		if (NULL == tmp_da) {
-			printk(KERN_ERR
-			       "sg_add: device array cannot be resized\n");
-			error = -ENOMEM;
-			goto out;
-		}
+
+		tmp_da = vmalloc(tmp_dev_max * sizeof(Sg_device *));
+		if (unlikely(!tmp_da))
+			goto expand_failed;
+
 		write_lock_irqsave(&sg_dev_arr_lock, iflags);
-		memset(tmp_da, 0, tmp_dev_max * sizeof (Sg_device *));
-		memcpy(tmp_da, sg_dev_arr,
-		       sg_dev_max * sizeof (Sg_device *));
-		vfree((char *) sg_dev_arr);
+		memset(tmp_da, 0, tmp_dev_max * sizeof(Sg_device *));
+		memcpy(tmp_da, sg_dev_arr, sg_dev_max * sizeof(Sg_device *));
+		old_sg_dev_arr = sg_dev_arr;
 		sg_dev_arr = tmp_da;
 		sg_dev_max = tmp_dev_max;
 	}
 
-find_empty_slot:
 	for (k = 0; k < sg_dev_max; k++)
 		if (!sg_dev_arr[k])
 			break;
-	if (k >= SG_MAX_DEVS) {
-		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
-		printk(KERN_WARNING
-		       "Unable to attach sg device <%d, %d, %d, %d>"
-		       " type=%d, minor number exceeds %d\n",
-		       scsidp->host->host_no, scsidp->channel, scsidp->id,
-		       scsidp->lun, scsidp->type, SG_MAX_DEVS - 1);
-		if (NULL != sdp)
-			vfree((char *) sdp);
-		error = -ENODEV;
-		goto out;
-	}
-	if (k < sg_dev_max) {
-		if (NULL == sdp) {
-			write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
-			sdp = (Sg_device *)vmalloc(sizeof(Sg_device));
-			write_lock_irqsave(&sg_dev_arr_lock, iflags);
-			if (!sg_dev_arr[k])
-				goto find_empty_slot;
-		}
-	} else
-		sdp = NULL;
-	if (NULL == sdp) {
-		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
-		printk(KERN_ERR "sg_add: Sg_device cannot be allocated\n");
-		error = -ENOMEM;
-		goto out;
-	}
+	if (unlikely(k >= SG_MAX_DEVS))
+		goto overflow;
 
-	SCSI_LOG_TIMEOUT(3, printk("sg_add: dev=%d \n", k));
 	memset(sdp, 0, sizeof(*sdp));
+	SCSI_LOG_TIMEOUT(3, printk("sg_alloc: dev=%d \n", k));
 	sprintf(disk->disk_name, "sg%d", k);
-	cdev->owner = THIS_MODULE;
-	cdev->ops = &sg_fops;
-	disk->major = SCSI_GENERIC_MAJOR;
 	disk->first_minor = k;
 	sdp->disk = disk;
 	sdp->device = scsidp;
@@ -1421,6 +1371,55 @@
 	sg_nr_dev++;
 	sg_dev_arr[k] = sdp;
 	write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
+	error = k;
+
+ out:
+	if (error < 0)
+		vfree(sdp);
+	vfree(old_sg_dev_arr);
+	return error;
+
+ expand_failed:
+	printk(KERN_ERR "sg_alloc: device array cannot be resized\n");
+	error = -ENOMEM;
+	goto out;
+
+ overflow:
+	write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
+	printk(KERN_WARNING
+	       "Unable to attach sg device <%d, %d, %d, %d> type=%d, minor "
+	       "number exceeds %d\n", scsidp->host->host_no, scsidp->channel,
+	       scsidp->id, scsidp->lun, scsidp->type, SG_MAX_DEVS - 1);
+	error = -ENODEV;
+	goto out;
+}
+
+static int
+sg_add(struct class_device *cl_dev)
+{
+	struct scsi_device *scsidp = to_scsi_device(cl_dev->dev);
+	struct gendisk *disk;
+	Sg_device *sdp = NULL;
+	struct cdev * cdev = NULL;
+	int error, k;
+
+	disk = alloc_disk(1);
+	if (!disk)
+		return -ENOMEM;
+	disk->major = SCSI_GENERIC_MAJOR;
+
+	error = -ENOMEM;
+	cdev = cdev_alloc();
+	if (!cdev)
+		goto out;
+	cdev->owner = THIS_MODULE;
+	cdev->ops = &sg_fops;
+
+	error = sg_alloc(disk, scsidp);
+	if (error < 0)
+		goto out;
+	k = error;
+	sdp = sg_dev_arr[k];
 
 	devfs_mk_cdev(MKDEV(SCSI_GENERIC_MAJOR, k),
 			S_IFCHR | S_IRUSR | S_IWUSR | S_IRGRP,
@@ -1543,6 +1542,7 @@
 MODULE_AUTHOR("Douglas Gilbert");
 MODULE_DESCRIPTION("SCSI generic (sg) driver");
 MODULE_LICENSE("GPL");
+MODULE_VERSION(SG_VERSION_STR);
 
 MODULE_PARM_DESC(def_reserved_size, "size of buffer reserved for each fd");
 MODULE_PARM_DESC(allow_dio, "allow direct I/O (default: 0 (disallow))");
@@ -2844,7 +2844,8 @@
 
 static int sg_proc_seq_show_version(struct seq_file *s, void *v)
 {
-	seq_printf(s, "%d\t%s\n", sg_version_num, sg_version_str);
+	seq_printf(s, "%d\t%s [%s]\n", sg_version_num, SG_VERSION_STR,
+		   sg_version_date);
 	return 0;
 }
 
#6 lore raw
Quoted 3 lines · Douglas Gilbert

Sorry, I missed this thread. The change looks good and survived about one hour of scsi_debug bashing (on i386). I also checked it against the previous version.

I hit the badness yesterday, the badness became a hang when I had hotplug, udev and scsi_id running. With the patch no badness and no hang.

-- Patrick Mansfield

#7 lore raw

PATCH 1/1 sg driver against lk 2.6.6

In reply to Douglas Gilbert

subject was: 2.6.6-rc3 ia64 smp_call_function() called with
interrupts disabled

Quoted 1 line · Douglas Gilbert

Matthew Wilcox wrote:

<snip>

Quoted 9 lines
Quoted 3 lines

Does anybody like this patch? It survived booting on my test box which only has one scsi device. More testing welcomed.

Sorry, I missed this thread. The change looks good and survived about one hour of scsi_debug bashing (on i386). I also checked it against the previous version.

Quoted 5 lines

Attached is my version with only a superficial change to 2 printk()s plus:
- bump version number
- introduce MODULE_VERSION
- increase over allocation of sg_dev_arr from 6 to 32

Here are some further refinements to this patch with help from Pat Mansfield:
- replace vmalloc() with kmalloc(,GFP_KERNEL)
- bump max sg devices from 8192 to 32768

Tested to 16k devices (and 8k devices on a box with 512MB ram), Patch is against lk 2.6.6 (or 2.6.6-bk1).

Doug Gilbert

drivers/scsi/sg.c266mw3 renamed from drivers/scsi/sg.c +89 -81
@@ -7,7 +7,7 @@
  * Original driver (sg.c):
  *        Copyright (C) 1992 Lawrence Foard
  * Version 2 and 3 extensions to driver:
- *        Copyright (C) 1998 - 2002 Douglas Gilbert
+ *        Copyright (C) 1998 - 2004 Douglas Gilbert
  *
  *  Modified  19-JAN-1998  Richard Gooch <[email protected]>  Devfs support
  *
@@ -17,27 +17,18 @@
  * any later version.
  *
  */
-#include <linux/config.h>
-static int sg_version_num = 30530;	/* 2 digits for each component */
+
+static int sg_version_num = 30531;	/* 2 digits for each component */
+#define SG_VERSION_STR "3.5.31"
+
 /*
  *  D. P. Gilbert ([email protected], [email protected]), notes:
  *      - scsi logging is available via SCSI_LOG_TIMEOUT macros. First
  *        the kernel/module needs to be built with CONFIG_SCSI_LOGGING
  *        (otherwise the macros compile to empty statements).
- *        Then before running the program to be debugged enter:
- *          # echo "scsi log timeout 7" > /proc/scsi/scsi
- *        This will send copious output to the console and the log which
- *        is usually /var/log/messages. To turn off debugging enter:
- *          # echo "scsi log timeout 0" > /proc/scsi/scsi
- *        The 'timeout' token was chosen because it is relatively unused.
- *        The token 'hlcomplete' should be used but that triggers too
- *        much output from the sd device driver. To dump the current
- *        state of the SCSI mid level data structures enter:
- *          # echo "scsi dump 1" > /proc/scsi/scsi
- *        To dump the state of sg's data structures use:
- *          # cat /proc/scsi/sg/debug
  *
  */
+#include <linux/config.h>
 #include <linux/module.h>
 
 #include <linux/fs.h>
@@ -51,7 +42,6 @@
 #include <linux/fcntl.h>
 #include <linux/init.h>
 #include <linux/poll.h>
-#include <linux/vmalloc.h>
 #include <linux/smp_lock.h>
 #include <linux/moduleparam.h>
 #include <linux/devfs_fs_kernel.h>
@@ -69,7 +59,7 @@
 
 #ifdef CONFIG_SCSI_PROC_FS
 #include <linux/proc_fs.h>
-static char *sg_version_str = "3.5.30 [20040124]";
+static char *sg_version_date = "20040516";
 
 static int sg_proc_init(void);
 static void sg_proc_cleanup(void);
@@ -82,7 +72,7 @@
 #define SG_ALLOW_DIO_DEF 0
 #define SG_ALLOW_DIO_CODE /* compile out by commenting this define */
 
-#define SG_MAX_DEVS 8192
+#define SG_MAX_DEVS 32768
 
 /*
  * Suppose you want to calculate the formula muldiv(x,m,d)=int(x * m / d)
@@ -110,7 +100,7 @@
 #define SG_SECTOR_SZ 512
 #define SG_SECTOR_MSK (SG_SECTOR_SZ - 1)
 
-#define SG_DEV_ARR_LUMP 6	/* amount to over allocate sg_dev_arr by */
+#define SG_DEV_ARR_LUMP 32	/* amount to over allocate sg_dev_arr by */
 
 static int sg_add(struct class_device *);
 static void sg_remove(struct class_device *);
@@ -1333,85 +1323,46 @@
 
 static int sg_sysfs_valid = 0;
 
-static int
-sg_add(struct class_device *cl_dev)
+static int sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 {
-	struct scsi_device *scsidp = to_scsi_device(cl_dev->dev);
-	struct gendisk *disk;
-	Sg_device *sdp = NULL;
+	Sg_device *sdp;
 	unsigned long iflags;
-	struct cdev * cdev = NULL;
+	void *old_sg_dev_arr = NULL;
 	int k, error;
 
-	disk = alloc_disk(1);
-	if (!disk)
+	sdp = kmalloc(sizeof(Sg_device), GFP_KERNEL);
+	if (!sdp) {
+		printk(KERN_WARNING "kmalloc Sg_device failure\n");
 		return -ENOMEM;
+	}
 
-	cdev = cdev_alloc();
-	if (! cdev)
-		return -ENOMEM;
 	write_lock_irqsave(&sg_dev_arr_lock, iflags);
-	if (sg_nr_dev >= sg_dev_max) {	/* try to resize */
+	if (unlikely(sg_nr_dev >= sg_dev_max)) {	/* try to resize */
 		Sg_device **tmp_da;
 		int tmp_dev_max = sg_nr_dev + SG_DEV_ARR_LUMP;
-
 		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
-		tmp_da = (Sg_device **)vmalloc(
-				tmp_dev_max * sizeof(Sg_device *));
-		if (NULL == tmp_da) {
-			printk(KERN_ERR
-			       "sg_add: device array cannot be resized\n");
-			error = -ENOMEM;
-			goto out;
-		}
+
+		tmp_da = kmalloc(tmp_dev_max * sizeof(Sg_device *), GFP_KERNEL);
+		if (unlikely(!tmp_da))
+			goto expand_failed;
+
 		write_lock_irqsave(&sg_dev_arr_lock, iflags);
-		memset(tmp_da, 0, tmp_dev_max * sizeof (Sg_device *));
-		memcpy(tmp_da, sg_dev_arr,
-		       sg_dev_max * sizeof (Sg_device *));
-		vfree((char *) sg_dev_arr);
+		memset(tmp_da, 0, tmp_dev_max * sizeof(Sg_device *));
+		memcpy(tmp_da, sg_dev_arr, sg_dev_max * sizeof(Sg_device *));
+		old_sg_dev_arr = sg_dev_arr;
 		sg_dev_arr = tmp_da;
 		sg_dev_max = tmp_dev_max;
 	}
 
-find_empty_slot:
 	for (k = 0; k < sg_dev_max; k++)
 		if (!sg_dev_arr[k])
 			break;
-	if (k >= SG_MAX_DEVS) {
-		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
-		printk(KERN_WARNING
-		       "Unable to attach sg device <%d, %d, %d, %d>"
-		       " type=%d, minor number exceeds %d\n",
-		       scsidp->host->host_no, scsidp->channel, scsidp->id,
-		       scsidp->lun, scsidp->type, SG_MAX_DEVS - 1);
-		if (NULL != sdp)
-			vfree((char *) sdp);
-		error = -ENODEV;
-		goto out;
-	}
-	if (k < sg_dev_max) {
-		if (NULL == sdp) {
-			write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
-			sdp = (Sg_device *)vmalloc(sizeof(Sg_device));
-			write_lock_irqsave(&sg_dev_arr_lock, iflags);
-			if (!sg_dev_arr[k])
-				goto find_empty_slot;
-		}
-	} else
-		sdp = NULL;
-	if (NULL == sdp) {
-		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
-		printk(KERN_ERR "sg_add: Sg_device cannot be allocated\n");
-		error = -ENOMEM;
-		goto out;
-	}
+	if (unlikely(k >= SG_MAX_DEVS))
+		goto overflow;
 
-	SCSI_LOG_TIMEOUT(3, printk("sg_add: dev=%d \n", k));
 	memset(sdp, 0, sizeof(*sdp));
+	SCSI_LOG_TIMEOUT(3, printk("sg_alloc: dev=%d \n", k));
 	sprintf(disk->disk_name, "sg%d", k);
-	cdev->owner = THIS_MODULE;
-	cdev->ops = &sg_fops;
-	disk->major = SCSI_GENERIC_MAJOR;
 	disk->first_minor = k;
 	sdp->disk = disk;
 	sdp->device = scsidp;
@@ -1421,6 +1372,61 @@
 	sg_nr_dev++;
 	sg_dev_arr[k] = sdp;
 	write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
+	error = k;
+
+ out:
+	if (error < 0)
+		kfree(sdp);
+	kfree(old_sg_dev_arr);
+	return error;
+
+ expand_failed:
+	printk(KERN_WARNING "sg_alloc: device array cannot be resized\n");
+	error = -ENOMEM;
+	goto out;
+
+ overflow:
+	write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
+	printk(KERN_WARNING
+	       "Unable to attach sg device <%d, %d, %d, %d> type=%d, minor "
+	       "number exceeds %d\n", scsidp->host->host_no, scsidp->channel,
+	       scsidp->id, scsidp->lun, scsidp->type, SG_MAX_DEVS - 1);
+	error = -ENODEV;
+	goto out;
+}
+
+static int
+sg_add(struct class_device *cl_dev)
+{
+	struct scsi_device *scsidp = to_scsi_device(cl_dev->dev);
+	struct gendisk *disk;
+	Sg_device *sdp = NULL;
+	struct cdev * cdev = NULL;
+	int error, k;
+
+	disk = alloc_disk(1);
+	if (!disk) {
+		printk(KERN_WARNING "alloc_disk failed\n");
+		return -ENOMEM;
+	}
+	disk->major = SCSI_GENERIC_MAJOR;
+
+	error = -ENOMEM;
+	cdev = cdev_alloc();
+	if (!cdev) {
+		printk(KERN_WARNING "cdev_alloc failed\n");
+		goto out;
+	}
+	cdev->owner = THIS_MODULE;
+	cdev->ops = &sg_fops;
+
+	error = sg_alloc(disk, scsidp);
+	if (error < 0) {
+		printk(KERN_WARNING "sg_alloc failed\n");
+		goto out;
+	}
+	k = error;
+	sdp = sg_dev_arr[k];
 
 	devfs_mk_cdev(MKDEV(SCSI_GENERIC_MAJOR, k),
 			S_IFCHR | S_IRUSR | S_IWUSR | S_IRGRP,
@@ -1526,7 +1532,7 @@
 		put_disk(sdp->disk);
 		sdp->disk = NULL;
 		if (NULL == sdp->headfp)
-			vfree((char *) sdp);
+			kfree((char *) sdp);
 	}
 
 	if (delay)
@@ -1543,6 +1549,7 @@
 MODULE_AUTHOR("Douglas Gilbert");
 MODULE_DESCRIPTION("SCSI generic (sg) driver");
 MODULE_LICENSE("GPL");
+MODULE_VERSION(SG_VERSION_STR);
 
 MODULE_PARM_DESC(def_reserved_size, "size of buffer reserved for each fd");
 MODULE_PARM_DESC(allow_dio, "allow direct I/O (default: 0 (disallow))");
@@ -1590,7 +1597,7 @@
 	unregister_chrdev_region(MKDEV(SCSI_GENERIC_MAJOR, 0),
 				 SG_MAX_DEVS);
 	if (sg_dev_arr != NULL) {
-		vfree((char *) sg_dev_arr);
+		kfree((char *) sg_dev_arr);
 		sg_dev_arr = NULL;
 	}
 	sg_dev_max = 0;
@@ -2495,7 +2502,7 @@
 			}
 			if (k < maxd)
 				sg_dev_arr[k] = NULL;
-			vfree((char *) sdp);
+			kfree((char *) sdp);
 			res = 1;
 		}
 		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
@@ -2844,7 +2851,8 @@
 
 static int sg_proc_seq_show_version(struct seq_file *s, void *v)
 {
-	seq_printf(s, "%d\t%s\n", sg_version_num, sg_version_str);
+	seq_printf(s, "%d\t%s [%s]\n", sg_version_num, SG_VERSION_STR,
+		   sg_version_date);
 	return 0;
 }
 
#8 lore raw

Re: [PATCH] sg driver against lk 2.6.6

James -

Is this patch in your queue?

I tried it out, and it worked fine for me with up to 16k of scsi_debug devices.

-- Patrick Mansfield

Quoted 90 lines · Douglas Gilbert

subject was: 2.6.6-rc3 ia64 smp_call_function() called with
interrupts disabled

Quoted 1 line · Douglas Gilbert

Matthew Wilcox wrote:

<snip>

Quoted 9 lines
Quoted 3 lines

Does anybody like this patch? It survived booting on my test box which only has one scsi device. More testing welcomed.

Sorry, I missed this thread. The change looks good and survived about one hour of scsi_debug bashing (on i386). I also checked it against the previous version.

Quoted 5 lines

Attached is my version with only a superficial change to 2 printk()s plus:
- bump version number
- introduce MODULE_VERSION
- increase over allocation of sg_dev_arr from 6 to 32

Here are some further refinements to this patch with help from Pat Mansfield:
- replace vmalloc() with kmalloc(,GFP_KERNEL)
- bump max sg devices from 8192 to 32768

Tested to 16k devices (and 8k devices on a box with 512MB ram), Patch is against lk 2.6.6 (or 2.6.6-bk1).

Doug Gilbert

drivers/scsi/sg.c266mw3 renamed from drivers/scsi/sg.c +7 -17
@@ -7,7 +7,7 @@
  * Original driver (sg.c):
  *        Copyright (C) 1992 Lawrence Foard
  * Version 2 and 3 extensions to driver:
- *        Copyright (C) 1998 - 2002 Douglas Gilbert
+ *        Copyright (C) 1998 - 2004 Douglas Gilbert
  *
  *  Modified  19-JAN-1998  Richard Gooch <[email protected]>  Devfs support
  *
@@ -17,27 +17,18 @@
  * any later version.
  *
  */
-#include <linux/config.h>
-static int sg_version_num = 30530;	/* 2 digits for each component */
+
+static int sg_version_num = 30531;	/* 2 digits for each component */
+#define SG_VERSION_STR "3.5.31"
+
 /*
  *  D. P. Gilbert ([email protected], [email protected]), notes:
  *      - scsi logging is available via SCSI_LOG_TIMEOUT macros. First
  *        the kernel/module needs to be built with CONFIG_SCSI_LOGGING
  *        (otherwise the macros compile to empty statements).
- *        Then before running the program to be debugged enter:
- *          # echo "scsi log timeout 7" > /proc/scsi/scsi
- *        This will send copious output to the console and the log which
- *        is usually /var/log/messages. To turn off debugging enter:
- *          # echo "scsi log timeout 0" > /proc/scsi/scsi
- *        The 'timeout' token was chosen because it is relatively unused.
- *        The token 'hlcomplete' should be used but that triggers too
- *        much output from the sd device driver. To dump the current
- *        state of the SCSI mid level data structures enter:
- *          # echo "scsi dump 1" > /proc/scsi/scsi
- *        To dump the state of sg's data structures use:
- *          # cat /proc/scsi/sg/debug
  *
  */
+#include <linux/config.h>
 #include <linux/module.h>
 
 #include <linux/fs.h>
@@ -51,7 +42,6 @@
 #include <linux/fcntl.h>
 #include <linux/init.h>
 #include <linux/poll.h>
-#include <linux/vmalloc.h>
 #include <linux/smp_lock.h>
 #include <linux/moduleparam.h>
 #include <linux/devfs_fs_kernel.h>
@@ -69,7 +59,7 @@
 
 #ifdef CONFIG_SCSI_PROC_FS
 #include <linux/proc_fs.h>
-static char *sg_version_str = "3.5.30 [20040124]";
+static char *sg_version_date = "20040516";
#9 lore raw

Re: [PATCH] sg driver against lk 2.6.6

Quoted 4 lines · Patrick Mansfield

Is this patch in your queue?

I tried it out, and it worked fine for me with up to 16k of scsi_debug devices.

Er, I can't find it ... I'll check back through the mail archives.

Thanks,

James