USB Storage oops in 2.5.69-bk8

Good. This clearly shows the problem. It's an error in the SCSI driver. It has shown up before and been discussed at length a few weeks ago. The problem is that a MODE-SENSE(6) command is requesting a transfer of 3 bytes, which is smaller than the minimum of 4 bytes for that command. The device doesn't like it and crashes.

The SCSI people are supposed to be working on this. First, we should be using MODE-SENSE(10) by default rather than MODE-SENSE(6). Second, we should never request a transfer smaller than the minimum allowed size. When these changes are ready you shouldn't have any more trouble.

The OOPS you got originally was caused by the lack of proper hot-unplugging cooperation between usb-storage and the SCSI driver.

Alan Stern

At GregKH's suggestion I have opened a bug in 'zilla:
http://bugme.osdl.org/show_bug.cgi?id=724
You might want to update it or close it.

Thank you,
florin

"NT is to UNIX what a doughnut is to a particle accelerator."

I remember the previous conversation on the MODE-SENSE, but I think there might be more here.

I do not believe the oops is related to the lack of proper hot-plugging support (unless this is another way it will manifest itself). The issue is more the interaction of the error handler and the usb storage driver.

The sequence:
"drivers/usb/core/message.c: usb_control/bulk_msg: timeout Error handler scsi_eh_1 waking up
scsi_eh_prt_fail_stats: 1:0:0:0 cmds failed: 0, cancel: 1"

would lead one two believe that a timeout was detected both in the usb core and in the mid layer. This would not be a good thing to have two timeout recovery actions happening. I am not familiar with the usb storage code so this may be incorrect.

-andmike
--
Michael Anderson
[email protected]

I think this is a red herring, and the sequence of log messages above is not especially pertinent. I don't have my copy of the original system log with me here, so this is relying on memory and might be wrong -- end disclaimer.

What happens is that the device crashes because of the 3-byte MODE-SENSE(6) request. The scsi error handler tries various levels of fault recovery, all the way up to a bus reset. Usb-storage implements the bus reset as a USB port reset, which requires usbcore to re-enumerate the device. The line

drivers/usb/core/message.c: usb_control/bulk_msg: timeout

refers to a failure during the re-enumeration. Apparently even the port reset was not enough to revive the device. I don't know the reason for the line

Error handler scsi_eh_1 waking up

since the error handler is already running -- maybe it's related to the hot-unplug sequence that usb-storage goes through. That sequence is pretty much guaranteed to cause an OOPS if the device is busy at the time, since it hasn't yet been updated properly. (I think Matt Dharm is waiting to hear from someone -- like you, Mike -- that the SCSI core is ready for hot-unplugging.)

When the USB port reset fails as it did here, it automatically causes a hot-unplug event. Usb-storage currently handles the SCSI part of that as follows:

	// Lock the host
	for each device on the host: sdev->online = 0;
	// Unlock the host
	scsi_remove_host(host);
	scsi_unregister_host(host);

My guess is that this sequence provokes that error handler message and causes the crash.

At any rate, the two timeout messages are referring to two different things. One is the failure of the MODE-SENSE and the other is the failure of the re-enumeration. They do not in themselves indicate a problem.

Alan Stern

That reset couldn't re-assign the address, so of course it failed.

In hub.c, usb_physical_reset_device() is leaving dev->state as USB_STATE_CONFIGURED after resetting the port, and of course it's not legal to set the address of a device in that state.

- Dave

Do you mean to say that this is a bug in usb_reset_device()? Isn't it always going to be true that after a port reset a device will need to be assigned an address?

Alan Stern

No; yes. What I mean is that something like this patch would seem to be needed, so USB_STATE_DEFAULT ("needs an address") is set when the reset completes. Untested, but it compiles.

The enumeration logic still relies on hidden side effects.

- Dave

@@ -737,6 +737,9 @@
 		if (status != -1) {
 			usb_clear_port_feature(hub,
 				port + 1, USB_PORT_FEAT_C_RESET);
+			dev->state = status
+					? USB_STATE_NOTATTACHED
+					: USB_STATE_DEFAULT;
 			return status;
 		}
 

@@ -1018,7 +1018,6 @@
 	dev->dev.dma_mask = parent->dma_mask;
 
 	/* it's not usable yet */
-	dev->state = USB_STATE_DEFAULT;
 
 	/* USB 2.0 section 5.5.3 talks about ep0 maxpacket ...
 	 * it's fixed size except for full speed devices.

Mike:

I just modified the usb-storage driver to simulate a failed device and tried it with my USB CD-RW drive. The SCSI error handler kicked in, tried a device reset (which I had doctored to simulate a failure), tried a bus reset (which actually did fail because of an bug somewhere else in the USB stack), couldn't do a host reset (because usb-storage doesn't support it), and finally decided to take the device off-line.

That much worked fine. But afterwards, when the process trying to read from the drive ended and the device file was closed, usb-storage received an ALLOW_MEDIUM_REMOVAL command. This should not have happened given that the device was off-line.

Here are the relevant parts of the kernel log:

May 21 10:09:29 localhost kernel: usb-storage: Command READ_10 (10 bytes)
May 21 10:09:29 localhost kernel: usb-storage:  28 00 00 00 00 44 00 00 40 00
(delay to simulate failure)
May 21 10:09:59 localhost kernel: usb-storage: usb_storage_command_abort called
May 21 10:09:59 localhost kernel: usb-storage: usb_stor_stop_transport called
May 21 10:09:59 localhost kernel: usb-storage: Bulk status result = 3
May 21 10:09:59 localhost kernel: usb-storage: -- command was aborted
May 21 10:09:59 localhost kernel: usb-storage: Bulk reset requested
(delay for automatically-generated device reset)
May 21 10:10:05 localhost kernel: usb-storage: Soft reset done
May 21 10:10:05 localhost kernel: usb-storage: scsi command aborted
May 21 10:10:05 localhost kernel: usb-storage: *** thread sleeping.
May 21 10:10:05 localhost kernel: usb-storage: queuecommand() called
May 21 10:10:05 localhost kernel: usb-storage: *** thread awakened.
May 21 10:10:05 localhost kernel: usb-storage: Command TEST_UNIT_READY (6 bytes)
May 21 10:10:05 localhost kernel: usb-storage:  00 00 00 00 00 00
(delay to simulate failure)
May 21 10:10:15 localhost kernel: usb-storage: usb_storage_command_abort called
May 21 10:10:15 localhost kernel: usb-storage: usb_stor_stop_transport called
May 21 10:10:15 localhost kernel: usb-storage: Bulk status result = 3
May 21 10:10:15 localhost kernel: usb-storage: -- command was aborted
May 21 10:10:15 localhost kernel: usb-storage: Bulk reset requested
(delay for automatically-generated device reset)
May 21 10:10:21 localhost kernel: usb-storage: Soft reset done
May 21 10:10:21 localhost kernel: usb-storage: scsi command aborted
May 21 10:10:21 localhost kernel: usb-storage: *** thread sleeping.
May 21 10:10:21 localhost kernel: usb-storage: usb_storage_device_reset called
(simulate failure of device reset)
May 21 10:10:21 localhost kernel: usb-storage: usb_storage_bus_reset called
(next line is due to an error in the USB stack, causing failure)
May 21 10:10:21 localhost kernel: drivers/usb/core/hub.c: USB device not accepting new address (error=-22)
May 21 10:10:21 localhost kernel: usb-storage: usb_reset_device returns -22
May 21 10:10:21 localhost kernel: scsi: Device offlined - not ready after error recovery: host 0 channel 0 id 0 lun 0
May 21 10:10:21 localhost kernel: SCSI cdrom error : <0 0 0 0> return code = 0x6050000
May 21 10:10:21 localhost kernel: end_request: I/O error, dev sr0, sector 272
May 21 10:10:21 localhost kernel: Buffer I/O error on device scsi/host0/bus0/target0/lun0/cd, logical block 68
May 21 10:10:21 localhost kernel: Buffer I/O error on device scsi/host0/bus0/target0/lun0/cd, logical block 69
(more I/O error messages, not shown)
May 21 10:10:21 localhost kernel: usb-storage: queuecommand() called
May 21 10:10:21 localhost kernel: usb-storage: *** thread awakened.
May 21 10:10:21 localhost kernel: usb-storage: Command ALLOW_MEDIUM_REMOVAL (6 bytes)
May 21 10:10:21 localhost kernel: usb-storage:  1e 00 00 00 00 00

I trust this won't be hard to fix.

Alan Stern

The above is the first thing output when the eh wakes up, so the re-enumeration could not have started yet (at least it could not have been initiated via scsi eh calling the bus reset or other abort/reset related functions), and the USB timeout message could not be part of the scsi eh, though we did not see the scsi_times_out log message (since it's wrapped in SCSI_LOG_TIMEOUT, not SCSI_LOG_ERROR_RECOVERY).

Turning on timeout (or even all) logging might give a bit more data:

echo "scsi log timeout 4" > /proc/scsi/scsi

Or (warning - logs the logging messages if logging to a scsi disk):

echo "scsi log all" > /proc/scsi/scsi

-- Patrick Mansfield

The problem is fixed in 2.5.70-bk5. I have closed the bug [1] in bugzilla.

Thank you,
florin

1. http://bugme.osdl.org/show_bug.cgi?id=724

"NT is to UNIX what a doughnut is to a particle accelerator."

Mike:

Do you, or does anybody else, object to the patch below? And if not, could you please ask Linus to apply it? It would improve error recovery for certain types of USB storage drives.

Alan Stern

===== scsi_error.c 1.27 vs edited =====

@@ -753,6 +753,8 @@
 	scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 	scmd->underflow = 0;
 	scmd->sc_data_direction = SCSI_DATA_NONE;
+	if ((scmd->serial_number -= 999999) == 0)
+		scmd->serial_number -= 999999;
 
 	rtn = scsi_send_eh_cmnd(scmd, SENSE_TIMEOUT);
 

It's just to insure that when the error handler sends a TEST-UNIT-READY, the serial number is different from that of the immediately-prior failed command (or from that of any command issued recently or likely to be issued in the near future).

Well, the patch changes the serial number regardless of its initial value, but otherwise yes. It would be okay with me to change scsi_core->sn. But the global serial number is currently a static variable defined in scsi.c and my change was to scsi_error.c -- I didn't want to export the variable needlessly.

It doesn't reserve any values. (I avoided 0 only because I noticed that the scsi core uses 0 as a reserved value.) Usb-storage just requires that serial numbers not be repeated.

Alan Stern

Sorry for the slow response I was under the weather and not doing much yesterday.

I agree on the change in SCSI core instead of a change just for scsi error handling. If you are depending on command serial number to be unique as the comments in scsi_dispatch_cmd already indicate we have problems (in USB storage case since can_queue is 1 it would not run into a issue until scsi error handling).

The patch below is a quick patch that moves the serial_number from a scsi mid level static to a per scsi host value. In looking at the usage of command serial_number it appears that moving to a per scsi host name space should be ok.

I have compile and boot tested it on my system with the ips and aic7xxx drivers.

The patch needs some more testing and some variable name updates, but should be ok to test USB and allow for discussion of this type of change.

-andmike
--
Michael Anderson
[email protected]

DESC
This patch is against scsi-misc-2.5 but also applies against 2.5.69.

Move scsi command serial number to per scsi host serial number. We also only increment the serial number under a lock now so the race on the value is removed. A new serial number is also acquired in the scsi_error handler on new commands.
EDESC

 drivers/scsi/hosts.h      |    7 +++++++
 drivers/scsi/scsi.c       |    7 ++-----
 drivers/scsi/scsi_error.c |    1 +
 3 files changed, 10 insertions(+), 5 deletions(-)

diff -puN drivers/scsi/hosts.h~scsi_serial_number drivers/scsi/hosts.h
@@ -487,6 +487,8 @@ struct Scsi_Host
     struct device host_gendev;
     struct class_device class_dev;
 
+    unsigned long serial_number;
+
     /*
      * We should ensure that this is aligned, both for better performance
      * and also because some compilers (m68k) don't automatically force

@@ -532,6 +534,11 @@ static inline struct device *scsi_get_de
         return shost->host_gendev.parent;
 }
 
+static inline unsigned long scsi_get_next_serial(struct Scsi_Host *shost)
+{
+        return (++shost->serial_number) ? shost->serial_number : 1;
+}
+
 struct Scsi_Device_Template
 {
     struct list_head list;

diff -puN drivers/scsi/scsi.c~scsi_serial_number drivers/scsi/scsi.c
@@ -83,7 +83,6 @@
  */
 unsigned long scsi_pid;
 struct scsi_cmnd *last_cmnd;
-static unsigned long serial_number;
 
 /*
  * List of all highlevel drivers.

@@ -398,11 +397,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
 	unsigned long timeout;
 	int rtn = 1;
 
-	/* Assign a unique nonzero serial_number. */
 	/* XXX(hch): this is racy */
-	if (++serial_number == 0)
-		serial_number = 1;
-	cmd->serial_number = serial_number;
 	cmd->pid = scsi_pid++;
 
 	/* 

@@ -471,6 +466,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
 					   host->hostt->queuecommand));
 
 		spin_lock_irqsave(host->host_lock, flags);
+		cmd->serial_number = scsi_get_next_serial(host);
 		rtn = host->hostt->queuecommand(cmd, scsi_done);
 		spin_unlock_irqrestore(host->host_lock, flags);
 		if (rtn) {

@@ -485,6 +481,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
 					host->hostt->command));
 
 		spin_lock_irqsave(host->host_lock, flags);
+		cmd->serial_number = scsi_get_next_serial(host);
 		cmd->result = host->hostt->command(cmd);
 		scsi_done(cmd);
 		spin_unlock_irqrestore(host->host_lock, flags);

diff -puN drivers/scsi/scsi_error.c~scsi_serial_number drivers/scsi/scsi_error.c
@@ -456,6 +456,7 @@ static int scsi_send_eh_cmnd(struct scsi
 		scmd->request->rq_status = RQ_SCSI_BUSY;
 
 		spin_lock_irqsave(scmd->device->host->host_lock, flags);
+		scmd->serial_number = scsi_get_next_serial(host);
 		host->hostt->queuecommand(scmd, scsi_eh_done);
 		spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 

_

1. cmdsn is per initiator, so I have no objections _in_principle_ to this patch. (haven't looked at the patch code, just read the idea) Actually cmdsn just insures delvery of the _command_ across
the interconnect, after that it doesn't matter. Normally transports play with cmdsn, but it is no error for the application client to provide it. Transports may ignore it completely, when they don't support it or when they generate their own, and thus SCSI Core SHOULD NOT rely on this to identify the command.

2. SCSI Core->cmdsn should _always_ contain the _next_ cmdsn to be assigned. This provides that at boot time, it contains 0, and this is consistent
with the fact that no commands have been issued yet. Assignment to a command should be atomic a la, cmd->cmdsn = scsi_core->cmdsn++;
This also provides that for a reasonable value v [RFC1982],
one can tell immediately the number of commands issued, a la, (scsi_core->cmdsn - v + MAX) % MAX is commands issued since v.
This is also consistent with the well known practice that indexes,
etc, always contain the _next_ index, so that that it serves as an index (to the next) and as a _size_ variable just by reading it.

(#2 should probably be fixed first)

3. Zero is indeed a reserved value, as per SAM-3r07, 5.1.

4. The error handler is NO exception. If the error handler sends a SCSI Command (i.e. a _CDB_) then it is supposed to assign a cmdsn, as per the rules. This is for delivery.

Luben

There is a bug in the above line it should be
return (++shost->serial_number) ? shost->serial_number : ++shost->serial_number;

No.

This name is ok with me.

Why compare with MAX_SN and not just let the value role over and check for false.

Yes I was.

-andmike
--
Michael Anderson
[email protected]

^^^^^^^^^^^^^^

I did NOT write the above mentioned function -- this was part of _your_ patch -- next time _please_ remove ``Luben wrote:'' in the reply, so that there's no confusion!

I'd appreciate that very much.

First, zero is reserved, and I was trying to keep the _next_ sn in the ``session'' cmdsn variable, but there's a little _bug_ up there too, so here's a corrected version:

static inline unsigned long scsi_get_cmdsn(struct Scsi_Host *shost)

{
	static const typeof(shost->serial_number) MAX_SN =
		~((typeof(shost->serial_number) 0);
	return  shost->serial_number == MAX_SN ?
		(shost->serial_number += 2)++  :
		shost->serial_number++;
}

Luben

My less than $.02:

As far as the code goes, it does not matter whether we store the current or next value in shost->serial_number, so we ought to use the simpler code (Mike's version). And (x += 2)++ won't compile.

Per naming: "get" is already overloaded and implies a put. We have code like scsi_get_cmd that allocates and returns a pointer/value, and then various scsi and other kernel get/put functions that take a pointer and increment or decrement ref counters. cmdsn is a rather cryptic abbreviation that my brain can't easily parse.

I suggest scsi_serial_number(host) or scsi_cmdsn(host).

-- Patrick Mansfield

I was _just_ mentioning what is normally done in implementations of some transports... (but actually they need to store the next, but it seems we'll not use this ability in SCSI Core, so yes, it doesn't really matter...)

You mean Eric's version -- this was in SCSI Core _before_ you two started paid work on SCSI Core.

Aaaah, I've mentioned this before several times, which is my usual disclaimer: the only C code which I send to linux scsi is in a patch. Anything else, doesn't matter how C-like it is, if it is in text, is just that: C-like.

I'm just trying to convey an idea.

Actually I _did_ set up a running verion of this as a patch today but I never sent it -- too much politics, I got discouraged and saw absolutely _no_ point.

I personally am getting tired of those
scsi_this_is_a_long_name_for_a_trivial_function()
function names in scsi so I'd go for the second.

Luben

Mike:

I tried it out, and it did exactly what I wanted. You, Luben, and the others can fight over the details.

Alan Stern

Fighting? Who's fighting? I was just describing what standards say about those kinds of things, so that SCSI Core is in line with those.

It's unfortunate that ppl can post comments like this; it's immature and doesn't help nor improve SCSI Core.

All we're trying to do here is _improve_ SCSI Core any which way we can and we all contribute what we can.

Comments like this are discouraging at best...

Luben

Sorry, that was meant as a joke. I guess I should have used a smiley...

Alan Stern

This patch looks OK to me, except that I don't see a good reason to go to a per host serial number. The SCSI commands are so short lived that the current global serial number seems adequate (and saves us 4 bytes per device).

On locking grounds, though it does look better to move it into the host structure (probably make explicit by documenting that the host lock needs to be held accessing the method).

This:

+static inline unsigned long scsi_get_next_serial(struct Scsi_Host *shost)
+{
+ return (++shost->serial_number) ? shost->serial_number : 1;
+}
+

Will hand out two 1 serial numbers when we start at zero or wrap.

Shouldn't that be

static inline unsigned long scsi_get_next_serial(struct Scsi_Host *shost)

{
	if(++shost->serial_number == 0)
		++shost->serial_number;

        return shost->serial_number;
}

?

James

I made the change on locking simplification a global lock to protect this value in the IO path sounded wrong. The overhead should be the same on single adapter systems, but would increase by 4 bytes for every adapter (real and pseudo).

Yes in the original patch snippet above 1 was passed out twice. In a previous response to this thread I indicated it should be like below or functionally similar to the code you have above.

return (++shost->serial_number) ? shost->serial_number : ++shost->serial_number;

-andmike
--
Michael Anderson
[email protected]

Mike:

You may know this already; if so please forgive the redundant information. The reworking of the host registration code in 2.5.70 has a bug that prevents the host's directory in /proc/scsi from being removed when the host is unregistered. The cause of the problem is that scsi_proc_host_rm() is checking for shost->hostt->present == 0, but it is called before scsi_free_shost() which is where shost->hostt->present gets decremented.

Since there doesn't really seem to be any need to check shost->hostt->present (hosts shouldn't be removed more than once), I removed that line. I also reset shost->hostt->proc_dir back to NULL, in case the same host template is used again later to register another host.

(There's another problem that prevents scsi_free_shost() from being called at all, but that's a bug in the driver model core. I've submitted a separate report about that.)

Alan Stern

===== scsi_proc.c 1.11 vs edited =====

@@ -149,8 +149,8 @@
 
 	sprintf(name,"%d", shost->host_no);
 	remove_proc_entry(name, shost->hostt->proc_dir);
-	if (!shost->hostt->present)
-		remove_proc_entry(shost->hostt->proc_name, proc_scsi);
+	remove_proc_entry(shost->hostt->proc_name, proc_scsi);
+	shost->hostt->proc_dir = NULL;
 }
 
 static void proc_print_scsidevice(struct scsi_device* sdev, char *buffer,

Alan,
This is bug, but I do not think this fix will work for non legacy callers. LLDDs that just use scsi_add_host and scsi_remove_host would have the remove_proc_entry called possibly too early.

I do not have another fix to offer right now as "present" has race issues and just moving it might not be a good idea.

-andmike
--
Michael Anderson
[email protected]

Why should we care when the proc entry is remove? The whole ->proc_info mess is marked obsolete in 2.5 so no one should rely on it. It's certainly better than leaking the proc_dir_entry :)

While it is marked obsolete, but it should still function shouldn't it? Even if we use the proc_info replacement you suggested in previous mail:
http://marc.theaimsgroup.com/?l=linux-scsi&m=105112638920306&w=2
we should have a similar cleanup issue.

The patch suggested gives the output shown below which leaves "n" number of file entries not accessible.

Ex.

# ls /proc/scsi/scsi_debug/
1  2  3
# echo "-1" > add_host
Synchronizing SCSI cache: 
# ls /proc/scsi
device_info  ips  scsi  sg

This proc cleanup routine also races with the read / write routines that previously where "protected" by the procfs try_module_get.

I was looking into closing this race, but if you feel it is not worthwhile I can go look at other issues we have.

-andmike
--
Michael Anderson
[email protected]

Mike:

Here is a more extensive attempt to fix the /proc/scsi problem. This patch should work with LLDDs that use either the legacy or the hotplug model. It should fix the problem you noted of trying to remove the directory while there are still hosts present. And it resolves at least one race that exists in the current code.

The major difference between the current code and my patch is that the patch creates the /proc/scsi/hostname directory during scsi_register() and removes it during scsi_unregister(). The actual /proc/scsi/hostname/hostnum files are still created and removed during scsi_add_host() and scsi_remove_host(). I doubt that change should cause anybody any problem; it was the only way to reconcile the procfs stuff with host probing.

I hope you like this attempt a little better.

Alan Stern

# This is a BitKeeper generated patch for the following project:
# Project Name: greg k-h's linux 2.5 USB kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1649 -> 1.1650
# drivers/scsi/scsi_priv.h 1.1 -> 1.2
# drivers/scsi/hosts.c 1.26 -> 1.27
# drivers/scsi/scsi_proc.c 1.11 -> 1.12
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/06/12 [email protected] 1.1650
# Fix procfs handling during host unregistration
# --------------------------------------------
#

diff -Nru a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
@@ -285,7 +285,7 @@
 		shost->eh_notify = NULL;
 	}
 
-	shost->hostt->present--;
+	scsi_proc_hostdir_rm(shost->hostt);
 	scsi_destroy_command_freelist(shost);
 	kfree(shost);
 }

@@ -417,7 +417,7 @@
 	 */
 	wait_for_completion(&sem);
 	shost->eh_notify = NULL;
-	shost->hostt->present++;
+	scsi_proc_hostdir_add(shost->hostt);
 	return shost;
 
 fail:

diff -Nru a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
@@ -42,6 +42,7 @@
 	(((scmd)->sense_buffer[0] & 0x70) == 0x70)
 
 struct Scsi_Device_Template;
+struct SHT;
 
 
 /*

@@ -100,11 +101,15 @@
 
 /* scsi_proc.c */
 #ifdef CONFIG_PROC_FS
+extern void scsi_proc_hostdir_add(struct SHT *);
+extern void scsi_proc_hostdir_rm(struct SHT *);
 extern void scsi_proc_host_add(struct Scsi_Host *);
 extern void scsi_proc_host_rm(struct Scsi_Host *);
 extern int scsi_init_procfs(void);
 extern void scsi_exit_procfs(void);
 #else
+# define scsi_proc_hostdir_add(sht)	do { } while (0)
+# define scsi_proc_hostdir_rm(sht)	do { } while (0)
 # define scsi_proc_host_add(shost)	do { } while (0)
 # define scsi_proc_host_rm(shost)	do { } while (0)
 # define scsi_init_procfs()		(0)

Alan,
This looks good. In scsi-misc-2.5 SHT has been removed so there will need to be a little name sync up.

I also had a patch that I was working on that removed the users of "->present" and just left the procfs users. This might be a good one to add on top of this patch.

We still have the issue outside of what this patch was trying to address with the proc_read/proc_write functions racing with the remove_proc_entry, but in previous statements this will currently be left as is for this obsolete interface.

-andmike
--
Michael Anderson
[email protected]

That shouldn't be difficult.

I agree.

That sounds like the right thing to do. This is a generic problem with procfs (maybe sysfs too). I think the only way to solve it is to add "open" and "close" routines that would increment/decrement a reference count for the host structure. Delaying the kfree of the structure until the reference count goes to 0 would prevent anything bad from happening when scsi_unregister() is called while some process still has the file open.

But since this is an obsolete interface anyway, it's probably not worth going to all that trouble.

Alan Stern

Yes, certainly.

This is a generic procfs problem. We could maybe use real file operations for the procfs files and then set the owner to the LLDD module. But probably it's not worth the effort..

This is a _trivial_ bug in SCSI Core (error handler, ...) which I hope will be fixed in the near future (Mike or Patrick will send a patch).

That is, as per my other email, two successive commands (CDBs) should never be sent with duplicate cmdsn's.

What we're talking about here is a _consistent_ path of a command through SCSI Core, before it reaches a LLDD. This is currently NOT the case, thus the duplicate cmdsn...

Luben