* Re: Patch: change the serial_number for error-handler commands
2003-05-21 18:03 ` Mike Anderson
@ 2003-05-21 18:50 ` Luben Tuikov
2003-05-21 19:18 ` Luben Tuikov
` (3 subsequent siblings)
4 siblings, 0 replies; 34+ messages in thread
From: Luben Tuikov @ 2003-05-21 18:50 UTC (permalink / raw)
To: Mike Anderson; +Cc: Alan Stern, linux-scsi
Mike Anderson wrote:
> Sorry for the slow response I was under the weather and not doing much
> yesterday.
>
> Alan Stern [stern@rowland.harvard.edu] wrote:
>
>>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.
>>
>>
>>>Q: Which value does USB Storage reserve for cmdsn, 0 or MAX?
>>
>>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.
>>
>
>
> 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.
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: Patch: change the serial_number for error-handler commands
2003-05-21 18:03 ` Mike Anderson
2003-05-21 18:50 ` Luben Tuikov
@ 2003-05-21 19:18 ` Luben Tuikov
2003-05-21 20:28 ` Mike Anderson
2003-05-21 19:57 ` Alan Stern
` (2 subsequent siblings)
4 siblings, 1 reply; 34+ messages in thread
From: Luben Tuikov @ 2003-05-21 19:18 UTC (permalink / raw)
To: Mike Anderson; +Cc: Alan Stern, linux-scsi
Mike Anderson wrote:
>
> 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
> --- sysfs-scsi-misc-2.5/drivers/scsi/hosts.h~scsi_serial_number Wed May 21 08:41:49 2003
> +++ sysfs-scsi-misc-2.5-andmike/drivers/scsi/hosts.h Wed May 21 08:41:49 2003
> @@ -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;
> +}
> +
scsi_get_next_serial()??? Do _you_ Mike like this name?
scsi_get_cmdsn() will be a lot more appropriate. It doesn't
reveal implementation (``next''), and tells that it's a serial
number for a command, ``sn'' stands for serial number pretty
universally in our society.
How about something like this:
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 : shost->serial_number;
}
BTW, are you relying on memset(..., 0, sizeof(struct Scsi_Host),
to set the serial number to 0?
--
Luben
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: Patch: change the serial_number for error-handler commands
2003-05-21 19:18 ` Luben Tuikov
@ 2003-05-21 20:28 ` Mike Anderson
2003-05-21 21:11 ` Luben Tuikov
0 siblings, 1 reply; 34+ messages in thread
From: Mike Anderson @ 2003-05-21 20:28 UTC (permalink / raw)
To: Luben Tuikov; +Cc: Alan Stern, linux-scsi
Luben Tuikov [tluben@rogers.com] wrote:
> >+static inline unsigned long scsi_get_next_serial(struct Scsi_Host *shost)
> >+{
> >+ return (++shost->serial_number) ? shost->serial_number : 1;
> >+}
> >+
>
There is a bug in the above line it should be
return (++shost->serial_number) ? shost->serial_number : ++shost->serial_number;
> scsi_get_next_serial()??? Do _you_ Mike like this name?
>
No.
> scsi_get_cmdsn() will be a lot more appropriate. It doesn't
> reveal implementation (``next''), and tells that it's a serial
> number for a command, ``sn'' stands for serial number pretty
> universally in our society.
>
This name is ok with me.
> How about something like this:
>
> 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 : shost->serial_number;
> }
>
Why compare with MAX_SN and not just let the value role over and check
for false.
> BTW, are you relying on memset(..., 0, sizeof(struct Scsi_Host),
> to set the serial number to 0?
Yes I was.
-andmike
--
Michael Anderson
andmike@us.ibm.com
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: Patch: change the serial_number for error-handler commands
2003-05-21 20:28 ` Mike Anderson
@ 2003-05-21 21:11 ` Luben Tuikov
2003-05-21 23:15 ` Patrick Mansfield
0 siblings, 1 reply; 34+ messages in thread
From: Luben Tuikov @ 2003-05-21 21:11 UTC (permalink / raw)
To: Mike Anderson; +Cc: Alan Stern, linux-scsi
Mike Anderson wrote:
> Luben Tuikov [tluben@rogers.com] wrote:
>
>> Mike Anderson wrote:
^^^^^^^^^^^^^^
>>>+static inline unsigned long scsi_get_next_serial(struct Scsi_Host *shost)
>>>+{
>>>+ return (++shost->serial_number) ? shost->serial_number : 1;
>>>+}
>>>+
>>
>
> There is a bug in the above line it should be
> return (++shost->serial_number) ? shost->serial_number : ++shost->serial_number;
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.
>>How about something like this:
>>
>>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 : shost->serial_number;
>>}
>>
>
>
> Why compare with MAX_SN and not just let the value role over and check
> for false.
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: Patch: change the serial_number for error-handler commands
2003-05-21 21:11 ` Luben Tuikov
@ 2003-05-21 23:15 ` Patrick Mansfield
2003-05-22 5:47 ` Luben Tuikov
0 siblings, 1 reply; 34+ messages in thread
From: Patrick Mansfield @ 2003-05-21 23:15 UTC (permalink / raw)
To: Luben Tuikov; +Cc: Mike Anderson, linux-scsi
On Wed, May 21, 2003 at 05:11:41PM -0400, Luben Tuikov wrote:
> Mike Anderson wrote:
> >
> > Why compare with MAX_SN and not just let the value role over and check
> > for false.
>
> 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++;
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: Patch: change the serial_number for error-handler commands
2003-05-21 23:15 ` Patrick Mansfield
@ 2003-05-22 5:47 ` Luben Tuikov
0 siblings, 0 replies; 34+ messages in thread
From: Luben Tuikov @ 2003-05-22 5:47 UTC (permalink / raw)
To: Patrick Mansfield; +Cc: Mike Anderson, linux-scsi
Patrick Mansfield wrote:
>
> 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,
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...)
> so we ought to use the simpler code
> (Mike's version).
You mean Eric's version -- this was in SCSI Core _before_ you two
started paid work on SCSI Core.
> And (x += 2)++ won't compile.
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.
> 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).
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: Patch: change the serial_number for error-handler commands
2003-05-21 18:03 ` Mike Anderson
2003-05-21 18:50 ` Luben Tuikov
2003-05-21 19:18 ` Luben Tuikov
@ 2003-05-21 19:57 ` Alan Stern
2003-05-21 20:42 ` Luben Tuikov
2003-05-21 21:19 ` James Bottomley
2003-06-11 17:41 ` PATCH: (as33) Remove /proc/scsi directory in scsi_remove_host() Alan Stern
4 siblings, 1 reply; 34+ messages in thread
From: Alan Stern @ 2003-05-21 19:57 UTC (permalink / raw)
To: Mike Anderson; +Cc: Luben Tuikov, linux-scsi
On Wed, 21 May 2003, Mike Anderson wrote:
> 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.
Mike:
I tried it out, and it did exactly what I wanted. You, Luben, and the
others can fight over the details.
Alan Stern
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: Patch: change the serial_number for error-handler commands
2003-05-21 19:57 ` Alan Stern
@ 2003-05-21 20:42 ` Luben Tuikov
2003-05-21 21:05 ` Alan Stern
0 siblings, 1 reply; 34+ messages in thread
From: Luben Tuikov @ 2003-05-21 20:42 UTC (permalink / raw)
To: Alan Stern; +Cc: Mike Anderson, linux-scsi
Alan Stern wrote:
>
> Mike:
>
> I tried it out, and it did exactly what I wanted. You, Luben, and the
> others can fight over the details.
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: Patch: change the serial_number for error-handler commands
2003-05-21 20:42 ` Luben Tuikov
@ 2003-05-21 21:05 ` Alan Stern
0 siblings, 0 replies; 34+ messages in thread
From: Alan Stern @ 2003-05-21 21:05 UTC (permalink / raw)
To: Luben Tuikov; +Cc: Mike Anderson, linux-scsi
On Wed, 21 May 2003, Luben Tuikov wrote:
> Alan Stern wrote:
> >
> > Mike:
> >
> > I tried it out, and it did exactly what I wanted. You, Luben, and the
> > others can fight over the details.
>
> 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...
Sorry, that was meant as a joke. I guess I should have used a smiley...
Alan Stern
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: Patch: change the serial_number for error-handler commands
2003-05-21 18:03 ` Mike Anderson
` (2 preceding siblings ...)
2003-05-21 19:57 ` Alan Stern
@ 2003-05-21 21:19 ` James Bottomley
2003-05-21 22:53 ` Mike Anderson
2003-06-11 17:41 ` PATCH: (as33) Remove /proc/scsi directory in scsi_remove_host() Alan Stern
4 siblings, 1 reply; 34+ messages in thread
From: James Bottomley @ 2003-05-21 21:19 UTC (permalink / raw)
To: Mike Anderson; +Cc: Alan Stern, Luben Tuikov, SCSI Mailing List
On Wed, 2003-05-21 at 14:03, Mike Anderson wrote:
> 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.
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: Patch: change the serial_number for error-handler commands
2003-05-21 21:19 ` James Bottomley
@ 2003-05-21 22:53 ` Mike Anderson
0 siblings, 0 replies; 34+ messages in thread
From: Mike Anderson @ 2003-05-21 22:53 UTC (permalink / raw)
To: James Bottomley; +Cc: Alan Stern, Luben Tuikov, SCSI Mailing List
James Bottomley [James.Bottomley@SteelEye.com] wrote:
> 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).
>
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).
> 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;
> }
>
> ?
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
andmike@us.ibm.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* PATCH: (as33) Remove /proc/scsi directory in scsi_remove_host()
2003-05-21 18:03 ` Mike Anderson
` (3 preceding siblings ...)
2003-05-21 21:19 ` James Bottomley
@ 2003-06-11 17:41 ` Alan Stern
2003-06-11 18:23 ` Mike Anderson
4 siblings, 1 reply; 34+ messages in thread
From: Alan Stern @ 2003-06-11 17:41 UTC (permalink / raw)
To: Mike Anderson; +Cc: SCSI development list
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 =====
--- 1.11/drivers/scsi/scsi_proc.c Mon May 26 14:01:11 2003
+++ edited/drivers/scsi/scsi_proc.c Wed Jun 11 13:22:43 2003
@@ -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,
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: PATCH: (as33) Remove /proc/scsi directory in scsi_remove_host()
2003-06-11 17:41 ` PATCH: (as33) Remove /proc/scsi directory in scsi_remove_host() Alan Stern
@ 2003-06-11 18:23 ` Mike Anderson
2003-06-12 6:04 ` Christoph Hellwig
0 siblings, 1 reply; 34+ messages in thread
From: Mike Anderson @ 2003-06-11 18:23 UTC (permalink / raw)
To: Alan Stern; +Cc: SCSI development list
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.
Alan Stern [stern@rowland.harvard.edu] wrote:
> ===== scsi_proc.c 1.11 vs edited =====
> --- 1.11/drivers/scsi/scsi_proc.c Mon May 26 14:01:11 2003
> +++ edited/drivers/scsi/scsi_proc.c Wed Jun 11 13:22:43 2003
> @@ -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,
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-andmike
--
Michael Anderson
andmike@us.ibm.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: PATCH: (as33) Remove /proc/scsi directory in scsi_remove_host()
2003-06-11 18:23 ` Mike Anderson
@ 2003-06-12 6:04 ` Christoph Hellwig
2003-06-12 6:51 ` Mike Anderson
0 siblings, 1 reply; 34+ messages in thread
From: Christoph Hellwig @ 2003-06-12 6:04 UTC (permalink / raw)
To: Alan Stern, SCSI development list
On Wed, Jun 11, 2003 at 11:23:24AM -0700, Mike Anderson wrote:
> 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.
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 :)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: PATCH: (as33) Remove /proc/scsi directory in scsi_remove_host()
2003-06-12 6:04 ` Christoph Hellwig
@ 2003-06-12 6:51 ` Mike Anderson
2003-06-12 21:00 ` PATCH: (as33b) " Alan Stern
0 siblings, 1 reply; 34+ messages in thread
From: Mike Anderson @ 2003-06-12 6:51 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Alan Stern, SCSI development list
Christoph Hellwig [hch@infradead.org] wrote:
> On Wed, Jun 11, 2003 at 11:23:24AM -0700, Mike Anderson wrote:
> > 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.
>
> 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
andmike@us.ibm.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* PATCH: (as33b) Remove /proc/scsi directory in scsi_remove_host()
2003-06-12 6:51 ` Mike Anderson
@ 2003-06-12 21:00 ` Alan Stern
2003-06-12 21:58 ` Mike Anderson
0 siblings, 1 reply; 34+ messages in thread
From: Alan Stern @ 2003-06-12 21:00 UTC (permalink / raw)
To: Mike Anderson; +Cc: Christoph Hellwig, SCSI development list
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 stern@ida.rowland.org 1.1650
# Fix procfs handling during host unregistration
# --------------------------------------------
#
diff -Nru a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
--- a/drivers/scsi/hosts.c Thu Jun 12 16:52:28 2003
+++ b/drivers/scsi/hosts.c Thu Jun 12 16:52:28 2003
@@ -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
--- a/drivers/scsi/scsi_priv.h Thu Jun 12 16:52:28 2003
+++ b/drivers/scsi/scsi_priv.h Thu Jun 12 16:52:28 2003
@@ -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)
diff -Nru a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
--- a/drivers/scsi/scsi_proc.c Thu Jun 12 16:52:28 2003
+++ b/drivers/scsi/scsi_proc.c Thu Jun 12 16:52:28 2003
@@ -40,6 +40,8 @@
struct proc_dir_entry *proc_scsi;
EXPORT_SYMBOL(proc_scsi);
+/* Protect sht->present and sht->proc_dir */
+static DECLARE_MUTEX(global_host_template_sem);
/* Used if the driver currently has no own support for /proc/scsi */
static int generic_proc_info(char *buffer, char **start, off_t offset,
@@ -112,13 +114,10 @@
return ret;
}
-void scsi_proc_host_add(struct Scsi_Host *shost)
+void scsi_proc_hostdir_add(struct SHT *sht)
{
- Scsi_Host_Template *sht = shost->hostt;
- struct proc_dir_entry *p;
- char name[10];
-
- if (!sht->proc_dir) {
+ down(&global_host_template_sem);
+ if (!sht->present++) {
sht->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
if (!sht->proc_dir) {
printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
@@ -127,30 +126,51 @@
}
sht->proc_dir->owner = sht->module;
}
+ up(&global_host_template_sem);
+}
+
+void scsi_proc_hostdir_rm(struct SHT *sht)
+{
+ down(&global_host_template_sem);
+ if (!--sht->present && sht->proc_dir) {
+ remove_proc_entry(sht->proc_name, proc_scsi);
+ sht->proc_dir = NULL;
+ }
+ up(&global_host_template_sem);
+}
+
+void scsi_proc_host_add(struct Scsi_Host *shost)
+{
+ Scsi_Host_Template *sht = shost->hostt;
+ struct proc_dir_entry *p;
+ char name[10];
+
+ if (!sht->proc_dir)
+ return;
sprintf(name,"%d", shost->host_no);
p = create_proc_read_entry(name, S_IFREG | S_IRUGO | S_IWUSR,
- shost->hostt->proc_dir, proc_scsi_read, shost);
+ sht->proc_dir, proc_scsi_read, shost);
if (!p) {
printk(KERN_ERR "%s: Failed to register host %d in"
"%s\n", __FUNCTION__, shost->host_no,
- shost->hostt->proc_name);
+ sht->proc_name);
return;
}
p->write_proc = proc_scsi_write;
- p->owner = shost->hostt->module;
-
+ p->owner = sht->module;
}
void scsi_proc_host_rm(struct Scsi_Host *shost)
{
char name[10];
+ if (!shost->hostt->proc_dir)
+ return;
+
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);
}
static void proc_print_scsidevice(struct scsi_device* sdev, char *buffer,
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: PATCH: (as33b) Remove /proc/scsi directory in scsi_remove_host()
2003-06-12 21:00 ` PATCH: (as33b) " Alan Stern
@ 2003-06-12 21:58 ` Mike Anderson
2003-06-13 14:38 ` Alan Stern
2003-06-15 13:01 ` Christoph Hellwig
0 siblings, 2 replies; 34+ messages in thread
From: Mike Anderson @ 2003-06-12 21:58 UTC (permalink / raw)
To: Alan Stern; +Cc: Christoph Hellwig, SCSI development list
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
andmike@us.ibm.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: PATCH: (as33b) Remove /proc/scsi directory in scsi_remove_host()
2003-06-12 21:58 ` Mike Anderson
@ 2003-06-13 14:38 ` Alan Stern
2003-06-15 13:01 ` Christoph Hellwig
1 sibling, 0 replies; 34+ messages in thread
From: Alan Stern @ 2003-06-13 14:38 UTC (permalink / raw)
To: Mike Anderson; +Cc: Christoph Hellwig, SCSI development list
On Thu, 12 Jun 2003, Mike Anderson wrote:
> Alan,
> This looks good. In scsi-misc-2.5 SHT has been removed so there
> will need to be a little name sync up.
That shouldn't be difficult.
> 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.
I agree.
> 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.
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: PATCH: (as33b) Remove /proc/scsi directory in scsi_remove_host()
2003-06-12 21:58 ` Mike Anderson
2003-06-13 14:38 ` Alan Stern
@ 2003-06-15 13:01 ` Christoph Hellwig
1 sibling, 0 replies; 34+ messages in thread
From: Christoph Hellwig @ 2003-06-15 13:01 UTC (permalink / raw)
To: Alan Stern, Christoph Hellwig, SCSI development list
On Thu, Jun 12, 2003 at 02:58:58PM -0700, Mike Anderson wrote:
> 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.
Yes, certainly.
> 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.
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..
^ permalink raw reply [flat|nested] 34+ messages in thread