Re: [PATCH v6 09/23] scsi: sd: Do not issue commands to suspended disks on shutdown

[Damien Le Moal <dlemoal@kernel.org>]

On 2023/09/25 22:22, Bart Van Assche wrote:
> On 9/22/23 17:29, Damien Le Moal wrote:
>> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
>> index 5eea762f84d1..4d42392fae07 100644
>> --- a/drivers/scsi/sd.h
>> +++ b/drivers/scsi/sd.h
>> @@ -150,6 +150,7 @@ struct scsi_disk {
>>   	unsigned	urswrz : 1;
>>   	unsigned	security : 1;
>>   	unsigned	ignore_medium_access_errors : 1;
>> +	unsigned	suspended : 1;
>>   };
>>   #define to_scsi_disk(obj) container_of(obj, struct scsi_disk, disk_dev)
> 
> If the 'suspended' member is retained, please do not use a bitfield for the
> but use type 'bool' instead. Updates of instances of type 'bool' are atomic
> while there is no guarantee in the C standard that bitfield updates will be
> atomic. Bitfield updates are typically translated into a combination of &,
> | and ~ operations.

Sure, I can make it a bool.

> Additionally, I'm not convinced that we need the new 'suspended' member.
> The Linux kernel runtime PM subsystem serializes I/O and system-wide power
> operations. No I/O happens during system-wide suspend or resume operations
> and no system-wide suspend or resume callbacks are invoked while I/O is
> ongoing. The only exception is I/O that is initiated as the result of error
> handling by suspend or resume callbacks, e.g. the SCSI commands submitted
> by sd_shutdown(). Even if sd_shutdown() is called indirectly by a suspend
> or resume callback, I don't think that it can happen that a suspend or
> resume operation is ongoing for the device sd_shutdown() operates on. If

Yes, but that is not what this patch addresses.

> scsi_remove_host() is called from inside a resume callback, resuming of the
> devices affected by sd_shutdown() will only be attempted after the host
> adapter resume callback has finished.

No it will not because the commands issued in sd_shutdown() are synchronous, so
the adapter resume will wait for these to complete. But they will never complete
as the adapter itself is not fully resumed, AND the disk may not be in a state
that allows commands to be executed. Deadlock.

It is easy to recreate this issue if you have a pm8001 adapter: remove that fix
patch I sent to correctly re-allocate IRQs on resume and do a suspend-resume
cycle: on resume, the adapter fails to allocate IRQs and gives up, calling
scsi_remove_host(). The system end being stuck in resume context with no forward
progress ever made.

It seems that you are suggesting that we should use some information from the
scsi_device->power structure to detect the suspended state... But as mentioned
before, these are PM internal and should not be touched without the device lock
held. So the little "suspended" falg simplifies things a lot.

> 
> Thanks,
> 
> Bart.

-- 
Damien Le Moal
Western Digital Research