Re: [isci PATCH v3 3/4] libsas: suspend / resume support

["Williams, Dan J" <dan.j.williams@xxxxxxxxx>]

On Wed, Mar 14, 2012 at 2:33 PM, Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> On Wed, 2012-03-14 at 17:11 -0400, Alan Stern wrote:
>> I see.  It's a nasty situation; I guess the best way to describe it is
>> a conflict between the requirements of the PM and SCSI subsystems:
>>
>>       The PM core runs suspend and resume operations in async
>>       threads, and these threads need to acquire the device lock;
>>
>>       The sd driver needs to insure that async probing is finished
>>       when starting its remove routine, and it is called with the
>>       device lock held.
>>
>> The best solution might be to use a workqueue for sd's async probing
>> instead of the "async" threads.  Then the work routine could be
>> cancelled without doing async_synchronize_full().
>
> 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);
> +
>  /**
>  *     sd_probe - called during driver initialization and whenever a
>  *     new scsi device is attached to the system. It is called once
> @@ -2717,7 +2719,7 @@ static int sd_probe(struct device *dev)
>        dev_set_drvdata(dev, sdkp);
>
>        get_device(&sdkp->dev); /* prevent release before async_schedule */
> -       async_schedule(sd_probe_async, sdkp);
> +       async_schedule_domain(sd_probe_async, sdkp, &sd_probe_domain);
>
>        return 0;
>
> @@ -2751,7 +2753,7 @@ static int sd_remove(struct device *dev)
>        sdkp = dev_get_drvdata(dev);
>        scsi_autopm_get_device(sdkp->device);
>
> -       async_synchronize_full();
> +       async_synchronize_full_domain(&sd_probe_domain);
>        blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
>        blk_queue_unprep_rq(sdkp->device->request_queue, NULL);
>        device_del(&sdkp->dev);

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.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html