On Thu, Mar 15, 2012 at 12:54 PM, Alan Stern [off-list ref] wrote:

On Thu, 15 Mar 2012, Williams, Dan J wrote:

quoted

quoted

Hmm... I was wondering why we needed a kernel global sync.  If this is
just for probe work what about just doing something like the following?
Keep the sync operation local to probe-work and not entangle with the
resume-work?  I'll give this a shot when I get back to my test system.

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index bd17cf8..ec69e35 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c

@@ -2629,6 +2629,8 @@ static void sd_probe_async(void *data, async_cookie_t cookie)

       put_device(&sdkp->dev);
 }

+static LIST_HEAD(sd_probe_domain);

Take a look at fea6d607e154cf96ab22254ccb48addfd43d4cb5; it might be a
good idea to make this domain available to scsi_bus_prepare().  For
example, it could made into a SCSI-wide domain, defined in the SCSI
core and exported for use by sd.

Nice, thanks for the pointer.  Yes, I'll up-level this.

quoted

This works fine for me for resolving the deadlock, but I found I also
needed the following to fix a spurious:

   scsi 6:0:1:0: Illegal state transition deleted->running

...in the resume path.

@@ -2348,10 +2349,11 @@ EXPORT_SYMBOL(scsi_device_quiesce);

  *
  *     Must be called with user context, may sleep.
  */
-void
-scsi_device_resume(struct scsi_device *sdev)
+void scsi_device_resume(struct scsi_device *sdev)
 {
-       if(scsi_device_set_state(sdev, SDEV_RUNNING))
+       /* check if the device was deleted during suspend */
+       if (sdev->sdev_state == SDEV_DEL ||
+           scsi_device_set_state(sdev, SDEV_RUNNING))
                return;
        scsi_run_queue(sdev->request_queue);
 }

Unless someone can point out something I'm missing I'll go ahead and
roll this into it's own patch and rebase/drop the hack out of the
libsas resume code.

The device might be in some other state.  Perhaps it would be better to
do

       if (sdev->sdev_state != SDEV_QUIESCE ||
                       scsi_device_set_state(sdev, SDEV_RUNNING))

I'm not sure what guarantees this function is supposed to provide, but
the comment indicates that it's meant just to handle quiesced devices.

I'm not sure either, but I can get on board with this change to say
"the world changed when you weren't looking, assume whomever changed
the state is taking care of the device".

--
Dan