On Thu, Mar 15, 2012 at 12:54 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 15 Mar 2012, Williams, Dan J wrote:
>
>> > 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.
>
>> 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
--
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
