Re: [PATCH v6 05/23] ata: libata-scsi: Disable scsi device manage_system_start_stop

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

On 2023/09/26 8:19, Damien Le Moal wrote:
> On 2023/09/25 16:27, Phillip Susi wrote:
>>
>> Damien Le Moal <dlemoal@kernel.org> writes:
>>
>>> However, restoring the ATA device to the active power mode must be
>>> synchronized with libata EH processing of the port resume operation to
>>> avoid either 1) seeing the start stop unit command being received too
>>> early when the port is not yet resumed and ready to accept commands, or
>>> after the port resume process issues commands such as IDENTIFY to
>>
>> I do not believe this is correct.  The drive must respond to IDENTIFY
>> and SET FEATURES while in standby mode.  Some of the information in the
>> IDENTIFY block may be flagged as not available because it requires media
>> access and the drive is in standby.  There is a bit in the IDENTIFY
>> block that indicates whether the drive will automatically spin up for
>> media access commands or not, and if not, then you must issue the SET
>> FEATURES command to spin it up.  For such drives, that VERIFY command
>> will fail.
> 
> Yes about the IDENTIFY command. But exactly as you said, some drives have
> metadata on the media and will not report everything, or we outright not like
> receiving an IDENTIFY command while spun down (I have a couple of these odd
> clown drives in my collection).
> 
> However, regarding the SET FEATURES command to spin up the drive, you are
> confusing the basic power management (STANDBY IMMEDIATE command support), which
> is a mandatory feature of ATA disks, with the Extended Power Conditions (EPC)
> feature set, which is optional. The latter one defines the behavior of the SET
> FEATURES command with the Extended Power Conditions subcommand to control the
> disk power state and power state transitions timers. The former, basic power
> management, does NOT have this. So trying what you suggest would only work for
> drives that support and enable EPC. Given that EPC is optional, and that we are
> not probing/supporting it currently in libata, we cannot rely on that.

Note: re-reading the specs, I found that the mandatory (simple) power management
feature set mandates support for the CHECK POWER MODE command, which reports the
current power state of the device without affecting it. So we could use that to
try to be a little more refined about resume. But sending a CHECK POWER MODE and
then do nothing or send a VERIFY command is in a sense more complicated than
always sending a VERIFY commands, even if that may be useless in some cases.

I will think about this as a follow up cleanup/improvement. Starting using "new"
commands that where not used until now is scary though. Every time we do that,
there are some regressions reported because so crappy drive that does not follow
the standards choke on that new command. And unfortunately, there are *a lot* of
such drive out there.

> 
>>> revalidate the device. In this last case, the risk is that the device
>>> revalidation fails with timeout errors as the drive is still spun down.
>>
>> If a request can timeout before the drive has time to spin up, then that
>> would be a problem outside of suspend/resume.  You would get such
>> timeouts any time you manually suspend the drive with hdparm -y, or the
>> drive auto suspends ( hdparm -S ).  The timeout needs to be long enough
>> for the drive to spin up.  IIRC, it defaults to 10 seconds, which is
>> plenty of time.
> 
> That already is all taken care of. That is the basics for even the initial scan
> on boot where we send commands to the disk while it is still spinning up. The
> timeout I am mentioning is the drive not responding at all because it is spun
> down, no matter how many times one retries. And given that the ATA specs clearly
> define that a drive should not change its power state with a reset, even the
> reset after the command timeout does not change anything with some drives (I do
> have some drives that actually spin up on reset, but many that don't, as per spec).
> 
>> It sounds like you are saying that you unconditionally wake the drive
>> with a VERIFY command to make sure that you can then IDENTIFY.  This
> 
> Exactly. As you said yourself, there are some drives that will not report
> everything unless they are spun up. And I have some old drives that really do
> not like receiving that command at all while spun down. So the safer approach
> taken is to spinup the drive upfront, before doing anything else.
> 
>> should not be needed.  In addition, if the drive has PuiS enabled, I
>> would like to leave it in standby after a system resume, not force it to
>> wake up.  After all, that is why it has PuiS enabled.
> 
> PUIS is another optional feature that we do not directly support in the kernel.
> If you want/need that, patches are welcome. With detection of that feature
> added, we could improve resume and avoid useless drive spinup. That is currently
> outside the scope of this series since we are not supporting PUIS currently.
> 
>>
> 

-- 
Damien Le Moal
Western Digital Research