SCSI hotplug support

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

@@ -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 *);

@@ -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);
+

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

Matthew Dharm                              Home: [email protected] 
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

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

Hi Erik,

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                   <[email protected]>         [Eindhoven, NL]
Physics: Plasma simulations    <[email protected]>     [TU Eindhoven, NL]
Linux: SCSI, Security          <[email protected]>    [SuSE Nuernberg, DE]
 (See mail header or public key servers for PGP2 and GPG public keys.)

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
[email protected]

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

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
[email protected]

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

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

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
[email protected]

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: [email protected] 
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

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.

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

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

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

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 <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

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.

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.

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

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

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.

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.

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 <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

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.

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.

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

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

My patch is not at all usb or ieee1394 specific.

Ok.

Ok. That sounds workable.

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

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 <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

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

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 <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

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

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

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

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.

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 <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

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

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.

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 <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

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

@@ -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);
+

@@ -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 *);

@@ -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 <[email protected]> */

@@ -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);
 }
 
 /*

Am Dienstag, 15. Oktober 2002 20:45 schrieb Erik Andersen:

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

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

Am Montag, 14. Oktober 2002 23:07 schrieb James Bottomley:

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

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.

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.

Read source first, post sanity comments later.

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

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.

I see.

Regards
Oliver

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.

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

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);
}

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

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

Regards
Oliver

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

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

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

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 <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

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.

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

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.." 

Hi James,

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.

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                   <[email protected]>         [Eindhoven, NL]
Physics: Plasma simulations    <[email protected]>     [TU Eindhoven, NL]
Linux: SCSI, Security          <[email protected]>    [SuSE Nuernberg, DE]
 (See mail header or public key servers for PGP2 and GPG public keys.)