[PATCH] SCSI hotplug support

[PATCH] SCSI hotplug support

[Erik Andersen]

Using the following patch I am able to make ieee1394 properly
connect and disconnect SBP-2 devices to/from the SCSI subsystem.
Without this patch, firewire devices are only registered with the
SCSI subsystem if they are connected when the host adaptor is
registered, and are only unregistered from the SCSI subsystem
when the firewire driver modules are removed from the kernel.
This is far from desirable...

This patch works by exporting within the kernel the functionality
already provided to userspace via /proc/scsi/scsi, i.e.:
	echo "scsi add-single-device 0 1 2 3" >/proc/scsi/scsi
and 
	echo "scsi remove-single-device 0 1 2 3" >/proc/scsi/scsi
is exported within the kernel as scsi_add_single_device() and 
scsi_remove_single_device().  A separate patch to the sbp2 driver
then uses these interfaces when devices are plugged or unplugged.
After looking though the current code, there does not seem to be
any other way to accomplish the same thing using the current SCSI
infrastructure.  Anyone have any objections to such a patch?  For
2.4.x?  Comments? 

Please CC me as I am not subscribed to linux-scsi, thanks,

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--


--- orig/drivers/scsi/hosts.h	2002-10-09 04:41:10.000000000 -0600
+++ linux-2.4.19/drivers/scsi/hosts.h	2002-10-09 04:41:10.000000000 -0600
@@ -532,6 +532,13 @@
 int scsi_register_device(struct Scsi_Device_Template * sdpnt);
 void scsi_deregister_device(struct Scsi_Device_Template * tpnt);
 
+/* Support for hot plugging and unplugging devices -- safe for 
+ * ieee1394 or USB devices, but probably not for normal SCSI... */
+extern int scsi_add_single_device(struct Scsi_Host *shpnt, 
+	int channel, int id, int lun);
+extern int scsi_remove_single_device(struct Scsi_Host *shpnt, 
+	int channel, int id, int lun);
+
 /* These are used by loadable modules */
 extern int scsi_register_module(int, void *);
 extern int scsi_unregister_module(int, void *);
--- orig/drivers/scsi/scsi.c	2002-10-09 04:41:10.000000000 -0600
+++ linux-2.4.19/drivers/scsi/scsi.c	2002-10-09 04:41:10.000000000 -0600
@@ -1553,6 +1553,96 @@
     }
 }
 
+/* Support for plugging/unplugging SCSI devices.  This feature is
+ * probably unsafe for standard SCSI devices, but is perfectly
+ * normal for things like ieee1394 or USB drives since these
+ * busses are designed for hotplugging.  Use at your own risk.... */
+int scsi_add_single_device(struct Scsi_Host *shpnt, 
+	int channel, int id, int lun)
+{
+	Scsi_Device *scd;
+
+	/* Do a bit of sanity checking */
+	if (shpnt==NULL) {
+		return  -ENXIO;
+	}
+
+	/* Check if they asked us to add an already existing device.
+	 * If so, ignore their misguided efforts. */
+	for (scd = shpnt->host_queue; scd; scd = scd->next) {
+		if ((scd->channel == channel && scd->id == id && scd->lun == lun)) {
+			break;
+		}
+	}
+	if (scd) {
+		return -ENOSYS;
+	}
+
+	scan_scsis(shpnt, 1, channel, id, lun);
+	return 0;
+}
+
+/* Support for plugging/unplugging SCSI devices.  This feature is
+ * probably unsafe for standard SCSI devices, but is perfectly
+ * normal for things like ieee1394 or USB drives since these
+ * busses are designed for hotplugging.  Use at your own risk.... */
+int scsi_remove_single_device(struct Scsi_Host *shpnt, 
+	int channel, int id, int lun)
+{
+	Scsi_Device *scd;
+	struct Scsi_Device_Template *SDTpnt;
+
+	/* Do a bit of sanity checking */
+	if (shpnt==NULL) {
+		return  -ENODEV;
+	}
+
+	/* Make sure the specified device is in fact present */
+	for (scd = shpnt->host_queue; scd; scd = scd->next) {
+		if ((scd->channel == channel && scd->id == id && scd->lun == lun)) {
+			break;
+		}
+	}
+	if (scd==NULL) {
+		return -ENODEV;
+	}
+
+	/* See if the specified device is busy */
+	if (scd->access_count)
+		return -EBUSY;
+
+	SDTpnt = scsi_devicelist;
+	while (SDTpnt != NULL) {
+		if (SDTpnt->detach)
+			(*SDTpnt->detach) (scd);
+		SDTpnt = SDTpnt->next;
+	}
+
+	if (scd->attached == 0) {
+		/* Nobody is using this device, so we
+		 * can now free all command structures.  */
+		if (shpnt->hostt->revoke)
+			shpnt->hostt->revoke(scd);
+		devfs_unregister (scd->de);
+		scsi_release_commandblocks(scd);
+
+		/* Now we can remove the device structure */
+		if (scd->next != NULL)
+			scd->next->prev = scd->prev;
+
+		if (scd->prev != NULL)
+			scd->prev->next = scd->next;
+
+		if (shpnt->host_queue == scd) {
+			shpnt->host_queue = scd->next;
+		}
+		blk_cleanup_queue(&scd->request_queue);
+		kfree((char *) scd);
+	}
+
+	return 0;
+}
+
 #ifdef CONFIG_PROC_FS
 static int scsi_proc_info(char *buffer, char **start, off_t offset, int length)
 {
@@ -1605,8 +1695,6 @@
 static int proc_scsi_gen_write(struct file * file, const char * buf,
                               unsigned long length, void *data)
 {
-	struct Scsi_Device_Template *SDTpnt;
-	Scsi_Device *scd;
 	struct Scsi_Host *HBA_ptr;
 	char *p;
 	int host, channel, id, lun;
@@ -1745,33 +1833,11 @@
 				break;
 			}
 		}
-		err = -ENXIO;
-		if (!HBA_ptr)
-			goto out;
-
-		for (scd = HBA_ptr->host_queue; scd; scd = scd->next) {
-			if ((scd->channel == channel
-			     && scd->id == id
-			     && scd->lun == lun)) {
-				break;
-			}
-		}
-
-		err = -ENOSYS;
-		if (scd)
-			goto out;	/* We do not yet support unplugging */
-
-		scan_scsis(HBA_ptr, 1, channel, id, lun);
-
-		/* FIXME (DB) This assumes that the queue_depth routines can be used
-		   in this context as well, while they were all designed to be
-		   called only once after the detect routine. (DB) */
-		/* queue_depth routine moved to inside scan_scsis(,1,,,) so
-		   it is called before build_commandblocks() */
-
-		err = length;
+		if ((err=scsi_add_single_device(HBA_ptr, channel, id, lun))==0)
+		    err = length;
 		goto out;
 	}
+
 	/*
 	 * Usage: echo "scsi remove-single-device 0 1 2 3" >/proc/scsi/scsi
 	 * with  "0 1 2 3" replaced by your "Host Channel Id Lun".
@@ -1797,58 +1863,8 @@
 				break;
 			}
 		}
-		err = -ENODEV;
-		if (!HBA_ptr)
-			goto out;
-
-		for (scd = HBA_ptr->host_queue; scd; scd = scd->next) {
-			if ((scd->channel == channel
-			     && scd->id == id
-			     && scd->lun == lun)) {
-				break;
-			}
-		}
-
-		if (scd == NULL)
-			goto out;	/* there is no such device attached */
-
-		err = -EBUSY;
-		if (scd->access_count)
-			goto out;
-
-		SDTpnt = scsi_devicelist;
-		while (SDTpnt != NULL) {
-			if (SDTpnt->detach)
-				(*SDTpnt->detach) (scd);
-			SDTpnt = SDTpnt->next;
-		}
-
-		if (scd->attached == 0) {
-			/*
-			 * Nobody is using this device any more.
-			 * Free all of the command structures.
-			 */
-                        if (HBA_ptr->hostt->revoke)
-                                HBA_ptr->hostt->revoke(scd);
-			devfs_unregister (scd->de);
-			scsi_release_commandblocks(scd);
-
-			/* Now we can remove the device structure */
-			if (scd->next != NULL)
-				scd->next->prev = scd->prev;
-
-			if (scd->prev != NULL)
-				scd->prev->next = scd->next;
-
-			if (HBA_ptr->host_queue == scd) {
-				HBA_ptr->host_queue = scd->next;
-			}
-			blk_cleanup_queue(&scd->request_queue);
-			kfree((char *) scd);
-		} else {
-			goto out;
-		}
-		err = 0;
+		err=scsi_remove_single_device(HBA_ptr, channel, id, lun);
+		goto out;
 	}
 out:
 	
--- orig/drivers/scsi/scsi_syms.c	2002-10-09 04:41:10.000000000 -0600
+++ linux-2.4.19/drivers/scsi/scsi_syms.c	2002-10-09 04:41:10.000000000 -0600
@@ -103,3 +103,9 @@
 extern int scsi_delete_timer(Scsi_Cmnd *);
 EXPORT_SYMBOL(scsi_add_timer);
 EXPORT_SYMBOL(scsi_delete_timer);
+
+/* Support for hot plugging and unplugging devices -- safe for 
+ * ieee1394 or USB devices, but probably not for normal SCSI... */
+EXPORT_SYMBOL(scsi_add_single_device);
+EXPORT_SYMBOL(scsi_remove_single_device);
+

Re: [PATCH] SCSI hotplug support

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 2078 bytes --]

Well... dang.  I've been hoping this would appear for quite some time.  I'm
just not good enough with the SCSI layer to really make this stable.

How stable is this?  I'd love to see this in 2.5 before the Oct 31 feature
freeze, so USB Mass Storage can use this.  Stability is my major concern --
someone has to agree to work on this to make it stable to use, and work out
any possible corner-cases and race conditions.

But, heck.. this is 2.5 -- I say let's include it, use it, and then shake
the bugs out.

Matt

On Sun, Oct 13, 2002 at 11:40:35PM -0600, Erik Andersen wrote:
> Using the following patch I am able to make ieee1394 properly
> connect and disconnect SBP-2 devices to/from the SCSI subsystem.
> Without this patch, firewire devices are only registered with the
> SCSI subsystem if they are connected when the host adaptor is
> registered, and are only unregistered from the SCSI subsystem
> when the firewire driver modules are removed from the kernel.
> This is far from desirable...
> 
> This patch works by exporting within the kernel the functionality
> already provided to userspace via /proc/scsi/scsi, i.e.:
> 	echo "scsi add-single-device 0 1 2 3" >/proc/scsi/scsi
> and 
> 	echo "scsi remove-single-device 0 1 2 3" >/proc/scsi/scsi
> is exported within the kernel as scsi_add_single_device() and 
> scsi_remove_single_device().  A separate patch to the sbp2 driver
> then uses these interfaces when devices are plugged or unplugged.
> After looking though the current code, there does not seem to be
> any other way to accomplish the same thing using the current SCSI
> infrastructure.  Anyone have any objections to such a patch?  For
> 2.4.x?  Comments? 
> 
> Please CC me as I am not subscribed to linux-scsi, thanks,
> 
>  -Erik

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

C:  Why are you upgrading to NT?
AJ: It must be the sick, sadistic streak that runs through me.
					-- Chief and A.J.
User Friendly, 5/12/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

On Mon Oct 14, 2002 at 12:06:02AM -0700, Matthew Dharm wrote:
> Well... dang.  I've been hoping this would appear for quite some time.  I'm
> just not good enough with the SCSI layer to really make this stable.
> 
> How stable is this?  I'd love to see this in 2.5 before the Oct 31 feature
> freeze, so USB Mass Storage can use this.  Stability is my major concern --
> someone has to agree to work on this to make it stable to use, and work out
> any possible corner-cases and race conditions.
> 
> But, heck.. this is 2.5 -- I say let's include it, use it, and then shake
> the bugs out.

I've not tried it with 2.5.x, but in 2.4.x things are 100%
perfectly stable for me and for the several people that I've had
testing it.  If this were included in the kernel, I would not
expect any maintainence to be necessary, since this patch is
mostly just taking existing scsi code and moving it into
standalone functions.

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] SCSI hotplug support

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 1389 bytes --]

Hi Erik,

On Mon, Oct 14, 2002 at 01:12:52AM -0600, Erik Andersen wrote:
> I've not tried it with 2.5.x, but in 2.4.x things are 100%
> perfectly stable for me and for the several people that I've had
> testing it.  If this were included in the kernel, I would not
> expect any maintainence to be necessary, since this patch is
> mostly just taking existing scsi code and moving it into
> standalone functions.

The split-up in smaller functions should have been done anyway.
Exposing the functions for useful purpose even more so.

There are some scary comments around the scsi add/remove-single-device
code which imply that this stuff only works by accident. 
I have used this functionality heavily for some time now and have never 
had a problem. Same for people I talked to.
But it would  probably make sense to review the code for possible
problems.

But I'm all for integrating your patch. It does not introduce new
problems. And if it exposes old ones, because the code is more
heavily used, this can only be good.

Can you create a 2.5 version?

Regards,
-- 
Kurt Garloff                   <kurt@garloff.de>         [Eindhoven, NL]
Physics: Plasma simulations    <K.Garloff@TUE.NL>     [TU Eindhoven, NL]
Linux: SCSI, Security          <garloff@suse.de>    [SuSE Nuernberg, DE]
 (See mail header or public key servers for PGP2 and GPG public keys.)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] SCSI hotplug support

[Mike Anderson]

Kurt Garloff [garloff@suse.de] wrote:
> There are some scary comments around the scsi add/remove-single-device
> code which imply that this stuff only works by accident. 
> I have used this functionality heavily for some time now and have never 
> had a problem. Same for people I talked to.
> But it would  probably make sense to review the code for possible
> problems.
> 
> But I'm all for integrating your patch. It does not introduce new
> problems. And if it exposes old ones, because the code is more
> heavily used, this can only be good.

The comments may be pointing at the issues of the locking around the
host_queue. Adds will probably be ok as we init the scsi_device before
adding it to the list (so one would see a ready scsi_device or NULL
?maybe?). The remove could be an issue if something  happened to be
traversing the list at the same time. 


-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

On Mon Oct 14, 2002 at 09:19:22AM -0700, Mike Anderson wrote:
> Kurt Garloff [garloff@suse.de] wrote:
> > There are some scary comments around the scsi add/remove-single-device
> > code which imply that this stuff only works by accident. 
> > I have used this functionality heavily for some time now and have never 
> > had a problem. Same for people I talked to.
> > But it would  probably make sense to review the code for possible
> > problems.
> > 
> > But I'm all for integrating your patch. It does not introduce new
> > problems. And if it exposes old ones, because the code is more
> > heavily used, this can only be good.
> 
> The comments may be pointing at the issues of the locking around the
> host_queue. Adds will probably be ok as we init the scsi_device before

It was my understanding that the comments (which were present
long before I changed anything) refer mainly to the fact that
hotswapping with a normal SCSI subsystem (without additional
electronic support for hotswapping) is electrically unsafe and is
likely to fry up your drives and your SCSI host controller.  For
people with hotswap safe hardware (or using ieee1394 or usb mass
storage devices), hotswapping is perfectly normal and safe, and
the scarry comments do not apply.

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Re: [PATCH] SCSI hotplug support

[Mike Anderson]

Erik Andersen [andersen@codepoet.org] wrote:
> It was my understanding that the comments (which were present
> long before I changed anything) refer mainly to the fact that
> hotswapping with a normal SCSI subsystem (without additional
> electronic support for hotswapping) is electrically unsafe and is
> likely to fry up your drives and your SCSI host controller.  For
> people with hotswap safe hardware (or using ieee1394 or usb mass
> storage devices), hotswapping is perfectly normal and safe, and
> the scarry comments do not apply.

I agree on the comment being more about hardware.

but..

While it may run many times the transversal of a list and references off
a element in that list concurrent with a process that might be removing
a entry from the list will eventually cause a problem. This is probably
hard to hit, but many drivers and the SCSI proc interface iterate over
the host_queue list so it is possible.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

On Mon Oct 14, 2002 at 05:47:19PM +0200, Kurt Garloff wrote:
> But I'm all for integrating your patch. It does not introduce new
> problems. And if it exposes old ones, because the code is more
> heavily used, this can only be good.
> 
> Can you create a 2.5 version?

I can certainly take a look.  I've been busy enough thus far
that I've not even tried 2.5.  Assuming this part of scsi.c
hasn't changed beyond all recognition, it should be pretty quick
work.

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] SCSI hotplug support

[James Bottomley]

andersen@codepoet.org said:
> Anyone have any objections to such a patch?  For 2.4.x?  Comments?

The patch looks fine per se.

However, I thought that the whole spirit of hotplug was that this type of 
problem should be solved in user space to which SCSI currently exposes a 
perfectly adequate interface.

Needing kernel hooks in a different subsystem to solve a hotplug problem looks 
slightly wrong to me.  Usually when this type of thing comes up its because 
we're missing a hotplug event (or a hotplug event doesn't carry the right 
information).  Could you elaborate more on the actual problem you're trying to 
solve?

James

Re: [PATCH] SCSI hotplug support

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> Needing kernel hooks in a different subsystem to solve a hotplug problem looks 
> slightly wrong to me.  Usually when this type of thing comes up its because 
> we're missing a hotplug event (or a hotplug event doesn't carry the right 
> information).  Could you elaborate more on the actual problem you're trying to 
> solve?
> 
> James

I believe one thing that we are missing is an event when a scsi_host is
added. Even in 2.5 we are not receiving a hotplug event for the addition
of a scsi_host, because of the way we init are LL drivers. This will be fixed
soon.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] SCSI hotplug support

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]

On Mon, Oct 14, 2002 at 08:57:21AM -0700, James Bottomley wrote:
> andersen@codepoet.org said:
> > Anyone have any objections to such a patch?  For 2.4.x?  Comments?
> 
> The patch looks fine per se.
> 
> However, I thought that the whole spirit of hotplug was that this type of 
> problem should be solved in user space to which SCSI currently exposes a 
> perfectly adequate interface.

Yes and no, I think.

While I think much of hotplug should exist in userspace, some of it must
exist in the kernel.

sbp2 and usb-storage get devices attaching and detaching all the time.  The
SCSI core needs to be able to handle that in the kernel.

In userspace, something should get a new device notification and decide
what to do with the new device. Format?  Mount?  Ignore?  But, it needs a
/dev node to be able to handle the device, therefore the kernel must do the
work up to that point.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

We've made a business out of making money from annoying and stupid people.
I think you fall under that category.
					-- Chief
User Friendly, 7/11/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] SCSI hotplug support

[James Bottomley]

mdharm-scsi@one-eyed-alien.net said:
> In userspace, something should get a new device notification and
> decide what to do with the new device. Format?  Mount?  Ignore?  But,
> it needs a /dev node to be able to handle the device, therefore the
> kernel must do the work up to that point. 

SCSI was designed with this in mind.  The add/remove-single-device API works 
before a dev node exists.  It expects four numeric parameters 
(host,channel,pun and lun), and will do the inquiry and attach the correct 
SCSI upper layer drivers.

Thus I still contend there's nothing you can get out of in-kernel exposure of 
these APIs that you couldn't do from a hotplug event.

I agree that we don't have the correct SCSI hot plugging in place today, but 
exposing in-kernel add/remove won't help with that.

James

Re: [PATCH] SCSI hotplug support

[Oliver Xymoron]

yOn Mon, Oct 14, 2002 at 10:39:34AM -0700, James Bottomley wrote:
> mdharm-scsi@one-eyed-alien.net said:
> > In userspace, something should get a new device notification and
> > decide what to do with the new device. Format?  Mount?  Ignore?  But,
> > it needs a /dev node to be able to handle the device, therefore the
> > kernel must do the work up to that point. 
> 
> SCSI was designed with this in mind.  The add/remove-single-device API works 
> before a dev node exists.  It expects four numeric parameters 
> (host,channel,pun and lun), and will do the inquiry and attach the correct 
> SCSI upper layer drivers.

Which works most of the time, but I'm pretty sure I've seen some ugly
races here when adding, oh, 40-50 devices at a go. Haven't looked at
this recently though.

-- 
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.." 

Re: [PATCH] SCSI hotplug support

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 2219 bytes --]

Hi James,

On Mon, Oct 14, 2002 at 10:39:34AM -0700, James Bottomley wrote:
> mdharm-scsi@one-eyed-alien.net said:
> > In userspace, something should get a new device notification and
> > decide what to do with the new device. Format?  Mount?  Ignore?  But,
> > it needs a /dev node to be able to handle the device, therefore the
> > kernel must do the work up to that point. 
> 
> SCSI was designed with this in mind.

Better not ask Eric Youngdale about this.

Basically the code is in there as you have no other way to trigger a SCSI
device detection, as the classical (parallel) SCSI host adapter drivers
don't scan themselves ...
Otherwise the mechanism would have been the other way round right from
the start: HBA drivers scan and tell upper layers what they found.
Which would have been the cleaner approach.

USB needs to know about attached devices anyway, so let's offer it the
cleaner way.

> The add/remove-single-device API works 
> before a dev node exists.  It expects four numeric parameters 
> (host,channel,pun and lun), and will do the inquiry and attach the correct 
> SCSI upper layer drivers.

I don't think you should take "hotpluging must go via userspace" to the
extreme.

There's nothing to be configured here. Just no decision to take. Because
no information is available on which this decision could be based.

Devices being recognized should show up und /proc/scsi/scsi resp. driverfs. 
No point in wasting a few thousand CPU cycles going through userspace which
can not do anything useful except just sending some ASCII string with the
numbers it got from the kernel back to the kernel to parse them.

Now, devices are available (in /proc/scsi/scsi and/or driverfs) and you 
can send userspace a hotplug event, because now userspace can inquire
information from the kernel and actually do take decisions and do something
useful.

Best regards,
-- 
Kurt Garloff                   <kurt@garloff.de>         [Eindhoven, NL]
Physics: Plasma simulations    <K.Garloff@TUE.NL>     [TU Eindhoven, NL]
Linux: SCSI, Security          <garloff@suse.de>    [SuSE Nuernberg, DE]
 (See mail header or public key servers for PGP2 and GPG public keys.)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

On Mon Oct 14, 2002 at 08:57:21AM -0700, James Bottomley wrote:
> andersen@codepoet.org said:
> > Anyone have any objections to such a patch?  For 2.4.x?  Comments?
> 
> The patch looks fine per se.
> 
> However, I thought that the whole spirit of hotplug was that this type of 
> problem should be solved in user space to which SCSI currently exposes a 
> perfectly adequate interface.

If the user space interface were perfectly adequate, I would not
have written this patch.  User space does not have sufficient
information to know _which_ devices must to be added or removed.
The best we can do from user space is a full rescan of _all_ scsi
host adaptors (http://www.garloff.de/kurt/linux/rescan-scsi-bus.sh) 
using something like
    echo "scsi remove-single-device 0 0 0 0" > /proc/scsi/scsi
    echo "scsi add-single-device 0 0 0 0" > /proc/scsi/scsi
for each and every SCSI device on your system.

That works just fine if all you are doing is (unwisely) powering
up a SCSI tape drive that is already connected to your Adaptec
29160.  If you have a firewire RAID array system (like I do) and
you are hotswapping drive 3 of the RAID array, you really don't
want to disconnect and reconnect each and every device.

> Needing kernel hooks in a different subsystem to solve a hotplug problem looks 
> slightly wrong to me.  Usually when this type of thing comes up its because 
> we're missing a hotplug event (or a hotplug event doesn't carry the right 
> information).  Could you elaborate more on the actual problem you're trying to 
> solve?

A detailed description of the sbp2 problem is here: 
    http://sourceforge.net/mailarchive/forum.php?thread_id=1151203&forum_id=5389

The executive summary:  Without this patch (and another patch I
posted to the linux-1394 mailing list to make sbp2.c to use this
scsi hotplug patch), hotplugging firewire drives will register
the drives with the 1394 layer, but will not register the drives
with the SCSI subsystem.  Unplugging firewire drives leaves the
(now absent) drives still registered with the SCSI subsystem.
This makes _very_ bad things happed to my RAID array when I
hotswap drives.

This patch allows SBP-2 devices to properly hook into and unhook
themselves from the SCSI subsystem when hotplugged.

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Re: [PATCH] SCSI hotplug support

[James Bottomley]

andersen@codepoet.org said:
> If the user space interface were perfectly adequate, I would not have
> written this patch.  User space does not have sufficient information
> to know _which_ devices must to be added or removed. The best we can
> do from user space is a full rescan of _all_ scsi host adaptors (http:/
> /www.garloff.de/kurt/linux/rescan-scsi-bus.sh)  using something like

The API you expose has identical inputs to the user space one.

Therefore it seems to me that your spb2 driver must already know the values to 
fill in to use the API.  So what's wrong with triggering a hotplug event from 
this driver that causes the add/remove single device to be done from user 
level?

James

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

On Mon Oct 14, 2002 at 02:07:18PM -0700, James Bottomley wrote:
> andersen@codepoet.org said:
> > If the user space interface were perfectly adequate, I would not have
> > written this patch.  User space does not have sufficient information
> > to know _which_ devices must to be added or removed. The best we can
> > do from user space is a full rescan of _all_ scsi host adaptors (http:/
> > /www.garloff.de/kurt/linux/rescan-scsi-bus.sh)  using something like
> 
> The API you expose has identical inputs to the user space one.
> 
> Therefore it seems to me that your spb2 driver must already know the values to 
> fill in to use the API.  So what's wrong with triggering a hotplug event from 
> this driver that causes the add/remove single device to be done from user 
> level?

That would be another way to do it.  Right now the SBP-2 hotplug
mechanism works perfectly for loading and unloading all the
needed kernel modules in the correct order when hotplugging.  

To make the ieee1394 hotplug mechanism also properly register and
unregister SBP-2 devices from user space, we will need to also
export the host, channel, id, and lun from nodemgr_call_policy()
in drivers/ieee1394/nodemgr.c.  But this becomes a layering
problem since not all 1394 devices are SBP-2 devices, and
therefore the nodemgr has no concept of channel, id, and lun.
This is further complicated since the parameters exported by the
1394 hotplug subsystem were fixed around 2.4.10.  Changing that
would require coordination with all the distro maintainers (esp
since linux//Documentation/Changes does not list a required
version of the hotplug utilities).

I have a 4 slot firewire RAID system with 4 hotswapable 120 GB
drives in it connected via a single firewire cable.  This is very
small compared to the potential size.  Imagine someone plugs in a
63 device firewire RAID box.  Then nodemgr.c determines if the
device being hotplugged is an SBP-2 device and if so execs
/sbin/hotplug with some extra SBP-2 specific arguments.  Then
/sbin/hotplug writes some junk into /proc/scsi/scsi 63 times, and
so 63 times we context switch back into kernel space to
register/unregister a device and then 63 times we context switch
back into user space so we can continue doing the hotplug thing.

And the point of this exercise again was?  Its not like userspace
is setting policy -- the sbp2.c driver knew exactly what the
Right Thing(tm) was all along.  I think using the function calls
per my patch is a far cleaner solution,

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Re: [PATCH] SCSI hotplug support

[Doug Ledford]

On Mon, Oct 14, 2002 at 03:54:17PM -0600, Erik Andersen wrote:
> 
> I have a 4 slot firewire RAID system with 4 hotswapable 120 GB
> drives in it connected via a single firewire cable.  This is very
> small compared to the potential size.  Imagine someone plugs in a
> 63 device firewire RAID box.  Then nodemgr.c determines if the
> device being hotplugged is an SBP-2 device and if so execs
> /sbin/hotplug with some extra SBP-2 specific arguments.  Then
> /sbin/hotplug writes some junk into /proc/scsi/scsi 63 times, and
> so 63 times we context switch back into kernel space to
> register/unregister a device and then 63 times we context switch
> back into user space so we can continue doing the hotplug thing.
> 
> And the point of this exercise again was?  Its not like userspace
> is setting policy -- the sbp2.c driver knew exactly what the
> Right Thing(tm) was all along.  I think using the function calls
> per my patch is a far cleaner solution,

First, 63 context switches is a red herring.  The total amount of work
required to attach a scsi disk is high enough that a context switch per
disk is totally lost noise.  Second, user space *does* set policy.  The
hot plug manager can end up doing things like changing the ownership of
the newly added device to that of the current console owner and then
automatically mounting it on /mnt/camera_drive (just an example).  IOW,
user space hot plug *needs* to be involved whether you make the kernel hot
plug work or not.  Secondly, once you accept that user space hot plug
*needs* to be involved, then letting it take care of the hot plugging for
you instead of writing a kernel patch makes more sense.

Also, it's likely that calling in from the hot plug manager is *much*
better from the kernel perspective for one very important reason, user
context.  The add_single_device operation will not be done from an
interrupt/tasklet/softirq/etc. context if the hot plug manager does it,
making sleeping, memory allocation, and such easier to get right.  What
context does the 1394 code call the scsi code from in your patch?

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

On Mon Oct 14, 2002 at 06:25:15PM -0400, Doug Ledford wrote:
> On Mon, Oct 14, 2002 at 03:54:17PM -0600, Erik Andersen wrote:
> > 
> > I have a 4 slot firewire RAID system with 4 hotswapable 120 GB
> > drives in it connected via a single firewire cable.  This is very
> > small compared to the potential size.  Imagine someone plugs in a
> > 63 device firewire RAID box.  Then nodemgr.c determines if the
> > device being hotplugged is an SBP-2 device and if so execs
> > /sbin/hotplug with some extra SBP-2 specific arguments.  Then
> > /sbin/hotplug writes some junk into /proc/scsi/scsi 63 times, and
> > so 63 times we context switch back into kernel space to
> > register/unregister a device and then 63 times we context switch
> > back into user space so we can continue doing the hotplug thing.
> > 
> > And the point of this exercise again was?  Its not like userspace
> > is setting policy -- the sbp2.c driver knew exactly what the
> > Right Thing(tm) was all along.  I think using the function calls
> > per my patch is a far cleaner solution,
> 
> First, 63 context switches is a red herring.  The total amount of work
> required to attach a scsi disk is high enough that a context switch per
> disk is totally lost noise.

Ok, I'll agree that the overhead is small compared to the rest of
the process.  Though I contend that it is still needless additional 
overhead.

>                              Second, user space *does* set policy.  The
> hot plug manager can end up doing things like changing the ownership of
> the newly added device to that of the current console owner and then
> automatically mounting it on /mnt/camera_drive (just an example).  IOW,

I suppose hotplug _could_ do these things.  It currently doesn't
do anything of the sort.  If someone wishes to modify
/etc/hotplug/ieee1394.agent to do these things, thats fine.  It
sounds to me like you are wishing for /sbin/hotplug to take on
some sortof devfsd type role.  If that is what you want to build,
thats up to you I suppose.  I really don't need or want that.  So
sure, I'll also agree with you that user space does set policy.

I personally perfer a policy of minimalism.  I'd prefer the
kernel continue to allocate SCSI drives (/dev/sdXXX) on a
first-come-first-served basis as it currently does and then use
things like UUIDs to keep mount points consistant.  That seems
far more palatable then to having /sbin/hotplug mounting things
and mucking with perms.

> user space hot plug *needs* to be involved whether you make the kernel hot
> plug work or not.  Secondly, once you accept that user space hot plug
> *needs* to be involved, then letting it take care of the hot plugging for
> you instead of writing a kernel patch makes more sense.

I'll grant that having /sbin/hotplug load needed kernel modules
is a good thing.  With some hacking I suppose it could muck about
with your device nodes if you are into that sortof thing.  But I
still have not conceeded that we need /sbin/hotplug involved in
registering and unregistering devices with the SCSI subsystem.

When I plug a Adaptec 1480 Cardbus SCSI adaptor into my laptop, I
don't need /sbin/hotplug to register the drives.  They just
magically get registered.  Similarly, if I have 3 SBP-2 drives
plugged into an Adaptec AFW-1430 card when I plug in the card,
the 3 SBP-2 drives are magically registered with the SCSI
subsystem.  So if we require /sbin/hotplug to be involved, we
will need to unify the coldplug and hotplug cases to ensure
identical behavior....

> Also, it's likely that calling in from the hot plug manager is *much*
> better from the kernel perspective for one very important reason, user
> context.  The add_single_device operation will not be done from an
> interrupt/tasklet/softirq/etc. context if the hot plug manager does it,
> making sleeping, memory allocation, and such easier to get right.  What
> context does the 1394 code call the scsi code from in your patch?

It is called from within the insmod's context.

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Re: [PATCH] SCSI hotplug support

[Doug Ledford]

On Mon, Oct 14, 2002 at 11:25:22PM -0600, Erik Andersen wrote:
> On Mon Oct 14, 2002 at 06:25:15PM -0400, Doug Ledford wrote:
> >                              Second, user space *does* set policy.  The
> > hot plug manager can end up doing things like changing the ownership of
> > the newly added device to that of the current console owner and then
> > automatically mounting it on /mnt/camera_drive (just an example).  IOW,
> 
> I suppose hotplug _could_ do these things.  It currently doesn't
> do anything of the sort.  If someone wishes to modify
> /etc/hotplug/ieee1394.agent to do these things, thats fine.  It
> sounds to me like you are wishing for /sbin/hotplug to take on
> some sortof devfsd type role.  If that is what you want to build,
> thats up to you I suppose.  I really don't need or want that.  So
> sure, I'll also agree with you that user space does set policy.

Well, currently we ship this capability in our products (although not for
ieee1394).  It's a combination of the pcmcia card manager daemon, PAM
modules, and the system security profile that make this happen (and our
hotplug program currently only does USB, but I seem to recall murmurs
about unifying it to do USB, ieee1394, and pcmcia so we could have one
central place to configure all these things).  The net result of all this
though is that when my wife inserts the 64MB Compact Flash card into her
laptop, it *is* recognized by the cardmgr, it is mounted on /mnt/camera,
it's ownership is changed to her uid.gid, and it's left for her to umount
(by right clicking on the drive icon on her desktop and then selecting
unmount volume) and eject when she feels like it.  You may not want this,
but a *large* percentage of the rest of the world *does* want this sort of
convenience.  Now, my point, again, is that if we are going to do this
stuff in user space, then the user space code can do the attach as well.  
No real need to split it up.

> I personally perfer a policy of minimalism.  I'd prefer the
> kernel continue to allocate SCSI drives (/dev/sdXXX) on a
> first-come-first-served basis as it currently does and then use
> things like UUIDs to keep mount points consistant.  That seems
> far more palatable then to having /sbin/hotplug mounting things
> and mucking with perms.

Who says /sbin/hotplug wouldn't use the UUID and volume label to mount it
properly?  You aren't even arguing the point, instead you're making stuff
up!  Who ever said that the recognition would be different, or that it
would not be on a first-come-first-served basis?  These are more red
herrings.  The *only* difference I'm talking about is where the device add
trigger gets triggered at.  Either the driver calls an internal scsi
function to trigger the device attachment, or the driver notifies hotplug
that the device is there and the hotplug manager (after suitable
modification to the hot plug driver to support this) calls into the scsi
function to trigger an attach.  Either way, it's the same scsi function
and it does the same thing.  The only difference then becomes which
context the scsi function is called from.

As someone who works on the scsi mid layer, I would prefer that all of the
entries into the disc attach code come from one place.  It's easier on me
if one program always calls the attach function instead of one program for
SES enclosures on SCSI parallel busses (a requirement because the scsi
busses can't detect a drive addition, only an ses knowledgable program
occasionally polling the ses enclosure would know about it), scsi drivers
for fiber channel fabrics (which the would get notification of during an
interrupt context, so they would have to find some means of getting to a
non-interrupt context and then calling the entry point or else the entry
point must be interrupt context safe), and scsi bridge drivers for
ieee1394 and usb busses (context is dependant on the method by which the
attach is done).  If we go your way, I have to worry about the independant
drivers and making sure they all call me from the proper context, etc.  
If I only have to worry about the hotplug manager, it's easier to make 
sure everything is right.

> > user space hot plug *needs* to be involved whether you make the kernel hot
> > plug work or not.  Secondly, once you accept that user space hot plug
> > *needs* to be involved, then letting it take care of the hot plugging for
> > you instead of writing a kernel patch makes more sense.
> 
> I'll grant that having /sbin/hotplug load needed kernel modules
> is a good thing.  With some hacking I suppose it could muck about
> with your device nodes if you are into that sortof thing.  But I
> still have not conceeded that we need /sbin/hotplug involved in
> registering and unregistering devices with the SCSI subsystem.
> 
> When I plug a Adaptec 1480 Cardbus SCSI adaptor into my laptop, I
> don't need /sbin/hotplug to register the drives.  They just
> magically get registered.  Similarly, if I have 3 SBP-2 drives
> plugged into an Adaptec AFW-1430 card when I plug in the card,
> the 3 SBP-2 drives are magically registered with the SCSI
> subsystem.  So if we require /sbin/hotplug to be involved, we
> will need to unify the coldplug and hotplug cases to ensure
> identical behavior....

More red herrings (who ever said the code paths for hotplug and coldplug
would ever be different?).  Can you please quit making up bogus arguments
and stick to the argument at hand?  Besides, I don't know what system you
are running, but on every Red Hat system shipped to date, the drives don't
*magically* get added when you plug in the card, they get added by the
*user space* cardmgr daemon (because it triggers the kernel actions
needed, just like I've been arguing for).

If it's that important to you that the driver call in to attach the device
instead of a user space funtion, then so be it.  But, it will *NOT* be
done the way your patch does it.  There are more things out there than
just usb and ieee1394 that want to do hot plug drives.  Fiber channel in
particular is one where if I don't provide an interrupt context safe
method of triggering this action, then the driver has to queue up the
action, set a timer, wait for the timer to wake it up, then unqueue and
finish the action from the timer.  I flat refuse to add a disc attach
method that won't work for all the different classes of drivers that want
this functionality.
  
Since disc attachment can take a long time, especially if a disc needs
spun up, it *must* be done from a process or similar context.  If it isn't
user space, then it needs to be a kernel thread.  Since not all of the
sources for calls into this function have that context easily at hand, one
needs to be created.  Call it the scsi-manager thread, fire it up when you
start up the scsi subsystem, then allow drivers and other things to queue
up actions that the manager thread can complete at its leisure.  That's
the only way I want an internal kernel call for adding discs that drivers
can call into.

Of course, doing it this way now presents a new problem for the hot plug 
manager.  Since the action is queue up but the trigger call returns 
immediately, you now have to program the hot plug manager to wait for the 
queued action to complete if you want it to be able to do anything else 
with the drive...

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

On Tue Oct 15, 2002 at 11:33:36AM -0400, Doug Ledford wrote:
> > When I plug a Adaptec 1480 Cardbus SCSI adaptor into my laptop, I
> > don't need /sbin/hotplug to register the drives.  They just
> > magically get registered.  Similarly, if I have 3 SBP-2 drives
> > plugged into an Adaptec AFW-1430 card when I plug in the card,
> > the 3 SBP-2 drives are magically registered with the SCSI
> > subsystem.  So if we require /sbin/hotplug to be involved, we
> > will need to unify the coldplug and hotplug cases to ensure
> > identical behavior....
> 
> More red herrings (who ever said the code paths for hotplug and coldplug
> would ever be different?).  Can you please quit making up bogus arguments
> and stick to the argument at hand?  Besides, I don't know what system you

But they ARE DIFFERENT!  I'm not making this up.  Try looking at
the code for a second, because I am bringing up a very real
issue.  Hotplug and coldplug are fundamentally different types 
of events, and handwaving does not change that.  I'll grant that
perhaps this difference may be unique to 1394.  I dunno.

In the hotplug case, SBP-2 devices are currently not registered
with SCSI at all. My patch allows them to be registered, as would
your proposal to let /sbin/hotplug do the heavy lifting.  No
problem here as either solution can work and will have the
expected results.

As the kernel is currently shipped, in the coldplug case (i.e.
devices are connected to the host adaptor at module load time)
devices get registered with the SCSI subsystem when the host
adaptor is registered (via a call to scsi_register_host() or
scsi_register_module() depending on the kernel version).  In this
case there is no call to /sbin/hotplug since this is not a
hotplug event.  

So I was poinging out that if you want to expand the role of
/sbin/hotplug, you will also need to modify the coldplug case to
ensure that /sbin/hotplug is involved.

> are running, but on every Red Hat system shipped to date, the drives don't
> *magically* get added when you plug in the card, they get added by the
> *user space* cardmgr daemon (because it triggers the kernel actions
> needed, just like I've been arguing for).

I use Debian on my devel laptop and I also build my own systems
by hand, but it doesn't really matter.  I have the cardmgr daemon
handle just the 16bit pcmcia cards (i.e.  Adaptec 1460).  I have
the kernel hotplug subsystem (with the /sbin/hotplug user space
helper) handle the cardbus cards since pcmcia-cs support for the
cardbus devices I use, is pretty lousy.

> If it's that important to you that the driver call in to attach the device
> instead of a user space funtion, then so be it.

So we agree that kernel space is allowed to attach devices to the
SCSI subsystem.  Cool. 

>                                                  But, it will *NOT* be
> done the way your patch does it.

Ok.  So tell what you consider an allowable way to do it.  This
is the part I've been waiting to hear.

>                                   There are more things out there than
> just usb and ieee1394 that want to do hot plug drives.  Fiber channel in

My patch is not at all usb or ieee1394 specific.

> particular is one where if I don't provide an interrupt context safe
> method of triggering this action, then the driver has to queue up the
> action, set a timer, wait for the timer to wake it up, then unqueue and
> finish the action from the timer.  I flat refuse to add a disc attach
> method that won't work for all the different classes of drivers that want
> this functionality.

Ok.

> Since disc attachment can take a long time, especially if a disc needs
> spun up, it *must* be done from a process or similar context.  If it isn't
> user space, then it needs to be a kernel thread.  Since not all of the
> sources for calls into this function have that context easily at hand, one
> needs to be created.  Call it the scsi-manager thread, fire it up when you
> start up the scsi subsystem, then allow drivers and other things to queue
> up actions that the manager thread can complete at its leisure.  That's
> the only way I want an internal kernel call for adding discs that drivers
> can call into.

Ok.  That sounds workable.

> Of course, doing it this way now presents a new problem for the hot plug 
> manager.  Since the action is queue up but the trigger call returns 
> immediately, you now have to program the hot plug manager to wait for the 
> queued action to complete if you want it to be able to do anything else 
> with the drive...

I think we need four entry points then into the scsi-manager
thread.  How about the following?

    extern int scsi_add_single_device(struct Scsi_Host *shpnt, 
	   int channel, int id, int lun);
    extern int scsi_remove_single_device(struct Scsi_Host *shpnt, 
	   int channel, int id, int lun);

    extern int blocking_scsi_add_single_device(struct Scsi_Host *shpnt, 
	   int channel, int id, int lun);
    extern int blocking_scsi_remove_single_device(struct Scsi_Host *shpnt, 
	   int channel, int id, int lun);

in this way, Fiber channel devices calling from interrupt context
and similar can call the non-blocking versions.  The blocking
versions will simply put the caller to sleep till the
registration is completed.  Agreeable?

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Re: [PATCH] SCSI hotplug support

[Doug Ledford]

On Mon, Oct 14, 2002 at 11:25:22PM -0600, Erik Andersen wrote:
> 
> It is called from within the insmod's context.

So it's called by what module then?  And what happens if we have a user 
add a hard drive and it gets the module loaded and then attaches the 
drive, then we attach another drive sometime later, and this time hotplug 
doesn't do anything because the right module is already loaded?  How does 
the drive get added then?  Am I missing something?

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

On Tue Oct 15, 2002 at 02:22:47PM -0400, Doug Ledford wrote:
> On Mon, Oct 14, 2002 at 11:25:22PM -0600, Erik Andersen wrote:
> > 
> > It is called from within the insmod's context.
> 
> So it's called by what module then?  And what happens if we have a user 
> add a hard drive and it gets the module loaded and then attaches the 
> drive, then we attach another drive sometime later, and this time hotplug 
> doesn't do anything because the right module is already loaded?  How does 
> the drive get added then?  Am I missing something?

Oops.  I spoke wrongly....  It is called from within
/sbin/hotplug's context.

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Re: [PATCH] SCSI hotplug support

[Doug Ledford]

On Tue, Oct 15, 2002 at 12:45:35PM -0600, Erik Andersen wrote:
> On Tue Oct 15, 2002 at 02:22:47PM -0400, Doug Ledford wrote:
> > On Mon, Oct 14, 2002 at 11:25:22PM -0600, Erik Andersen wrote:
> > > 
> > > It is called from within the insmod's context.
> > 
> > So it's called by what module then?  And what happens if we have a user 
> > add a hard drive and it gets the module loaded and then attaches the 
> > drive, then we attach another drive sometime later, and this time hotplug 
> > doesn't do anything because the right module is already loaded?  How does 
> > the drive get added then?  Am I missing something?
> 
> Oops.  I spoke wrongly....  It is called from within
> /sbin/hotplug's context.

Then what the sam hell has all this crap been for!?!?  We've been saying 
all along "Add the device from hotplug instead of kernel module", and now 
you tell us that it's already happening from hotplug's context?  If it's 
already happening from hotplug's context, then why not just do the 
fprintf(); that I put in one of my emails and be done with it?  And what 
about the scenario I asked about in my last email regarding a second disc 
and the situation where no module needs loaded?  How is that handled?

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

On Tue Oct 15, 2002 at 03:13:30PM -0400, Doug Ledford wrote:
> On Tue, Oct 15, 2002 at 12:45:35PM -0600, Erik Andersen wrote:
> > On Tue Oct 15, 2002 at 02:22:47PM -0400, Doug Ledford wrote:
> > > On Mon, Oct 14, 2002 at 11:25:22PM -0600, Erik Andersen wrote:
> > > > 
> > > > It is called from within the insmod's context.
> > > 
> > > So it's called by what module then?  And what happens if we have a user 
> > > add a hard drive and it gets the module loaded and then attaches the 
> > > drive, then we attach another drive sometime later, and this time hotplug 
> > > doesn't do anything because the right module is already loaded?  How does 
> > > the drive get added then?  Am I missing something?
> > 
> > Oops.  I spoke wrongly....  It is called from within
> > /sbin/hotplug's context.
> 
> Then what the sam hell has all this crap been for!?!?  We've been saying 
> all along "Add the device from hotplug instead of kernel module", and now 
> you tell us that it's already happening from hotplug's context?  If it's 
> already happening from hotplug's context, then why not just do the 
> fprintf(); that I put in one of my emails and be done with it?  And what 

The main reason why I'd prefer to put this stuff into the sbp2
driver is to avoid 1394 layering violations. For ieee1394
devices, the 1394 nodemgr makes the hotplug calls.  But it does
the hotplug thing for all 1394 devices, and has no knowledge of
whether a device is an SBP-2 device (one that need to be
connected to the SCSI subsystem).  I suspect the 1394 maintainers
would reject patches that infect the nodemgr with SBP-2 and/or
SCSI specific stuff.  Sigh.  We seem to be at an impasse.  We
either screw up SCSI or we screw up 1394... 

> about the scenario I asked about in my last email regarding a second disc 
> and the situation where no module needs loaded?  How is that handled?

Same deal.  The 1394 nodemgr invokes /sbin/hotplug.

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Re: [PATCH] SCSI hotplug support

[James Bottomley]

andersen@codepoet.org said:
> I suspect the 1394 maintainers would reject patches that infect the
> nodemgr with SBP-2 and/or SCSI specific stuff.  Sigh.  We seem to be
> at an impasse.  We either screw up SCSI or we screw up 1394... 

Not necessarily.  We're back around to my original though which was that the 
hotplug event isn't carrying enough information.  Since 1394 can have may 
different types of device attaching, it stands to reason that some of them 
will need to pass extra information through the hotplug event.

Driverfs can be used for this in 2.5, but in 2.4 I suspect you need to enhance the 1394 nodemgr to allow customisable information.  Note, this doesn't require nodemgr to know about sbp-2, but it does require nodemgr to know "this device wants to add this information to the hotplug event", which does sound generic enough to be acceptable without layering violations.

James

Re: [PATCH] SCSI hotplug support

[Scott Merritt]

On Tue, 15 Oct 2002 13:32:16 -0600
Erik Andersen <andersen@codepoet.org> wrote:

> The main reason why I'd prefer to put this stuff into the sbp2
> driver is to avoid 1394 layering violations.

I don't know much about this at all, but could the sbp2 driver "trigger" a second scsi-hotplug event ?  Hope I'm not make a truly silly suggestion ...

Regards, Scott.

Re: [PATCH] SCSI hotplug support

[Doug Ledford]

On Tue, Oct 15, 2002 at 01:32:16PM -0600, Erik Andersen wrote:
> > about the scenario I asked about in my last email regarding a second disc 
> > and the situation where no module needs loaded?  How is that handled?
> 
> Same deal.  The 1394 nodemgr invokes /sbin/hotplug.

That doesn't answer the question.  Your patch provided a kernel interface
to be called by other kernel code (during module init time if I'm
correct).  As such, hotplug itself is *not* attaching the disk, it is
letting the module init take care of it.  If /sbin/hotplug isn't attaching
the disk, but instead it is being done by the kernel driver during insmod
(using the facilities provided by your patch), THEN HOW DOES IT HAPPEN
WHEN THE MODULE IS ALREADY LOADED?  Are you loading *another* copy of the
same module to handle the additional disk?  Do you have a facility by
which the hotplug manager can tell an existing module about a new disk?  
Or does it just not get added?  Or does the 1394 node manager notice that
the module for the new disk is already loaded and tell the module about
the new disk without even invoking hotplug?  Please answer this for me.

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

On Tue Oct 15, 2002 at 03:55:16PM -0400, Doug Ledford wrote:
> On Tue, Oct 15, 2002 at 01:32:16PM -0600, Erik Andersen wrote:
> > > about the scenario I asked about in my last email regarding a second disc 
> > > and the situation where no module needs loaded?  How is that handled?
> > 
> > Same deal.  The 1394 nodemgr invokes /sbin/hotplug.
> 
> That doesn't answer the question.  Your patch provided a kernel interface
> to be called by other kernel code (during module init time if I'm
> correct).  As such, hotplug itself is *not* attaching the disk, it is
> letting the module init take care of it.  If /sbin/hotplug isn't attaching
> the disk, but instead it is being done by the kernel driver during insmod
> (using the facilities provided by your patch), THEN HOW DOES IT HAPPEN
> WHEN THE MODULE IS ALREADY LOADED?  Are you loading *another* copy of the
> same module to handle the additional disk?  Do you have a facility by
> which the hotplug manager can tell an existing module about a new disk?  
> Or does it just not get added?  Or does the 1394 node manager notice that
> the module for the new disk is already loaded and tell the module about
> the new disk without even invoking hotplug?  Please answer this for me.

I added this 
    printk("ieee1394 nodmgr: invoking hotplug in the context "
	    "of '%s' (pid %d)\n", current->comm, current->pid);
to nodemgr_call_policy() in drivers/ieee1394/nodemgr.c.  I stuck
a similar printk into the sbp2 driver for when I was hooking and
unhooking devices into the SCSI subsystem.

When loading the ohci1394 module I see
    ieee1394 nodmgr: invoking hotplug in the context of 'knodemgrd' (pid 24179)

With my 1394 RAID array cold-plugged into my cardbus 1394 card,
the 4 drives are registered with the SCSI subsystem.  Looks like
I forgot to test my sbp2 patch with cold-plugging -- with my
patch in place the 4 drives end up being registered _twice_ with
the SCSI subsystem.  It got one copy of the devices from calling
scsi_register_host (I'm testing with 2.4.x), and then another
copy from me trying to hotplug the devices.  oops.  I'll sort
that bit out.  Anyways, when doing cold-plugging the sbp2 devices
are registered with the SCSI subsystem in the context of
'modprobe' (which was busy loading the sbp2 module at the time
things were registered with the SCSI subsystem).  So in the
coldplug case, /sbin/hotplug was not in any way involved.

Then when I hotplugged and hotunplugged the 1394 devices, hotplug
was again called in the context of 'knodemgrd'.  So I was wrong
about the context from which things were called.  Sorry about
that...

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Re: [PATCH] SCSI hotplug support

[Doug Ledford]

On Tue, Oct 15, 2002 at 04:07:00PM -0600, Erik Andersen wrote:
> I added this 
>     printk("ieee1394 nodmgr: invoking hotplug in the context "
> 	    "of '%s' (pid %d)\n", current->comm, current->pid);
> to nodemgr_call_policy() in drivers/ieee1394/nodemgr.c.  I stuck
> a similar printk into the sbp2 driver for when I was hooking and
> unhooking devices into the SCSI subsystem.
> 
> When loading the ohci1394 module I see
>     ieee1394 nodmgr: invoking hotplug in the context of 'knodemgrd' (pid 24179)

So nodmgr is a kernel thread named knodemgrd that then execs hotplug which 
then execs insmod, aka starting context is that of a kernel thread.  
That's safe for sleeping.  That's what I wanted to know.

> With my 1394 RAID array cold-plugged into my cardbus 1394 card,
> the 4 drives are registered with the SCSI subsystem.  Looks like
> I forgot to test my sbp2 patch with cold-plugging -- with my
> patch in place the 4 drives end up being registered _twice_ with
> the SCSI subsystem.  It got one copy of the devices from calling
> scsi_register_host (I'm testing with 2.4.x), and then another
> copy from me trying to hotplug the devices.  oops.  I'll sort
> that bit out.  Anyways, when doing cold-plugging the sbp2 devices
> are registered with the SCSI subsystem in the context of
> 'modprobe' (which was busy loading the sbp2 module at the time
> things were registered with the SCSI subsystem).  So in the
> coldplug case, /sbin/hotplug was not in any way involved.
> 
> Then when I hotplugged and hotunplugged the 1394 devices, hotplug
> was again called in the context of 'knodemgrd'.  So I was wrong
> about the context from which things were called.  Sorry about
> that...

The context name is only half of it.  Whether or not it's in interrupt or 
tasklet context is more important (and this doesn't change the name, being 
in knodemgrd context has current->comm == knodemgrd whether you are in the 
middle of an interrupt or not).  In this case though, it's pretty easy to 
tell it isn't interrupt context.  That's fine.  However, you still haven't 
tested the scenario I mentioned at the start of this particular part of 
the debate.  If you plug in one drive, then plug in another, does the 
second one get added?  Possible way to test this without having separate 
drives: load the sbp2 module *before* hotplugging your disks and see what 
happens.

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

On Tue Oct 15, 2002 at 10:40:16PM -0400, Doug Ledford wrote:
> middle of an interrupt or not).  In this case though, it's pretty easy to 
> tell it isn't interrupt context.  That's fine.  However, you still haven't 
> tested the scenario I mentioned at the start of this particular part of 
> the debate.  If you plug in one drive, then plug in another, does the 
> second one get added?  Possible way to test this without having separate 
> drives: load the sbp2 module *before* hotplugging your disks and see what 
> happens.

I have several firewire drives so testing this is not a
problem...  The answer is yes it is added.  With my latest patch
applied to the kernel, I had 4 drives plugged into my 1394 card
(my 1394 RAID box) when I loaded the sbp2 module.  All 4
coldplugged drives showed up in /proc/scsi/scsi.  I then plugged
in another drive, at which point all 5 drives showed up under
/proc/scsi/scsi as expected.

I made an update of my patch.  I have not attempted to convert
the 1394 hotplug driver to echo stuff into /proc/scsi/scsi.
However, this patch attempts to close the longstanding race
conditions inherent in add-single-device and remove-single-device
by using the BKL, and adds some better function docs.  I didn't
see any other locks in the scsi code that looked sufficient to
handle the races, so please correct me if there is a better
way...  This also fixes a stupid bug in the sbp2 part of my
previous patch that would cause coldplugged sbp2 devices to be
also hotplugged, causing them to be double registered with the
SCSI subsystem.

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--


--- linux-2.4.19/drivers/scsi/scsi_syms.c	2002-08-02 19:39:44.000000000 -0500
+++ linux-2.4.20-pre10-erik/drivers/scsi/scsi_syms.c	2002-10-09 16:41:01.000000000 -0500
@@ -103,3 +103,9 @@
 extern int scsi_delete_timer(Scsi_Cmnd *);
 EXPORT_SYMBOL(scsi_add_timer);
 EXPORT_SYMBOL(scsi_delete_timer);
+
+/* Support for hot plugging and unplugging devices -- safe for 
+ * ieee1394 or USB devices, but probably not for normal SCSI... */
+EXPORT_SYMBOL(scsi_add_single_device);
+EXPORT_SYMBOL(scsi_remove_single_device);
+
--- linux-2.4.19/drivers/scsi/hosts.h	2002-10-18 05:16:17.000000000 -0500
+++ linux-2.4.20-pre10-erik/drivers/scsi/hosts.h	2002-10-09 17:00:13.000000000 -0500
@@ -532,6 +532,13 @@
 int scsi_register_device(struct Scsi_Device_Template * sdpnt);
 void scsi_deregister_device(struct Scsi_Device_Template * tpnt);
 
+/* Support for hot plugging and unplugging devices -- safe for 
+ * ieee1394 or USB devices, but probably not for normal SCSI... */
+extern int scsi_add_single_device(struct Scsi_Host *shpnt, 
+	int channel, int id, int lun);
+extern int scsi_remove_single_device(struct Scsi_Host *shpnt, 
+	int channel, int id, int lun);
+
 /* These are used by loadable modules */
 extern int scsi_register_module(int, void *);
 extern int scsi_unregister_module(int, void *);
--- linux-2.4.19/drivers/scsi/scsi.c	2002-10-18 05:16:17.000000000 -0500
+++ linux-2.4.20-pre10-erik/drivers/scsi/scsi.c	2002-10-17 21:12:48.000000000 -0500
@@ -1553,6 +1553,165 @@
     }
 }
 
+
+/*
+ * Function:    scsi_add_single_device()
+ *
+ * Purpose:     Support for hotplugging SCSI devices.  This function
+ *              implements the actual functionality for
+ *                  echo "scsi add-single-device 0 1 2 3" >/proc/scsi/scsi
+ *
+ * Arguments:   shpnt   - pointer to the SCSI host structure
+ *              channel - channel of the device to add
+ *              id      - id of the device to add
+ *              lun     - lun of the device to add
+ *
+ * Returns:     0 on success or an error code
+ *
+ * Lock status: None needed.  However we do grab the BKL to avoid racing with
+ *              scsi_register*(), scsi_unregister*(), scsi_remove_single_device()
+ *              and anything else that might be messing with with the Scsi_Host or
+ *              other fundamental data structures.
+ *
+ * Notes:       This feature is probably unsafe for standard SCSI devices, 
+ *              but is perfectly normal for things like ieee1394 or USB 
+ *              drives since these busses are designed for hotplugging.  
+ *              Use at your own risk....
+ */
+int scsi_add_single_device(struct Scsi_Host *shpnt, int channel, 
+	int id, int lun)
+{
+	Scsi_Device *scd;
+
+	/* Do a bit of sanity checking */
+	if (shpnt==NULL) {
+		return  -ENXIO;
+	}
+
+	/* get the big kernel lock to avoid racing with
+	 * scsi_register*(), scsi_unregister*(), scsi_remove_single_device() 
+	 * or anything else that might be messing with with the Scsi_Host or
+	 * other fundamental data structures.  */
+	lock_kernel();
+
+	/* Check if they asked us to add an already existing device.
+	 * If so, ignore their misguided efforts. */
+	for (scd = shpnt->host_queue; scd; scd = scd->next) {
+		if ((scd->channel == channel && scd->id == id && scd->lun == lun)) {
+			break;
+		}
+	}
+	if (scd) {
+		unlock_kernel();
+		return -ENOSYS;
+	}
+
+	/* We have the BKL, so we should be safe while we muck 
+	 * about with Scsi_Device internals. */
+	scan_scsis(shpnt, 1, channel, id, lun);
+	unlock_kernel();
+	return 0;
+}
+
+/* Support for plugging/unplugging SCSI devices.  This feature is
+ * probably unsafe for standard SCSI devices, but is perfectly
+ * normal for things like ieee1394 or USB drives since these
+ * busses are designed for hotplugging.  Use at your own risk.... */
+/*
+ * Function:    scsi_remove_single_device()
+ *
+ * Purpose:     Support for hot-unplugging SCSI devices.  This function
+ *              implements the actual functionality for
+ *                  echo "scsi remove-single-device 0 1 2 3" >/proc/scsi/scsi
+ *
+ * Arguments:   shpnt   - pointer to the SCSI host structure
+ *              channel - channel of the device to add
+ *              id      - id of the device to add
+ *              lun     - lun of the device to add
+ *
+ * Returns:     0 on success or an error code
+ *
+ * Lock status: None needed.  However we do grab the BKL to avoid racing with
+ *              scsi_register*(), scsi_unregister*(), scsi_add_single_device()
+ *              and anything else that might be messing with with the Scsi_Host or
+ *              other fundamental data structures.
+ *
+ * Notes:       This feature is probably unsafe for standard SCSI devices, 
+ *              but is perfectly normal for things like ieee1394 or USB 
+ *              drives since these busses are designed for hotplugging.  
+ *              Use at your own risk....
+ */
+int scsi_remove_single_device(struct Scsi_Host *shpnt, int channel, 
+	int id, int lun)
+{
+	Scsi_Device *scd;
+	struct Scsi_Device_Template *SDTpnt;
+
+	/* Do a bit of sanity checking */
+	if (shpnt==NULL) {
+		return  -ENODEV;
+	}
+
+	/* get the big kernel lock to avoid racing with
+	 * scsi_register*(), scsi_unregister*(), scsi_add_single_device() 
+	 * or anything else that might be messing with with the Scsi_Host or
+	 * other fundamental data structures.  */
+	lock_kernel();
+
+	/* Make sure the specified device is in fact present */
+	for (scd = shpnt->host_queue; scd; scd = scd->next) {
+		if ((scd->channel == channel && scd->id == id && scd->lun == lun)) {
+			break;
+		}
+	}
+	if (scd==NULL) {
+		unlock_kernel();
+		return -ENODEV;
+	}
+
+	/* See if the specified device is busy.  We have the BKL, so we
+	 * should not race with sd_open(), sd_release() and similar that 
+	 * increment access_count */
+	if (scd->access_count) {
+		unlock_kernel();
+		return -EBUSY;
+	}
+
+	SDTpnt = scsi_devicelist;
+	while (SDTpnt != NULL) {
+		if (SDTpnt->detach)
+			(*SDTpnt->detach) (scd);
+		SDTpnt = SDTpnt->next;
+	}
+
+	/* We have the BKL, so we should be safe while we muck 
+	 * about with Scsi_Device internals. */
+	if (scd->attached == 0) {
+		/* Nobody is using this device, so we
+		 * can now free all command structures.  */
+		if (shpnt->hostt->revoke)
+			shpnt->hostt->revoke(scd);
+		devfs_unregister (scd->de);
+		scsi_release_commandblocks(scd);
+
+		/* Now we can remove the device structure */
+		if (scd->next != NULL)
+			scd->next->prev = scd->prev;
+
+		if (scd->prev != NULL)
+			scd->prev->next = scd->next;
+
+		if (shpnt->host_queue == scd) {
+			shpnt->host_queue = scd->next;
+		}
+		blk_cleanup_queue(&scd->request_queue);
+		kfree((char *) scd);
+	}
+
+	unlock_kernel();
+	return 0;
+}
+
 #ifdef CONFIG_PROC_FS
 static int scsi_proc_info(char *buffer, char **start, off_t offset, int length)
 {
@@ -1605,8 +1764,6 @@
 static int proc_scsi_gen_write(struct file * file, const char * buf,
                               unsigned long length, void *data)
 {
-	struct Scsi_Device_Template *SDTpnt;
-	Scsi_Device *scd;
 	struct Scsi_Host *HBA_ptr;
 	char *p;
 	int host, channel, id, lun;
@@ -1721,13 +1878,12 @@
 	/*
 	 * Usage: echo "scsi add-single-device 0 1 2 3" >/proc/scsi/scsi
 	 * with  "0 1 2 3" replaced by your "Host Channel Id Lun".
-	 * Consider this feature BETA.
+	 *
+	 * Consider this feature pre-BETA.
+	 *
 	 *     CAUTION: This is not for hotplugging your peripherals. As
 	 *     SCSI was not designed for this you could damage your
-	 *     hardware !
-	 * However perhaps it is legal to switch on an
-	 * already connected device. It is perhaps not
-	 * guaranteed this device doesn't corrupt an ongoing data transfer.
+	 *     hardware and thoroughly confuse the SCSI subsystem.
 	 */
 	if (!strncmp("add-single-device", buffer + 5, 17)) {
 		p = buffer + 23;
@@ -1745,33 +1901,11 @@
 				break;
 			}
 		}
-		err = -ENXIO;
-		if (!HBA_ptr)
-			goto out;
-
-		for (scd = HBA_ptr->host_queue; scd; scd = scd->next) {
-			if ((scd->channel == channel
-			     && scd->id == id
-			     && scd->lun == lun)) {
-				break;
-			}
-		}
-
-		err = -ENOSYS;
-		if (scd)
-			goto out;	/* We do not yet support unplugging */
-
-		scan_scsis(HBA_ptr, 1, channel, id, lun);
-
-		/* FIXME (DB) This assumes that the queue_depth routines can be used
-		   in this context as well, while they were all designed to be
-		   called only once after the detect routine. (DB) */
-		/* queue_depth routine moved to inside scan_scsis(,1,,,) so
-		   it is called before build_commandblocks() */
-
-		err = length;
+		if ((err=scsi_add_single_device(HBA_ptr, channel, id, lun))==0)
+		    err = length;
 		goto out;
 	}
+
 	/*
 	 * Usage: echo "scsi remove-single-device 0 1 2 3" >/proc/scsi/scsi
 	 * with  "0 1 2 3" replaced by your "Host Channel Id Lun".
@@ -1781,7 +1915,6 @@
 	 *     CAUTION: This is not for hotplugging your peripherals. As
 	 *     SCSI was not designed for this you could damage your
 	 *     hardware and thoroughly confuse the SCSI subsystem.
-	 *
 	 */
 	else if (!strncmp("remove-single-device", buffer + 5, 20)) {
 		p = buffer + 26;
@@ -1797,58 +1930,8 @@
 				break;
 			}
 		}
-		err = -ENODEV;
-		if (!HBA_ptr)
-			goto out;
-
-		for (scd = HBA_ptr->host_queue; scd; scd = scd->next) {
-			if ((scd->channel == channel
-			     && scd->id == id
-			     && scd->lun == lun)) {
-				break;
-			}
-		}
-
-		if (scd == NULL)
-			goto out;	/* there is no such device attached */
-
-		err = -EBUSY;
-		if (scd->access_count)
-			goto out;
-
-		SDTpnt = scsi_devicelist;
-		while (SDTpnt != NULL) {
-			if (SDTpnt->detach)
-				(*SDTpnt->detach) (scd);
-			SDTpnt = SDTpnt->next;
-		}
-
-		if (scd->attached == 0) {
-			/*
-			 * Nobody is using this device any more.
-			 * Free all of the command structures.
-			 */
-                        if (HBA_ptr->hostt->revoke)
-                                HBA_ptr->hostt->revoke(scd);
-			devfs_unregister (scd->de);
-			scsi_release_commandblocks(scd);
-
-			/* Now we can remove the device structure */
-			if (scd->next != NULL)
-				scd->next->prev = scd->prev;
-
-			if (scd->prev != NULL)
-				scd->prev->next = scd->next;
-
-			if (HBA_ptr->host_queue == scd) {
-				HBA_ptr->host_queue = scd->next;
-			}
-			blk_cleanup_queue(&scd->request_queue);
-			kfree((char *) scd);
-		} else {
-			goto out;
-		}
-		err = 0;
+		err=scsi_remove_single_device(HBA_ptr, channel, id, lun);
+		goto out;
 	}
 out:
 	
--- linux-2.4.19/drivers/ieee1394/sbp2.c	2002-10-18 05:16:14.000000000 -0500
+++ linux-2.4.20-pre10-erik/drivers/ieee1394/sbp2.c	2002-10-18 05:29:20.000000000 -0500
@@ -562,6 +562,8 @@
 	.update = 	sbp2_update
 };
 
+static int sbp2_init_has_finished_initializing;
+
 /* List of device firmware's that require a forced 36 byte inquiry.  */
 static u32 sbp2_broken_inquiry_list[] = {
 	0x00002800,	/* Stefan Richter <richtest@bauwesen.tu-cottbus.de> */
@@ -1082,6 +1084,8 @@
 
 	hpsb_register_protocol(&sbp2_driver);
 
+	sbp2_init_has_finished_initializing++;
+
 	return 0;
 }
 
@@ -1681,6 +1685,23 @@
 
 	SBP2_INFO("Logged into SBP-2 device");
 
+	/* Try to hook ourselves into the SCSI subsystem, but only if
+	 * sbp2_init has finished doing its thing.  If we register
+	 * anything while sbp2_init is handling coldplugged devices, we
+	 * will will end up double registering things (a bad idea). */
+	if (sbp2_init_has_finished_initializing) {
+	    struct Scsi_Host *HBA_ptr;
+	    HBA_ptr=hi->scsi_host;
+	    if (HBA_ptr==NULL) {
+		return 0;
+	    }
+	    if (scsi_add_single_device(HBA_ptr, 0, scsi_id->id, 0)==0) {
+		SBP2_DEBUG("connecting SBP-2 device into the SCSI subsystem");
+	    } else {
+		SBP2_INFO("Unable to connect SBP-2 device into the SCSI subsystem");
+	    }
+	}
+
 	return(0);
 
 }
@@ -1739,8 +1760,23 @@
 
 	SBP2_INFO("Logged out of SBP-2 device");
 
-	return(0);
+	/* Now that we are logged out of the SBP-2 device, lets
+	 * try to un-hook ourselves from the SCSI subsystem */
+	{
+	    struct Scsi_Host *HBA_ptr;
+
+	    HBA_ptr=hi->scsi_host;
+	    if (HBA_ptr==NULL) {
+		return 0;
+	    }
+	    if (scsi_remove_single_device(HBA_ptr, 0, scsi_id->id, 0)==0) {
+		SBP2_DEBUG("disconnecting SBP-2 device from the SCSI subsystem");
+	    } else {
+		SBP2_INFO("Unable to disconnect SBP-2 device from the SCSI subsystem");
+	    }
+	}
 
+	return(0);
 }
 
 /*

Re: [PATCH] SCSI hotplug support

[Oliver Neukum]

Am Dienstag, 15. Oktober 2002 20:45 schrieb Erik Andersen:
> On Tue Oct 15, 2002 at 02:22:47PM -0400, Doug Ledford wrote:
> > On Mon, Oct 14, 2002 at 11:25:22PM -0600, Erik Andersen wrote:
> > > It is called from within the insmod's context.
> >
> > So it's called by what module then?  And what happens if we have a user
> > add a hard drive and it gets the module loaded and then attaches the
> > drive, then we attach another drive sometime later, and this time hotplug
> > doesn't do anything because the right module is already loaded?  How does
> > the drive get added then?  Am I missing something?
>
> Oops.  I spoke wrongly....  It is called from within
> /sbin/hotplug's context.

That sounds very odd. Are you trying to say that it is done in the context
of a kernel thread which later calls /sbin/hotplug, or what?

	Regards
		Oliver

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

On Tue Oct 15, 2002 at 11:43:44PM +0200, Oliver Neukum wrote:
> Am Dienstag, 15. Oktober 2002 20:45 schrieb Erik Andersen:
> > On Tue Oct 15, 2002 at 02:22:47PM -0400, Doug Ledford wrote:
> > > On Mon, Oct 14, 2002 at 11:25:22PM -0600, Erik Andersen wrote:
> > > > It is called from within the insmod's context.
> > >
> > > So it's called by what module then?  And what happens if we have a user
> > > add a hard drive and it gets the module loaded and then attaches the
> > > drive, then we attach another drive sometime later, and this time hotplug
> > > doesn't do anything because the right module is already loaded?  How does
> > > the drive get added then?  Am I missing something?
> >
> > Oops.  I spoke wrongly....  It is called from within
> > /sbin/hotplug's context.
> 
> That sounds very odd. Are you trying to say that it is done in the context
> of a kernel thread which later calls /sbin/hotplug, or what?

I was wrong again.  See the msg I just posted where (rather than
guessing) I actually test things....

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Re: [PATCH] SCSI hotplug support

[Oliver Neukum]

Am Montag, 14. Oktober 2002 23:07 schrieb James Bottomley:
> andersen@codepoet.org said:
> > If the user space interface were perfectly adequate, I would not have
> > written this patch.  User space does not have sufficient information
> > to know _which_ devices must to be added or removed. The best we can
> > do from user space is a full rescan of _all_ scsi host adaptors (http:/
> > /www.garloff.de/kurt/linux/rescan-scsi-bus.sh)  using something like
>
> The API you expose has identical inputs to the user space one.
>
> Therefore it seems to me that your spb2 driver must already know the values
> to fill in to use the API.  So what's wrong with triggering a hotplug event
> from this driver that causes the add/remove single device to be done from
> user level?

It's harder than doing it the simple way. User space really can do nothing but
do the call. Plus, you can use such a kernel API to really free the device's
memory, because you cannot know when user space, or indeed if, has
freed the device.
Doing it in kernel is the only sane thing.

	Regards
		Oliver

Re: [PATCH] SCSI hotplug support

[Doug Ledford]

On Tue, Oct 15, 2002 at 12:19:53AM +0200, Oliver Neukum wrote:
> Am Montag, 14. Oktober 2002 23:07 schrieb James Bottomley:
> > andersen@codepoet.org said:
> > > If the user space interface were perfectly adequate, I would not have
> > > written this patch.  User space does not have sufficient information
> > > to know _which_ devices must to be added or removed. The best we can
> > > do from user space is a full rescan of _all_ scsi host adaptors (http:/
> > > /www.garloff.de/kurt/linux/rescan-scsi-bus.sh)  using something like
> >
> > The API you expose has identical inputs to the user space one.
> >
> > Therefore it seems to me that your spb2 driver must already know the values
> > to fill in to use the API.  So what's wrong with triggering a hotplug event
> > from this driver that causes the add/remove single device to be done from
> > user level?
> 
> It's harder than doing it the simple way. User space really can do nothing but
> do the call.

Not true, see my last email.  User space can do a *lot* more than just 
make the call to add the device, and as a matter of fact it already does.

> Plus, you can use such a kernel API to really free the device's
> memory, because you cannot know when user space, or indeed if, has
> freed the device.

Not true at all.  Define a revoke/slave_detach entry point to your driver 
and you get called when the device is removed so that you can free up any 
of your resources.

> Doing it in kernel is the only sane thing.

Read source first, post sanity comments later.

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: [PATCH] SCSI hotplug support

[Oliver Neukum]

> > It's harder than doing it the simple way. User space really can do
> > nothing but do the call.
>
> Not true, see my last email.  User space can do a *lot* more than just
> make the call to add the device, and as a matter of fact it already does.

But surely it can do more if it can be called with knowledge of a device node,
can't it? Nobody argues against doing a hotplug call. But divorcing
recognisition on the bus and attaching a driver to it seems to be bad
to me. After all there's nothing you can do with a SCSI device without device
node.
Plus you keep a difference between the coldplugging and the hotplugging case,
which is not nice.

> > Plus, you can use such a kernel API to really free the device's
> > memory, because you cannot know when user space, or indeed if, has
> > freed the device.
>
> Not true at all.  Define a revoke/slave_detach entry point to your driver
> and you get called when the device is removed so that you can free up any
> of your resources.

I see.

	Regards
		Oliver

Re: [PATCH] SCSI hotplug support

[Doug Ledford]

On Tue, Oct 15, 2002 at 09:53:29AM +0200, Oliver Neukum wrote:
> 
> > > It's harder than doing it the simple way. User space really can do
> > > nothing but do the call.
> >
> > Not true, see my last email.  User space can do a *lot* more than just
> > make the call to add the device, and as a matter of fact it already does.
> 
> But surely it can do more if it can be called with knowledge of a device node,
> can't it? Nobody argues against doing a hotplug call. But divorcing
> recognisition on the bus and attaching a driver to it seems to be bad
> to me. After all there's nothing you can do with a SCSI device without device
> node.
> Plus you keep a difference between the coldplugging and the hotplugging case,
> which is not nice.

You guys aren't thinking.  Come on, what's the realistic difference in the 
hot plug user space manager in the two cases:

Case 1, scsi does the attach, hot plug does the rest:

hot_plug_attach_notifier(dev_t device)
{
  do stuff..
}

Case 2, scsi doesn't do the attach:
hot_plug_attach_notifier(int host, int bus, int target, int lun)
{
  fprintf(/proc/scsi/scsi, "scsi add-single-device %d %d %d %d", host, 
bus, target, lun);
  if(device_added_successfully) {
    do stuff..
  }
}


Now, please, someone tell me why everyone is whining about user space 
doing so little to accomplish what the much larger patch that was 
posted does in kernel space?  My point is, and was, that since we need the 
user space manager *anyway* to handle things the kernel will *never* 
handle, there is no real reason to split up the code segments into a 
kernel space portion and a user space portion.  In fact, it simply makes 
things more complex because if something breaks and you have both a kernel 
part and a user space part, you have to track down which one screwed the 
pooch.  This way at least it's all in one spot.

Besides, I've not heard anyone address my concerns about what context the
disc attach is done in with the kernel space patch.

> > > Plus, you can use such a kernel API to really free the device's
> > > memory, because you cannot know when user space, or indeed if, has
> > > freed the device.
> >
> > Not true at all.  Define a revoke/slave_detach entry point to your driver
> > and you get called when the device is removed so that you can free up any
> > of your resources.
> 
> I see.
> 
> 	Regards
> 		Oliver

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: [PATCH] SCSI hotplug support

[Oliver Neukum]

> You guys aren't thinking.  Come on, what's the realistic difference in the
> hot plug user space manager in the two cases:
>
> Case 1, scsi does the attach, hot plug does the rest:
>
> hot_plug_attach_notifier(dev_t device)
> {
>   do stuff..
> }
>
> Case 2, scsi doesn't do the attach:
> hot_plug_attach_notifier(int host, int bus, int target, int lun)
> {
>   fprintf(/proc/scsi/scsi, "scsi add-single-device %d %d %d %d", host,
> bus, target, lun);
>   if(device_added_successfully) {
>     do stuff..
>   }
> }

It would have to be:
hot_plug_attach_notifier(int host, int bus, int target, int lun)
{
fprintf(/proc/scsi/scsi, "scsi add-single-device %d %d %d %d", host, bus, target, lun);
dev = find_matching_device(host, bus, target, lun);
if (!dev)
	exit();
err = check_for_device_still_present();
if (err)
	exit();
do_stuff(dev);
}

> Now, please, someone tell me why everyone is whining about user space
> doing so little to accomplish what the much larger patch that was
> posted does in kernel space?  My point is, and was, that since we need the
> user space manager *anyway* to handle things the kernel will *never*

We don't need it. We may want to have it. It works without it, not as
well, but it works.

> Besides, I've not heard anyone address my concerns about what context the
> disc attach is done in with the kernel space patch.

Shouldn't we discuss this when we conclude that it's worth doing at all?

	Regards
		Oliver

Re: [PATCH] SCSI hotplug support

[James Bottomley]

dledford@redhat.com said:
> Now, please, someone tell me why everyone is whining about user space
> doing so little to accomplish what the much larger patch that was
> posted does in kernel space?  My point is, and was, that since we need the
> user space manager *anyway* to handle things the kernel will *never*

oliver@neukum.name said:
> We don't need it. We may want to have it. It works without it, not as
> well, but it works. 

But this is the philosophy difference.  I see hotplug as a replacement for 
kernel code.  I also see it as a way of ditching all the cross subsystem glue 
in the kernel and having the hotplug manager work out what should go on.  
Duplicating the hotplug code in the kernel just in case the user isn't using 
hotplug seems to me to be a waste of effort and an unwanted addition of 
complexity.

The point also is that Eric wants this code to cope with the case where a 
drive is added or removed from the firewire chassis *after* the system has 
been initiallised.  By very definition this is a hotplug event.  It's not 
unreasonable to require the user to do something manual if they choose not to 
use the hotplug infrastructure.

James

Re: [PATCH] SCSI hotplug support

[Erik Andersen]

On Tue Oct 15, 2002 at 10:35:33AM -0400, Doug Ledford wrote:
> Now, please, someone tell me why everyone is whining about user space 
> doing so little to accomplish what the much larger patch that was 
> posted does in kernel space?  My point is, and was, that since we need the 

Much larger patch?  If you look closely, that patch is actually
very small.  It simply moves the code already the proc/scsi/scsi
"scsi add-single-device" and "scsi remove-single-device" handlers
into standalone functions.  There is actually not so much as a
single line of new code...

> user space manager *anyway* to handle things the kernel will *never* 
> handle, there is no real reason to split up the code segments into a 
> kernel space portion and a user space portion.  In fact, it simply makes 
> things more complex because if something breaks and you have both a kernel 
> part and a user space part, you have to track down which one screwed the 
> pooch.  This way at least it's all in one spot.
> 
> Besides, I've not heard anyone address my concerns about what context the
> disc attach is done in with the kernel space patch.

I answered that last night.  It is called from within insmod
(i.e. not an interrupt handler or any such problematic case),

 -Erik

--
Erik B. Andersen             http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Re: [PATCH] SCSI hotplug support

[Doug Ledford]

On Tue, Oct 15, 2002 at 11:47:08AM -0600, Erik Andersen wrote:
> On Tue Oct 15, 2002 at 10:35:33AM -0400, Doug Ledford wrote:
> > Now, please, someone tell me why everyone is whining about user space 
> > doing so little to accomplish what the much larger patch that was 
> > posted does in kernel space?  My point is, and was, that since we need the 
> 
> Much larger patch?  If you look closely, that patch is actually
> very small.  It simply moves the code already the proc/scsi/scsi
> "scsi add-single-device" and "scsi remove-single-device" handlers
> into standalone functions.  There is actually not so much as a
> single line of new code...

Well, there is to.  You then export the functions.  Aside from the export 
and the intended use, I'm actually all for the patch and moving that code 
into separate functions.  I just don't want those functions called from 
all over the place.

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: [PATCH] SCSI hotplug support

[Scott Merritt]

On Tue, 15 Oct 2002 10:35:33 -0400
Doug Ledford <dledford@redhat.com> wrote:

> Now, please, someone tell me why everyone is whining about user space 
> doing so little to accomplish what the much larger patch that was ...

Not *everyone* is whining about funnelling the scsi attachments back through user space :)  I *much* prefer this approach as it makes it easier for users to install extra/different processing if necessary.  In some sense, this is similar to my previous objections about the scsi drivers insisting on automatically reading the partition tables whenever they (think) they have encountered a scsi block device.