[PATCH / RFC] scsi_set_host_offline

[PATCH / RFC] scsi_set_host_offline

[Mike Anderson]

I have added a scsi_set_host_offline which supplements
scsi_set_device_offline. host_lock should not be held on call of either
one of these functions.

Currently scsi_set_host_offline is just setting
each sdev in my_devices offline, but could be expanded without changing
the api to the LLDD. 

I also added a for_each function. Currently scsi_set_host_offline is the
only user and could have included the functionality, but I thought there
may be possible re-use in the ~50 references to my_devices. The for_each
function also calls scsi_device_[get/put] which are not complete yet,
and would need to handle the selected policy of when a list entry is
removed from the list.

I have only tested the interface using scsi_debug.

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

=====
name:		00_scsi_set_host_offline-1.diff
version:	2003-03-03.12:59:06-0800
against:	scsi-misc-2.5

 scsi.c      |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 scsi.h      |    1 +
 scsi_syms.c |    1 +
 3 files changed, 53 insertions(+)

=====
===== drivers/scsi/scsi.c 1.97 vs edited =====
--- 1.97/drivers/scsi/scsi.c	Wed Feb 26 00:00:06 2003
+++ edited/drivers/scsi/scsi.c	Mon Mar  3 12:31:03 2003
@@ -1300,6 +1300,38 @@
 }
 
 /**
+ * scsi_host_for_each_device - call function for each host child device
+ * @shost:	struct Scsi_Host to interate over.
+ * @data:	void pointer argumnet passed to called function.
+ * @fn:		function to call for each device.
+ *
+ **/
+static int scsi_host_for_each_device(struct Scsi_Host *shost,
+			 void * data, int (*fn)(struct scsi_device *, void *))
+{
+	struct list_head *lh;
+	struct scsi_device *sdev;
+	unsigned long flags;
+	int error = 0;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	for (lh = shost->my_devices.next; lh != &shost->my_devices;){
+		sdev = list_entry(lh, struct scsi_device, siblings);
+		scsi_device_get(sdev);
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		error = fn(sdev, data);
+		spin_lock_irqsave(shost->host_lock, flags);
+		lh = lh->next;
+		scsi_device_put(sdev);
+		if (error)
+			break;
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	return error;
+}
+
+/**
  * scsi_set_device_offline - set scsi_device offline
  * @sdev:	pointer to struct scsi_device to offline. 
  *
@@ -1338,6 +1370,25 @@
 	} else {
 		/* FIXME: Send online state change hotplug event */
 	}
+}
+
+/**
+ * scsi_set_device_offline - wrapper.
+ **/
+static int __scsi_set_device_offline(struct scsi_device *sdev, void *data)
+{
+	scsi_set_device_offline(sdev);
+	return 0;
+}
+
+/**
+ * scsi_set_host_offline - set all scsi_devices on a host offline
+ * @shost:	pointer to struct Scsi_Host.
+ *
+ **/
+void scsi_set_host_offline(struct Scsi_Host *shost)
+{
+	scsi_host_for_each_device(shost, NULL, __scsi_set_device_offline);
 }
 
 /*
===== drivers/scsi/scsi.h 1.65 vs edited =====
--- 1.65/drivers/scsi/scsi.h	Sun Feb 23 10:34:55 2003
+++ edited/drivers/scsi/scsi.h	Mon Mar  3 12:34:21 2003
@@ -455,6 +455,7 @@
 extern void scsi_slave_detach(struct scsi_device *);
 extern int scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
+extern void scsi_set_host_offline(struct Scsi_Host *);
 extern void scsi_set_device_offline(struct scsi_device *);
 extern void scsi_done(Scsi_Cmnd * SCpnt);
 extern void scsi_finish_command(Scsi_Cmnd *);
===== drivers/scsi/scsi_syms.c 1.29 vs edited =====
--- 1.29/drivers/scsi/scsi_syms.c	Sun Feb 23 10:36:05 2003
+++ edited/drivers/scsi/scsi_syms.c	Mon Mar  3 12:33:01 2003
@@ -80,6 +80,7 @@
 EXPORT_SYMBOL(scsi_device_put);
 EXPORT_SYMBOL(scsi_add_device);
 EXPORT_SYMBOL(scsi_remove_device);
+EXPORT_SYMBOL(scsi_set_host_offline);
 EXPORT_SYMBOL(scsi_set_device_offline);
 
 /*

Re: [PATCH / RFC] scsi_set_host_offline

[Oliver Neukum]

Am Montag, 3. März 2003 23:15 schrieb Mike Anderson:
> I have added a scsi_set_host_offline which supplements
> scsi_set_device_offline. host_lock should not be held on call of either
> one of these functions.

Very good.

> Currently scsi_set_host_offline is just setting
> each sdev in my_devices offline, but could be expanded without changing
> the api to the LLDD.

Don't we need to disallow further devices being discovered on this
host's busses before we set the existing devices offline?

	Regards
		Oliver

-
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

Re: [PATCH / RFC] scsi_set_host_offline

[Matthew Dharm]

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

On Tue, Mar 04, 2003 at 01:03:18AM +0100, Oliver Neukum wrote:
> Don't we need to disallow further devices being discovered on this
> host's busses before we set the existing devices offline?

Hrm... yes, I think this could be a problem.

For 99% of USB storage devices, it's not.  One device == one host with one
target.  However, for the other 1% of devices (and all 'real' SCSI hosts),
this could be an issue.  I could be trying to yank the device (hot-unplug)
at the same moment someone is doing an add-single-device.

Matt

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

I need a computer?
					-- Customer
User Friendly, 2/19/1998

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

Re: [PATCH / RFC] scsi_set_host_offline

[Oliver Neukum]

Am Dienstag, 4. März 2003 21:36 schrieb Matthew Dharm:
> On Tue, Mar 04, 2003 at 01:03:18AM +0100, Oliver Neukum wrote:
> > Don't we need to disallow further devices being discovered on this
> > host's busses before we set the existing devices offline?
>
> Hrm... yes, I think this could be a problem.
>
> For 99% of USB storage devices, it's not.  One device == one host with one
> target.  However, for the other 1% of devices (and all 'real' SCSI hosts),
> this could be an issue.  I could be trying to yank the device (hot-unplug)
> at the same moment someone is doing an add-single-device.

This is not limited to this case. Device addition could as well be through
ordinary scanning of the bus which the SCSI layer does.

	Regards
		Oliver


-
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

Re: [PATCH / RFC] scsi_set_host_offline

[Mike Anderson]

Matthew Dharm [mdharm-scsi@one-eyed-alien.net] wrote:
> On Tue, Mar 04, 2003 at 01:03:18AM +0100, Oliver Neukum wrote:
> > Don't we need to disallow further devices being discovered on this
> > host's busses before we set the existing devices offline?
> 
> Hrm... yes, I think this could be a problem.
> 
> For 99% of USB storage devices, it's not.  One device == one host with one
> target.  However, for the other 1% of devices (and all 'real' SCSI hosts),
> this could be an issue.  I could be trying to yank the device (hot-unplug)
> at the same moment someone is doing an add-single-device.
> 

While the calling of scsi_set_host_offline may mean no more additions
for a set of hosts it may be not be the case for the whole set.

A host offline value may be a good thing to add. The policy around the
state maybe hard to generalize.

If a LLDD decided that it did not want any more devices added it can
today fail calls to queuecommand or slave_alloc (which some already do).
The LLDD may need to handle these in the future depending on what
functions scsi_set_host_offline maybe racing with.

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

Re: [PATCH / RFC] scsi_set_host_offline

[Oliver Neukum]

> If a LLDD decided that it did not want any more devices added it can
> today fail calls to queuecommand or slave_alloc (which some already do).
> The LLDD may need to handle these in the future depending on what
> functions scsi_set_host_offline maybe racing with.

It seems to me that there's a race with scan_scsis() and scan_scsis_single()
which cannot be closed with failing queuecommand, as there's a window where
the INQUIRY has succeeded but has not yet been evaluated by the SCSI layer.
I am not quite sure how slave_alloc() is involved into this, but IMHO it's 
wrong to force LLDDs to know about devices if they are hotpluggable.
Could you clarify?

	Regards
		Oliver

Re: [PATCH / RFC] scsi_set_host_offline

[James Bottomley]

On Tue, 2003-03-04 at 17:12, Oliver Neukum wrote:
> It seems to me that there's a race with scan_scsis() and scan_scsis_single()
> which cannot be closed with failing queuecommand, as there's a window where
> the INQUIRY has succeeded but has not yet been evaluated by the SCSI layer.

Consider the offline function as a work in progress.  I think the model
we'll ultimately move to is to use the sysfs refcounting, so we could
call the offline function, have the device refuse all incoming requests,
but still process pending ones (which, if this is a forced remove should
fail).  Then call a remove hook when the outstanding command count drops
to zero.

> I am not quite sure how slave_alloc() is involved into this, but IMHO it's 
> wrong to force LLDDs to know about devices if they are hotpluggable.
> Could you clarify?

By design LLDs need to know about configured devices because they may
(although not all do) need to allocate resources to process them.  The
slave configure/alloc/destroy is the interface for communicating this
information:

slave_alloc informs the LLD that we're about to try the device, so it
should set its internal queues up for an inquiry command.

If inquiry succeeds, we process the info and call slave_configure to
adjust the queue to its initial depth based on the device parameters.

If the inquiry fails, slave_destroy is called to free the allocated
resources.

For a functional device, slave_destroy is called when we remove it to
tell the LLD to destroy the resources it has allocated to handle the
device.

James

Re: [PATCH / RFC] scsi_set_host_offline

[Oliver Neukum]

Am Mittwoch, 5. März 2003 17:52 schrieben Sie:
> On Tue, 2003-03-04 at 17:12, Oliver Neukum wrote:
> > It seems to me that there's a race with scan_scsis() and
> > scan_scsis_single() which cannot be closed with failing queuecommand, as
> > there's a window where the INQUIRY has succeeded but has not yet been
> > evaluated by the SCSI layer.
>
> Consider the offline function as a work in progress.  I think the model

I do. But problems have to be pointed out and fixes proposed.

> we'll ultimately move to is to use the sysfs refcounting, so we could
> call the offline function, have the device refuse all incoming requests,
> but still process pending ones (which, if this is a forced remove should
> fail).  Then call a remove hook when the outstanding command count drops
> to zero.

What exactly is a refcount supposed to do? It doesn't help the main problem.
You must prevent new references arising. This requires locking and flags.

> > I am not quite sure how slave_alloc() is involved into this, but IMHO
> > it's wrong to force LLDDs to know about devices if they are hotpluggable.
> > Could you clarify?
>
> By design LLDs need to know about configured devices because they may
> (although not all do) need to allocate resources to process them.  The
> slave configure/alloc/destroy is the interface for communicating this
> information:

Very well, but that system should not be overloaded with hotplugging issues.
IMHO we must not require drivers to support this interface just because they
are hotpluggable.

	Regards
		Oliver

-
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

Re: [PATCH / RFC] scsi_set_host_offline

[Mike Anderson]

Oliver Neukum [oliver@neukum.name] wrote:
> Am Mittwoch, 5. M?rz 2003 17:52 schrieben Sie:
> > On Tue, 2003-03-04 at 17:12, Oliver Neukum wrote:
> > > It seems to me that there's a race with scan_scsis() and
> > > scan_scsis_single() which cannot be closed with failing queuecommand, as
> > > there's a window where the INQUIRY has succeeded but has not yet been
> > > evaluated by the SCSI layer.
> >
> > Consider the offline function as a work in progress.  I think the model
> 
> I do. But problems have to be pointed out and fixes proposed.
> 
> > we'll ultimately move to is to use the sysfs refcounting, so we could
> > call the offline function, have the device refuse all incoming requests,
> > but still process pending ones (which, if this is a forced remove should
> > fail).  Then call a remove hook when the outstanding command count drops
> > to zero.
> 
> What exactly is a refcount supposed to do? It doesn't help the main problem.
> You must prevent new references arising. This requires locking and flags.
> 

Refcounting is need for proper freeing so it needs to be part of the
solution. We need to have the refcounting implementation prevent gets
post "offline"

Yes, currently the kobject_get does not contain any support for
preventing the incrementing of the ref count. This needs to be wrapped
by the implementor (I believe as I do not see the object state support
being implemented in the core code).

> > > I am not quite sure how slave_alloc() is involved into this, but IMHO
> > > it's wrong to force LLDDs to know about devices if they are hotpluggable.
> > > Could you clarify?
> >
> > By design LLDs need to know about configured devices because they may
> > (although not all do) need to allocate resources to process them.  The
> > slave configure/alloc/destroy is the interface for communicating this
> > information:
> 
> Very well, but that system should not be overloaded with hotplugging issues.
> IMHO we must not require drivers to support this interface just because they
> are hotpluggable.

It is not required, but lack of any online state in a driver will
possibly cause extended time to cancel the IO. We would not want
excessive checking in the main IO path so there will be code that races
with "offline" and makes it to queuecommand.

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

Re: [PATCH / RFC] scsi_set_host_offline

[Oliver Neukum]

> > What exactly is a refcount supposed to do? It doesn't help the main
> > problem. You must prevent new references arising. This requires locking
> > and flags.
>
> Refcounting is need for proper freeing so it needs to be part of the
> solution. We need to have the refcounting implementation prevent gets
> post "offline"
>
> Yes, currently the kobject_get does not contain any support for
> preventing the incrementing of the ref count. This needs to be wrapped
> by the implementor (I believe as I do not see the object state support
> being implemented in the core code).

True but it misses an important part of the problem. It mixes references
through IO in progress with devices attached to a host. These are independent
issues.

> It is not required, but lack of any online state in a driver will
> possibly cause extended time to cancel the IO. We would not want
> excessive checking in the main IO path so there will be code that races
> with "offline" and makes it to queuecommand.

The main IO path is independent of the problems arising from offlining a
host. This issue must be dealt with in connection with offlining a _device_.

However offlining a host causes additional problems because devices
can be added and taken away from a host.
It takes a flag to make sure that no further devices can be added to the
host. This does not influence the main IO path, only device detection
and removal, which are not critical to performance.

	Regards
		Oliver

Re: [PATCH / RFC] scsi_set_host_offline

[Patrick Mansfield]

Looks nice. Two nits.

On Mon, Mar 03, 2003 at 02:15:32PM -0800, Mike Anderson wrote:

> +	spin_lock_irqsave(shost->host_lock, flags);
> +	for (lh = shost->my_devices.next; lh != &shost->my_devices;){

Uh oh you are missing a space above, better call the code style police :)

> +		sdev = list_entry(lh, struct scsi_device, siblings);
> +		scsi_device_get(sdev);
> +		spin_unlock_irqrestore(shost->host_lock, flags);
> +		error = fn(sdev, data);
> +		spin_lock_irqsave(shost->host_lock, flags);
> +		lh = lh->next;
> +		scsi_device_put(sdev);
> +		if (error)
> +			break;

Embedding "!error &&" in the for conditional is a bit cleaner.

-- Patrick Mansfield

Re: [PATCH / RFC] scsi_set_host_offline

[Matthew Dharm]

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

Mike --

As far as I can tell, this patch was never accepted.  Any idea when this
will be resolved?

usb-storage is still waiting to implement proper device unplug...

Matt

On Mon, Mar 03, 2003 at 02:15:32PM -0800, Mike Anderson wrote:
> I have added a scsi_set_host_offline which supplements
> scsi_set_device_offline. host_lock should not be held on call of either
> one of these functions.
> 
> Currently scsi_set_host_offline is just setting
> each sdev in my_devices offline, but could be expanded without changing
> the api to the LLDD. 
> 
> I also added a for_each function. Currently scsi_set_host_offline is the
> only user and could have included the functionality, but I thought there
> may be possible re-use in the ~50 references to my_devices. The for_each
> function also calls scsi_device_[get/put] which are not complete yet,
> and would need to handle the selected policy of when a list entry is
> removed from the list.
> 
> I have only tested the interface using scsi_debug.
> 
> -andmike
> --
> Michael Anderson
> andmike@us.ibm.com
> 
> =====
> name:		00_scsi_set_host_offline-1.diff
> version:	2003-03-03.12:59:06-0800
> against:	scsi-misc-2.5
> 
>  scsi.c      |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  scsi.h      |    1 +
>  scsi_syms.c |    1 +
>  3 files changed, 53 insertions(+)
> 
> =====
> ===== drivers/scsi/scsi.c 1.97 vs edited =====
> --- 1.97/drivers/scsi/scsi.c	Wed Feb 26 00:00:06 2003
> +++ edited/drivers/scsi/scsi.c	Mon Mar  3 12:31:03 2003
> @@ -1300,6 +1300,38 @@
>  }
>  
>  /**
> + * scsi_host_for_each_device - call function for each host child device
> + * @shost:	struct Scsi_Host to interate over.
> + * @data:	void pointer argumnet passed to called function.
> + * @fn:		function to call for each device.
> + *
> + **/
> +static int scsi_host_for_each_device(struct Scsi_Host *shost,
> +			 void * data, int (*fn)(struct scsi_device *, void *))
> +{
> +	struct list_head *lh;
> +	struct scsi_device *sdev;
> +	unsigned long flags;
> +	int error = 0;
> +
> +	spin_lock_irqsave(shost->host_lock, flags);
> +	for (lh = shost->my_devices.next; lh != &shost->my_devices;){
> +		sdev = list_entry(lh, struct scsi_device, siblings);
> +		scsi_device_get(sdev);
> +		spin_unlock_irqrestore(shost->host_lock, flags);
> +		error = fn(sdev, data);
> +		spin_lock_irqsave(shost->host_lock, flags);
> +		lh = lh->next;
> +		scsi_device_put(sdev);
> +		if (error)
> +			break;
> +	}
> +	spin_unlock_irqrestore(shost->host_lock, flags);
> +
> +	return error;
> +}
> +
> +/**
>   * scsi_set_device_offline - set scsi_device offline
>   * @sdev:	pointer to struct scsi_device to offline. 
>   *
> @@ -1338,6 +1370,25 @@
>  	} else {
>  		/* FIXME: Send online state change hotplug event */
>  	}
> +}
> +
> +/**
> + * scsi_set_device_offline - wrapper.
> + **/
> +static int __scsi_set_device_offline(struct scsi_device *sdev, void *data)
> +{
> +	scsi_set_device_offline(sdev);
> +	return 0;
> +}
> +
> +/**
> + * scsi_set_host_offline - set all scsi_devices on a host offline
> + * @shost:	pointer to struct Scsi_Host.
> + *
> + **/
> +void scsi_set_host_offline(struct Scsi_Host *shost)
> +{
> +	scsi_host_for_each_device(shost, NULL, __scsi_set_device_offline);
>  }
>  
>  /*
> ===== drivers/scsi/scsi.h 1.65 vs edited =====
> --- 1.65/drivers/scsi/scsi.h	Sun Feb 23 10:34:55 2003
> +++ edited/drivers/scsi/scsi.h	Mon Mar  3 12:34:21 2003
> @@ -455,6 +455,7 @@
>  extern void scsi_slave_detach(struct scsi_device *);
>  extern int scsi_device_get(struct scsi_device *);
>  extern void scsi_device_put(struct scsi_device *);
> +extern void scsi_set_host_offline(struct Scsi_Host *);
>  extern void scsi_set_device_offline(struct scsi_device *);
>  extern void scsi_done(Scsi_Cmnd * SCpnt);
>  extern void scsi_finish_command(Scsi_Cmnd *);
> ===== drivers/scsi/scsi_syms.c 1.29 vs edited =====
> --- 1.29/drivers/scsi/scsi_syms.c	Sun Feb 23 10:36:05 2003
> +++ edited/drivers/scsi/scsi_syms.c	Mon Mar  3 12:33:01 2003
> @@ -80,6 +80,7 @@
>  EXPORT_SYMBOL(scsi_device_put);
>  EXPORT_SYMBOL(scsi_add_device);
>  EXPORT_SYMBOL(scsi_remove_device);
> +EXPORT_SYMBOL(scsi_set_host_offline);
>  EXPORT_SYMBOL(scsi_set_device_offline);
>  
>  /*

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

You are needink to look more evil.  You likink very strong coffee?
					-- Pitr to Dust Puppy
User Friendly, 10/16/1998

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

Re: [PATCH / RFC] scsi_set_host_offline

[Mike Anderson]

Matthew Dharm [mdharm-scsi@one-eyed-alien.net] wrote:
> Mike --
> 
> As far as I can tell, this patch was never accepted.  Any idea when this
> will be resolved?
> 
> usb-storage is still waiting to implement proper device unplug...
> 

I reposted the patch with refreshed offsets for the newer kernel
version.

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



-------------------------------------------------------
This SF.net email is sponsored by:
The Definitive IT and Networking Event. Be There!
NetWorld+Interop Las Vegas 2003 -- Register today!
http://ads.sourceforge.net/cgi-bin/redirect.pl?keyn0001en
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel