Skip to contentSkip to conversation

switch scsi upper driver probing to the driver model

PATCH In review Christoph Hellwig 11 participants 16 messages May 16, 2003 – May 19, 2003

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

PATCH 1/1 switch scsi upper driver probing to the driver model

Upper drivers now use the LDM ->probe/->remove callbacks and the core list code. Note that this means there is only one driver per scsi device and not multiple, e.g. you can't have sd _and_ sg for the same device. Personally I think that's not a problem anymore with the generic SG_IO in place, but if you scream loud enough I could come up with a hack that makes the sg nodes a property of the scsi midlayer instead of a LDM-style driver and we could get the old behaviour back.

drivers/scsi/hosts.c +6 -15
@@ -229,27 +229,18 @@
 int scsi_add_host(struct Scsi_Host *shost, struct device *dev)
 {
 	Scsi_Host_Template *sht = shost->hostt;
-	struct scsi_device *sdev;
-	int error = 0, saved_error = 0;
+	int error;
 
 	printk(KERN_INFO "scsi%d : %s\n", shost->host_no,
 			sht->info ? sht->info(shost) : sht->name);
 
 	error = scsi_sysfs_add_host(shost, dev);
-	if (error)
-		return error;
-
-	scsi_proc_host_add(shost);
-
-	scsi_scan_host(shost);
+	if (!error) {
+		scsi_proc_host_add(shost);
+		scsi_scan_host(shost);
+	};
 			
-	list_for_each_entry (sdev, &shost->my_devices, siblings) {
-		error = scsi_attach_device(sdev);
-		if (error)
-			saved_error = error;
-	}
-
-	return saved_error;
+	return error;
 }
 
 /**

===== drivers/scsi/hosts.h 1.63 vs edited =====

drivers/scsi/hosts.h +10 -17
@@ -532,25 +532,18 @@
         return shost->host_gendev.parent;
 }
 
-struct Scsi_Device_Template
-{
-    struct list_head list;
-    const char * name;
-    struct module * module;	  /* Used for loadable modules */
-    unsigned char scsi_type;
-    int (*attach)(Scsi_Device *); /* Attach devices to arrays */
-    void (*detach)(Scsi_Device *);
-    int (*init_command)(Scsi_Cmnd *);     /* Used by new queueing code. 
-                                           Selects command for blkdevs */
-    void (*rescan)(Scsi_Device *);
-    struct device_driver scsi_driverfs_driver;
+struct scsi_driver {
+	struct module		*owner;
+	struct device_driver	gendrv;
+
+	int (*init_command)(struct scsi_cmnd *);
+	void (*rescan)(struct device *);
 };
+#define to_scsi_driver(drv) \
+	container_of((drv), struct scsi_driver, gendrv)
 
-/*
- * Highlevel driver registration/unregistration.
- */
-extern int scsi_register_device(struct Scsi_Device_Template *);
-extern int scsi_unregister_device(struct Scsi_Device_Template *);
+extern int scsi_register_driver(struct scsi_driver *);
+extern void scsi_unregister_driver(struct scsi_driver *);
 
 /*
  * HBA allocation/freeing.

===== drivers/scsi/ide-scsi.c 1.23 vs edited =====

drivers/scsi/ide-scsi.c +2 -2
@@ -770,8 +770,8 @@
 	struct gendisk *disk = cmd->request->rq_disk;
 
 	if (disk) {
-		struct Scsi_Device_Template **p = disk->private_data;
-		if (strcmp((*p)->scsi_driverfs_driver.name, "sg") == 0)
+		struct scsi_driver **p = disk->private_data;
+		if (strcmp((*p)->gendrv.name, "sg") == 0)
 			return test_bit(IDESCSI_SG_TRANSFORM, &scsi->transform);
 	}
 	return test_bit(IDESCSI_TRANSFORM, &scsi->transform);

===== drivers/scsi/osst.c 1.43 vs edited =====

drivers/scsi/osst.c +20 -22

100 lines not on this page · raw message

===== drivers/scsi/osst.h 1.9 vs edited =====

drivers/scsi/osst.h +1 -1
@@ -530,7 +530,7 @@
 
 /* The OnStream tape drive descriptor */
 typedef struct {
-  struct Scsi_Device_Template *driver;
+  struct scsi_driver *driver;
   unsigned capacity;
   Scsi_Device* device;
   struct semaphore lock;       /* for serialization */

===== drivers/scsi/scsi.c 1.112 vs edited =====

drivers/scsi/scsi.c +0 -115

133 lines not on this page · raw message

===== drivers/scsi/scsi_lib.c 1.90 vs edited =====

drivers/scsi/scsi_lib.c +4 -32
@@ -613,29 +613,6 @@
 }
 
 /*
- * Function:    scsi_get_request_dev()
- *
- * Purpose:     Find the upper-level driver that is responsible for this
- *              request
- *
- * Arguments:   request   - I/O request we are preparing to queue.
- *
- * Lock status: No locks assumed to be held, but as it happens the
- *              q->queue_lock is held when this is called.
- *
- * Returns:     Nothing
- *
- * Notes:       The requests in the request queue may have originated
- *              from any block device driver.  We need to find out which
- *              one so that we can later form the appropriate command.
- */
-static struct Scsi_Device_Template *scsi_get_request_dev(struct request *req)
-{
-	struct gendisk *p = req->rq_disk;
-	return p ? *(struct Scsi_Device_Template **)p->private_data : NULL;
-}
-
-/*
  * Function:    scsi_io_completion()
  *
  * Purpose:     Completion processing for block device I/O requests.
@@ -847,11 +824,7 @@
 		return;
 	}
 	if (result) {
-		struct Scsi_Device_Template *sdt;
-
-		sdt = scsi_get_request_dev(cmd->request);
-		printk("SCSI %s error : <%d %d %d %d> return code = 0x%x\n",
-		       (sdt ? sdt->name : "device"),
+		printk("SCSI error : <%d %d %d %d> return code = 0x%x\n",
 		       cmd->device->host->host_no,
 		       cmd->device->channel,
 		       cmd->device->id,
@@ -945,7 +918,6 @@
 
 static int scsi_prep_fn(struct request_queue *q, struct request *req)
 {
-	struct Scsi_Device_Template *sdt;
 	struct scsi_device *sdev = q->queuedata;
 	struct scsi_cmnd *cmd;
 
@@ -1001,6 +973,7 @@
 	 * happening now.
 	 */
 	if (req->flags & (REQ_CMD | REQ_BLOCK_PC)) {
+		struct scsi_driver *drv;
 		int ret;
 
 		/*
@@ -1015,8 +988,6 @@
 		 * some kinds of consistency checking may cause the	
 		 * request to be rejected immediately.
 		 */
-		sdt = scsi_get_request_dev(req);
-		BUG_ON(!sdt);
 
 		/* 
 		 * This sets up the scatter-gather table (allocating if
@@ -1029,7 +1000,8 @@
 		/*
 		 * Initialize the actual SCSI command for this request.
 		 */
-		if (unlikely(!sdt->init_command(cmd))) {
+		drv = *(struct scsi_driver **)req->rq_disk->private_data;
+		if (unlikely(!drv->init_command(cmd))) {
 			scsi_release_buffers(cmd);
 			scsi_put_command(cmd);
 			return BLKPREP_KILL;

===== drivers/scsi/scsi_priv.h 1.6 vs edited =====

drivers/scsi/scsi_priv.h +1 -8
@@ -41,9 +41,6 @@
 #define SCSI_SENSE_VALID(scmd) \
 	(((scmd)->sense_buffer[0] & 0x70) == 0x70)
 
-struct Scsi_Device_Template;
-
-
 /*
  * scsi_target: representation of a scsi target, for now, this is only
  * used for single_lun devices. If no one has active IO to the target,
@@ -68,9 +65,6 @@
 extern void scsi_done(struct scsi_cmnd *cmd);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
 extern int scsi_retry_command(struct scsi_cmnd *cmd);
-extern int scsi_attach_device(struct scsi_device *sdev);
-extern void scsi_detach_device(struct scsi_device *sdev);
-extern void scsi_rescan_device(struct scsi_device *sdev);
 extern int scsi_insert_special_req(struct scsi_request *sreq, int);
 extern void scsi_init_cmd_from_req(struct scsi_cmnd *cmd,
 		struct scsi_request *sreq);
@@ -117,12 +111,11 @@
 extern void scsi_free_sdev(struct scsi_device *);
 extern void scsi_free_shost(struct Scsi_Host *);
 extern void scsi_host_get(struct Scsi_Host *);
+extern void scsi_rescan_device(struct device *dev);
 
 /* scsi_sysfs.c */
 extern int scsi_device_register(struct scsi_device *);
 extern void scsi_device_unregister(struct scsi_device *);
-extern int scsi_upper_driver_register(struct Scsi_Device_Template *);
-extern void scsi_upper_driver_unregister(struct Scsi_Device_Template *);
 extern void scsi_sysfs_init_host(struct Scsi_Host *);
 extern int scsi_sysfs_add_host(struct Scsi_Host *, struct device *);
 extern void scsi_sysfs_remove_host(struct Scsi_Host *);

===== drivers/scsi/scsi_scan.c 1.85 vs edited =====

drivers/scsi/scsi_scan.c +14 -7
@@ -1091,22 +1091,29 @@
 				    uint channel, uint id, uint lun)
 {
 	struct scsi_device *sdev;
-	int error = -ENODEV, res;
+	int res;
 
 	res = scsi_probe_and_add_lun(shost, channel, id, lun, NULL, &sdev);
-	if (res == SCSI_SCAN_LUN_PRESENT)
-		error = scsi_attach_device(sdev);
-
-	if (error)
-		sdev = ERR_PTR(error);
+	if (res != SCSI_SCAN_LUN_PRESENT)
+		sdev = ERR_PTR(-ENODEV);
 	return sdev;
 }
 
 int scsi_remove_device(struct scsi_device *sdev)
 {
-	scsi_detach_device(sdev);
 	scsi_device_unregister(sdev);
 	return 0;
+}
+
+void scsi_rescan_device(struct device *dev)
+{
+	struct scsi_driver *drv = to_scsi_driver(dev->driver);
+
+	if (try_module_get(drv->owner)) {
+		if (drv->rescan)
+			drv->rescan(dev);
+		module_put(drv->owner);
+	}
 }
 
 /**

===== drivers/scsi/scsi_syms.c 1.36 vs edited =====

drivers/scsi/scsi_syms.c +2 -2
@@ -29,8 +29,8 @@
  * This source file contains the symbol table used by scsi loadable
  * modules.
  */
-EXPORT_SYMBOL(scsi_register_device);
-EXPORT_SYMBOL(scsi_unregister_device);
+EXPORT_SYMBOL(scsi_register_driver);
+EXPORT_SYMBOL(scsi_unregister_driver);
 EXPORT_SYMBOL(scsi_register_host);
 EXPORT_SYMBOL(scsi_unregister_host);
 EXPORT_SYMBOL(scsi_add_host);

===== drivers/scsi/scsi_sysfs.c 1.16 vs edited =====

drivers/scsi/scsi_sysfs.c +17 -53

103 lines not on this page · raw message

===== drivers/scsi/sd.c 1.112 vs edited =====

drivers/scsi/sd.c +48 -114

273 lines not on this page · raw message

===== drivers/scsi/sg.c 1.54 vs edited =====

drivers/scsi/sg.c +34 -51

187 lines not on this page · raw message

===== drivers/scsi/sr.c 1.78 vs edited =====

drivers/scsi/sr.c +20 -58

138 lines not on this page · raw message

===== drivers/scsi/sr.h 1.9 vs edited =====

drivers/scsi/sr.h +1 -2
@@ -25,8 +25,7 @@
 #define IOCTL_TIMEOUT 30*HZ
 
 typedef struct scsi_cd {
-	struct Scsi_Device_Template *driver;
-	struct list_head list;
+	struct scsi_driver *driver;
 	unsigned capacity;	/* size in blocks                       */
 	Scsi_Device *device;
 	unsigned int vendor;	/* vendor code, see sr_vendor.c         */

===== drivers/scsi/st.c 1.61 vs edited =====

drivers/scsi/st.c +22 -33

129 lines not on this page · raw message

===== drivers/scsi/st.h 1.13 vs edited =====

drivers/scsi/st.h +1 -1
@@ -70,7 +70,7 @@
 
 /* The tape drive descriptor */
 typedef struct {
-	struct Scsi_Device_Template *driver;
+	struct scsi_driver *driver;
 	Scsi_Device *device;
 	struct semaphore lock;	/* For serialization */
 	struct completion wait;	/* For SCSI commands */
#1 lore raw
Quoted 10 lines · Christoph Hellwig [[email protected]] wrote:

Upper drivers now use the LDM ->probe/->remove callbacks and the core list code. Note that this means there is only one driver per scsi device and not multiple, e.g. you can't have sd _and_ sg for the same device. Personally I think that's not a problem anymore with the generic SG_IO in place, but if you scream loud enough I could come up with a hack that makes the sg nodes a property of the scsi midlayer instead of a LDM-style driver and we could get the old behaviour back.

I ran your patch and I dead locked on st_probe. This is due to the code in st_probe and osst_probe that do device registers on the sdev_driverfs_dev.bus which is the bus that the probe is being called from already. I had an old patch that removed these before due the driver model requirement that objects on the device list must be of one type. At the time I did not have a place to move these attributes.

We now have /bus/scsi/drivers/{uld} for driver attributes. We also could create a class for other attributes if needed. We can also use udev for the creation of special nodes.

ftp://ftp.kernel.org/pub/linux/utils/kernel/hotplug/

In the short term is the st and osst maintainers are ok with it we could remove them and then create them in there proper home.

-andmike
--
Michael Anderson
[email protected]

#2 lore raw
Quoted 8 lines · Mike Anderson

I ran your patch and I dead locked on st_probe. This is due to the code in st_probe and osst_probe that do device registers on the sdev_driverfs_dev.bus which is the bus that the probe is being called from already. I had an old patch that removed these before due the driver model requirement that objects on the device list must be of one type. At the time I did not have a place to move these attributes.

Quoted 8 lines

We now have /bus/scsi/drivers/{uld} for driver attributes. We also could create a class for other attributes if needed. We can also use udev for the creation of special nodes.

ftp://ftp.kernel.org/pub/linux/utils/kernel/hotplug/

In the short term is the st and osst maintainers are ok with it we could remove them and then create them in there proper home.

As the osst maintainer I concur. I am going to need a class, I believe, as I want to export some info from the tape header, i.e. per device (very useful for me for support purposes). So some re-arrangement is going to happen anyway.

Thanks, Willem Riede.

#3 lore raw
In reply to Mike Anderson
Quoted 1 line · Mike Anderson

I ran your patch and I dead locked on st_probe.

Hmm, I loaded st fine here, but st_probe is of course never called when I have no tapes.`

Quoted 6 lines

This is due to the code
in st_probe and osst_probe that do device registers on the sdev_driverfs_dev.bus which is the bus that the probe is being called from already. I had an old patch that removed these before due the driver model requirement that objects on the device list must be of one type. At the time I did not have a place to move these attributes.

Most of the attrributes should just go away - the code looks like someone played with the driver model because it looked new and shiny..

#4 lore raw
In reply to Mike Anderson
Quoted 6 lines · Mike Anderson

I ran your patch and I dead locked on st_probe. This is due to the code in st_probe and osst_probe that do device registers on the sdev_driverfs_dev.bus which is the bus that the probe is being called from already. I had an old patch that removed these before due the driver model requirement that objects on the device list must be of one type. At the time I did not have a place to move these attributes.

Okay, here's the patch to remove the strange kdev and type attributes that caused this problems. All the useful attributes are still left, they're attributes of the right struct device.

drivers/scsi/osst.c +1 -58
@@ -5323,22 +5323,6 @@
 
 #endif
 
-/* Driverfs file support */
-static ssize_t osst_device_kdev_read(struct device *driverfs_dev, char *page)
-{
-	kdev_t kdev; 
-	kdev.value=(int)(long)driverfs_dev->driver_data;
-	return sprintf(page, "%x\n",kdev.value);
-}
-static DEVICE_ATTR(kdev,S_IRUGO,osst_device_kdev_read,NULL);
-
-static ssize_t osst_device_type_read(struct device *driverfs_dev, char *page) 
-{
-	return sprintf (page, "CHR\n");
-}
-static DEVICE_ATTR(type,S_IRUGO,osst_device_type_read,NULL);
-
-
 static struct file_operations osst_fops = {
 	.owner =        THIS_MODULE,
 	.read =         osst_read,
@@ -5519,45 +5503,16 @@
 	write_unlock(&os_scsi_tapes_lock);
 
 	for (mode = 0; mode < ST_NBR_MODES; ++mode) {
-		char name[8], devfs_name[64];
+		char devfs_name[64];
 
 		/*  Rewind entry  */
-		sprintf(name, "ot%s", osst_formats[mode]);
 		sprintf(devfs_name, "%s/ot%s", SDp->devfs_name, osst_formats[mode]);
-
-		sprintf(tpnt->driverfs_dev_r[mode].bus_id, "%s:%s", 
-				SDp->sdev_driverfs_dev.bus_id, name);
-		sprintf(tpnt->driverfs_dev_r[mode].name, "%s%s", 
-				SDp->sdev_driverfs_dev.name, name);
-		tpnt->driverfs_dev_r[mode].parent = &SDp->sdev_driverfs_dev;
-		tpnt->driverfs_dev_r[mode].bus = SDp->sdev_driverfs_dev.bus;
-		tpnt->driverfs_dev_r[mode].driver_data =
-			(void *)(long)__mkdev(OSST_MAJOR, dev_num + (mode << 5));
-		device_register(&tpnt->driverfs_dev_r[mode]);
-		device_create_file(&tpnt->driverfs_dev_r[mode], 
-				&dev_attr_type);
-		device_create_file(&tpnt->driverfs_dev_r[mode], &dev_attr_kdev);
 		devfs_register(NULL, devfs_name, 0,
 					OSST_MAJOR, dev_num + (mode << 5),
 					S_IFCHR | S_IRUGO | S_IWUGO,
 					&osst_fops, NULL);
 		/*  No-rewind entry  */
-		sprintf (name, "ot%sn", osst_formats[mode]);
 		sprintf(devfs_name, "%s/ot%sn", SDp->devfs_name, osst_formats[mode]);
-
-		sprintf(tpnt->driverfs_dev_n[mode].bus_id, "%s:%s", 
-				SDp->sdev_driverfs_dev.bus_id, name);
-		sprintf(tpnt->driverfs_dev_n[mode].name, "%s%s", 
-				SDp->sdev_driverfs_dev.name, name);
-		tpnt->driverfs_dev_n[mode].parent= &SDp->sdev_driverfs_dev;
-		tpnt->driverfs_dev_n[mode].bus = SDp->sdev_driverfs_dev.bus;
-		tpnt->driverfs_dev_n[mode].driver_data =
-			(void *)(long)__mkdev(OSST_MAJOR, dev_num + (mode << 5) + 128);
-		device_register(&tpnt->driverfs_dev_n[mode]);
-		device_create_file(&tpnt->driverfs_dev_n[mode], 
-				&dev_attr_type);
-		device_create_file(&tpnt->driverfs_dev_n[mode], 
-				&dev_attr_kdev);
 		devfs_register(NULL, devfs_name, 0,
 					OSST_MAJOR, dev_num + (mode << 5) + 128,
 					S_IFCHR | S_IRUGO | S_IWUGO,
@@ -5598,18 +5553,6 @@
 		os_scsi_tapes[i] = NULL;
 		osst_nr_dev--;
 		write_unlock(&os_scsi_tapes_lock);
-		for (mode = 0; mode < ST_NBR_MODES; ++mode) {
-			device_remove_file(&tpnt->driverfs_dev_r[mode],
-					&dev_attr_type);
-			device_remove_file(&tpnt->driverfs_dev_r[mode],
-					&dev_attr_kdev);
-			device_unregister(&tpnt->driverfs_dev_r[mode]);
-			device_remove_file(&tpnt->driverfs_dev_n[mode],
-					&dev_attr_type);
-			device_remove_file(&tpnt->driverfs_dev_n[mode],
-					&dev_attr_kdev);
-			device_unregister(&tpnt->driverfs_dev_n[mode]);
-			}
 		if (tpnt->header_cache != NULL) vfree(tpnt->header_cache);
 		if (tpnt->buffer) {
 			normalize_buffer(tpnt->buffer);
drivers/scsi/osst.h +0 -2
@@ -555,8 +555,6 @@
   /* Mode characteristics */
   ST_mode modes[ST_NBR_MODES];
   int current_mode;
-  struct device driverfs_dev_r[ST_NBR_MODES];
-  struct device driverfs_dev_n[ST_NBR_MODES];
 
   /* Status variables */
   int partition;
drivers/scsi/st.c +1 -56

84 lines not on this page · raw message

drivers/scsi/st.h +0 -2
@@ -103,8 +103,6 @@
 	/* Mode characteristics */
 	ST_mode modes[ST_NBR_MODES];
 	int current_mode;
-	struct device driverfs_dev_r[ST_NBR_MODES];
-	struct device driverfs_dev_n[ST_NBR_MODES];
 
 	/* Status variables */
 	int partition;
#5 lore raw
Quoted 4 lines · Christoph Hellwig [[email protected]] wrote:

Okay, here's the patch to remove the strange kdev and type attributes that caused this problems. All the useful attributes are still left, they're attributes of the right struct device.

Thanks, I am doing insmod / rmmod with sd, sg, sr, and st upper levels binding and unbinding.

-andmike
--
Michael Anderson
[email protected]

#6 lore raw
In reply to Mike Anderson
Quoted 2 lines · Mike Anderson
Quoted 1 line · Christoph Hellwig [[email protected]] wrote:

Upper drivers now use the LDM ->probe/->remove callbacks and the

...

Quoted 7 lines

I ran your patch and I dead locked on st_probe. This is due to the code in st_probe and osst_probe that do device registers on the sdev_driverfs_dev.bus which is the bus that the probe is being called from already. I had an old patch that removed these before due the driver model requirement that objects on the device list must be of one type. At the time I did not have a place to move these attributes.

Quoted 9 lines

We now have /bus/scsi/drivers/{uld} for driver attributes. We also could create a class for other attributes if needed. We can also use udev for the creation of special nodes.

ftp://ftp.kernel.org/pub/linux/utils/kernel/hotplug/

In the short term is the st and osst maintainers are ok with it we could remove them and then create them in there proper home.

I don't mind removal of the mt* files. I tested in 2.5.69-bk11 a modification to st that was essentially what Christoph has just posted. No problems there (i.e., tape found and works ok, sysfs shows the tape as SCSI device).

However, when I add to this Christoph's patch to change probing, I don't find any tapes any more. What I did was to first modprobe aic7xxx. According to dmesg, the device was found:

(scsi2:A:5): 10.000MB/s transfers (10.000MHz, offset 8)
Vendor: HP Model: C1533A Rev: 9608
Type: Sequential-Access ANSI SCSI revision: 02
Attached scsi generic sg0 at scsi2, channel 0, id 5, lun 0, type 1

It did not appear in /sysfs/bus/scsi/devices (the system disk and the cdrom were there):

# tree /sysfs/bus/scsi/devices/
/sysfs/bus/scsi/devices/
|-- 0:0:0:0 -> ../../../devices/pci0/00:1e.0/02:02.0/host0/0:0:0:0
`-- 1:0:0:0 -> ../../../devices/ide-scsi/host1/1:0:0:0

Modprobing st did not result in any messages (each attached tape causes one line to be printed). The tape device does not exist:

# mt status
/dev/nst0: No such device or address
Signature · 1 line
Kai
#7 lore raw
Quoted 11 lines · Kai Makisara

However, when I add to this Christoph's patch to change probing, I don't find any tapes any more. What I did was to first modprobe aic7xxx. According to dmesg, the device was found:

(scsi2:A:5): 10.000MB/s transfers (10.000MHz, offset 8)
Vendor: HP Model: C1533A Rev: 9608
Type: Sequential-Access ANSI SCSI revision: 02
Attached scsi generic sg0 at scsi2, channel 0, id 5, lun 0, type 1

It did not appear in /sysfs/bus/scsi/devices (the system disk and the cdrom were there):

That exactly what I said in my mail to Douglas before - now there always is one driver for each device. If you have sg loaded but not st (yet) sg will claim your tapes, not st.

#8 lore raw
Quoted 3 lines · Christoph Hellwig

That exactly what I said in my mail to Douglas before - now there always is one driver for each device. If you have sg loaded but not st (yet) sg will claim your tapes, not st.

In principle, I'm not opposed to single attachment. However, we need the SG_IO demonstrably working for all devices (including tape) before we try turning it off.

So, let's look at the SG_IO for cdev patch before considering this one.

James

#9 lore raw
In reply to Kai Makisara

On Sat, 17 May 2003, Kai Makisara wrote:

...

Quoted 7 lines

I don't mind removal of the mt* files. I tested in 2.5.69-bk11 a modification to st that was essentially what Christoph has just posted. No problems there (i.e., tape found and works ok, sysfs shows the tape as SCSI device).

However, when I add to this Christoph's patch to change probing, I don't find any tapes any more. What I did was to first modprobe aic7xxx.

...

Quoted 3 lines

Modprobing st did not result in any messages (each attached tape causes one line to be printed). The tape device does not exist:

I have done more experiments that may be useful. I used both a SCSI disk (sd) and a SCSI tape (st) to see if the problems are related to the high level driver. The behaviour with sd and st was similar, i.e., this problem is not related to st specifically.

The result from the experiments was that nothing will be recognised properly after the sg driver is initialised. I had it in the kernel in the first experiment. The same applies to using sg as a module. If I load first sd and/or st, the devices will be recognised. If I load sg and then sd and/or st, the devices will not be recognised. They are not properly visible as sg devices either. It does not seem to matter if the HBA driver is in kernel or a module or loaded before or after the high-level driver.

To some extent these results are expected. Your said that only one high-level driver can be attached to one device. What is not explainable is why they don't show as sg devices or in the sysfs /bus/scsi/devices directory.

I think it should be possible to attach several high-level drivers to one device. It is useful in some cases.

Signature · 1 line
Kai
#10 lore raw
In reply to Christoph Hellwig
Quoted 4 lines · Christoph Hellwig

Upper drivers now use the LDM ->probe/->remove callbacks and the core list code. Note that this means there is only one driver per scsi device and not multiple, e.g. you can't have sd _and_ sg for the same device.

Is that "_and_" symmetric? That is, can one have the sg device in preference to the sd device? If so, how is that selected?

Quoted 5 lines

Personally I think that's not a problem anymore
with the generic SG_IO in place, but if you scream loud enough I could come up with a hack that makes the sg nodes a property of the scsi midlayer instead of a LDM-style driver and we could get the old behaviour back.

Christoph,
This patch would have some interesting side effects:
- no generic interface (apart from SCSI_IOCTL_SEND_COMMAND) for tape devices (st and osst). While the maintainers of st and osst might like this, not all the user apps would :-)
- the SG_IO ioctl in the block level
- limits maximum transfer size to 65536 bytes
- does not support mmap-ed IO [and would need something like a reserved buffer concept to do it]
- does not support direct IO
- needs work (debuggings)
- does not support async IO (either does SG_IO
in sg but it can be done other ways in sg)
- the extra ioctls that sg defines are either fudged or missing. This might upset programs other than cdrecord; cdparanoia comes to mind

It was my understanding that the device model has been recently modified by Greg KH to cope with the dual nature of the interface that sg causes. Evidently similar situations occur elsewhere in the kernel.

Whether the sg driver is an upper level SCSI driver or part of the mid level actually makes little difference. The sgbind patch that I canvassed the other day actually fits well into sg's functionality being subsumed into the mid level with a single virtual device node (e.g. /dev/scsi or /dev/scsibind ?) for access with each open file descriptor getting its own state.

But then there is the problem of backward compatibility...

Doug Gilbert

#11 lore raw
Quoted 3 lines · Douglas Gilbert

Is that "_and_" symmetric? That is, can one have the sg device in preference to the sd device? If so, how is that selected?

You can. In fact there's nothing in this patch that preferes sd over sg except the existing link order. A better way to control the driver matching would be nice but that nothing that should be done in the scsi code but rather a generic driver model thing.

Quoted 5 lines

Christoph,
This patch would have some interesting side effects:
- no generic interface (apart from SCSI_IOCTL_SEND_COMMAND) for tape devices (st and osst). While the maintainers of st and osst might like this, not all the user apps would :-)

I have some WIP code to make parts of drivers/block/scsi_ioctl.c working for scsi char drivers.

Quoted 3 lines

- the extra ioctls that sg defines are either fudged or missing. This might upset programs other than cdrecord; cdparanoia comes to mind

Hmm, what does cdparanoia complain about? And yes, I think we need to kill all those return 0 ioctls from the block layer - they might have helped to bring up the SG_IO code with cdrecord but they are a really stupid idea long term.

Quoted 4 lines

It was my understanding that the device model has been recently modified by Greg KH to cope with the dual nature of the interface that sg causes. Evidently similar situations occur elsewhere in the kernel.

Greg, any comments on this? I haven't found any code like that in drivers/base/ and I can't understand how this is supposed to work..`

#12 lore raw
Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · Douglas Gilbert

It was my understanding that the device model has been recently modified by Greg KH to cope with the dual nature of the interface that sg causes. Evidently similar situations occur elsewhere in the kernel.

Greg, any comments on this? I haven't found any code like that in drivers/base/ and I can't understand how this is supposed to work..`

I don't really know what you all are talking about here, but... In talking with Mike Anderson about the driver class code changes, he and I agreed that sg could just be made a struct class_device. It could go under a "scsi" class if you want, and would point to the scsi devices (in the device tree) that the sg driver is bound to.

Does that make more sense?

thanks,

greg k-h

#13 lore raw
In reply to Douglas Gilbert
Quoted 2 lines · Douglas Gilbert

- the SG_IO ioctl in the block level
- limits maximum transfer size to 65536 bytes

Correct, we can remove that though.

Quoted 2 lines

- does not support mmap-ed IO [and would need something like a reserved buffer concept to do it]

Why should it?

Quoted 1 line

- does not support direct IO

False, it uses direct IO all the time if the alignment is correct.

Quoted 1 line

- needs work (debuggings)

Know of any bugs?

Signature · 1 line
Jens Axboe
#14 lore raw
In reply to Douglas Gilbert
Quoted 5 lines · Douglas Gilbert

Christoph,
This patch would have some interesting side effects:
- no generic interface (apart from SCSI_IOCTL_SEND_COMMAND) for tape devices (st and osst). While the maintainers of st and osst might like this, not all the user apps would :-)

I would certainly like that sg access could not "interfere" with osst while it is in control, but there would obviously have to be a way to assign individual devices to specific drivers (as in tape1 is handled by st and tape2 by sg). And that would have to be dynamic (as in I did a backup on tape1 with osst and now I want to tinker with another tape using sg). Such mechanism doesn't exist, I think.

Thanks, Willem Riede.

#15 lore raw
In reply to Christoph Hellwig
Quoted 8 lines · Christoph Hellwig

Upper drivers now use the LDM ->probe/->remove callbacks and the core list code. Note that this means there is only one driver per scsi device and not multiple, e.g. you can't have sd _and_ sg for the same device. Personally I think that's not a problem anymore with the generic SG_IO in place, but if you scream loud enough I could come up with a hack that makes the sg nodes a property of the scsi midlayer instead of a LDM-style driver and we could get the old behaviour back.

The sg attributes (calls to device_create_file) should be removed (at least until sg has its own class or place to put the attributes), since sg_probe creates a "type" attribute, then when we return back into scsi_device_register and try to create the scsi_device "type" attribute, we fail and call scsi_device_unregister.

And scsi_scan.c ignores the failure, probably leading to some strangeness or at best a memory leak.

The end result is that there is no scsi_device under sysfs if sg_probe attached to the scsi_device.

-- Patrick Mansfield